[DebugInfo] Give warning instead of error for premature terminator in .debug_aranges section.

[junfd]

Summary

llvm-profgen gives error message when the input binary contains premature terminator in .debug_aranges section. These zero length items point to some rodata with zero size type in embed Rust Library. Considering Zero-Sized Types are a valid feature in Rust. They are not real error. This change makes the "error:" message into a warning to avoid misleading.

Why do we still want a warning on such case? because it doesn't follow dwarf standard. https://bugs.llvm.org/show_bug.cgi?id=46805 contains early discussion.

[ayermolo]

test?

[wenlei]

The description is a bit unclear. llvm-profgen giving an error isn't a reason to make this change. But if the error doesn't make sense in general, that's a reason. That's something worth explaining.

[junfd]

In D124121#3463082, @ayermolo wrote:

test?

The original case is too big to use as a test case for llvm-profgen. I will try creating a small case.

[wenlei]

Looks like this is extending https://reviews.llvm.org/D85313. Makes sense to me, but @dblaikie @jhenderson are probably better reviewers.

[jhenderson]

It's been a while since I looked at this code, so let me just confirm my understanding is correct:

1. You have a debug aranges table entry with address and size 0, but which corresponds to a real piece of data of size 0.
2. Such entries are treated as "warning" level problems, i.e. don't prevent continued parsing of the data within DWARFDebugArangesSet.
3. Prior to your change, "warning" level issues were reported via the same callback as "recoverable errors" within the DWARFDebugAranges code.
4. Your change changes the warning to be handled via a different callback, so that llvm-profdata can handle them differently.

All seems reasonable to me.

Re. testing, are there no existing ones that break with this change? If we need new tests, a lot of the DWARF code has gtest unit tests, including DWARFDebugArangesSet. I wonder if you could use those existing ones as a basis for testing these code chanegs?

[dblaikie]

Might be easier to understand with the upstream changes to llvm-profdata. "Prior to your change, "warning" level issues were reported via the same callback as "recoverable errors" within the DWARFDebugAranges code." - maybe llvm-profdata could just treat "recoverable errors" as warnings, effectively?

[junfd]

In D124121#3472754, @dblaikie wrote:

Might be easier to understand with the upstream changes to llvm-profdata. "Prior to your change, "warning" level issues were reported via the same callback as "recoverable errors" within the DWARFDebugAranges code." - maybe llvm-profdata could just treat "recoverable errors" as warnings, effectively?

Thanks for commenting this. The call stack is deep. I didn't see a way to configure this in llvm-profgen without changing all of these functions on the path. Even it is possible, I think current fix is more efficient. What's more, there are two levels of error messages during exacting address ranges sections (in function DWARFDebugArangeSet::extract). some are better to be reported as error even "recoverable", some like this one only needs warning. It is more reasonable to make llvm library give consistent message. current llvm-dwarfdump give warning because it call DWARFDebugArangeSet::extract through other path which use WarningHandler.

[junfd]

In D124121#3466853, @jhenderson wrote:

It's been a while since I looked at this code, so let me just confirm my understanding is correct:

1. You have a debug aranges table entry with address and size 0, but which corresponds to a real piece of data of size 0.
2. Such entries are treated as "warning" level problems, i.e. don't prevent continued parsing of the data within DWARFDebugArangesSet.
3. Prior to your change, "warning" level issues were reported via the same callback as "recoverable errors" within the DWARFDebugAranges code.
4. Your change changes the warning to be handled via a different callback, so that llvm-profdata can handle them differently.

All seems reasonable to me.

I don't think llvm-profdata handle these error message at all. it just calls "Symbolizer->symbolizeInlinedCode(SymbolizerPath.str(), Addr)" . After the different handler is hooked in the code where I changed, the extract function (DWARFDebugArangeSet::extract) directly print error/warning message out by the following line:

WarningHandler(createStringError(
          errc::invalid_argument,
          "address range table at offset 0x%" PRIx64
          " has a premature terminator entry at offset 0x%" PRIx64,
          Offset, EntryOffset));

Re. testing, are there no existing ones that break with this change? If we need new tests, a lot of the DWARF code has gtest unit tests, including DWARFDebugArangesSet. I wonder if you could use those existing ones as a basis for testing these code chanegs?

It doesn't break any existing tests. I will check in a test case under llvm-profgen.

[junfd]

Upload test.

[jhenderson]

llvm/test/tools/llvm-profgen/premature-warning.s

1 ↗

(On Diff #425085)

I feel like the test name needs to include "aranges" somewhere in it, e.g. premature-aranges-end-warning.s

2 ↗

(On Diff #425085)

clang can't be used for tests under llvm/test, because it may not have been built. You should use llvm-mc (there are plenty of examples of the latter).

5 ↗

(On Diff #425085)

I'd check the full warning, as this pattern could match other warnings from the aranges section.

7–15 ↗

(On Diff #425085)

In addition to the above, I'd note the version of clang you used to generate this assembly.

Up to you, but I'd consider using YAML as input instead of asm. For the simple test case here, you might find it leads to more readable and maintainable input, once you've done the initial legwork of writing the debug_info and debug_abbrev sections.

[junfd]

llvm/test/tools/llvm-profgen/premature-warning.s

1 ↗

(On Diff #425085)

Make sense. I will change the name.

2 ↗

(On Diff #425085)

Will do. thanks for pointing this out.

5 ↗

(On Diff #425085)

Sure. I will change it.

7–15 ↗

(On Diff #425085)

I saw some tests using YAML and coverts to obj file. could it be convert to executable file further? llvm-profgen require the binary input to be a valid executable.

[jhenderson]

llvm/test/tools/llvm-profgen/premature-warning.s

7–15 ↗

(On Diff #425085)

YAML will just convert the YAML into whatever its literal representation is. If you've written it correctly, you could use it to generate an executable, yes, although that would likely require you to know a fair amount about the file format, or to just use obj2yaml to generate the YAML input, although at that point, it may not be particularly readable (there's usually a lot of cruft in an executable that isn't actually needed for a test case).

However, your comment raised a further issue, which might torpedo this test in its current form anyway: just as the main lit tests can't use clang, they also can't use a linker, such as LLD either, for the same reasons. That means you simply cannot build from source or assembly. The only way to make this test usable directly where it is is to use yaml2obj. You'd have to put the test in cross-project-tests to have access to clang and/or LLD, but I'm not convinced that's the right place for this sort of test, since linked addresses aren't really involved.

[junfd]

Move test into llvm-symbolizer, which calls into the same stack as
llvm-profgen and shows "error:" for premature terminator in
debug_aranges section.

[jhenderson]

llvm/test/tools/llvm-symbolizer/debug-aranges-premature-end.s

1 ↗

(On Diff #425965)

Nice. This is much simpler. That being said, it would be nice if we could avoid this # REQUIRES line by using YAML as the input still. I believe the following is all that would be needed:

# RUN: yaml2obj %s -o %t
# RUN: llvm-symbolizer 0xa --obj=%t.o 2>&1 | FileCheck %s

# CHECK: warning: address range table at offset 0x0 has a premature terminator entry at offset 0x10

--- !ELF
FileHeader:
  Class: ELFCLASS64
  Data:  ELFDATA2LSB
  Type:  ET_EXEC
Sections:
  - Name: foo
    Type: SHT_PROGBITS
    Size: 12
DWARF:
  debug_aranges:
    - Version:              2
      CuOffset:             0
      Descriptors:
        - Address: 0
          Length:  0
        - Address: 0x5678
          Length:  0x20

If I'm not mistaken, you should be able even to drop the foo section entirely though.

[junfd]

change the test to yaml format.

[junfd]

rebase to latest.

[jhenderson]

LGTM, with nit.

llvm/test/tools/llvm-symbolizer/debug-aranges-premature-end.yaml

13–14

Nit: remove excess indentation.

[junfd]

Clean excess indentation

[junfd]

@jhenderson Thanks very much for all the valuable feedback.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAranges.h

 class DWARFDebugAranges {
 public:
   void generate(DWARFContext *CTX);
   uint64_t findAddress(uint64_t Address) const;
 
 private:
   void clear();
   void extract(DWARFDataExtractor DebugArangesData,
-               function_ref<void(Error)> RecoverableErrorHandler);
+               function_ref<void(Error)> RecoverableErrorHandler,
+               function_ref<void(Error)> WarningHandler);
 
   /// Call appendRange multiple times and then call construct.
   void appendRange(uint64_t CUOffset, uint64_t LowPC, uint64_t HighPC);
   void construct();
 
   struct Range {
     explicit Range(uint64_t LowPC, uint64_t HighPC, uint64_t CUOffset)
       : LowPC(LowPC), Length(HighPC - LowPC), CUOffset(CUOffset) {}

llvm/lib/DebugInfo/DWARF/DWARFDebugAranges.cpp

 #include <cassert>
 #include <cstdint>
 #include <set>
 
 using namespace llvm;
 
 void DWARFDebugAranges::extract(
     DWARFDataExtractor DebugArangesData,
-    function_ref<void(Error)> RecoverableErrorHandler) {
+    function_ref<void(Error)> RecoverableErrorHandler,
+    function_ref<void(Error)> WarningHandler) {
   if (!DebugArangesData.isValidOffset(0))
     return;
   uint64_t Offset = 0;
   DWARFDebugArangeSet Set;
 
   while (DebugArangesData.isValidOffset(Offset)) {
-    if (Error E =
-            Set.extract(DebugArangesData, &Offset, RecoverableErrorHandler)) {
+    if (Error E = Set.extract(DebugArangesData, &Offset, WarningHandler)) {
       RecoverableErrorHandler(std::move(E));
       return;
     }
     uint64_t CUOffset = Set.getCompileUnitDIEOffset();
     for (const auto &Desc : Set.descriptors()) {
       uint64_t LowPC = Desc.Address;
       uint64_t HighPC = Desc.getEndAddress();
       appendRange(CUOffset, LowPC, HighPC);
     }
     ParsedCUOffsets.insert(CUOffset);
   }
 }
 
 void DWARFDebugAranges::generate(DWARFContext *CTX) {
   clear();
   if (!CTX)
     return;
 
   // Extract aranges from .debug_aranges section.
   DWARFDataExtractor ArangesData(CTX->getDWARFObj().getArangesSection(),
                                  CTX->isLittleEndian(), 0);
-  extract(ArangesData, CTX->getRecoverableErrorHandler());
+  extract(ArangesData, CTX->getRecoverableErrorHandler(),
+          CTX->getWarningHandler());
 
   // Generate aranges from DIEs: even if .debug_aranges section is present,
   // it may describe only a small subset of compilation units, so we need to
   // manually build aranges for the rest of them.
   for (const auto &CU : CTX->compile_units()) {
     uint64_t CUOffset = CU->getOffset();
     if (ParsedCUOffsets.insert(CUOffset).second) {
       Expected<DWARFAddressRangesVector> CURanges = CU->collectAddressRanges();

llvm/test/tools/llvm-symbolizer/debug-aranges-premature-end.yaml

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-symbolizer 0xa --obj=%t 2>&1 | FileCheck %s
+
+# CHECK: warning: address range table at offset 0x0 has a premature terminator entry at offset 0x10
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_EXEC
+DWARF:
+  debug_aranges:
+    - Version:              2
+      CuOffset:             0
+      Descriptors:
+        - Address: 0
+          Length:  0
+        - Address: 0x5678
+          Length:  0x20