This is an archive of the discontinued LLVM Phabricator instance.

Align definition of DW_OP_plus with DWARF spec [3/3]
ClosedPublic

Authored by sdesmalen on Jun 5 2017, 6:46 AM.

Details

Summary

This patch is part of 3 patches that together form a single patch, but must be introduced in stages in order not to break things.

The way that LLVM interprets DW_OP_plus in DIExpression nodes is basically that of the DW_OP_plus_uconst operator since LLVM expects an unsigned constant operand. This unnecessarily restricts the DW_OP_plus operator, preventing it from being used to describe the evaluation of runtime values on the expression stack. These patches try to align the semantics of DW_OP_plus and DW_OP_minus with that of the DWARF definition, which pops two elements off the expression stack, performs the operation and pushes the result back on the stack.

This is done in three stages:
• The first patch (LLVM) adds support for DW_OP_plus_uconst.
• The second patch (Clang) contains changes all its uses from DW_OP_plus to DW_OP_plus_uconst.
• The third patch (LLVM) changes the semantics of DW_OP_plus and DW_OP_minus to be in line with its DWARF meaning. This patch includes the bitcode upgrade from legacy DIExpressions.

Patch by Sander de Smalen.

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.Jun 5 2017, 6:46 AM
aprantl requested changes to this revision.Jun 5 2017, 8:52 AM

This also needs a bitcode upgrade to transform the old use of DW_OP_minus to the new one. See my comment on D33892 for details.

This revision now requires changes to proceed.Jun 5 2017, 8:52 AM
sdesmalen updated this revision to Diff 101560.Jun 6 2017, 7:10 AM
sdesmalen edited edge metadata.

Incremented the expression's Version operand and added backwards compatibility with older bitcode files by constructing a {DW_OP_constu, <opnd>, DW_OP_minus} from a {DW_OP_minus, <opnd>}.

sdesmalen updated this revision to Diff 101720.Jun 7 2017, 6:01 AM
sdesmalen updated this revision to Diff 101923.Jun 8 2017, 8:05 AM

rebased to patch [1/3]

Ah ok, I see. The cleanest way to do this would be to

  1. Implement support for DW_OP_plus_constu in LLVM
  2. Switch over Clang to use DW_OP_plus_constu

3a. Retire the old DW_OP_plus and implement the bitcode upgrade in LLVM
3b. Retire the old DW_OP_minus (AFAIK not used by clang) and implement the bitcode upgrade in LLVM
(The last two can be one joined commit so we only need to bump the DIExpression version number once and keep the upgrade code simple).

I understand that this may sound like a lot of busywork at first, but the code is already there and with some git rebase -i and git commit -p reordering the patches this way should go relatively fast. It would have the big advantage that each commit can be committed individually and safely be reverted. (The current patches increment the version number in 1/3 and implement the bitcode upgrade for DW_OP_minus later in 3/3).

Would this work for you?

sdesmalen updated this revision to Diff 102163.Jun 12 2017, 2:57 AM
sdesmalen edited the summary of this revision. (Show Details)

Patch [1/3], now just adds support for DW_OP_plus_uconst.
Patch [3/3] changes meaning of DW_OP_plus and DW_OP_minus to be in line with its DWARF meaning. This patch includes the bitcode upgrade from legacy DIExpressions.

Hi Adrian, thanks for your thorough review! I agree with reordering the functionality within the patches, it is better to have only a single bitcode upgrade.
I have updated the patches accordingly, hopefully they're getting in the right shape now :)

aprantl accepted this revision.Jun 12 2017, 9:19 AM

This generally LGTM, there is only a bitcode upgrade test for DW_OP_minus missing. I'm marking the review as accepted under the condition that the missing test is added before committing.

test/Bitcode/DIExpression-deref.ll
18 ↗(On Diff #102163)

Please also add a test for the old DW_OP_minus. (If you choose to add it to this testcase, you will need to check out a much older version of llvm, rebuild llvm-dis, and re-create the .bc file with it. Otherwise you can add a new upgrade test just for DW_OP_minus and assemble it with today's trunk).

This revision is now accepted and ready to land.Jun 12 2017, 9:19 AM
sdesmalen added inline comments.Jun 12 2017, 9:29 AM
test/Bitcode/DIExpression-deref.ll
18 ↗(On Diff #102163)

Good spot, I'll add a test for the DW_OPminus too.

In order for me to do that, what is the preferred way get changes to binary files as a patch file that I can upload to Phabricator?
When I tried this previously (having creating a patch file using 'git diff --binary'), Phabricator was not able to parse the patch file.

IIRC git diff -U9999 should work just fine. In any case I am not going to review the binary file — I am just going to trust that it is what you say it is in the accompanying testcase :-)

sdesmalen updated this revision to Diff 102343.Jun 13 2017, 9:00 AM

Added test for DW_OP_minus bitcode upgrade:

  • test/Bitcode/DIExpression-minus-upgrade.ll
  • test/Bitcode/DIExpression-minus-upgrade.ll.bc
aprantl accepted this revision.Jun 13 2017, 9:02 AM

Looks good. Please watch the stage2 and LTO bots carefully after committing.

sdesmalen updated this revision to Diff 102521.Jun 14 2017, 4:10 AM

Somehow I missed unit-test test/DebugInfo/MIR/ARM/split-superreg-complex.mir, so updated it.
It should pass all unit tests now.

fhahn edited the summary of this revision. (Show Details)Jun 14 2017, 6:13 AM
fhahn added a subscriber: fhahn.
fhahn closed this revision.Jun 14 2017, 6:15 AM
This revision was automatically updated to reflect the committed changes.