[PATCH] D84817: Add verification for DW_AT_decl_file and DW_AT_call_file.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: Higuoxing.
jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:548
+        if (!LT->hasFileAtIndex(*FileIdx)) {
+          bool isDWARF5 = LT->Prologue.getVersion() >= 5;
+          uint32_t LastFileIdx = LT->getLastValidFileIndex();
----------------
Maybe best to make this not named after the version, e.g. `IsZeroIndexed`?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:558
+        ReportError("DIE has " + AttributeString(Attr) +
+                    " that references a file by index " +
+                    llvm::formatv("{0}", *FileIdx) +
----------------
On first read, I found "by" to feel not quite the right word. Perhaps "with" would be better? (Not 100% sure either way though).


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:560
+                    llvm::formatv("{0}", *FileIdx) +
+                    " and the compile unit has no line table.");
+      }
----------------
Here and below, from a quick glance, it doesn't look like most of the verifier error messages have a trailing full stop, so these probably shouldn't either.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/verify_attr_file_indexes.yaml:1
+
+# RUN: yaml2obj %s | not llvm-dwarfdump --verify - | FileCheck %s --implicit-check-not=error:
----------------
Nit: delete blank line at file start.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/verify_attr_file_indexes.yaml:20
+# CHECK:      Errors detected.
+--- !mach-o
+FileHeader:
----------------
Rather than using mach-o, which has a lot of noise in its YAML, it might be worth switching to ELF. I think yaml2obj's ELF support is more-or-less sufficient now to achieve the desired goal. I think all you'd need to do is to use an ELF file header (you can find some simple examples in the yaml2obj tests), but keep the DWARF block the same.

There are probably ways to reduce the DWARF bit of the YAML too. @Higuoxing is actively working on improving this, so might be able to offer some tips, but I don't know how much could be done there yet.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/verify_file_encoding.yaml:18
+
+--- !mach-o
+FileHeader:
----------------
Same comments as other test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84817