[PATCH] D83049: [DebugInfo] Do not hang when parsing a malformed .debug_pub* section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: lld/ELF/DWARF.h:59
 
-  const llvm::DWARFSection &getGnuPubnamesSection() const override {
+  const LLDDWARFSection &getGnuPubnamesSection() const override {
     return gnuPubnamesSection;
----------------
Why did the types here need to change?


================
Comment at: lld/ELF/SyntheticSections.cpp:2713
+  for (const LLDDWARFSection *pub : {&pubNames, &pubTypes}) {
+    DWARFDataExtractor data(obj, *pub, config->isLE, 0);
+    DWARFDebugPubTable table;
----------------
Could you put a comment here saying what the `0` means (i.e. `/*Something=*/0`), please?


================
Comment at: lld/test/ELF/gdb-index-invalid-pubnames.s:5
+
+# CHECK: warning: {{.*}}(.debug_gnu_pubnames): unexpected end of data at offset 0x1 while reading [0x0, 0x4)
+
----------------
It might make sense to extend this test to show that LLD doesn't use the warned-for pubnames section, if there's a way for that to make sense.


================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-debug-pubnames-invalid.s:1
+# RUN: llvm-mc -triple x86_64 %s -filetype=obj -o %t
+# RUN: not llvm-dwarfdump -debug-pubnames %t 2>&1 | FileCheck %s
----------------
I guess the pubnames section is fairly straightforward, but I personally feel like it would be best to write the testing for this and D83050 as gtest unit tests. That being said, a single test that shows how llvm-dwarfdump handles errors for malformed pubnames sections might make sense, although in that case I'd put it alongside the debug_line_invalid.test in the llvm-dwarfdump directory.

I would probably add basic coverage for the .debug_pubtypes and the GNU variant sections too, since they go through different code paths to get there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83049