[llvm] r262519 - Really fix ASAN leak/etc issues with MemorySSA unittests

[Daniel Berlin via llvm-commits]

George is looking into what to do here.
They get handed to users and used by them, so it's practically owned by the
memory ssa pass, but handed to callers.

It would be really nice to have it so if they use it after memoryssa dies,
they get an obvious error instead of use after free.

On Wed, Mar 2, 2016 at 1:49 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Wed, Mar 2, 2016 at 1:16 PM, Daniel Berlin via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: dannyb
>> Date: Wed Mar  2 15:16:28 2016
>> New Revision: 262519
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=262519&view=rev
>> Log:
>> Really fix ASAN leak/etc issues with MemorySSA unittests
>>
>> Modified:
>>     llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp
>>     llvm/trunk/unittests/Transforms/Utils/MemorySSA.cpp
>>
>> Modified: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp?rev=262519&r1=262518&r2=262519&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp Wed Mar  2 15:16:28 2016
>> @@ -231,7 +231,7 @@ MemorySSAWalker *MemorySSA::buildMemoryS
>>    assert(!this->AA && !this->DT &&
>>           "MemorySSA without a walker already has AA or DT?");
>>
>> -  auto *Result = new CachingMemorySSAWalker(this, AA, DT);
>> +  Walker = new CachingMemorySSAWalker(this, AA, DT);
>>
>
> Any chance of using unique_ptr to manage the lifetime of the Walker(s)?
>
>
>>    this->AA = AA;
>>    this->DT = DT;
>>
>> @@ -343,7 +343,7 @@ MemorySSAWalker *MemorySSA::buildMemoryS
>>      for (auto &MA : *Accesses) {
>>        if (auto *MU = dyn_cast<MemoryUse>(&MA)) {
>>          Instruction *Inst = MU->getMemoryInst();
>> -        MU->setDefiningAccess(Result->getClobberingMemoryAccess(Inst));
>> +        MU->setDefiningAccess(Walker->getClobberingMemoryAccess(Inst));
>>        }
>>      }
>>    }
>> @@ -354,7 +354,6 @@ MemorySSAWalker *MemorySSA::buildMemoryS
>>      if (!Visited.count(&BB))
>>        markUnreachableAsLiveOnEntry(&BB);
>>
>> -  Walker = Result;
>>    return Walker;
>>  }
>>
>>
>> Modified: llvm/trunk/unittests/Transforms/Utils/MemorySSA.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Transforms/Utils/MemorySSA.cpp?rev=262519&r1=262518&r2=262519&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/unittests/Transforms/Utils/MemorySSA.cpp (original)
>> +++ llvm/trunk/unittests/Transforms/Utils/MemorySSA.cpp Wed Mar  2
>> 15:16:28 2016
>> @@ -21,6 +21,7 @@ using namespace llvm;
>>
>>  TEST(MemorySSA, RemoveMemoryAccess) {
>>    LLVMContext &C(getGlobalContext());
>> +  std::unique_ptr<Module> M(new Module("Remove memory access", C));
>>    IRBuilder<> B(C);
>>    DataLayout DL("e-i64:64-f80:128-n8:16:32:64-S128");
>>    TargetLibraryInfoImpl TLII;
>> @@ -29,13 +30,13 @@ TEST(MemorySSA, RemoveMemoryAccess) {
>>    // We create a diamond where there is a store on one side, and then a
>> load
>>    // after the merge point.  This enables us to test a bunch of different
>>    // removal cases.
>> -  std::unique_ptr<Function> F(Function::Create(
>> +  Function *F = Function::Create(
>>        FunctionType::get(B.getVoidTy(), {B.getInt8PtrTy()}, false),
>> -      GlobalValue::ExternalLinkage, "F"));
>> -  BasicBlock *Entry(BasicBlock::Create(C, "", F.get()));
>> -  BasicBlock *Left(BasicBlock::Create(C, "", F.get()));
>> -  BasicBlock *Right(BasicBlock::Create(C, "", F.get()));
>> -  BasicBlock *Merge(BasicBlock::Create(C, "", F.get()));
>> +      GlobalValue::ExternalLinkage, "F", M.get());
>> +  BasicBlock *Entry(BasicBlock::Create(C, "", F));
>> +  BasicBlock *Left(BasicBlock::Create(C, "", F));
>> +  BasicBlock *Right(BasicBlock::Create(C, "", F));
>> +  BasicBlock *Merge(BasicBlock::Create(C, "", F));
>>    B.SetInsertPoint(Entry);
>>    B.CreateCondBr(B.getTrue(), Left, Right);
>>    B.SetInsertPoint(Left);
>> @@ -49,10 +50,10 @@ TEST(MemorySSA, RemoveMemoryAccess) {
>>    std::unique_ptr<MemorySSA> MSSA(new MemorySSA(*F));
>>    std::unique_ptr<DominatorTree> DT(new DominatorTree(*F));
>>    std::unique_ptr<AssumptionCache> AC(new AssumptionCache(*F));
>> -  AAResults *AA = new AAResults(TLI);
>> -  BasicAAResult *BAA = new BasicAAResult(DL, TLI, *AC, &*DT);
>> -  AA->addAAResult(*BAA);
>> -  MemorySSAWalker *Walker = MSSA->buildMemorySSA(AA, &*DT);
>> +  AAResults AA(TLI);
>> +  BasicAAResult BAA(DL, TLI, *AC, &*DT);
>> +  AA.addAAResult(BAA);
>> +  std::unique_ptr<MemorySSAWalker> Walker(MSSA->buildMemorySSA(&AA,
>> &*DT));
>>    // Before, the load will be a use of a phi<store, liveonentry>. It
>> should be
>>    // the same after.
>>    MemoryUse *LoadAccess =
>> cast<MemoryUse>(MSSA->getMemoryAccess(LoadInst));
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160302/f276b2b4/attachment.html>