[Lldb-commits] [PATCH] D138176: [dsymutil] Fix assertion: (Ref > InputDIE.getOffset()), function cloneDieReferenceAttribute

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D138176#3997559 <https://reviews.llvm.org/D138176#3997559>, @avl wrote:

> In D138176#3995675 <https://reviews.llvm.org/D138176#3995675>, @JDevlieghere wrote:
>
>> Last week I debugged this further. Here's what I believe is going on. Consider the following simplified example with two CUs and a type `Foo`.
>>
>>   0x01: DW_TAG_compile_unit
>>   
>>   0x02: DW_TAG_class_type
>>           DW_AT_containing_type (0x11)
>>           DW_AT_name            ("Foo")
>>   
>>   0x03: DW_TAG_pointer_type
>>           DW_AT_type            (0x02 "Foo") 
>>   
>>   ...
>>   
>>   0x10: DW_TAG_compile_unit
>>   
>>   0x11: DW_TAG_class_type
>>          DW_AT_containing_type (0x11)
>>          DW_AT_name            ("Foo")
>>   
>>   ...
>>
>> Assume that in the first compile unit, we have a relocation that causes us to keep the pointer to foo.
>>
>> During the analysis phase, when looking for DIEs to keep, we encounter the definition of `Foo`. Through an ODR attribute (in this case `DW_AT_containing_type`) we see the same definition of `Foo` in the second CU. We decide to keep the definition in CU2 (0x11) and mark the decl context as having a canonical ODR DIE. As a result, we don't keep the definition in CU1.
>>
>> During the emission phase, we encounter the pointer to foo again (0x03). As part of cloning, we go over its attributes and encounter 0x02 through the `DW_AT_type`. The canonical DIE in CU2 hasn't been cloned yet, so the `RefInfo.Clone` is false and so is the DeclContext's canonical DIE offset. As a result, we trip off the assertion because the reference (0x2) is less than the input DIEs offset (0x3) and therefore should already have been emitted. Instead, we should note the (forward) reference and fix it up later, which is what this patch does.
>
> I tryed to create a test according to this description. But, in its current state it does not lead to the assertion. So the real case, is probable more complicated.
> In this example the type from the first compile unit is always cloned.
> F25630194: odr-two-units-in-single-file10.test <https://reviews.llvm.org/F25630194>

Yes, I tried as well and I wasn't able to trick the algorithm into keeping the the type from the second CU.

>> In the patch I called it a backward reference, because the original DIE comes before the DIE being emitted now, but as shown here, it really is a forward reference, so maybe it's better to keep the original naming.
>>
>> This reproduces for several internal projects. The smallest one is an LTO build with 1.8 million lines of IR. I've been trying to reduce it with `llvm-reduce`, `delta` and `bugpoint` since last Thursday, but all of these tools are struggling with DI metadata and the size of the file. I'll give it another day or so but I'm not holding my breath this will generate anything that can be used as a test.
>>
>> With the reduction running in the background, I've been trying to hand-craft a test that exposes the behavior I've described here, but with every DIE it becomes exponentially harder to trick dsymutil's algorithm into traversing the DIEs in the order you want.
>
> Though I think it would be useful to finally have a test case for this situation, it looks OK to approve this patch. If there would be reference to the uncloned DIE then this assertion should catch it:
>
>   void CompileUnit::fixupForwardReferences() {
>     for (const auto &Ref : ForwardDIEReferences) {
>       DIE *RefDie;
>       const CompileUnit *RefUnit;
>       PatchLocation Attr;
>       DeclContext *Ctxt;
>       std::tie(RefDie, RefUnit, Ctxt, Attr) = Ref;
>       if (Ctxt && Ctxt->hasCanonicalDIE()) {
>         assert(Ctxt->getCanonicalDIEOffset() &&
>                "Canonical die offset is not set");   <<<<<<<<<<<<<<<<
>         Attr.set(Ctxt->getCanonicalDIEOffset());
>       } else
>         Attr.set(RefDie->getOffset() + RefUnit->getStartOffset());
>     }
>   }

Yup, exactly.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138176/new/

https://reviews.llvm.org/D138176