This is an archive of the discontinued LLVM Phabricator instance.

[DwarfDebug] Refuse to emit DW_OP_LLVM_arg values wider than 64 bits
ClosedPublic

Authored by rickyz on Dec 8 2021, 6:54 AM.

Details

Summary

DwarfExpression::addUnsignedConstant(const APInt &Value) only supports
wider-than-64-bit values when it is used to emit a top-level DWARF
expression representing the location of a variable. Before this change,
it was possible to call addUnsignedConstant on >64 bit values within a
subexpression when substituting DW_OP_LLVM_arg values.

This can trigger an assertion failure (e.g. PR52584, PR52333) when it
happens in a fragment (DW_OP_LLVM_fragment) expression, as
addUnsignedConstant on >64 bit values splits the constant into separate
DW_OP_pieces, which modifies DwarfExpression::OffsetInBits.

This change papers over the assertion errors by bailing on overly wide
DW_OP_LLVM_arg values. A more comprehensive fix might be to be to split
wide values into pointer-sized fragments.

[0] https://github.com/llvm/llvm-project/blob/e71fa03/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp#L799-L805

Diff Detail

Event Timeline

rickyz created this revision.Dec 8 2021, 6:54 AM
rickyz updated this revision to Diff 392789.Dec 8 2021, 8:17 AM

Minor rewording/test cleanup.

rickyz updated this revision to Diff 392791.Dec 8 2021, 8:18 AM
rickyz edited the summary of this revision. (Show Details)

Update commit message.

aprantl added a subscriber: aprantl.Dec 8 2021, 8:51 AM
aprantl added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
802

Can you add a comment explaining what the more principled fix would be to the code you added?

llvm/test/DebugInfo/X86/pr52584.ll
2

Can you FileCheck for something (ideally the missing DW_AT_location) here? Otherwise this test even succeeds if you symlink llc to /bin/true.

Thanks for working on this!

I just tested this patch with D70631, the review where I discovered the issue.
After I applied your patch the build issue I had has been resolved.

rickyz updated this revision to Diff 393045.Dec 8 2021, 11:51 PM
rickyz marked 2 inline comments as done.
rickyz edited the summary of this revision. (Show Details)

Respond to comments, avoid emitting a partially-filled DW_AT_location on encountering an overly wide argument.

rickyz published this revision for review.Dec 8 2021, 11:53 PM
rickyz added a reviewer: aprantl.
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2021, 11:53 PM
rickyz updated this revision to Diff 393053.Dec 9 2021, 12:08 AM

Minor comment wording change.

aprantl accepted this revision.Dec 9 2021, 9:18 AM

Thanks!

This revision is now accepted and ready to land.Dec 9 2021, 9:18 AM
rickyz added a comment.Dec 9 2021, 1:31 PM

Thanks for the review! I don't have commit access, so can you please commit this on my behalf? Thanks!

This revision was landed with ongoing or failed builds.Dec 10 2021, 9:33 AM
This revision was automatically updated to reflect the committed changes.