Round up zero-sized symbols to 1 byte in `.debug_aranges` (without breaking other logic).

[pcwalton]

Summary

This commit modifies the AsmPrinter to avoid emitting any zero-sized symbols to
the .debug_aranges table, by rounding their size up to 1. Entries with zero
length violate the DWARF 5 spec, which states:

Each descriptor is a triple consisting of a segment selector, the beginning
address within that segment of a range of text or data covered by some entry
owned by the corresponding compilation unit, followed by the non-zero length
of that range.

In practice, these zero-sized entries produce annoying warnings in lld and
cause GNU binutils to truncate the table when parsing it.

Other parts of LLVM, such as DWARFDebugARanges in the DebugInfo module
(specifically the appendRange method), already avoid emitting zero-sized
symbols to .debug_aranges, but not comprehensively in the AsmPrinter. In fact,
the AsmPrinter does try to avoid emitting such zero-sized symbols when labels
aren't involved, but doesn't when the symbol to emitted is a difference of two
labels; this patch extends that logic to handle the case in which the symbol is
defined via labels.

Furthermore, this patch fixes a bug in which available_externally symbols
would cause unpredictable values to be emitted into the .debug_aranges table
under certain circumstances. In practice I don't believe that this caused
issues up until now, but the root cause of this bug--an invalid DenseMap
lookup--triggered failures in Chromium when combined with an earlier version of
this patch. Therefore, this patch fixes that bug too.

This is a revised version of diff D126257, which was reverted due to breaking
tests. The now-reverted version of this patch didn't distinguish between
symbols that didn't have their size reported to the DwarfDebug handler and
those that had their size reported to be zero. This new version of the patch
instead restricts the special handling only to the symbols whose size is
definitively known to be zero.

[pcwalton]

Test failures look unrelated?

[ayermolo]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

3056

Can you change back to unit64_t. I believe LLVM coding standard errs on using specific types when it's not obvious what type it is.

[ayermolo]

llvm/test/CodeGen/X86/dwarf-aranges-zero-size.ll

9

Would it be possible to add source code + compiler command that generated this?

[bjope]

llvm/test/CodeGen/X86/dwarf-aranges-zero-size.ll

19

As mentioned in comments in the earlier review, "align" is specified in bits. And aligning to 1 bit seem a bit weird (downstream this trigger and assertion for us, because we assert that align is a multiple of the char size since llvm will divide this by the char size to get alignment in bytes).
Is rust really using alignments smaller than the char size?

[pcwalton]

llvm/test/CodeGen/X86/dwarf-aranges-zero-size.ll

19

Good catch, I submitted a fix to upstream rustc: https://github.com/rust-lang/rust/pull/97846

[pcwalton]

Updated the diff to address comments.

[ayermolo]

Looks ok to me.
@dblaikie thoughts?

[hans]

When trying this on Chromium (in the context of https://crbug.com/1335630) we hit a densemap assert in llvm::DwarfDebug::emitDebugARanges()
See the attached reproducer.

string_piece_unittest-c72fb0.sh12 KBDownload

string_piece_unittest-c72fb0.cpp6 MBDownload

[dblaikie]

In D126835#3577753, @hans wrote:

When trying this on Chromium (in the context of https://crbug.com/1335630) we hit a densemap assert in llvm::DwarfDebug::emitDebugARanges()
See the attached reproducer.

string_piece_unittest-c72fb0.sh12 KBDownload

string_piece_unittest-c72fb0.cpp6 MBDownload

Thanks for the reproducer, @hansw - sounds worth addressing before this moves forward.

(I'm still sort of feeling like we should fix the underlying entities to be non-zero length always (even if they're zero length at the language level - they can still be non-zero length as an implementation detail providing unique addresses))

[ayermolo]

In D126835#3579779, @dblaikie wrote:

In D126835#3577753, @hans wrote:

When trying this on Chromium (in the context of https://crbug.com/1335630) we hit a densemap assert in llvm::DwarfDebug::emitDebugARanges()
See the attached reproducer.

string_piece_unittest-c72fb0.sh12 KBDownload

string_piece_unittest-c72fb0.cpp6 MBDownload

Thanks for the reproducer, @hansw - sounds worth addressing before this moves forward.

(I'm still sort of feeling like we should fix the underlying entities to be non-zero length always (even if they're zero length at the language level - they can still be non-zero length as an implementation detail providing unique addresses))

Doesn't it rely on various feeders into llvm doing the right thing, or can this be done in some kind of language agnostic way?

[dblaikie]

In D126835#3582639, @ayermolo wrote:

In D126835#3579779, @dblaikie wrote:

In D126835#3577753, @hans wrote:

When trying this on Chromium (in the context of https://crbug.com/1335630) we hit a densemap assert in llvm::DwarfDebug::emitDebugARanges()
See the attached reproducer.

string_piece_unittest-c72fb0.sh12 KBDownload

string_piece_unittest-c72fb0.cpp6 MBDownload

Thanks for the reproducer, @hansw - sounds worth addressing before this moves forward.

(I'm still sort of feeling like we should fix the underlying entities to be non-zero length always (even if they're zero length at the language level - they can still be non-zero length as an implementation detail providing unique addresses))

Doesn't it rely on various feeders into llvm doing the right thing, or can this be done in some kind of language agnostic way?

Possible it could require changes to producers if we wanted to make it invalid at the LLVM IR level to have a zero-length global variable (we already can't have a zero-length IR function - it has to have a terminator, even if it's "unreachable").

But I don't think that's necessary/required - this could be implemented in the LLVM backend(s) (ideally as target-neutrally as possible) to emit a trap for any functions that end up not ending in a terminator instruction (including truly zero-length functions) (various slight variations of this support already exist in LLVM - but I wasn't able to stare at them long enough to get the particular variant I really wanted - but maybe that was a "perfect being the enemy of the good" situation and using one of the existing features (either the non-zero-length thing implemented for Windows (I think that just puts a zero rather than trap) or the "trap on unreachable" (that puts traps on even non-final unreachables, which is more pessimism than I was hoping for - but maybe low cost enough) that Sony and Apple use), extending it for global variables as well as for functions)

[pcwalton]

Minimal reproduction:

template <typename T>
struct Baz {
    static constexpr int boo = int(-1);
};

extern template struct Baz<char>;

void bar(const int& a);

void foo() {
  bar(Baz<char>::boo);
}

I have a fix; it seems to be a subtle pre-existing bug in LLVM actually that happened to be masked until now.

[pcwalton]

So this was a fun one. The previous version of this patch accidentally caused a
pre-existing bug to surface and break Chromium. In certain circumstances
including the Chromium test case, available_externally symbols would cause
invalid lookups in the SymSize table. Before this patch, the lookups were
done with the [] operator, which would succeed but cause an unspecified value
to be emitted in the .debug_aranges table. But this patch moves those lookups
to be done by dereferencing the result of the .find() method, which causes
asserts if the key is not found.

This patch fixes this bug and adds a test. It took quite a while to reduce all
150kloc+ of code...

[pcwalton]

This should be ready for review now.

[dblaikie]

In D126835#3583004, @dblaikie wrote:

In D126835#3582639, @ayermolo wrote:

In D126835#3579779, @dblaikie wrote:

In D126835#3577753, @hans wrote:

When trying this on Chromium (in the context of https://crbug.com/1335630) we hit a densemap assert in llvm::DwarfDebug::emitDebugARanges()
See the attached reproducer.

string_piece_unittest-c72fb0.sh12 KBDownload

string_piece_unittest-c72fb0.cpp6 MBDownload

Thanks for the reproducer, @hansw - sounds worth addressing before this moves forward.

(I'm still sort of feeling like we should fix the underlying entities to be non-zero length always (even if they're zero length at the language level - they can still be non-zero length as an implementation detail providing unique addresses))

Doesn't it rely on various feeders into llvm doing the right thing, or can this be done in some kind of language agnostic way?

Possible it could require changes to producers if we wanted to make it invalid at the LLVM IR level to have a zero-length global variable (we already can't have a zero-length IR function - it has to have a terminator, even if it's "unreachable").

But I don't think that's necessary/required - this could be implemented in the LLVM backend(s) (ideally as target-neutrally as possible) to emit a trap for any functions that end up not ending in a terminator instruction (including truly zero-length functions) (various slight variations of this support already exist in LLVM - but I wasn't able to stare at them long enough to get the particular variant I really wanted - but maybe that was a "perfect being the enemy of the good" situation and using one of the existing features (either the non-zero-length thing implemented for Windows (I think that just puts a zero rather than trap) or the "trap on unreachable" (that puts traps on even non-final unreachables, which is more pessimism than I was hoping for - but maybe low cost enough) that Sony and Apple use), extending it for global variables as well as for functions)

I'd still be highly encouraging of going somewhere in this ^ direction (& in a direction away from using .debug_aranges as well), but the patch looks OK for what it is.

[dblaikie]

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

[ayermolo]

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change. Talking with @clayborg pointing to end-list in ranges is a good work around (along with lldb change).
Long term @zr33 (our summer intern) is working on mechanism to write out .debug_Info/.debug_abbrev using the same mechanics as in codegen/dwarflinker. Once that is enabled we can leave this pattern as is without converting to ranges.

[dblaikie]

In D126835#3600547, @ayermolo wrote:

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change.

Sorry, what I meant is: This issue (D126835) and the other (D127897) would probably both be better-off solved by changing the compiler/LLVM to never produce zero-length entries.

Long term @zr33 (our summer intern) is working on mechanism to write out .debug_Info/.debug_abbrev using the same mechanics as in codegen/dwarflinker.
Once that is enabled we can leave this pattern as is without converting to ranges.

Still not really following - "converting to ranges" as in "changing consumers to use ranges instead of aranges"? sure - though I think that change would be a good thing in general, since aranges take up a bunch of object space.

[ayermolo]

In D126835#3600910, @dblaikie wrote:

In D126835#3600547, @ayermolo wrote:

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change.

Sorry, what I meant is: This issue (D126835) and the other (D127897) would probably both be better-off solved by changing the compiler/LLVM to never produce zero-length entries.

Oh I see.
Some more context. Debug info that D127897 is handling is coming from GCC.

Long term @zr33 (our summer intern) is working on mechanism to write out .debug_Info/.debug_abbrev using the same mechanics as in codegen/dwarflinker.
Once that is enabled we can leave this pattern as is without converting to ranges.

Still not really following - "converting to ranges" as in "changing consumers to use ranges instead of aranges"? sure - though I think that change would be a good thing in general, since aranges take up a bunch of object space.

My bad I should have been more clear.. This is just what BOLT does. We convert DW_AT_low_pc/DW_AT_high_pc to DW_AT_low_pc/DW_AT_ranges.. This is because BOLT can break up functions.
Due to the limits of our current implementation if DIEs share a an abbrev table we must convert all of them.

[dblaikie]

In D126835#3601856, @ayermolo wrote:

In D126835#3600910, @dblaikie wrote:

In D126835#3600547, @ayermolo wrote:

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change.

Sorry, what I meant is: This issue (D126835) and the other (D127897) would probably both be better-off solved by changing the compiler/LLVM to never produce zero-length entries.

Oh I see.
Some more context. Debug info that D127897 is handling is coming from GCC.

Oh, that surprises me - could you provide repro steps?

[ayermolo]

In D126835#3602110, @dblaikie wrote:

In D126835#3601856, @ayermolo wrote:

In D126835#3600910, @dblaikie wrote:

In D126835#3600547, @ayermolo wrote:

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change.

Sorry, what I meant is: This issue (D126835) and the other (D127897) would probably both be better-off solved by changing the compiler/LLVM to never produce zero-length entries.

Oh I see.
Some more context. Debug info that D127897 is handling is coming from GCC.

Oh, that surprises me - could you provide repro steps?

int conv32(const void* ptr) { return *(const int *) ptr; }

int foo(const void* ptr) {
  return conv32(ptr) + 2;
}

gcc -g -gstrict-dwarf -gz=none -O3 helper.cpp -c
Version 11.2.1

[dblaikie]

In D126835#3602351, @ayermolo wrote:

In D126835#3602110, @dblaikie wrote:

In D126835#3601856, @ayermolo wrote:

In D126835#3600910, @dblaikie wrote:

In D126835#3600547, @ayermolo wrote:

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change.

Sorry, what I meant is: This issue (D126835) and the other (D127897) would probably both be better-off solved by changing the compiler/LLVM to never produce zero-length entries.

Oh I see.
Some more context. Debug info that D127897 is handling is coming from GCC.

Oh, that surprises me - could you provide repro steps?

int conv32(const void* ptr) { return *(const int *) ptr; }

int foo(const void* ptr) {
  return conv32(ptr) + 2;
}

gcc -g -gstrict-dwarf -gz=none -O3 helper.cpp -c
Version 11.2.1

Huh - I'd argue that's /probably/ a bug in GCC? Describing a zero-length inlined scope seems of minimal value. I guess some very late optimization probably causes this in GCC. Might be worth filing a bug on GCC or otherwise asking them about that situation (if you do, please CC me - I'd be curious to follow along).

[pcwalton]

The test failures in libomp don't look related to me.

[ayermolo]

In D126835#3602445, @dblaikie wrote:

In D126835#3602351, @ayermolo wrote:

In D126835#3602110, @dblaikie wrote:

In D126835#3601856, @ayermolo wrote:

In D126835#3600910, @dblaikie wrote:

In D126835#3600547, @ayermolo wrote:

In D126835#3600524, @dblaikie wrote:

Looks like this would be another issue with zero-length symbols: https://reviews.llvm.org/D127897 (just cross-referencing/collating)

Well it's an issue right now even without this change.

Sorry, what I meant is: This issue (D126835) and the other (D127897) would probably both be better-off solved by changing the compiler/LLVM to never produce zero-length entries.

Oh I see.
Some more context. Debug info that D127897 is handling is coming from GCC.

Oh, that surprises me - could you provide repro steps?

int conv32(const void* ptr) { return *(const int *) ptr; }

int foo(const void* ptr) {
  return conv32(ptr) + 2;
}

gcc -g -gstrict-dwarf -gz=none -O3 helper.cpp -c
Version 11.2.1

Huh - I'd argue that's /probably/ a bug in GCC? Describing a zero-length inlined scope seems of minimal value. I guess some very late optimization probably causes this in GCC. Might be worth filing a bug on GCC or otherwise asking them about that situation (if you do, please CC me - I'd be curious to follow along).

Let me put it on my TODO list.

[ayermolo]

In D126835#3606570, @pcwalton wrote:

The test failures in libomp don't look related to me.

OK, will commit on Monday. Thanks

[pcwalton]

I verified that the failing tests don't break locally.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (DwarfCompileUnit *CU : CUs) {
     Asm->OutStreamer->AddComment("Segment Size (in bytes)");
     Asm->emitInt8(0);
 
     Asm->OutStreamer->emitFill(Padding, 0xff);
 
     for (const ArangeSpan &Span : List) {
       Asm->emitLabelReference(Span.Start, PtrSize);
 
-      // Calculate the size as being from the span start to it's end.
-      if (Span.End) {
+      // Calculate the size as being from the span start to its end.
+      //
+      // If the size is zero, then round it up to one byte. The DWARF
+      // specification requires that entries in this table have nonzero
+      // lengths.
+      auto SizeRef = SymSize.find(Span.Start);
+      if ((SizeRef == SymSize.end() || SizeRef->second != 0) && Span.End) {
         Asm->emitLabelDifference(Span.End, Span.Start, PtrSize);
       } else {
         // For symbols without an end marker (e.g. common), we
         // write a single arange entry containing just that one symbol.
-        uint64_t Size = SymSize[Span.Start];
+        uint64_t Size = SizeRef->second;
         if (Size == 0)
           Size = 1;
 
         Asm->OutStreamer->emitIntValue(Size, PtrSize);
       }
     }
 
     Asm->OutStreamer->AddComment("ARange terminator");

llvm/test/CodeGen/X86/dwarf-aranges-zero-size.ll

+; Ensures that the AsmPrinter rounds up zero-sized symbols in `.debug_aranges` to one byte.
+
+; Generated from the following Rust source:
+; 
+;     pub static EXAMPLE: () = ();
+;
+; Compiled with:
+;
+;     $ rustc --crate-type=lib --target=x86_64-unknown-linux-gnu --emit=llvm-ir -g dwarf-aranges-zero-size.rs
+
+; RUN: llc --generate-arange-section < %s | FileCheck %s
+; CHECK: .section .debug_aranges
+; CHECK: .quad _ZN23dwarf_aranges_zero_size7EXAMPLE17h8ab19f2b0c3b238dE
+; CHECK-NEXT: .quad 1
+; CHECK: .section
+
+; ModuleID = 'dwarf_aranges_zero_size.fbc28187-cgu.0'
+source_filename = "dwarf_aranges_zero_size.fbc28187-cgu.0"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@_ZN23dwarf_aranges_zero_size7EXAMPLE17h8ab19f2b0c3b238dE = constant <{ [0 x i8] }> zeroinitializer, align 1, !dbg !0
+@__rustc_debug_gdb_scripts_section__ = linkonce_odr unnamed_addr constant [34 x i8] c"\01gdb_load_rust_pretty_printers.py\00", section ".debug_gdb_scripts", align 1
+
+!llvm.module.flags = !{!5, !6, !7}
+!llvm.dbg.cu = !{!8}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "EXAMPLE", linkageName: "_ZN23dwarf_aranges_zero_size7EXAMPLE17h8ab19f2b0c3b238dE", scope: !2, file: !3, line: 1, type: !4, isLocal: false, isDefinition: true, align: 8)
+!2 = !DINamespace(name: "dwarf_aranges_zero_size", scope: null)
+!3 = !DIFile(filename: "dwarf-aranges-zero-size.rs", directory: "/Users/pcwalton/Desktop", checksumkind: CSK_MD5, checksum: "c2d51547bfaf5562b9c9061311fe4140")
+!4 = !DIBasicType(name: "()", encoding: DW_ATE_unsigned)
+!5 = !{i32 7, !"PIC Level", i32 2}
+!6 = !{i32 2, !"RtLibUseGOT", i32 1}
+!7 = !{i32 2, !"Debug Info Version", i32 3}
+!8 = distinct !DICompileUnit(language: DW_LANG_Rust, file: !9, producer: "clang LLVM (rustc version 1.60.0-nightly (0c292c966 2022-02-08))", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !10, globals: !11)
+!9 = !DIFile(filename: "dwarf-aranges-zero-size.rs/@/dwarf_aranges_zero_size.fbc28187-cgu.0", directory: "/Users/pcwalton/Desktop")
+!10 = !{}
+!11 = !{!0}