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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2019-10-15 12:27 p.m., Kevin Buettner wrote:
> Hi Simon,
> 
> You raise some good questions.  I modified the test associated with
> this bug so that two different CUs attempt to inherit a third CU. 
> This has turned up another bug in GDB which I spent the rest of the
> day looking at.  (GDB goes into an infinite loop!)
> 
> I'll make the following observations, however...
> 
> - method_list is used solely for keeping track of C++ methods for
> delayed physname computations.
> 
> - method_list is cleared in process_full_comp_unit(),
> process_full_type_unit(), and compute_delayed_physnames().
> That latter function(), compute_delayed_physnames(), is called
> after DIE processing in the first two functions.  So method_list
> is made to be empty prior to DIE processing and then made empty
> (as a result of delayed physname computation) again after DIE
> processing (in the above mentioned functions).
> 
> So, what is the right thing to do with regard to method_list for
> inherit_abstract_dies()?
> 
> Yesterday, as part of my investigations, I made inherit_abstract_dies()
> call compute_delayed_physnames in place of the code in my posted
> patch.  That also works, at least for my test case.  I'll note that
> for my original (posted) patch, compute_delayed_physnames will be
> called with the inheriting CU.  In the alternate approach (in which
> compute_delayed_physnames is called from inherit_abstract_dies),
> compute_delayed_physnames is called with the origin CU.  I don't
> yet know which is more correct or if there are cases where it'll
> make a difference.
> 
> So... I'm continuing my investigations and will let you know when
> I have some answers.  In the interim, if anyone has some insights
> about these matters, I'd very much like to hear them.
> 
> Kevin
> 
Hi Kevin,

I've spent a bit of time to better understand what inherited abstract DIEs
are and to step in that code.

I observed that when a DIE A inherits from another DIE X, we process DIE X completely
from scratch, going through process_die, creating the struct type instances, adding
items to method_list for delayed name computation.  If another DIE B inherits from
DIE X, then we'll go through process_die again, creating another struct type, adding
items to method_list for delayed name computation again.  There is no (and there
shouldn't be) any state shared between the processing of X while inherited by A and
the processing of X while inherited by B.  In theory, the compiler could have decided
to have two completely separate DIE trees under A and under B.  But to reduce
duplication, it decided to use that abstract origin thing, so A and B could share some
children.  However, we should still conceptually treat them as separate trees.  Btw,
I'm writing all this so I assimilate it better, but also so you can tell me if you
think I'm wrong.

I am confident that what you do in this patch is right.  Let's say A, B and X, the same
DIEs as above, are all in different CUs.  Before your patch, while processing A, the
delayed method info for things described in X would be put in X's CU's method_info list
and stay there, and it would never get used.  When processing B, the delayed method info
for things described in X would also be put in X's CU's method_info list and never get
used.  If, we then happen to process the CU that contains X, we'll start by clearing that
CU's method_info list in process_full_comp_unit and start from scratch.

With your patch, the entries generated while parsing X as inherited by A get moved to A's
CU, and the entries generated while parsing X as inherited by B get transferred to B's CU.
That's as if A and B had their own independent subtree, which is what we want.  If we then
happen to process X's CU, that CU's method_list will be empty, as expected.

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.

Also, note this in inherit_abstract_dies:

  /* We're inheriting ORIGIN's children into the scope we'd put DIE's
     symbols in.  */
  origin_previous_list_in_scope = origin_cu->list_in_scope;
  origin_cu->list_in_scope = cu->list_in_scope;

  ...

  origin_cu->list_in_scope = origin_previous_list_in_scope;

I think this code is solving the same kind of problem for `list_in_scope`, but differently:
it temporarily moves A's CU's list in X's CU.  Here too, I think it would be clearer if
`list_in_scope` wasn't attached to a CU, but something passed down the stack.  Again, we
wouldn't have to do anything special in inherit_abstract_dies, just pass that list down.

One last thing, I think that those method_list.clear() calls in process_full_comp_unit and
process_full_type_unit are unnecessary, and are confusing because they suggest that there
might be something in there.  When we start processing a CU with either of these functions,
I don't think there should ever be some content.  I'd try to change them to

  gdb_assert (cu->method_list.empty ());

Simon