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

[Kevin Buettner <kevinb at redhat dot com>]

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.)

> 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.

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.

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.

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.

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...)

Kevin