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

[Greg Clayton via Phabricator via llvm-commits]

clayborg 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()) {
----------------
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...


================
Comment at: tools/llvm-dwarfdump/llvm-dwarfdump.cpp:107
+      stream << "Errors detected.\n";
+      exit(1);
+    }
----------------
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?


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1711
+            Values:
+  )";
+  auto ErrOrSections = DWARFYAML::EmitDebugSections(StringRef(yamldata));
----------------
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.


https://reviews.llvm.org/D32707