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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

First off, thank you very much for getting this started!

I agree with David that it would be better to commit this check by check and with a testcase for each one.

Generating testcases shouldn't be too much work. We could just start with a correct C program, compile it with `clang -S` and then inject an error into the assembler output, and then create a .s test with a run-line like this:
`llvm-mc -triple x86_64-unknown-linux test.s -filetype=obj -o - | llvm-dwarfdump --verify -`
The only dwarfback to this approach is that it will not fly for any DWARF generated by the assembler itself, such as .loc directives.



================
Comment at: include/llvm/DebugInfo/DIContext.h:165
+  virtual bool verify(raw_ostream &OS, DIDumpType DumpType = DIDT_All) {
+    return true; // No verifier? Just say things went well.
+  }
----------------
// No verifier? Just say things went well.
return true; 


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:298
+
+  // Returns true if the two ranges intersect
+bool RangesIntersect(const std::pair<uint64_t, uint64_t> &a,
----------------
Weird indentation here and later on, too.
Can you run the entire patch through clang-format?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:298
+
+  // Returns true if the two ranges intersect
+bool RangesIntersect(const std::pair<uint64_t, uint64_t> &a,
----------------
aprantl wrote:
> Weird indentation here and later on, too.
> Can you run the entire patch through clang-format?
/// Returns true if the two ranges intersect.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:318
+          if (Ranges[I].first > Ranges[I].second) {
+            OS << "error: invalid range in DIE\n";
+            HasErrors = true;
----------------
We should probably factor out the "error: " part so its consistently formatted everywhere (and so we can colorize it, etc.)


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:328
+      }
+      if (Ranges.size() > 1)
+        std::sort(Ranges.begin(), Ranges.end());
----------------
I would just remove this. That check will likely get inlined from std::sort anyway.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:337
+      for (const auto &R: Ranges)
+        OS << " [" << format("0x%" PRIx32, R.first) << "-"
+            << format("0x%" PRIx32, R.second) << ")";
----------------
FYI: We had a discussion earlier this week about how to best format ranges:

http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170424/448535.html

and decided to write a helper function that outputs them as [x,y) by default.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:362
+          return true;
+      } else if (RHS.Ranges.empty())
+        return false;
----------------
According to the LLVM style both `else`s here should be removed, since the if has an early exit on all paths.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:367
+        size_t RightSize = RHS.Ranges.size();
+        for (size_t i=0; i<LeftSize; ++i) {
+          if (i >= RightSize)
----------------
`I`


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:397
+      OS << "Verifying .debug_info...\n";
+    else
+      OS << "Verifying .debug_aranges...\n";
----------------
This is bound to break when we add more kinds?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:401
+      unsigned NumDies = CU->getNumDIEs();
+      for (unsigned i=0; i<NumDies; ++i) {
+        auto Die = CU->getDIEAtIndex(i);
----------------
`I`


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:404
+        assert(Die); // Make sure DIE is valid
+        //const auto DieOffset = Die.getOffset();
+        const auto Tag = Die.getTag();
----------------
delete


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:407
+        const uint32_t DieDepth = Die.getDebugInfoEntry()->getDepth();
+        bool SkipAttributes = false;
+        switch (Tag)
----------------
This function should probably be split up into several static helpers.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:164
 
+void DWARFDebugLine::Row::dumpTableHeader(raw_ostream &OS) {
+  OS << "Address            Line   Column File   ISA Discriminator Flags\n"
----------------
separate commit?


https://reviews.llvm.org/D32625