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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
