[PATCH] D32707: lldb-dwarfdump -verify [-quiet]

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:290
+  if (DumpType == DIDT_All || DumpType == DIDT_Info) {
+    OS << "Verifying .debug_info...\n";
+    for (const auto &CU : compile_units()) {
----------------
clayborg wrote:
> aprantl wrote:
> > Would it make sense to split this entire verifier into a separate function?
> > Alternatively, if many verifiers need to iterate over the same data, would it make sense to split this out into a verify/visitCompileUnit function?
> One issue with splitting it out is we will need to pass all data structures to those functions. For instance, the .debug_arange verifier will need access to a set of FunctionInfos (see previous patch). So I can split it out, but this function needs to be called even when .debug_info isn't being verified if we are verifying .debug_aranges because we will need to truth of what function ranges are in a CU and be able to compare it to the contents of the .debug_aranges. Let me know what you think...
Perhaps we can model it after llvm/lib/IR/Verifier.cpp.
It uses a private Verifier class that has visit[Enity] methods that implement various checks. State is passed through class members.

Would something like that work?


================
Comment at: tools/llvm-dwarfdump/llvm-dwarfdump.cpp:107
+      stream << "Errors detected.\n";
+      exit(1);
+    }
----------------
clayborg wrote:
> aprantl wrote:
> > Do we really want to exit() here? What happens when llvm-dwarfdump is invoked on a .dSYM or static archive with more than one object file?
> I could make a static variable that gets set to true and exit after the iterations of all files and objects is done. It would be a bit messy to try and plumb the bool return value through since the dump and verify share the same code right now?
We should probably just make `verify` a separate action from `dump`.


================
Comment at: tools/llvm-dwarfdump/llvm-dwarfdump.cpp:117
 
 static void DumpInput(StringRef Filename) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> BuffOrErr =
----------------
... and rename this to `ProcessInput` or something


================
Comment at: tools/llvm-dwarfdump/llvm-dwarfdump.cpp:129
   if (auto *Obj = dyn_cast<ObjectFile>(BinOrErr->get()))
     DumpObjectFile(*Obj, Filename);
   else if (auto *Fat = dyn_cast<MachOUniversalBinary>(BinOrErr->get()))
----------------
```
... {
  if (Dump && !Quiet)
     DumpObjectFile(*Obj, Filename);
  if (Verify)
     Result |= VerifyObjectFile(...)
}
```

etc.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1711
+            Values:
+  )";
+  auto ErrOrSections = DWARFYAML::EmitDebugSections(StringRef(yamldata));
----------------
clayborg wrote:
> aprantl wrote:
> > Whoa. This looks nice! Out of curiosity, is there a specific advantage to doing this as a unit test as opposed to a FileCheck-based test?
> Easier to debug and edit and fix is my best answer.
As long as we don't need any regex-matching on the output, this should be fine. And we can easily convert it to FileCheck later on.


https://reviews.llvm.org/D32707