This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs
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