This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix potential CU mismatch for attachRangesOrLowHighPC
ClosedPublic

Authored by DianQK on Oct 16 2022, 7:51 AM.

Details

Summary

When a CU attaches some ranges for a subprogram or an inlined code, the CU should be that of the subprogram/inlined code that was emitted.
If not, then these emitted ranges will use the incorrect base of the CU in emitRangeList.

A reproducible example is:
When linking these two LLVM IRs, dsymutil will report no mapping for range or inconsistent range data warnings.

foo.swift

swift
import AppKit.NSLayoutConstraint

public class Foo {
    
    public var c: Int {
        get {
            Int(NSLayoutConstraint().constant)
        }
        set {
        }
    }

}

main.swift

swift
// no mapping for range
let f: Foo! = nil
// inconsistent range data
//let l: Foo = Foo()

I made an Xcode project for this example.

Diff Detail

Event Timeline

DianQK created this revision.Oct 16 2022, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2022, 7:51 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
DianQK requested review of this revision.Oct 16 2022, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2022, 7:51 AM
DianQK edited the summary of this revision. (Show Details)Oct 16 2022, 8:29 AM

If we wanted to fix this whole class of errors, maybe it's time we moved most of the DIE creation operations back to DwarfDebug - and any unit-specific operations would then have to explicitly look up the right unit from the DIE before they could be used. This would reduce the chance of accidentally using the wrong unit? (it'd cause more unit lookups - which should only be walking up the DIE chain. - if we make sure every DIE is added to its parent early enough, which seems plausible)

I tried, for instance, removing the DwarfUnit lookup in getOrCreateSubprogramDIE where it calls applySubprogramAttributes and sinking that down into the places where it was needed - which was only, so far as test coverage would indicate, at the getOrCreateSourceID calls (since in LTO we do produce separate line tables for each CU - so looking up/adding the file name in the right CU is necessary). That then uncovered a few cases where that code was being used with un-parented DIEs - specifically imported entities and variables. I was able to add those to the right parents earlier with a few changes. https://reviews.llvm.org/P8297

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
450

Could you use static_cast<DwarfCompileUnit*>(SPDie->getUnit()) here, instead of DD->lookup? (looks like that's what getOrCreateSubprogramDIE does?

& maybe that's cheap enough to use up in the caller, and avoid having to thread the ContextCU up and down through this stack?

Maybe even just build that CU change into attachRangesOrLowHighPC the same way it's built into getOrCreateSubprogramDIE?

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
231

ContextCU should probably be passed by reference-to-pointer - since you've updated the two callers so they don't pass null, there's no need for the implementation to consider whether the caller might've passed null, I think?

Alternatively, I guess, the caller could perform the lookup again, since they got the DIE back as a result, right? Maybe that's better than threading this through all the construct* abstractions to get it back out again?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2266

Guessing this addition is basically untested - could you add a Split DWARF test to cover this? (& verify that the test fails without these changes applied)

llvm/test/DebugInfo/Generic/cross-cu-inlining-ranges.ll
4

Maybe some more detailed description of the particular circumstances where it's difficult to attach to the right context/explains why this test has the features it has?

(does it need a global variable? Does that global variable need a non-trivial type (if the global variable's necessary, but maybe it could be "int" rather than a structure type?) there's quite a few calls (both all the calls to f in main, and multiple in foo)

& maybe a snippet of source code to help explain the situation?

& I'd still be curious if this can be reproduced with C or C++/clang produced IR - might be more generally explicable/understandable by LLVM developers who all work in C++ at least.

9–13

Any particular reason you need 5 calls rather than, say, 1?

DianQK updated this revision to Diff 468201.Oct 17 2022, 7:48 AM

Update code

If we wanted to fix this whole class of errors, maybe it's time we moved most of the DIE creation operations back to DwarfDebug - and any unit-specific operations would then have to explicitly look up the right unit from the DIE before they could be used. This would reduce the chance of accidentally using the wrong unit? (it'd cause more unit lookups - which should only be walking up the DIE chain. - if we make sure every DIE is added to its parent early enough, which seems plausible)

Thank you very much for your patient and detailed comments.
I think it's a good idea.

And I solved part of the problem. The rest may need more additional time (Maybe tomorrow? I'm not sure about my free time).

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
450

Probably not, after trying your patch, I can still find some cases in attachRangesOrLowHighPC where the unit does not exist. Maybe we need more changes?

And getUnit always return DwarfCompileUnit in attachRangesOrLowHighPC? I'm not quite sure about the logic here yet.

llvm/test/DebugInfo/Generic/cross-cu-inlining-ranges.ll
4

Thanks to cross-cu-inlining-2.ll, I did it! I have removed the global variable.

The global variable is used to ensure a fixed order of DIE creation.
(Just because I found this issue from the example of global variables.)

I'm not sure if this can be reproduced with C or C++/clang produced IR. (I'll try again later.)

9–13

I use this when debugging something. I deleted those.

DianQK updated this revision to Diff 468220.Oct 17 2022, 8:36 AM
DianQK marked an inline comment as done.

Add more detailed description for test

dblaikie added inline comments.Oct 17 2022, 6:01 PM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
450

Probably not, after trying your patch, I can still find some cases in attachRangesOrLowHighPC where the unit does not exist. Maybe we need more changes?

But each of the places where it doesn't have a unit is a risk that it gets attached to the wrong thing, right? (but not a guaranteed bug, so yeah, maybe not the thing we should hold this all up on)

And getUnit always return DwarfCompileUnit in attachRangesOrLowHighPC? I'm not quite sure about the logic here yet.

Yeah, that should be fine - DwarfTypeUnits can't have address ranges, so anything we're attaching ranges to should be a CU.

In any case, could we still avoid all the DwarfCompileUnit *&ContextCU additions and have the caller in DwarfDebug::endFunctionImpl lookup the DwarfCompileUnit from the returned DIE the same way it's being done here ^? It'd avoid a lot of threading things back and forth through all these construct APIs?

657

Looks like some of these sites didn't get updated to

1053

This whole chain that takes the ContextCU by pointer (rather than ref to pointer) - as an input only parameter, probably doesn't neeed to be changed. It should probably call with ContextCU as this instead, eg: somewhere up the stack, it should be ContextCU->... insntead of otherCU->...(..., ContextCU);?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2266

Ping here ^

2266–2267

Also, I think the right thing to do here is to call constructSubprogramScopeDIE on the ContextSkelCU maybe, rather than on SkelCU?

DianQK edited the summary of this revision. (Show Details)Oct 17 2022, 6:57 PM
DianQK marked an inline comment as done.Oct 17 2022, 7:35 PM
DianQK added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
450

But each of the places where it doesn't have a unit is a risk that it gets attached to the wrong thing, right? (but not a guaranteed bug, so yeah, maybe not the thing we should hold this all up on)

Yeah, so even if it's not a bug, bug if any DIE can have a unit (add by parent chain?) after DIE::get(DIEValueAllocator, foo), any subsequent operations on the unit will be friendly. So for this change, maybe we can create another Diff?

Yeah, that should be fine - DwarfTypeUnits can't have address ranges, so anything we're attaching ranges to should be a CU.

Thanks for your explain.

In any case, could we still avoid all the DwarfCompileUnit *&ContextCU additions and have the caller in DwarfDebug::endFunctionImpl lookup the DwarfCompileUnit from the returned DIE the same way it's being done here ^? It'd avoid a lot of threading things back and forth through all these construct APIs?

I will try later.

657

DwarfCompileUnit &ContextCU? I'll give it a try.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2266

I didn't miss it here. (I just don't have time to double-check this. During the working day, I may only have an hour on this thing.)

Maybe I also need to understand how a Split DWARF would be different.

I can check all the "Done" checkboxes to determine if I've missed something.

DianQK updated this revision to Diff 468529.Oct 18 2022, 7:07 AM
DianQK marked an inline comment as done.

Remove ContextCU as an argument

DianQK marked 3 inline comments as done.Oct 18 2022, 7:10 AM

Mark something as completed.

dblaikie added inline comments.Oct 18 2022, 8:48 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1021–1024

Hmm, I'm not following - can you help me understand why this got pulled out of updateSubprogramScopeDIE?

1022–1023

Could use auto here, since the type is explicit in the static_cast on the right hand side. Might make it fit in a single line.

DianQK updated this revision to Diff 468725.Oct 18 2022, 3:44 PM

Use auto

DianQK marked an inline comment as done.Oct 19 2022, 7:12 AM
DianQK added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1021–1024

I need get SPDie to get ContextCU before attachRangesOrLowHighPC.
The attachRangesOrLowHighPC call in updateSubprogramScopeDIE and later createAndAddScopeChildren.

Maybe I could use the following code:

DIE *SPDie = getOrCreateSubprogramDIE(Sub, includeMinimalInlineScopes());
auto *ContextCU = static_cast<DwarfCompileUnit *>(SPDie->getUnit());
DIE &ScopeDIE = ContextCU->updateSubprogramScopeDIE(Sub);

I tried. It fails at cross-cu-inlining-2.ll (maybe because call getOrCreateSubprogramDIE in different CU?).

Dwarf before the change is:

0x00000038:       DW_TAG_subprogram [5] * (0x0000002b)
                    DW_AT_low_pc [DW_FORM_addr]	(0x0000000000000010 ".text")
                    DW_AT_high_pc [DW_FORM_data4]	(0x00000001)
                    DW_AT_frame_base [DW_FORM_exprloc]	(DW_OP_reg7 RSP)
                    DW_AT_name [DW_FORM_strp]	( .debug_str[0x00000036] = "bar")
                    DW_AT_decl_file [DW_FORM_data1]	("A.swift")
                    DW_AT_decl_line [DW_FORM_data1]	(21)
                    DW_AT_external [DW_FORM_flag_present]	(true)

After:

0x00000038:       DW_TAG_subprogram [5]   (0x0000002b)
                    DW_AT_name [DW_FORM_strp]	( .debug_str[0x00000036] = "bar")
                    DW_AT_decl_file [DW_FORM_data1]	("A.swift")
                    DW_AT_decl_line [DW_FORM_data1]	(21)
                    DW_AT_external [DW_FORM_flag_present]	(true)

0x0000003f:       DW_TAG_subprogram [6] * (0x0000002b)
                    DW_AT_low_pc [DW_FORM_addr]	(0x0000000000000010 ".text")
                    DW_AT_high_pc [DW_FORM_data4]	(0x00000001)
                    DW_AT_frame_base [DW_FORM_exprloc]	(DW_OP_reg7 RSP)

I think it's ok. But the old version is better?

dblaikie accepted this revision.Oct 21 2022, 5:14 PM

Looks good - thanks for your patience with the review.

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1021–1024

Right, the old version is better. It could be made to work. Something like this:

diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
index f2551e1f3419..b2fe2cf396e1 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
@@ -443,8 +443,14 @@ void DwarfCompileUnit::attachLowHighPC(DIE &D, const MCSymbol *Begin,
 // Find DIE for the given subprogram and attach appropriate DW_AT_low_pc
 // and DW_AT_high_pc attributes. If there are global variables in this
 // scope then create and insert DIEs for these variables.
-DIE &DwarfCompileUnit::updateSubprogramScopeDIE(const DISubprogram *SP,
-                                                DIE *SPDie) {
+DIE &DwarfCompileUnit::updateSubprogramScopeDIE(const DISubprogram *SP) {
+  DIE *SPDie = getOrCreateSubprogramDIE(SP, includeMinimalInlineScopes());
+  auto *ContextCU = static_cast<DwarfCompileUnit *>(SPDie->getUnit());
+  return ContextCU->updateSubprogramScopeDIEImpl(SP, SPDie);
+}
+
+DIE &DwarfCompileUnit::updateSubprogramScopeDIEImpl(const DISubprogram *SP, DIE *SPDie) {
+
   SmallVector<RangeSpan, 2> BB_List;
   // If basic block sections are on, ranges for each basic block section has
   // to be emitted separately.
@@ -1013,9 +1019,8 @@ sortLocalVars(SmallVectorImpl<DbgVariable *> &Input) {
 
 DIE &DwarfCompileUnit::constructSubprogramScopeDIE(const DISubprogram *Sub,
                                                    LexicalScope *Scope) {
-  DIE *SPDie = getOrCreateSubprogramDIE(Sub, includeMinimalInlineScopes());
-  auto *ContextCU = static_cast<DwarfCompileUnit *>(SPDie->getUnit());
-  DIE &ScopeDIE = ContextCU->updateSubprogramScopeDIE(Sub, SPDie);
+  DIE &ScopeDIE = updateSubprogramScopeDIE(Sub);
+  auto *ContextCU = static_cast<DwarfCompileUnit *>(ScopeDIE.getUnit());
 
   if (Scope) {
     assert(!Scope->getInlinedAt());
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
index 0b0d97e74bc4..d346c79b5f7b 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
@@ -191,7 +191,8 @@ public:
   /// DW_AT_low_pc and DW_AT_high_pc attributes. If there are global
   /// variables in this scope then create and insert DIEs for these
   /// variables.
-  DIE &updateSubprogramScopeDIE(const DISubprogram *SP, DIE *SPDie);
+  DIE &updateSubprogramScopeDIE(const DISubprogram *SP);
+  DIE &updateSubprogramScopeDIEImpl(const DISubprogram *SP, DIE *D);
 
   void constructScopeDIE(LexicalScope *Scope, DIE &ParentScopeDIE);

But yeah, since this is the only caller, seems simpler to do it here as you've done. Thanks for walking me through it.

This revision is now accepted and ready to land.Oct 21 2022, 5:14 PM
DianQK marked 4 inline comments as done.Oct 22 2022, 6:20 AM

Thank you very much for your help!

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1021–1024

Thanks, I have a patch added a constructSubprogramScopeDIEImplused to reduce ContextCU using that was not published, Because I found some code add *Impl doing similar things.
But only two locations need ContextCU, so I removed this.

But why did I forget to use this on updateSubprogramScopeDIE? I'm being foolish.

DianQK updated this revision to Diff 469891.Oct 22 2022, 6:21 AM
DianQK marked an inline comment as done.

add *Impl

This revision was landed with ongoing or failed builds.Oct 22 2022, 6:26 AM
This revision was automatically updated to reflect the committed changes.