Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2019-10-17 9:08 p.m., Kevin Buettner wrote:
> On Thu, 17 Oct 2019 01:30:12 -0400
> Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
>> On 2019-10-16 11:54 p.m., Simon Marchi wrote:
>>> I think that what's confusing in all this is the fact that the
>>> method_info list is currently attached to a particular CU. 
>>> Instead, I think it should be attached to the operation of
>>> processing of a CU, and used to collect all delayed method infos
>>> while processing that CU, even if some of these infos come from
>>> inherited DIEs from another CU.  Concretely, it would mean to have
>>> a local instance of std::vector<delayed_method_info> in
>>> process_full_comp_unit/process_full_type_unit and to pass it by
>>> pointer/reference through the call stack to any code who might
>>> want to append to it.  We wouldn't have to do anything special in
>>> inherit_abstract_dies, just pass this reference to the list down
>>> the stack.  I don't know how feasible it would be in practice to
>>> do that change, maybe it's too much work or would end up ugly. 
>>> I'll give it a try.  But your patch gives essentially the same
>>> result, and works with what we have today. 
>>
>> A little follow up to the above.
>>
>> I prototyped that change here in a WIP patch (so, not intended to be
>> reviewed):
>>
>> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/128
>>
>> I got no regression in gdb.dwarf2.  However, it's a bit invasive. 
>> If we want to pass other objects in the same fashion, it will
>> quickly become very heavy.
>>
>> What we could do though, is to introduce a new type (e.g.  struct
>> dwarf2_cu_processing_context) and pass that around instead.  Its
>> lifetime would be the duration of process_full_comp_unit /
>> process_full_type_unit, just like std::vector in the patch above,
>> but could contain many fields.
> 
> I looked over your patch; it looks reasonable to me.  If we go that
> route, I like the idea of introducing a dwarf2_cu_processing_context
> struct.  (But see below for some later misgivings that I have/had.)

Ok, I can try to make something cleaner, but I don't know when it would be ready,
and I wouldn't want that to block the GDB 9.1 branch creation (or release) for
that.  Would you like to still push your patch (or a perhaps updated version of it)
so that we have the fix in GDB 9.1?

>> I found something potentially problematic though (applies to both
>> your and my patch).  When we process the delayed_method_info objects
>> in compute_delayed_physnames, we call:
>>
>> dwarf2_physname (mi.name, mi.die, cu);
>>
>> mi.die could be a die coming from X's CU (to keep the example from
>> my previous message), but the cu in the call above is A's CU (the CU
>> we are processing).  I am pretty sure that this function (and what
>> it calls) expect the passed DIE to come from the passed CU.  If they
>> don't match, I guess we could have some bad surprises.
> 
> I spent a little time trying to figure out what CU is used for in
> dwarf2_physname() and its callees.  What I noticed most is that
> cu->language is used to figure out some appropriate thing to do.
> I seem to remember Keith running into problems with mismatched
> languages in different CUs.  So there might be problems if the
> languages associated with the CUs are different.

Yeah, I remember this saga, but I did not really follow it.  Keith, would
you have an idea of what would be the right CU to pass here?

> There are also obstacks associated with each CU.  I noticed this
> call in dwarf2_compute_name():
> 
> 		  dwarf2_const_value_attr (attr, type, name,
> 					   &cu->comp_unit_obstack, cu,
> 					   &value, &bytes, &baton);
> 
> For this matter, I think that we want the inheriting CU's obstack to
> be used, so this might be okay.  However, due to potentially differing
> lifetimes, there could be problems if data structures allocated in one
> obstack point to data structures in another; I don't know if that could
> happen or not.

Hmm right.  Maybe it's fine in practice since the lifetimes of the two dwarf_cu
objects are probably similar.  But I agree it's not ideal.

> 
> There are also errors which fetch the objfile name via...
> 
>   objfile_name (cu->per_cu->dwarf2_per_objfile->objfile)
> 
> I don't think this will cause a crash or anything, but it has
> the potential to misidentify the problematic objfile.

Aren't the two CUs necessarily in the same objfile?

> There may be other concerns too; I'm certain that I didn't look at all
> of the ways that CU is used in dwarf2_physname and its callees.

I don't think it's humanly possible to manually check all the possible branches
this code can take.  I say, let's do a quick pass to check for the obvious (like
what you found above), but otherwise I'm fine with this patch, it already makes
things better than they are now.

> Also, while looking at all of that, it occurred to me that struct
> dwarf2_cu already contains elements of a CU processing context.  E.g. 
> the rust_unions member looks very similar to method_list.  Plus the
> comment for dwarf2_cu says "Internal state when decoding a particular
> compilation unit."  That led me to wonder if it truly makes sense to
> pass around both a struct dwarf2_cu and a struct
> dwarf2_cu_processing_context.  (I don't know the answer, but it's
> something to mull over...)

I'll check closer when actually trying to write that patch, but for now
I believe that it's fine to pass both objects, as they have different
meanings.

The dwarf2_cu_processing_ctx would contain things related to the root CU
we are parsing.  The dwarf2_cu would represent things related to the CU
we are in right now.  So when crossing a CU boundary (like in the
abstract origin case that started this thread), the passed
dwarf2_cu_processing_ctx wouldn't change, but the passed dwarf2_cu would
change.

Simon