Re: [RFC/dwarf-2] Add support for included files

[Joel Brobecker <brobecker at gnat dot com>]

> I actually have still more comments:

Jim, you'll be happy, your comments make the patch neater :-).
Once you get to know the data structures a little bit more
(I really should have studied them a little harder before working
on this), everything seems to be falling into place almost naturally...

Attached is a revised version of the patch, up to date as of now,
and incorporating all your comments, except one which I wasn't sure
about.

> - Rather than passing 'line_offset' by reference to read_partial_die,
>   could you add a line_offset field to 'struct partial_die_info, and have
>   read_partial_die set that?  That would seem more consistent with the
>   rest of what read_partial_die does.  Then
>   'dwarf2_build_include_psymtabs' should take a pointer to the CU die.

This brings a nice cleanup.

>   That's assuming Daniel doesn't see any problem enlarging
>   partial_die_info.  If he does, maybe we could add the fields to
>   'struct dwarf2_cu' instead: it's a little odd to have
>   'read_partial_die' set the cu's fields directly, but not horribly
>   so, as long as the die's tag is DW_TAG_compile_unit.

partial_die_info seems better suited to hold that information.
Hopefully Daniel won't object.

> - Is it really okay to pass NULL to dwarf_decode_lines for comp_dir?
>   Won't the filenames of the partial symbol tables be different from
>   those of the symbol tables?  To fix this, read_partial_die would
>   have to check for DW_AT_comp_dir attributes, too.

I don't think we need to worry about the comp_dir when building
the psymtabs for included files. The filename used for the psymtab
is the same as the filename used to build the symtab (checked it
by inspecting start_subfile() and end_symtab()). The only thing
we could gain from computing the comp_dir as well during that phase
is to compute the psymtab fullname. But then comp_dir is used
to set the symtab dirname, not to compute the fullname. So it doesn't
help improve the consistency between psymtab and symtab.

I think that part is fine...

... At least for now :-).

> - It seems to me the file_is_included array should be in struct
>   line_header, with add_file_name managing its allocation /
>   initialization in parallel with file_names.  It's already the case
>   that that structure is mutated by the act of processing the line
>   number program, so I don't see that it's a serious change to the
>   usage patterns of that structure type.

Good idea. I included it, and the code becomes one notch cleaner.

> - I'd rather see the call to dwarf2_build_include_psymtabs before the
>   assignment to info_ptr, not after.  Not that it matters much.

Done.

> - Thanks for fixing the comments on dwarf_decode_lines --- especially
>   the references to arguments that had been missing for some time now.

My pleasure :-).

2004-04-30  J. Brobecker  <brobecker@gnat.com>

        * dwarf2read.c (line_header): Add new included_p field in
        field file_names.
        (partial_die_info): New field has_stmt_list. New field line_offset.
        (dwarf2_create_include_psymtab): New function.
        (dwarf2_build_include_psymtabs): New function.
        (add_file_name): Add forward declaration. Initialize new field.
        (dwarf_decode_lines): Add new parameter. Enhance this procedure
        to be able to determine the list of files included by the
        given unit, and build their associated psymtabs.
        (dwarf2_build_psymtabs_hard): Build the psymtabs for the included
        files as well.
        (psymtab_to_symtab_1): Build the symtabs of all dependencies as well.
        (read_file_scope): Update call to dwarf_decode_lines.
        (read_partial_die): Handle DW_AT_stmt_list attributes.

Tested on x86-linux, no regression. Fixes the two sep.exp KFAILS.
OK to apply?

Thanks,
-- 
Joel

Attachment: separate2.diff
Description: Text document