[PATCH] D32754: Add line table verification to lldb-dwarfdump --verify.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:404
+      if (auto StmtFormValue = CU->getUnitDIE().find(DW_AT_stmt_list)) {
+        if (auto DataOpt = toSectionOffset(StmtFormValue)) {
+          LineTableOffset = *DataOpt;
----------------
clayborg wrote:
> dblaikie wrote:
> > FIXME to handle this as a verification failure too? (if stmt_list isn't usable as a section offset)
> This is handled when verifying the .debug_info section. No need to duplicate this here.
Should it be an assertion here, then? (is this verification still run, even if the debug_info section verification fails?)


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:415
+      } else {
+        continue; // No line table for this compile unit.
+      }
----------------
clayborg wrote:
> Yeah, I thought of that but have been asked to do the "if (auto StmtFormValue" way many times. The way you did it above is the way I wanted to do it. I will change it.
Yeah, it depends - both are certainly used a lot.

Exactly how much code is unindented by inverting the condition makes it worthwhile is a bit subjective. All good.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1984
+  strm.flush();
+  const char *err = "error: .debug_line[0x00000000] row[1] decreases in "
+      "address from previous row:";
----------------
clayborg wrote:
> dblaikie wrote:
> > Probably make this a StringRef
> We search the std::string named "str" for this below and use must be a C string as part of the EXPECT_TRUE. No need for the StringRef here? Unless I wrap "str" into a StringRef. Let me know your preference here.
Could use SmallString instead of std::string (with raw_svector_ostream (SmallString is a StringVector)) & then SmallString's got a find(StringRef).


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1914
+  // the address decreases.
+  StringRef yamldata(R"(
+    debug_str:
----------------
Prefer copy-init over direct init where possible. ie:

  T v = u;

rather than

  T v(u);

(so 'StringRef yamldata = R"(... ' here)


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:2056
+
+  auto &DebugSections = *ErrOrSections;
+
----------------
can this be 'const auto &'? (or skip the variable and pass '*ErrOrSections' to the DWARFContextInMemory ctor directly)


https://reviews.llvm.org/D32754