[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: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:
> dstenb wrote:
> > dstenb wrote:
> > > StephenTozer wrote:
> > > > dstenb wrote:
> > > > > 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.
> > > > I have some concerns about returning information that could be misleading (by misattributing the source location of an operation), since that can be worse than getting no information at all. I think it would be a suitable stopgap to disable those tools for line tables with `MaxOperationsPerInst > 1`, since that ensures that we are no worse off in any case. Open to other options though, or a justification for allowing the "incorrect" output.
> > > I don't have any stronger justification than that I personally see value in at least getting some line information for an address belonging to a bundle, rather than none. I know that Binutils can parse line number programs with op-index, but I don't know what e.g. addr2line will emit for such cases. I have unfortunately not been able to try that out with a non-modified GDB on a mainstream target.
> > > 
> > > But I fully understand your concern, and that would in some sense go against LLVM's philosophy, e.g. as we drop incompatible locations when merging instructions.
> > > 
> > > If it is preferred, I can add an assert to `findRowInSeq()` that catches if the op-index is larger than 0? Then we will be able to correctly parse line number programs with `maximum_operation_per_instruction > 1`, correctly fetch line number information for addresses with a single operation or multiple operations with identical locations in such programs, and we can then "bubble out" the assert towards the tools as we gradually add proper op-index support.
> > > 
> > > That would change the "line table program at offset [...] contains a special opcode at offset [...], but the prologue maximum_operations_per_instruction value is [...], which is unsupported. Assuming a value of 1 instead" error to an assert (which would only hit when retrieving line information for an address with multiple operations), which maybe is not acceptable?
> > I guess it should be an llvm_unreachable rather than assert though, as we'd otherwise basically run into the same issue in a non-assert build.
> I agree that replacing an error message with an assert may be a problem - perhaps the answer would be to keep the existing error message declaring that `maximum_operation_per_instruction > 1` is not supported, but adjust it to indicate that support is incomplete/experimental and may produce incorrect information? The ideal is that we never produce wrong information, but as you point out: we're already producing "wrong" information for VLIW line tables by ignoring `maximum_operation_per_instruction`, so I don't think the patch should be blocked on not fixing everything.
Thanks, that is definitely preferable! I've done that in the latest diff.


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

https://reviews.llvm.org/D152536