[PATCH] D143513: [DebugInfo] Allow parsing line tables aligned to 4 or 8-byte boundaries

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1508
 
+bool DWARFDebugLine::SectionParser::lineTableHeaderHasValidVersion(
+    uint64_t Offset) {
----------------
We know we're in the DWARFDebugLine class, so we can simplify this function name somewhat. Perhaps to `headerHasValidVersion` or even simply `hasValidVersion`.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1514-1517
+  if (!Cursor) {
+    consumeError(Cursor.takeError());
+    return false;
+  }
----------------
I'm not convinced you should be throwing away the error here. At the very least, there should be a comment explaining why it is reasonable to do so.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1537-1538
+
+  // Heuristic: If the version is valid, then this is probably a line table,
+  // otherwise the offset might need alignment.
+  if (lineTableHeaderHasValidVersion(Offset))
----------------



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1547-1548
+  for (unsigned Align : PossibleAlignments) {
+    uint64_t AlignedOffset =
+        (Offset + (Align - 1)) & ~static_cast<uint64_t>(Align - 1);
+    if (!DebugLineData.isValidOffset(AlignedOffset)) {
----------------
Can you use `alignTo` to do this alignment?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1553
+      Done = true;
+      return;
+    }
----------------
Does this return imply that the `PossibleAlignments` must be in ascending order? That's fine for now, but I wonder if it's a bit brittle (I guess you could argue that if there's not enough space for alignment, it's not going to be big enough for a line table header, so it's moot, but if you want to rely on that, it should at least be explained in the comment, in case something changes in the future).


================
Comment at: llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s:4-6
+// This test is based on a real example from ARM C/C++ Compiler.
+// It verifies llvm-dwarfdump is able to dump line tables even if they've been
+// placed at aligned offsets.
----------------
It seems a little odd to be mixing comment marker styles (`#` and `//`). In newer tests, I tend to go with `#` for lit directives (like you have done) and `##` for real comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s:22
+# CHECK: min_inst_length: 2
+# CHECK-NOT: warning: parsing line table prologue at offset 0x00000027: unsupported version 512
+# CHECK: default_is_stmt: 1
----------------
This CHECK-NOT will be quite fragile - if somebody changes the warning wording for whatever reason, it will fail to catch the warning, should it be emitted. Instead, I'd just do `CHECK-NOT: warning:`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143513