[llvm-commits] [llvm] r78127 - in /llvm/trunk: lib/ExecutionEngine/ExecutionEngine.cpp unittests/ExecutionEngine/ExecutionEngineTest.cpp unittests/ExecutionEngine/Makefile

[Jeffrey Yasskin]

On Tue, Aug 4, 2009 at 7:30 PM, Chris Lattner<clattner at apple.com> wrote:
>
> On Aug 4, 2009, at 4:53 PM, Jeffrey Yasskin wrote:
>
>> Author: jyasskin
>> Date: Tue Aug  4 18:53:16 2009
>> New Revision: 78127
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=78127&view=rev
>> Log:
>> Make ExecutionEngine::updateGlobalMapping(GV, NULL) properly remove
>> GV's old
>> address from the reverse mapping, and add a test that this works now.
>
> Hey Jeffrey,
>
> Should the GlobalMapping move to using AssertingVH?  I think that
> would define this class of bugs away.

I'd actually like to move them and the other maps in ExecutionEngine
to using CallbackVH so that users don't have to deal with
removing/changing global mappings unless the object actually moves.

But whether or not I do that, this bug made it impossible to do things like:

T1 *obj1 = new T1;
engine.addGlobalMapping(GV, obj1);
...
engine.updateGlobalMapping(GV, NULL);
delete obj1;
...
T2 *obj2 = new T2;  // Happens to land at the same address.
engine.addGlobalMapping(UnrelatedGV, obj2);

and I don't see how AssertingVH would make any of that illegal or work better.

>>
>> Added:
>>    llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp
>> Modified:
>>    llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp
>>    llvm/trunk/unittests/ExecutionEngine/Makefile
>>
>> Modified: llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp?rev=78127&r1=78126&r2=78127&view=diff
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> ======================================================================
>> --- llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp (original)
>> +++ llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp Tue Aug  4
>> 18:53:16 2009
>> @@ -179,7 +179,7 @@
>>     }
>>
>>     if (!state.getGlobalAddressReverseMap(locked).empty())
>> -      state.getGlobalAddressReverseMap(locked).erase(Addr);
>> +      state.getGlobalAddressReverseMap(locked).erase(OldVal);
>>     return OldVal;
>>   }
>>
>>
>> Added: llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp?rev=78127&view=auto
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> ======================================================================
>> --- llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp
>> (added)
>> +++ llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp Tue
>> Aug  4 18:53:16 2009
>> @@ -0,0 +1,92 @@
>> +//===- ExecutionEngineTest.cpp - Unit tests for ExecutionEngine
>> -----------===//
>> +//
>> +//                     The LLVM Compiler Infrastructure
>> +//
>> +// This file is distributed under the University of Illinois Open
>> Source
>> +// License. See LICENSE.TXT for details.
>> +//
>> +//
>> =
>> =
>> =
>> ----------------------------------------------------------------------=
>> ==//
>> +
>> +#include "llvm/DerivedTypes.h"
>> +#include "llvm/GlobalVariable.h"
>> +#include "llvm/LLVMContext.h"
>> +#include "llvm/Module.h"
>> +#include "llvm/ADT/OwningPtr.h"
>> +#include "llvm/ExecutionEngine/Interpreter.h"
>> +#include "gtest/gtest.h"
>> +
>> +using namespace llvm;
>> +
>> +namespace {
>> +
>> +class ExecutionEngineTest : public testing::Test {
>> +protected:
>> +  ExecutionEngineTest()
>> +    : M(new Module("<main>", getGlobalContext())),
>> +      Engine(EngineBuilder(M).create()) {
>> +  }
>> +
>> +  virtual void SetUp() {
>> +    ASSERT_TRUE(Engine.get() != NULL);
>> +  }
>> +
>> +  GlobalVariable *NewExtGlobal(const Type *T, const Twine &Name) {
>> +    return new GlobalVariable(*M, T, false,  // Not constant.
>> +                              GlobalValue::ExternalLinkage, NULL,
>> Name);
>> +  }
>> +
>> +  Module *const M;
>> +  const OwningPtr<ExecutionEngine> Engine;
>> +};
>> +
>> +TEST_F(ExecutionEngineTest, ForwardGlobalMapping) {
>> +  GlobalVariable *G1 = NewExtGlobal(Type::Int32Ty, "Global1");
>> +  int32_t Mem1 = 3;
>> +  Engine->addGlobalMapping(G1, &Mem1);
>> +  EXPECT_EQ(&Mem1, Engine->getPointerToGlobalIfAvailable(G1));
>> +  int32_t Mem2 = 4;
>> +  Engine->updateGlobalMapping(G1, &Mem2);
>> +  EXPECT_EQ(&Mem2, Engine->getPointerToGlobalIfAvailable(G1));
>> +  Engine->updateGlobalMapping(G1, NULL);
>> +  EXPECT_EQ(NULL, Engine->getPointerToGlobalIfAvailable(G1));
>> +  Engine->updateGlobalMapping(G1, &Mem2);
>> +  EXPECT_EQ(&Mem2, Engine->getPointerToGlobalIfAvailable(G1));
>> +
>> +  GlobalVariable *G2 = NewExtGlobal(Type::Int32Ty, "Global1");
>> +  EXPECT_EQ(NULL, Engine->getPointerToGlobalIfAvailable(G2))
>> +    << "The NULL return shouldn't depend on having called"
>> +    << " updateGlobalMapping(..., NULL)";
>> +  // Check that update...() can be called before add...().
>> +  Engine->updateGlobalMapping(G2, &Mem1);
>> +  EXPECT_EQ(&Mem1, Engine->getPointerToGlobalIfAvailable(G2));
>> +  EXPECT_EQ(&Mem2, Engine->getPointerToGlobalIfAvailable(G1))
>> +    << "A second mapping shouldn't affect the first.";
>> +}
>> +
>> +TEST_F(ExecutionEngineTest, ReverseGlobalMapping) {
>> +  GlobalVariable *G1 = NewExtGlobal(Type::Int32Ty, "Global1");
>> +
>> +  int32_t Mem1 = 3;
>> +  Engine->addGlobalMapping(G1, &Mem1);
>> +  EXPECT_EQ(G1, Engine->getGlobalValueAtAddress(&Mem1));
>> +  int32_t Mem2 = 4;
>> +  Engine->updateGlobalMapping(G1, &Mem2);
>> +  EXPECT_EQ(NULL, Engine->getGlobalValueAtAddress(&Mem1));
>> +  EXPECT_EQ(G1, Engine->getGlobalValueAtAddress(&Mem2));
>> +
>> +  GlobalVariable *G2 = NewExtGlobal(Type::Int32Ty, "Global2");
>> +  Engine->updateGlobalMapping(G2, &Mem1);
>> +  EXPECT_EQ(G2, Engine->getGlobalValueAtAddress(&Mem1));
>> +  EXPECT_EQ(G1, Engine->getGlobalValueAtAddress(&Mem2));
>> +  Engine->updateGlobalMapping(G1, NULL);
>> +  EXPECT_EQ(G2, Engine->getGlobalValueAtAddress(&Mem1))
>> +    << "Removing one mapping doesn't affect a different one.";
>> +  EXPECT_EQ(NULL, Engine->getGlobalValueAtAddress(&Mem2));
>> +  Engine->updateGlobalMapping(G2, &Mem2);
>> +  EXPECT_EQ(NULL, Engine->getGlobalValueAtAddress(&Mem1));
>> +  EXPECT_EQ(G2, Engine->getGlobalValueAtAddress(&Mem2))
>> +    << "Once a mapping is removed, we can point another GV at the"
>> +    << " now-free address.";
>> +}
>> +
>> +}
>>
>> Modified: llvm/trunk/unittests/ExecutionEngine/Makefile
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ExecutionEngine/Makefile?rev=78127&r1=78126&r2=78127&view=diff
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> ======================================================================
>> --- llvm/trunk/unittests/ExecutionEngine/Makefile (original)
>> +++ llvm/trunk/unittests/ExecutionEngine/Makefile Tue Aug  4
>> 18:53:16 2009
>> @@ -8,12 +8,11 @@
>> ##=
>> =
>> =
>> ----------------------------------------------------------------------=
>> ==##
>>
>> LEVEL = ../..
>> +TESTNAME = ExecutionEngine
>> +LINK_COMPONENTS := engine interpreter
>>
>> include $(LEVEL)/Makefile.config
>>
>> PARALLEL_DIRS = JIT
>>
>> -include $(LEVEL)/Makefile.common
>> -
>> -clean::
>> -     $(Verb) $(RM) -f *Tests
>> +include $(LLVM_SRC_ROOT)/unittests/Makefile.unittest
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>