Page MenuHomePhabricator

[safestack] Fixup llvm.dbg.value when rewriting unsafe allocas.
ClosedPublic

Authored by eugenis on Jun 3 2016, 2:09 PM.

Details

Reviewers
pcc
Summary

When moving unsafe allocas to the unsafe stack, dbg.declare intrinsics are
updated to refer to the new location.

This change does the same to dbg.value intrinsics.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 59620.Jun 3 2016, 2:09 PM
eugenis retitled this revision from to [safestack] Fixup llvm.dbg.value when rewriting unsafe allocas..
eugenis updated this object.
eugenis added a reviewer: pcc.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: llvm-commits.
pcc edited edge metadata.Jun 13 2016, 4:49 PM

Can you also add a test case for dynamic alloca?

It looks like the same thing could also be done for ASan (but that can be a separate change).

lib/Transforms/Utils/Local.cpp
1276

This won't terminate if there are any non-llvm.dbg.value uses of the value (or if replaceOneDbgValueForAlloca gives up).

In D20986#457042, @pcc wrote:

Can you also add a test case for dynamic alloca?

Done. I've noticed that this call with Offset=0 does not do anything so I removed it.
I also could not make clang emit a dbg.value for a dynamic alloca because dbg.declare-to-dbg.value optimization is done only for scalar allocas, so I hacked the IR a bit for the test.

lib/Transforms/Utils/Local.cpp
1276

fixed

eugenis updated this revision to Diff 60918.Jun 15 2016, 3:06 PM
eugenis edited edge metadata.
pcc added inline comments.Jun 15 2016, 3:31 PM
lib/Transforms/Utils/Local.cpp
1276

Please add negative tests covering both cases.

test/Transforms/SafeStack/debug-loc-dynamic.ll
4

I was confused about what you were trying to test here (given that you removed the call to replaceDbgValueForAlloca for dynamic allocas) until I realized that you were just testing that the IR is unchanged. Can you please explicitly mention this?

test/Transforms/SafeStack/debug-loc2.ll
20

Do you want to match %[[USP]] here?

24

Same here

eugenis updated this revision to Diff 61031.Jun 16 2016, 2:45 PM
eugenis marked 4 inline comments as done.

debug-loc2.ll test extended to cover the two negative cases.

pcc accepted this revision.Jun 16 2016, 3:39 PM
pcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 16 2016, 3:39 PM
eugenis closed this revision.Jun 16 2016, 3:41 PM

r272968.
Thanks for the review!