This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Allow op-index in line number programs
ClosedPublic

Authored by dstenb on Jun 9 2023, 7:00 AM.

Details

Summary

This extends DWARFDebugLine to properly parse line number programs with
maximum_operations_per_instruction > 1 for VLIW targets.

No functions that use that parsed output to retrieve line information
have been extended to support multiple op-indexes. This means that when
retrieving information for an address with multiple op-indexes, e.g.
when using llvm-addr2line, the penultimate row for that address will be
used, which in most cases is the row for the second largest op-index.
This will be addressed in further changes, but this patch at least
allows us to correctly parse such line number programs, with a warning
showing that the line number information may be incorrect (incomplete).

Diff Detail

Event Timeline

dstenb created this revision.Jun 9 2023, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2023, 7:00 AM
dstenb requested review of this revision.Jun 9 2023, 7:00 AM

Sending a gentle ping on this (and the parent revision).

I assume that making upstream VLIW targets emit line number programs with maximum_operations_per_instruction > 1 will require quite a bit of work, so the immediate gains of these patches are small in that regard, but perhaps this is interesting for some other downstream VLIW target?

Largely LGTM, some comments inline.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
670–671

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.

1279–1280

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.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
1092–1093

It's not technically a part of supporting op-index, but might be a good idea to also include a MinInstLength > 1 in this test - as mentioned above I think the calculation for the address offset is wrong if MinInstLength > 1, which I expect it would be for an actual VLIW line table.

dstenb updated this revision to Diff 536787.Jul 3 2023, 8:55 AM
dstenb marked an inline comment as done.

Address review comments.

dstenb added inline comments.Jul 3 2023, 8:57 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
670–671

Thanks for catching that! I have updated the test fixture with MinInstLength > 1 as you suggest to test this.

1279–1280

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.

StephenTozer added inline comments.Jul 3 2023, 1:24 PM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1279–1280

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.

dstenb added inline comments.Jul 7 2023, 2:41 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1279–1280

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?

dstenb marked an inline comment as not done.Jul 7 2023, 2:41 AM
dstenb added inline comments.Jul 7 2023, 3:35 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1279–1280

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.

StephenTozer added inline comments.Jul 7 2023, 4:35 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1279–1280

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.

dstenb updated this revision to Diff 539055.Jul 11 2023, 6:23 AM
dstenb edited the summary of this revision. (Show Details)
  • Add recoverable error describing that support for maximum_operations_per_instruction > 1 is experimental, and that the line number information may be incorrect.
  • Bring back an error for maximum_operations_per_instruction = 0, and upgrade that error message from "not supported", to "invalid".
  • This replaces the MaxOpsPerInstFixture with two explicit test cases for maximum_operations_per_instruction = 0. I could not reuse that fixture for maximum_operations_per_instruction > 1, since the advance would be done with MaxOpsPerInst > 1 so the existing expects in the fixture would fail.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1279–1280

Thanks, that is definitely preferable! I've done that in the latest diff.

StephenTozer accepted this revision.Jul 11 2023, 7:31 AM

Changes LGTM!

This revision is now accepted and ready to land.Jul 11 2023, 7:31 AM

Thanks for the review! Will push this shortly.

This revision was landed with ongoing or failed builds.Jul 12 2023, 4:48 AM
This revision was automatically updated to reflect the committed changes.