[PATCH] D115343: [DwarfDebug] Refuse to emit DW_OP_LLVM_arg values wider than 64 bits

[Ricky Zhou via Phabricator via llvm-commits]

rickyz created this revision.
Herald added a subscriber: hiraditya.
rickyz updated this revision to Diff 392789.
rickyz added a comment.
rickyz updated this revision to Diff 392791.
rickyz edited the summary of this revision.
rickyz updated this revision to Diff 393045.
rickyz marked 2 inline comments as done.
rickyz edited the summary of this revision.
rickyz added a reviewer: aprantl.
rickyz published this revision for review.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Minor rewording/test cleanup.


rickyz added a comment.

Update commit message.


Mordante added a comment.

Thanks for working on this!

I just tested this patch with D70631 <https://reviews.llvm.org/D70631>, the review where I discovered the issue.
After I applied your patch the build issue I had has been resolved.


rickyz added a comment.

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



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:802
         APInt RawBytes = Entry.getConstantFP()->getValueAPF().bitcastToAPInt();
+        if (RawBytes.getBitWidth() > 64)
+          return false;
----------------
Can you add a comment explaining what the more principled fix would be to the code you added?


================
Comment at: llvm/test/DebugInfo/X86/pr52584.ll:1
+; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu -o /dev/null
+
----------------
Can you FileCheck for something (ideally the missing DW_AT_location) here? Otherwise this test even succeeds if you symlink llc to /bin/true.


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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115343

Files:
  llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
  llvm/test/DebugInfo/X86/pr52584.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D115343.393045.patch
Type: text/x-patch
Size: 7224 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211209/867de8e5/attachment.bin>