This is an archive of the discontinued LLVM Phabricator instance.

[asan] Invalid debug info for promotable allocas
ClosedPublic

Authored by kubamracek on Jul 14 2015, 12:48 AM.

Details

Summary

Hi everyone,

Since r230724 ("Skip promotable allocas to improve performance at -O0"), there is a regression in the generated debug info for those non-instrumented variables. When inspecting such a variable's value in LLDB, you often get garbage instead of the actual value. Fred was able to track this down:

What happens is quite sad: The variable doesn’t get instrumented and thus should behave like a standard stack variable. However, ASAN’s instrumentation is inserted before the creation of the non-instrumented alloca. The only allocas that are considered standard stack variables are the ones declared in the first basic-block, but the initial instrumentation setup in the function breaks that invariant. Indeed, if moving the alloca to the first BB, things work. (This shows that the llvm.dbg.declare intrinsic is badly defined. Depending on whether the alloca it refers to is in the first BB or not, it means something quite different…)

The simplest way that I can think of preventing this issue is to make sure that the uninstrumented allocas stay in the first BB. I think this is safe, because static promotable allocas shouldn’t depend on any other value, and moving them up they will still dominate their uses, thus the SSA form should be maintained. I might very well be missing some corner cases though. The following patch does that.

Diff Detail

Event Timeline

kubamracek retitled this revision from to [asan] Invalid debug info for promotable allocas.
kubamracek updated this object.
kubamracek added reviewers: samsonov, glider, kcc.
kubamracek added subscribers: llvm-commits, samsonov, glider and 4 others.
kcc edited edge metadata.Jul 14 2015, 9:37 AM

Two style comments.
Also, please add a test.

lib/Transforms/Instrumentation/AddressSanitizer.cpp
528

NonInstrumentedStaticAllocaVec

1741

This code tends to not have {} in such case.

kubamracek updated this revision to Diff 29760.Jul 15 2015, 3:05 AM
kubamracek edited edge metadata.

Added test. Addressed style comments.

kcc added inline comments.Jul 15 2015, 2:17 PM
test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll
14

Please comment why %non_instrumented will not get instrumented.

22

Please make sure the test works in a non-asserts build.
I remember there were issues with non-asserts build removing the var names.

samsonov added inline comments.Jul 15 2015, 3:12 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1741

According to logic, this should be InsBefore, not AllocaVec[0].

kubamracek updated this revision to Diff 29881.Jul 16 2015, 3:54 AM

Addressing review comments. I checked that the test passes in a release (RelWithDebInfo) build.

glider edited edge metadata.Jul 16 2015, 5:45 AM

LGTM as soon as others are happy.

kcc accepted this revision.Jul 16 2015, 2:49 PM
kcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 16 2015, 2:49 PM
This revision was automatically updated to reflect the committed changes.