Page MenuHomePhabricator

[compiler-rt] Fix alloca_instruments_all_paddings.cc test to work under higher -O levels
ClosedPublic

Authored by kubamracek on Feb 21 2015, 2:35 AM.

Details

Summary

Based on the failure from http://reviews.llvm.org/D7741. In alloca_instruments_all_paddings.cc we’re using a dynamic alloca, which is being instrumented, but under higher -O levels all the other allocas are being optimized out. So we end up with only a single dynamic alloca to instrument, and zero static allocas. In FunctionStackPoisoner::poisonStack we then have:

assert(AllocaVec.size() > 0 || DynamicAllocaVec.size() > 0);

if (ClInstrumentAllocas)
  // Handle dynamic allocas.
  for (auto &AllocaCall : DynamicAllocaVec)
    handleDynamicAllocaCall(AllocaCall);

if (AllocaVec.size() == 0) return;
…rest of the function

So we skip the “rest of the function” when there are no static allocas (even when we do have dynamic allocas). However, the “rest of the function” is important, as it ensures that the stack is un-poisoned before return. At the very end of the function we have:

if (ClInstrumentAllocas)
  // Unpoison dynamic allocas.
  for (auto &AllocaCall : DynamicAllocaVec)
    unpoisonDynamicAlloca(AllocaCall);

which doesn’t get executed in this case, but it should. This fix just includes this code when we return early from the AllocaVec.size() check in the beginning of the function.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 20462.Feb 21 2015, 2:35 AM
kubamracek retitled this revision from to [compiler-rt] Fix alloca_instruments_all_paddings.cc test to work under higher -O levels.
kubamracek updated this object.
kubamracek edited the test plan for this revision. (Show Details)
kubamracek added subscribers: Unknown Object (MLST), kcc, zaks.anna and 2 others.
samsonov added inline comments.
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1650–1652

Is it wrong to just call

unpoisonDynamicAlloca(AllocaCall);

here, and leave the rest of the function the same?

kubamracek updated this revision to Diff 20473.Feb 22 2015, 3:10 AM

Updating patch.

samsonov accepted this revision.Feb 23 2015, 11:50 AM
samsonov added a reviewer: samsonov.

LGTM

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1647

Please add {} for this if as well.

This revision is now accepted and ready to land.Feb 23 2015, 11:50 AM
kubamracek closed this revision.Feb 24 2015, 1:50 AM

Landed in r230316 (llvm) and r230317 (compiler-rt).