[PATCH] D152536: [DWARF] Allow op-index in line number programs

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:643-644
+  uint64_t AddrOffset =
+      ((Row.OpIndex + OperationAdvance) * LineTable->Prologue.MinInstLength) /
+      MaxOpsPerInst;
   Row.Address.Address += AddrOffset;
----------------
StephenTozer wrote:
> If I understand correctly this calculation is incorrect, the division by MaxOpsPerInst comes before the multiply by MinInstLength.
> 
> For example, if `MaxOpsPerInst = 4, MinInstLength = 8, Row.OpIndex = 1, OperationAdvance = 2`, then this calculation would give `AddrOffset = ((2 + 1) * 8) / 4 = 6`, when it //should// be `((2 + 1) / 4) * 8 = 0` since we're actually advancing to a different OpIndex within the same instruction.
Thanks for catching that! I have updated the test fixture with MinInstLength > 1 as you suggest to test this.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1253-1254
+  //
+  // TODO: This function, and its users, needs to be update to return multiple
+  // rows for bundles with multiple op-indexes.
   DWARFDebugLine::Row Row;
----------------
StephenTozer wrote:
> Any idea about the complexity of implementing this? It seems to me that most users either 1) already want a range, in which case this function can just return a pair of `{firstIndex, lastIndex}` and the caller can determine the range, or 2) want the info for a specific operation, in which case an OpIndex needs to be passed. I've not personally done anything with VLIW architectures but I'm assuming it's possible for different operations in the same instruction to have different source locations and even be in different inline frames, in which case it would be necessary to update a dozen or so callsites to pass an OpIndex through - seems like it would still be fairly simple in terms of code though.
I have some downstream WIP patches that starts addressing 1). I suspect it is mainly a lot of mechanical reworking of the code to propagate that information to the tools, but I can't show my working yet. I am not sure in how many cases 2) would be needed, but it seems that that for example could be useful if, whatever you are running a test on, can pinpoint which individual operation in a bundle that triggered a crash/interrupt.

There are some user interface decisions that need to be made, e.g. what the output for llvm-addr2line --inlines should look like when printing an address with multiple op-indexes. The locations for such addresses can, as you say, originate from different inline frames.

My hope is that, following some framework changes, mostly in this file, we can gradually enable proper op-index support in tools. Until we get to that point with a tool/option we should just be able to pick one of the locations if handling an address with multiple op-indexes.


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

https://reviews.llvm.org/D152536