[DWARF][codegen] Fix for Aranges when split inlining is present

[ayermolo]

Summary

When we enable -fsplit-dwarf-inlining we end up with two entries
in .debug_aranges for each CU. Because it processes Skeleton CU
inline information and DWO CU.

Furthermore address calculations were incorrect because we were processing sections in Skeleton CU.

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2941–2942 ↗

(On Diff #405478)

Should we avoid putting these entries in ArangeLabels to begin with, rather than filtering them out here?

llvm/test/DebugInfo/X86/fission-inline-aranges.ll

12–32

Does this need templates, using declarations, and if/scopes, etc? Or could it be simplified a bit? If it can't be, might be worth some explanatory text about why all these features are needed to tickle the bug?

[ayermolo]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2941–2942 ↗

(On Diff #405478)

I looked at where this is added, and I think it would involve passing a flag through multiple levels of API. I wasn't sure what would be more acceptable. Just cleaning it up here, or doing that. This is more self contained.

[ayermolo]

llvm/test/DebugInfo/X86/fission-inline-aranges.ll

12–32

I re-purposed fission-inline.ll. Maybe just add extra checks there?

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2941–2942 ↗

(On Diff #405478)

Could you roughly summarize the layers of API it'd have to go through? There might be some nice way to structure it.

llvm/test/DebugInfo/X86/fission-inline-aranges.ll

12–32

Be best to make something simpler if possible, I think.

What's the DWARF (or source) that's of interest/necessary to reproduce the issue?

[ayermolo]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2941–2942 ↗

(On Diff #405478)

Umm, nevermind. I looked at that code with fresh set of eyes and I think only API that needs to be modified is addLocalLabelAddress and in addLabelAddress we can do a check something like:

bool IsSplitDebugInlining = this->getCUNode()->getSplitDebugInlining();
bool IsSkeletonCU = DD->useSplitDwarf() && !Skeleton;
bool ShouldEmitArangeLabel = IsSplidDebugInlining && IsSkeletonCU || !DD->useSplitDwarf();

[ayermolo]

bool IsSplitDebugInlining = this->getCUNode()->getSplitDebugInlining();
bool IsSkeletonCU = DD->useSplitDwarf() && !Skeleton;
bool NoneSkeletonCU = DD->useSplitDwarf() && Skeleton;
bool ShouldEmitArangeLabel = (!IsSplitDebugInlining && IsSkeletonCU) || NoneSkeletonCU || !DD->useSplitDwarf();

Going to do a bit more testing, before updating this review.

[ayermolo]

Changed the check to not emit in the first place.

[ayermolo]

Updated the test, added another test when inlining is disabled.
Updated check. What we want to do is not add labels for skeleton CU at all I think.

[ayermolo]

@dblaikie what do you think about latest version, and reduced tests?

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

70–71

This boils down to:

bool ShouldEmitArangeLabel = Skeleton || !DD->useSplitDwarf();

I think?

DD->useSplitDwarf() && Skeleton) || !DD->useSplitDwarf();
->
(D->useSplitDwarf() || !DD->useSplitDwarf()) && (Skeleton || !D->useSplitDwarf())
->
true && (Skeleton || !D->useSplitDwarf())
->
Skeleton || !D->useSplitDwarf()

76–77

Could this be moved to above line 74 and then removed from the addLocalLabelAddress rather than duplicated? (would avoid needing to pass the extra parameter to addLocalLabelAddress)

505

Could you remove this unrelated change (you can commit it separately if you like)

[ayermolo]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

70–71

ah right.

76–77

addLocalLabelAddress is a public API. Right now it's only used from here, but in the future if someone else uses it, they will need to know that addArangeLabel needs to be called before it is invoked. Maybe just eliminate it entirely and move functionality into addLabelAddress?

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

76–77

LLVM's concept of "public" API is fairly loose. We move functionality around pretty regularly.

I don't mind too much rolling the functionality in, it's not a very long function. Either way.

[ayermolo]

Addressed comments.
I was thinking in next commit role addLocalLabelAddress into addLabelAddress

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

74–76

Could potentially commit the addArangeLabel refactor as a separate preliminary commit to make it clear what's changing (& narrow the area to revert if the functional part has issues), but no worries either way.

[ayermolo]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

74–76

Posted D119192.

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

DwarfCompileUnit::DwarfCompileUnit(unsigned UID, const DICompileUnit *Node,
   insertDIE(Node, &getUnitDie());
   MacroLabelBegin = Asm->createTempSymbol("cu_macro_begin");
 }
 
 /// addLabelAddress - Add a dwarf label attribute data and value using
 /// DW_FORM_addr or DW_FORM_GNU_addr_index.
 void DwarfCompileUnit::addLabelAddress(DIE &Die, dwarf::Attribute Attribute,
                                        const MCSymbol *Label) {
+  if ((Skeleton || !DD->useSplitDwarf()) && Label)
+    DD->addArangeLabel(SymbolCU(this, Label));
+
   // Don't use the address pool in non-fission or in the skeleton unit itself.
   if ((!DD->useSplitDwarf() || !Skeleton) && DD->getDwarfVersion() < 5)
     return addLocalLabelAddress(Die, Attribute, Label);
 
-  if (Label)
-    DD->addArangeLabel(SymbolCU(this, Label));
-
   bool UseAddrOffsetFormOrExpressions =
       DD->useAddrOffsetForm() || DD->useAddrOffsetExpressions();
 
   const MCSymbol *Base = nullptr;
   if (Label->isInSection() && UseAddrOffsetFormOrExpressions)
     Base = DD->getSectionLabel(&Label->getSection());
 
   if (!Base || Base == Label) {
     unsigned idx = DD->getAddressPool().getIndex(Label);
addAttribute(Die, Attribute, dwarf::DW_FORM_LLVM_addrx_offset,
                  new (DIEValueAllocator) DIEAddrOffset(
                      DD->getAddressPool().getIndex(Base), Label, Base));
 }
 
 void DwarfCompileUnit::addLocalLabelAddress(DIE &Die,
                                             dwarf::Attribute Attribute,
                                             const MCSymbol *Label) {
   if (Label)
-    DD->addArangeLabel(SymbolCU(this, Label));
-
-  if (Label)
     addAttribute(Die, Attribute, dwarf::DW_FORM_addr, DIELabel(Label));
   else
     addAttribute(Die, Attribute, dwarf::DW_FORM_addr, DIEInteger(0));
 }
 
 unsigned DwarfCompileUnit::getOrCreateSourceID(const DIFile *File) {
   // If we print assembly, we can't separate .file entries according to
   // compile units. Thus all files will belong to the default compile unit.
case TargetFrameLowering::DwarfFrameBase::WasmFrameBase: {
         addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_WASM_location);
         addSInt(*Loc, dwarf::DW_FORM_sdata, TI_GLOBAL_RELOC);
         if (!isDwoUnit()) {
           addLabel(*Loc, dwarf::DW_FORM_data4, SPSym);
         } else {
           // FIXME: when writing dwo, we need to avoid relocations. Probably
           // the "right" solution is to treat globals the way func and data
           // symbols are (with entries in .debug_addr).
-          // For now, since we only ever use index 0, this should work as-is.       
+          // For now, since we only ever use index 0, this should work as-is.
           addUInt(*Loc, dwarf::DW_FORM_data4, FrameBase.Location.WasmLoc.Index);
         }
         addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_stack_value);
         addBlock(*SPDie, dwarf::DW_AT_frame_base, Loc);
       } else {
         DIELoc *Loc = new (DIEValueAllocator) DIELoc;
         DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
         DIExpressionCursor Cursor({});

llvm/test/DebugInfo/X86/fission-inline-aranges.ll

+; RUN: llc -generate-arange-section -split-dwarf-file=foo.dwo -O0 < %s -mtriple=x86_64-unknown-linux-gnu -filetype=obj > %t
+; RUN: llvm-dwarfdump -v -debug-aranges %t | FileCheck %s
+
+; Test that only one entry is emmitted in .debug_aranges per CU.
+
+; struct foo {
+;    static void f2();
+;    static void f3(...);
+;  };
+;
+;  void foo::f3(...) {
+;      f2();
+; }
+
+; Check that we emit only one entry in .debug_aranges
+
+; CHECK:    cu_offset
+; CHECK-NOT: cu_offset
+
+; Function Attrs: mustprogress noinline optnone uwtable
+define dso_local void @_ZN3foo2f3Ez(...) #0 align 2 !dbg !8 {
+entry:
+  call void @_ZN3foo2f2Ev(), !dbg !18
+  ret void, !dbg !19
+}
+
+declare dso_local void @_ZN3foo2f2Ev() #1
+
+attributes #0 = { mustprogress noinline optnone uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 14.0.0", isOptimized: false, runtimeVersion: 0, splitDebugFilename: "main.dwo", emissionKind: FullDebug, nameTableKind: GNU)
+!1 = !DIFile(filename: "main.cpp", directory: "/tmp/dbginfo")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 7, !"uwtable", i32 1}
+!6 = !{i32 7, !"frame-pointer", i32 2}
+!7 = !{!"clang version 14.0.0"}
+!8 = distinct !DISubprogram(name: "f3", linkageName: "_ZN3foo2f3Ez", scope: !9, file: !1, line: 6, type: !15, scopeLine: 6, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, declaration: !14, retainedNodes: !17)
+!9 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo", file: !1, line: 1, size: 8, flags: DIFlagTypePassByValue, elements: !10, identifier: "_ZTS3foo")
+!10 = !{!11, !14}
+!11 = !DISubprogram(name: "f2", linkageName: "_ZN3foo2f2Ev", scope: !9, file: !1, line: 2, type: !12, scopeLine: 2, flags: DIFlagPrototyped | DIFlagStaticMember, spFlags: 0)
+!12 = !DISubroutineType(types: !13)
+!13 = !{null}
+!14 = !DISubprogram(name: "f3", linkageName: "_ZN3foo2f3Ez", scope: !9, file: !1, line: 3, type: !15, scopeLine: 3, flags: DIFlagPrototyped | DIFlagStaticMember, spFlags: 0)
+!15 = !DISubroutineType(types: !16)
+!16 = !{null, null}
+!17 = !{}
+!18 = !DILocation(line: 7, column: 6, scope: !8)
+!19 = !DILocation(line: 8, column: 2, scope: !8)

llvm/test/DebugInfo/X86/fission-no-inline-aranges.ll

+; RUN: llc -generate-arange-section -split-dwarf-file=foo.dwo -O0 < %s -mtriple=x86_64-unknown-linux-gnu -filetype=obj > %t
+; RUN: llvm-dwarfdump -v -debug-aranges %t | FileCheck %s
+
+; Test that only one entry is emmitted in .debug_aranges per CU.
+
+; struct foo {
+;    static void f2();
+;    static void f3(...);
+;  };
+;
+;  void foo::f3(...) {
+;      f2();
+; }
+
+; Check that we emit only one entry in .debug_aranges
+
+; CHECK:    cu_offset
+; CHECK-NOT: cu_offset
+
+; Function Attrs: mustprogress noinline optnone uwtable
+define dso_local void @_ZN3foo2f3Ez(...) #0 align 2 !dbg !8 {
+entry:
+  call void @_ZN3foo2f2Ev(), !dbg !18
+  ret void, !dbg !19
+}
+
+declare dso_local void @_ZN3foo2f2Ev() #1
+
+attributes #0 = { mustprogress noinline optnone uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 14.0.0", isOptimized: false, runtimeVersion: 0, splitDebugFilename: "main.dwo", emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: GNU)
+!1 = !DIFile(filename: "main.cpp", directory: "/tmp/dbginfo")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 7, !"uwtable", i32 1}
+!6 = !{i32 7, !"frame-pointer", i32 2}
+!7 = !{!"clang version 14.0.0"}
+!8 = distinct !DISubprogram(name: "f3", linkageName: "_ZN3foo2f3Ez", scope: !9, file: !1, line: 6, type: !15, scopeLine: 6, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, declaration: !14, retainedNodes: !17)
+!9 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo", file: !1, line: 1, size: 8, flags: DIFlagTypePassByValue, elements: !10, identifier: "_ZTS3foo")
+!10 = !{!11, !14}
+!11 = !DISubprogram(name: "f2", linkageName: "_ZN3foo2f2Ev", scope: !9, file: !1, line: 2, type: !12, scopeLine: 2, flags: DIFlagPrototyped | DIFlagStaticMember, spFlags: 0)
+!12 = !DISubroutineType(types: !13)
+!13 = !{null}
+!14 = !DISubprogram(name: "f3", linkageName: "_ZN3foo2f3Ez", scope: !9, file: !1, line: 3, type: !15, scopeLine: 3, flags: DIFlagPrototyped | DIFlagStaticMember, spFlags: 0)
+!15 = !DISubroutineType(types: !16)
+!16 = !{null, null}
+!17 = !{}
+!18 = !DILocation(line: 7, column: 6, scope: !8)
+!19 = !DILocation(line: 8, column: 2, scope: !8)