[llvm] r191792 - Debug Info: remove duplication of DIEs when a DIE is part of the type system
Manman Ren
mren at apple.com
Wed Oct 2 18:45:36 PDT 2013
> On Oct 2, 2013, at 4:13 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
>
>> On Wed, Oct 2, 2013 at 4:05 PM, Manman Ren <manman.ren at gmail.com> wrote:
>>
>>
>>
>>> On Wed, Oct 2, 2013 at 10:42 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>
>>>
>>>> On Tue, Oct 1, 2013 at 7:05 PM, Manman Ren <manman.ren at gmail.com> wrote:
>>>>
>>>> I am now a little confused about the mapping issue.
>>>> The patch replaces every getDIE of a Type, a SP, and a Static Member with DwarfDebug.getType|SP|StaticMemberDie.
>>>>
>>>> getDIE was used before creating a DIE, and it was also used before generating a reference to the DIE via ref4 or ref_addr.
>>>>
>>>> For the usage before creating a DIE, if we still use the CU mapping, we will create multiple DIEs for the same MDNode across CUs, if we replace it with the DwarfDebug mapping, we will not create multiple DIEs.
>>>>
>>>> For the usage before generating a reference, it does not make sense to use CU mapping if we only have a single DIE for the MDNode (i.e using DwarfDebug mapping before creating a DIE); it does not make sense to use DwarfDebug mapping if we already
>>>> have multiple DIEs for the MDNode.
>>>>
>>>> So it sounds to me, the usage of getDIE before creating a DIE determines whether we have multiple DIEs for a given MDNode and other usages of getDIE should either use CU mapping or DwarfDebug mapping, but not both for a given MDNode.
>>>
>>> OK, I think I'm understanding you here. Just throwing around thoughts, nothing terribly deeply analyzed yet, bu it might be easier to understand if we simply had CompileUnit::getDIE have a list of tags for which it would delegate to the cross-CU mapping. Then it'd be simply one mapping in DwarfDebug used from one place (getDIE) and would be hidden from all callers, keeping the rest of the code fairly consistent/simple... just a thought, at least.
>>
>> Sounds reasonable, I will work on that.
>>>
>>> But specifically you're just interested in sharing types between CUs, yes? The sharing of Subprograms is just really for sharing them when they're members of a type, yes?
>>
>> Yes.
>>> (I suppose subprograms could appear in multiple CUs if they were inline in a header and used in multiple CUs)
>>>
>>> Would it perhaps be easier to remove this patch and just use type units that I'm working on implementing at the moment?
>>
>>
>> This patch does two things:
>> 1> move the map from CompileUnit to DwarfDebug
>> 2> correctly use ref_addr if necessary (by using a worklist)
>>
>> There are some functionality overlapping between this patch and the type units, but there are certainly differences as well.
>
> I'm not so interested in a comparison of their implementation as I am in a comparison of their observable result to debug info.
>
>> Both need to have a map in DwarfDebug that maps type MDNodes to DIE(s), in your proposed patch,
>> DenseMap<const MDNode *, std::pair<uint64_t, SmallVectorImpl<DIE*>* > > TypeUnits
>> --> I am not quite sure why we need the pair, can you add some comments in your proposed patch? :)
>
> It was mostly just out as a straw man. The pair is a semi-descriminated union, the vector is your 'work list', I believe. While we're generating the type unit for some type T, the pair will have a non-null vector and an irrelevant uint64 (because we haven't finished generating the type, so we can't compute the hash for it). When a non-null vector is seen, DIEs that should be skeletons for the type unit are added to the vector. When we finish a type unit, we attach the signature to all those DIEs. From then on, whenever we're looking to build a skeleton DIE for a type, we have the signature in the uint64_t (because the vector pointer is null, we know the signature is valid) and we just attach it immediately.
>
>> In my patch, we have a DenseMap<const MDNode *, DIE*> MDTypeNodeToDieMap
>>
>> With type units, the across-CU references will be replaced with references to type units.
>> With this patch, we use ref_addr.
>>
>> For LTO's purpose, we don't need to calculate the hash since all we need is to make sure we only generate a single DIE for a given type MDNode.
>> Calculating the hash costs time :)
>
> Enough to matter? I wonder.
I wonder about that too. Can we do any measurement now?
>
> Of course type units will duplicate more things - they'll have the indirection of the type skeleton (a structure/class_type with a signature attribute), and duplicate member functions for implicit members (nested types, member template instantiations, and implicit special member functions) and function definitions. But I don't know just how significant that win will be/whether it'll be worth doing when we get most of the win from type units - maybe the extra won't be enough to justify the added complexity. (indeed this commit didn't come with any numbers about how much it helps)
>
>> I would like to revisit this (maybe revert this) later on when type units are working.
>
> I'd rather revert this before type units (& then revisit this afterwards) as it may complicate the type unit work.
What kind of complication do you see ?
Manman
> I think the extra benefit (using direct ref_addr, rather than signatures) of this approach over type units may be implementable via an incremental improvement to type units.
>
>>
>>>> + /// Similar to getCompileUnit, returns null when DIE is not added to an
>>>> + /// owner yet.
>>>> + DIE *checkCompileUnit();
>>>
>>> Why is this not just "getParent() == NULL"? Why do you need a walk for the DIE?
>> Yeah, that should work.
>> <-- It is possible that a DIE is added to a parent but the parent is not yet added to an owner yet, so we still have to walk along the parent chain.
>>
>> Thanks,
>> Manman
>>
>>>
>>> While I have discussed with Eric the idea that DIEs could be referenced cross-CU, I'm a bit hesitant to put this sort of thing in without further thought/consideration, and it seems like type units are a more general purpose solution to this and other issues anyway. (though they don't immediately address the inline non-member function example I mentioned above, those would still be duplicated across CUs - so if that's really important we might have to do something like this anyway)
>>>
>>>>
>>>> Thanks,
>>>> Manman
>>>>
>>>>
>>>>> On Tue, Oct 1, 2013 at 5:58 PM, Manman Ren <manman.ren at gmail.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On Tue, Oct 1, 2013 at 4:10 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>
>>>>>>>> The idea is that we're going to have references across-CUs? Do you have any idea if debuggers tolerate/handle this kind of referencing?
>>>>>>>
>>>>>>> Yes, the DIE will be referenced across-CUs. We have been using ref_addr for some cases,
>>>>>>
>>>>>> In which cases do we already do this?
>>>>>
>>>>> In DwarfDebug::updateSubprogramScopeDIE, when we add abstract_origin of a subprogram.
>>>>> I agree that we are now using ref_addr broadly for LTO builds.
>>>>>
>>>>>>
>>>>>>> and lldb is fine with it.
>>>>>>
>>>>>> lldb isn't the only DWARF consumer.
>>>>>
>>>>> Other debuggers should work with ref_addr.
>>>>>
>>>>>>
>>>>>>> I tried to move the mapping for MDNodes related to the type system, from CompileUnit to DwarfDebug, and I didn't include namespace in this commit.
>>>>>>> Theoretically, for everything that can be shared across CUs, we should add it to the DwarfDebug map to remove duplication.
>>>>>>
>>>>>> Then it would be nice to do this as a single, generic solution over all things that can be referenced rather than piecemeal with special cases for each kind of thing, I think.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> So a strawman alternative proposal would be: add all the MDNode/DIE mappings to both the local CompileUnit and 'global' DwarfDebug maps and have different lookup functions depending on the context - using the DwarfDebug mapping when we want to reference anything for any reason, and the CompileUnit mapping when we want to add children (eg: adding new things to a namespace, etc) or attributes.
>>>>>>>
>>>>>>> I don't quite get why we need two mappings for one MDNode.
>>>>>>
>>>>>> Because, say we have a namespace node - we need a CU-specific mapping so that when we need to add entities to the namespace (another type, global variable, etc) we can lookup the CU-local instance of the namespace. But whewhy we need two mappings for one MDNode.
>>>>>
>>>>> Because, say we have a namespace node - we need a CU-specific mapping so that when we need to add entities to the namespace (another type, global variable, etc) we can lookup the CU-local instance of the namespace. But when we only need to refer to the namespace (eg: for a namespace alias) we can lookup a cross-CU mapping.
>>>>
>>>> If we already have multiple DIEs for the same MDNode (that is why we keep a CU-specific mapping, right?), when referring to the namespace DIE, can we just use CU-specific mapping?
>>>> I am not familiar with how a namespace alias works, so don't quite know why we need a cross-CU mapping when referring to the namespace.
>>>>
>>>>>
>>>>> This is exactly what you've already implemented - the mappings you added to DwarfDebug are the second mapping - the first mapping already exists within the CompileUnit and is still used for the cases where we want to add members (at least I hope we are, because we need to do that in some cases).
>>>>
>>>> Oops, I moved the mapping from CompileUnit to DwarfDebug. Are you referring to "the namespace alias" by "some cases"?
>>>>
>>>>>
>>>>>> I tried to categorize MDNodes to two groups, one group belonging to CompileUnit (Variables etc that will be referenced in a single CU), the other group belonging to DwarfDebug (type system that can be shared across CUs).
>>>>>
>>>>> I don't believe that will be sufficient, but I could be wrong. Namespaces seem like a pretty solid example where a thing would need to be mapped in both a CU-specific and cross-CU way.
>>>>>
>>>>> For types, currently there are cases where we insert member DIEs into type DIEs lazily (related to the vtable debug info optimization, wherein we emit only a declaration of a type when that type is dynamic (has a vtable) and the vtable isn't emitted in this translation unit).
>>>>
>>>> Do we have an existing testing case for this? I can try to extend it to multiple CUs to check what will happen.
>>>>
>>>>> It might not be wrong to have us insert those member DIEs in an existing instance of the type in another compile_unit - but I'm hesitant to assume that's good and right.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> But I'm not sure how effective this would be - like I said, might need some thought/discussion to understand your solution & consider alternatives.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Sometimes, when we try to add an attribute to a DIE, the DIE is not yet added
>>>>>>>>>> to its owner yet, so we don't know whether we should use ref_addr or ref4.
>>>>>>>>>> We create a worklist that will be processed during finalization to add
>>>>>>>>>> attributes with the correct form (ref_addr or ref4).
>>>>>>>>>
>>>>>>>>> Do you have an example of this as a test case? (I really wish we had an easy way to see test coverage - so I could just tell at a glance whether the worklist code is being exercised by your test cases... oh well)
>>>>>>>>
>>>>>>>> Yes, we don't always immediately add a newly-created DIE to a parent. So it is possible to have DIEs without an owner when calling addDIEEntry.
>>>>>>>> One interesting example is:
>>>>>>>> getOrCreateTypeDIE --> addToContextOwner --> getOrCreateContextDIE (the original type DIE can be referenced during the construction of the context, and when that happens, the type DIE does not have an owner yet)
>>>>>>>
>>>>>>> But you know that the reference to that type DIE will be from within the same CU as the type itself - since it's a child. So you know the reference will be a ref4, no?
>>>>>>
>>>>>> For this case, yes, it is going to be ref4. But I am not sure whether we can say the same for all cases when a DIE is not added to an owner.
>>>>>> It also means we need to pass in an extra flag telling addDIEEntry that we are sure it is a ref4 and it is not an easy task to track the flag across the chain of
>>>>>> function calls: getOrCreateTypeDIE --> addToContextOwner --> getOrCreateContextDIE --> ... --> addDIEEntry.
>>>>>
>>>>> Why would you need an extra flag? If we know that any time we come back around to the same entity that isn't already inserted (ie: the condition you already know because you use it to decide to add things to a work list) it must be a local reference, so you answer the question immediately rather than using a work list.
>>>>
>>>> How can we tell that we are coming back around to the same entity? Maybe I misunderstood something here.
>>>> When we see a DIE without an owner and add things to a work list, I don't think there is a guarantee that the DIE and the Entry belong to the same CU.
>>>>
>>>> Thanks,
>>>> Manman
>>>>
>>>>>
>>>>> I could be wrong, but I want to have a good reason to do it some other way not just "maybe sometimes this might not happen".
>>>>>
>>>>> - David
>
>
>
>
>
>
>
> div>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131002/3b0835d1/attachment.html>
More information about the llvm-commits
mailing list