Re: [PING][PATCH,DWARF,gfortran] Change DWARF-2 output for Fortran COMMON symbols

[George Helffrich <george at geology dot bristol dot ac dot uk>]

On 24 Dec 2007, at 23:54, Geoff Keating wrote:

> George Helffrich <george@geology.bristol.ac.uk> writes:
>
> > The patch only changes gfortran's debug information under DWARF to use
> > DW_TAG_common_block to describe identifiers located in Fortran common.
> > The implementation follows DWARF-2 documentation of this facility.  No
> > part of the code is specifically a workaround.  If there are any
> > objectionable parts, I will explain and/or change as required.
>
> The parts I'm concerned about are those associated with the comment
>
> > Format of entry is rather tricky in order to be
> > acceptable to gdb's relocation mechanism for COMMON-based symbols.
>
> including most of the routines common_check and all of dw_expand_expr.

Thanks for your feedback.  I hope that I can clarify your queries by
referring to the DWARF-2 output produced by compiling the following 
program
(commdecl.f) with gfortran (patches applied) and Intel ifort, with
-S -gdwarf-2.  The respective .s files are attached for reference, with 
lines
prefixed with "//**" added by me to flag areas of interest.

Attachment: commdecl.f
Description: Binary data

Attachment: commdecl-gfortran.s
Description: Binary data

Attachment: commdecl-ifor.s
Description: Binary data

To help you assess whether the structure of the DIE is outre' or not,
here is an analysis of the respective DIEs for the declarations.  (The
comment that you flag dates from an early development stage and is
now somewhat misleading.)

The structure of the DIEs for the common block "ecco"
both contain the name and source coordinates; the address
is coded as DW_AT_location and DW_OP_addr in both cases.
The only difference is the presence of DW_AT_MIPS_linkage_name.
gcc uses this for TREE_PUBLIC symbols that correspond to external
symbols.

In gfortran's (patched) case, the DIE for "lo" (as a child of the
DW_TAG_common_block) continues with a DW_TAG_member. The member's name 
is
"lo", its source coordinates reported, its type, and its
location (DW_AT_data_member_location), as required for any structure 
type.
The location is the offset in the common block (zero in this case).

In ifort's case, the DIE for "lo" (as a child of the 
DW_TAG_common_block)
continues with a DW_TAG_variable.  The variable's name is "lo", its
declaration location and its type is specified, and DW_AT_location 
provides
its location, given as _ecco_, and additionally is flagged as 
DW_AT_external
(as opposed to gfortran's DIE for the common block "ecco" which has this
flag).

This illustrates the difference in philosophy in building the DIE's.  In
(patched) gfortran's case, it is considered as a structure with fields,
as opposed to (unpatched) gfortran's and ifort's considering it a 
location
at some fixed offset from an external symbol.

common_check makes the test for a common symbol, which is flagged by it 
being
a VAR_DECL with TREE_PUBLIC, TREE_STATIC and which has a VALUE_EXPR.  
The
VALUE_EXPR is the offset of the identifier into the common block.  To 
make
this check, it has to determine whether the symbol is a MEM and whether 
it
has the form of an external symbol + or - an offset.  It walks the 
expression
tree to do this, using dw_expand_expr, and returns the relevant parts -
external symbol name and offset - when the verdict is rendered.

If the architecture of the code is at issue, please advise whether you
would prefer to see the DW_TAG_common_block / DW_TAG_variable more 
closely
integrated with the existing code.  At the development stage it was
clearer to code this alternative separately, rather than incorporating 
it
into add_location_or_const_value_attribute.  Nevertheless, I think 
subsuming
it will be ugly because logic to omit the type for the common block 
itself in
gen_variable_die will be required in addition to
add_location_or_const_value_attribute logic, so the determination 
whether a
variable is common-based or not will need to be made in two separate 
routines.