[PATCH] D58952: [llvm] Skip over empty line table entries.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Hmm, trying to stare at this function it's confusing me a fair bit. I'd expect this to be more obvious/legible than it is, but it's possible I'm just misunderstanding.

For instance, I don't know how this bit of code:

  if (RowPos->Address.Address > Address.Address) {

Ever happens - if the initial condition in the function (Seq.containsPC(Address)) is satisfied.

Similarly I think:

  while (RowPos + 1 < LastRow && RowPos->Address.Address == (RowPos + 1)->Address.Address) {

I /think/ this loop should never exit through the first condition, again, if Seq.containtsPC(Address) is true - since we searched for an address that is within the sequence, we can't have been sorting for the address in the last row (because that address isn't in the sequence - because it's half open (exclusive of the last element)).

I hope this can all be simplified a bit. I'm thinking something like...

  if (!containsPC)
    return Unknown
  RowPos = lower_bound
  if (RowPos->Address != Address)
    --RowPos; //no further bounds checking needed - could have some asserts (because if the sequence contains the address, and the row we find is > the address, it must have an earlier row
  while (next(RowPos)->Address == RowPos->Address) //again, no bounds checking required, RowPos can't be the last row here for similar reasons
    ++RowPos
  return Seq.FirstRowIndex + (RowPos - FirstRow);

Does this seem OK? Does it not account for some cases?



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:882-883
+  }
+  // In some cases, e.g. first instruction in a function, the compiler generates
+  // two entries, both with the same address. We want the last one.
+  // There are 2 cases wrte RowPos and the addresses in records before/after it:
----------------
I'd still be inclined to describe this as "a zero-length address range" or similar, rather than two entries for the same address - though I realize that gets into the weird semantic debate murky waters so maybe it's not worth worrying about.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:884
+  // two entries, both with the same address. We want the last one.
+  // There are 2 cases wrte RowPos and the addresses in records before/after it:
+  // 1) RowPos's address is the one we looked for. In this case, we want to
----------------
'wrte'? Is that a typo of 'wrt'?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:886
+  // 1) RowPos's address is the one we looked for. In this case, we want to
+  // skip any potential empty records.
+  // 2) RowPos's address is less than the one we looked for. In that case, we
----------------
'empty ranges' perhaps? (since each row itself doesn't represent a thing that is empty/non-empty - it's pairs of rows that define ranges and those ranges can be empty)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:888
+  // 2) RowPos's address is less than the one we looked for. In that case, we
+  // arrived here by first having had found the first range with a greater address,
+  // then decrementing 1. If the address of this range is part of a sequence of
----------------
s/having had/having/ I think reads more smoothly?


================
Comment at: llvm/test/tools/llvm-symbolizer/only-empty-ranges.s:22
+#
+# 0x0 should pick the second line in the table - line 1, col 12
+# CHECK:    func
----------------
line /2/, col 12 I think? (


================
Comment at: llvm/test/tools/llvm-symbolizer/only-empty-ranges.s:25-26
+# CHECK:	/scratch/a.cpp:2:12
+# 0x2 does not exist in the table, so we should pick the closest line with a
+# lower address - in this case, the second line.
+# CHECK:    func
----------------
Not only the closest with a lower address, but the one that describes that range.

eg: 0x4 or 0x5 shouldn't be described by line 3, column 3 - they aren't described by this line table at all, even though it's the "closest line with a lower address".


================
Comment at: llvm/test/tools/llvm-symbolizer/only-empty-ranges.s:29-30
+# CHECK:	/scratch/a.cpp:2:12
+# 0x3 also has a sequence of empty ranges, so we should pick the last one.
+# This also verifies we don't attempt to access outside boundaries.
+# CHECK:    func
----------------
"the last one" sounds like "the last one of the empty ranges" which isn't quite right - we want to ignore all the empty ranges and pick the range that covers 0x3 (necessarily a non-empty one).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58952