Re: [PATCH v2] gdb/dwarf: fix reading subprogram with DW_AT_specification (PR gdb/26693)

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2020-10-21 4:42 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon>     0x00000014:   DW_TAG_subprogram
> Simon>                     DW_AT_name [DW_FORM_string]     ("apply")
>
> Simon>     0x0000001b:   DW_TAG_subprogram
> Simon>                     DW_AT_specification [DW_FORM_ref4]      (0x00000014 "apply")
>
> gdb currently goes through a lot of work to support DWARF like this, but
> I wish it didn't have to.  It complicates the DWARF reader -- but IIRC
> both gcc and clang just emit specializations with names like
> "apply<int>".  So, I wonder if we could drop support for this at some
> point.

The object that contained this kind of DWARF, that made GDB crash, was
compiled with ICC.  So unless we decide to drop support for ICC, I don't
think we can get rid of it any time soon.

> Simon> Since the caller of load_full_comp_unit knows about the existing
> Simon> dwarf2_cu object for the CU we are reading (the one load_full_comp_unit
> Simon> is expected to find), we can simply make it pass it down, instead of
> Simon> having load_full_comp_unit look up the per-objfile map.
>
> Sounds reasonable.
>
> Simon> @@ -9231,12 +9233,12 @@ die_eq (const void *item_lhs, const void *item_rhs)
> Simon>  static void
> Simon>  load_full_comp_unit (dwarf2_per_cu_data *this_cu,
> Simon>  		     dwarf2_per_objfile *per_objfile,
> Simon> +		     dwarf2_cu *existing_cu,
>
> I think a comment explaining why this is needed would be really helpful.
>
> The patch looks good to me.  Thank you very much for the detailed
> explanation.

I would write this, does that sound good?

/* Load the DIEs associated with PER_CU into memory.

   In some cases, the caller, while reading partial symbols, will need to load
   the full symbols for the CU for some reason.  It will already have a
   dwarf2_cu object for THIS_CU and pass it as EXISTING_CU, so it can be re-used
   rather than creating a new one.  */

Thanks for the review.

Simon