[PATCH] D74560: Introduce DWARFDataExtractor::getInitialLength

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

As suggested, please split this up into separate patches.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h:43
+  std::pair<uint64_t, dwarf::DwarfFormat>
+  getInitialLength(uint64_t *Off, Error *Err = nullptr) const;
+
----------------
Perhaps a comment explaining what is meant by an initial length field, wouldn't be out-of-order?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp:31
+          errc::invalid_argument,
+          "Unsupported reserved unit length of value 0x%8.8" PRIx64
+          " at offset 0x%" PRIx64,
----------------
I think it's more common for error/warning messages to use lower-case for their first word.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:53
 # FATAL-NEXT: Line table prologue
-# FATAL-NEXT: total_length: 0xfffffffe
+# FATAL-NEXT: total_length: 0x00000000
 # FATAL-NOT:  debug_line
----------------
I don't think this is a good change in behaviour. We should still be able to dump the total length field, with the reserved value.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:195
 
-# RESERVED: warning: parsing line table prologue at offset 0x00000048 unsupported reserved unit length found of value 0xfffffffe
+# RESERVED: warning: Unsupported reserved unit length of value 0xfffffffe at offset 0x48
 
----------------
This is a regression in the quality of the error, as it is not clear what section this error comes from. Same goes for many of the other changes in errors.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_rnglists_invalid.s:5
 # SHORT-NOT: range list header
-# SHORT: error: section is not large enough to contain a .debug_rnglists table length at offset 0
+# SHORT: error: unexpected end of data
 # SHORT-NOT: range list header
----------------
This error is basically useless. Where? What section? What does "unexpected end of data" mean?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_rnglists_reserved_length.s:5
 
-# CHECK: error: .debug_rnglists table at offset 0x0 has unsupported reserved unit length of value 0xfffffff0
+# CHECK: error: Unsupported reserved unit length of value 0xfffffff0 at offset 0x0
 
----------------
Same as above for .debug_line.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp:42
+
+  EXPECT_THAT_EXPECTED(GetWithError({}), Failed());
+  EXPECT_EQ(GetWithoutError({}), ErrorResult);
----------------
Ideally, we should check the error returned in this and similar cases, to show that it is what was expected.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp:45
+
+  EXPECT_THAT_EXPECTED(GetWithError({0x00, 0x01, 0x02}), Failed());
+  EXPECT_EQ(GetWithoutError({0x00, 0x01, 0x02}), ErrorResult);
----------------
It may be worth a brief comment before each pair of checks to show what the intent of the particularly case is.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp:48-52
+  EXPECT_THAT_EXPECTED(
+      GetWithError({0x00, 0x01, 0x02, 0x03}),
+      HasValue(std::make_tuple(0x00010203, dwarf::DWARF32, 4)));
+  EXPECT_EQ(GetWithoutError({0x00, 0x01, 0x02, 0x03}),
+            std::make_tuple(0x00010203, dwarf::DWARF32, 4));
----------------
Will this test work if the host endianness is different? It looks to me like 0x00010203 != {0x00, 0x01, 0x02, 0x03} for little endian hosts, since in that case, 0x00010203 is {0x03, 0x02, 0x01, 0x00}.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp:66
+
+  EXPECT_THAT_EXPECTED(GetWithError({0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0x02, 0x03}),
+                       Failed());
----------------
clang-format?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74560/new/

https://reviews.llvm.org/D74560