[llvm] r233747 - [ExecutionEngine] Fix MCJIT::addGlobalMapping.
Lang Hames
lhames at gmail.com
Tue May 12 10:48:26 PDT 2015
Hi Josh,
Thanks for spotting this. Could you file a bug report at
http://llvm.org/bugs and CC me on it?
Cheers,
Lang.
On Thu, May 7, 2015 at 10:34 AM, Josh Klontz <josh.klontz at gmail.com> wrote:
> Lang,
>
> This commit appears to cause a minor regression, hitting:
> assert(!Name.empty() && "getNameWithPrefix requires non-empty name");
> when constructing an Interpreter whose Module contains unnamed global
> variables:
> @0 = private unnamed_addr constant [4 x i8] c"bar\00"
> Giving this global string constant a name fixes the problem:
> @foo = private unnamed_addr constant [4 x i8] c"bar\00"
>
> Happy to submit a bug report or patch if helpful.
>
> v/r,
> Josh
>
> On Tue, Mar 31, 2015 at 4:31 PM, Lang Hames <lhames at gmail.com> wrote:
>
>> Author: lhames
>> Date: Tue Mar 31 15:31:14 2015
>> New Revision: 233747
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=233747&view=rev
>> Log:
>> [ExecutionEngine] Fix MCJIT::addGlobalMapping.
>>
>> This patch fixes MCJIT::addGlobalMapping by changing the implementation
>> of the
>> ExecutionEngineState class. The new implementation maintains a
>> bidirectional
>> mapping between symbol names (std::strings) and addresses (uint64_ts),
>> rather
>> than a mapping between Value*s and void*s.
>>
>> This has fix has been made for backwards compatibility, however the
>> strongly
>> preferred way to resolve unknown symbols is by writing a custom
>> RuntimeDyld::SymbolResolver (formerly RTDyldMemoryManager) and overriding
>> the
>> findSymbol method. The addGlobalMapping method is a hangover from the
>> legacy JIT
>> (which has was removed in 3.6), and may be deprecated in a future release
>> as
>> part of a clean-up of the ExecutionEngine interface.
>>
>> Patch by Murat Bolat. Thanks Murat!
>>
>>
>> Modified:
>> llvm/trunk/include/llvm/ExecutionEngine/ExecutionEngine.h
>> llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp
>> llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/ExecutionEngine/ExecutionEngine.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/ExecutionEngine.h?rev=233747&r1=233746&r2=233747&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ExecutionEngine/ExecutionEngine.h (original)
>> +++ llvm/trunk/include/llvm/ExecutionEngine/ExecutionEngine.h Tue Mar 31
>> 15:31:14 2015
>> @@ -59,46 +59,34 @@ namespace object {
>> /// table. Access to this class should be serialized under a mutex.
>> class ExecutionEngineState {
>> public:
>> - struct AddressMapConfig : public ValueMapConfig<const GlobalValue*> {
>> - typedef ExecutionEngineState *ExtraData;
>> - static sys::Mutex *getMutex(ExecutionEngineState *EES);
>> - static void onDelete(ExecutionEngineState *EES, const GlobalValue
>> *Old);
>> - static void onRAUW(ExecutionEngineState *, const GlobalValue *,
>> - const GlobalValue *);
>> - };
>> -
>> - typedef ValueMap<const GlobalValue *, void *, AddressMapConfig>
>> - GlobalAddressMapTy;
>> + typedef StringMap<uint64_t> GlobalAddressMapTy;
>>
>> private:
>> - ExecutionEngine &EE;
>>
>> - /// GlobalAddressMap - A mapping between LLVM global values and their
>> - /// actualized version...
>> + /// GlobalAddressMap - A mapping between LLVM global symbol names
>> values and
>> + /// their actualized version...
>> GlobalAddressMapTy GlobalAddressMap;
>>
>> /// GlobalAddressReverseMap - This is the reverse mapping of
>> GlobalAddressMap,
>> /// used to convert raw addresses into the LLVM global value that is
>> emitted
>> /// at the address. This map is not computed unless
>> getGlobalValueAtAddress
>> /// is called at some point.
>> - std::map<void *, AssertingVH<const GlobalValue> >
>> GlobalAddressReverseMap;
>> + std::map<uint64_t, std::string> GlobalAddressReverseMap;
>>
>> public:
>> - ExecutionEngineState(ExecutionEngine &EE);
>>
>> GlobalAddressMapTy &getGlobalAddressMap() {
>> return GlobalAddressMap;
>> }
>>
>> - std::map<void*, AssertingVH<const GlobalValue> > &
>> - getGlobalAddressReverseMap() {
>> + std::map<uint64_t, std::string> &getGlobalAddressReverseMap() {
>> return GlobalAddressReverseMap;
>> }
>>
>> /// \brief Erase an entry from the mapping table.
>> ///
>> /// \returns The address that \p ToUnmap was happed to.
>> - void *RemoveMapping(const GlobalValue *ToUnmap);
>> + uint64_t RemoveMapping(StringRef Name);
>> };
>>
>> /// \brief Abstract interface for implementation execution of LLVM
>> modules,
>> @@ -161,6 +149,9 @@ protected:
>> /// abort.
>> void *(*LazyFunctionCreator)(const std::string &);
>>
>> + /// getMangledName - Get mangled name.
>> + std::string getMangledName(const GlobalValue *GV);
>> +
>> public:
>> /// lock - This lock protects the ExecutionEngine and MCJIT classes.
>> It must
>> /// be held while changing the internal state of any of those classes.
>> @@ -232,7 +223,8 @@ public:
>> /// Map the address of a JIT section as returned from the memory
>> manager
>> /// to the address in the target process as the running code will see
>> it.
>> /// This is the address which will be used for relocation resolution.
>> - virtual void mapSectionAddress(const void *LocalAddress, uint64_t
>> TargetAddress) {
>> + virtual void mapSectionAddress(const void *LocalAddress,
>> + uint64_t TargetAddress) {
>> llvm_unreachable("Re-mapping of section addresses not supported with
>> this "
>> "EE!");
>> }
>> @@ -290,6 +282,7 @@ public:
>> /// existing data in memory. Mappings are automatically removed when
>> their
>> /// GlobalValue is destroyed.
>> void addGlobalMapping(const GlobalValue *GV, void *Addr);
>> + void addGlobalMapping(StringRef Name, uint64_t Addr);
>>
>> /// clearAllGlobalMappings - Clear all global mappings and start over
>> again,
>> /// for use in dynamic compilation scenarios to move globals.
>> @@ -303,14 +296,17 @@ public:
>> /// address. This updates both maps as required. If "Addr" is null,
>> the
>> /// entry for the global is removed from the mappings. This returns
>> the old
>> /// value of the pointer, or null if it was not in the map.
>> - void *updateGlobalMapping(const GlobalValue *GV, void *Addr);
>> + uint64_t updateGlobalMapping(const GlobalValue *GV, void *Addr);
>> + uint64_t updateGlobalMapping(StringRef Name, uint64_t Addr);
>> +
>> + /// getAddressToGlobalIfAvailable - This returns the address of the
>> specified
>> + /// global symbol.
>> + uint64_t getAddressToGlobalIfAvailable(StringRef S);
>>
>> /// getPointerToGlobalIfAvailable - This returns the address of the
>> specified
>> /// global value if it is has already been codegen'd, otherwise it
>> returns
>> /// null.
>> - ///
>> - /// This function is deprecated for the MCJIT execution engine. It
>> doesn't
>> - /// seem to be needed in that case, but an equivalent can be added if
>> it is.
>> + void *getPointerToGlobalIfAvailable(StringRef S);
>> void *getPointerToGlobalIfAvailable(const GlobalValue *GV);
>>
>> /// getPointerToGlobal - This returns the address of the specified
>> global
>> @@ -474,7 +470,7 @@ public:
>> }
>>
>> protected:
>> - ExecutionEngine() : EEState(*this) {}
>> + ExecutionEngine() {}
>> explicit ExecutionEngine(std::unique_ptr<Module> M);
>>
>> void emitGlobals();
>>
>> Modified: llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp?rev=233747&r1=233746&r2=233747&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp (original)
>> +++ llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp Tue Mar 31
>> 15:31:14 2015
>> @@ -22,6 +22,7 @@
>> #include "llvm/IR/Constants.h"
>> #include "llvm/IR/DataLayout.h"
>> #include "llvm/IR/DerivedTypes.h"
>> +#include "llvm/IR/Mangler.h"
>> #include "llvm/IR/Module.h"
>> #include "llvm/IR/Operator.h"
>> #include "llvm/IR/ValueHandle.h"
>> @@ -61,8 +62,7 @@ ExecutionEngine *(*ExecutionEngine::Inte
>> void JITEventListener::anchor() {}
>>
>> ExecutionEngine::ExecutionEngine(std::unique_ptr<Module> M)
>> - : EEState(*this),
>> - LazyFunctionCreator(nullptr) {
>> + : LazyFunctionCreator(nullptr) {
>> CompilingLazily = false;
>> GVCompilationDisabled = false;
>> SymbolSearchingDisabled = false;
>> @@ -154,38 +154,52 @@ Function *ExecutionEngine::FindFunctionN
>> }
>>
>>
>> -void *ExecutionEngineState::RemoveMapping(const GlobalValue *ToUnmap) {
>> - GlobalAddressMapTy::iterator I = GlobalAddressMap.find(ToUnmap);
>> - void *OldVal;
>> +uint64_t ExecutionEngineState::RemoveMapping(StringRef Name) {
>> + GlobalAddressMapTy::iterator I = GlobalAddressMap.find(Name);
>> + uint64_t OldVal;
>>
>> // FIXME: This is silly, we shouldn't end up with a mapping -> 0 in the
>> // GlobalAddressMap.
>> if (I == GlobalAddressMap.end())
>> - OldVal = nullptr;
>> + OldVal = 0;
>> else {
>> + GlobalAddressReverseMap.erase(I->second);
>> OldVal = I->second;
>> GlobalAddressMap.erase(I);
>> }
>>
>> - GlobalAddressReverseMap.erase(OldVal);
>> return OldVal;
>> }
>>
>> +std::string ExecutionEngine::getMangledName(const GlobalValue *GV) {
>> + MutexGuard locked(lock);
>> + Mangler Mang(DL);
>> + SmallString<128> FullName;
>> + Mang.getNameWithPrefix(FullName, GV->getName());
>> + return FullName.str();
>> +}
>> +
>> void ExecutionEngine::addGlobalMapping(const GlobalValue *GV, void
>> *Addr) {
>> MutexGuard locked(lock);
>> + addGlobalMapping(getMangledName(GV), (uint64_t) Addr);
>> +}
>>
>> - DEBUG(dbgs() << "JIT: Map \'" << GV->getName()
>> - << "\' to [" << Addr << "]\n";);
>> - void *&CurVal = EEState.getGlobalAddressMap()[GV];
>> +void ExecutionEngine::addGlobalMapping(StringRef Name, uint64_t Addr) {
>> + MutexGuard locked(lock);
>> +
>> + assert(!Name.empty() && "Empty GlobalMapping symbol name!");
>> +
>> + DEBUG(dbgs() << "JIT: Map \'" << Name << "\' to [" << Addr << "]\n";);
>> + uint64_t &CurVal = EEState.getGlobalAddressMap()[Name];
>> assert((!CurVal || !Addr) && "GlobalMapping already established!");
>> CurVal = Addr;
>>
>> // If we are using the reverse mapping, add it too.
>> if (!EEState.getGlobalAddressReverseMap().empty()) {
>> - AssertingVH<const GlobalValue> &V =
>> - EEState.getGlobalAddressReverseMap()[Addr];
>> - assert((!V || !GV) && "GlobalMapping already established!");
>> - V = GV;
>> + std::string &V = EEState.getGlobalAddressReverseMap()[CurVal];
>> + assert((!V.empty() || !Name.empty()) &&
>> + "GlobalMapping already established!");
>> + V = Name;
>> }
>> }
>>
>> @@ -200,13 +214,19 @@ void ExecutionEngine::clearGlobalMapping
>> MutexGuard locked(lock);
>>
>> for (Module::iterator FI = M->begin(), FE = M->end(); FI != FE; ++FI)
>> - EEState.RemoveMapping(FI);
>> + EEState.RemoveMapping(getMangledName(FI));
>> for (Module::global_iterator GI = M->global_begin(), GE =
>> M->global_end();
>> GI != GE; ++GI)
>> - EEState.RemoveMapping(GI);
>> + EEState.RemoveMapping(getMangledName(GI));
>> }
>>
>> -void *ExecutionEngine::updateGlobalMapping(const GlobalValue *GV, void
>> *Addr) {
>> +uint64_t ExecutionEngine::updateGlobalMapping(const GlobalValue *GV,
>> + void *Addr) {
>> + MutexGuard locked(lock);
>> + return updateGlobalMapping(getMangledName(GV), (uint64_t) Addr);
>> +}
>> +
>> +uint64_t ExecutionEngine::updateGlobalMapping(StringRef Name, uint64_t
>> Addr) {
>> MutexGuard locked(lock);
>>
>> ExecutionEngineState::GlobalAddressMapTy &Map =
>> @@ -214,10 +234,10 @@ void *ExecutionEngine::updateGlobalMappi
>>
>> // Deleting from the mapping?
>> if (!Addr)
>> - return EEState.RemoveMapping(GV);
>> + return EEState.RemoveMapping(Name);
>>
>> - void *&CurVal = Map[GV];
>> - void *OldVal = CurVal;
>> + uint64_t &CurVal = Map[Name];
>> + uint64_t OldVal = CurVal;
>>
>> if (CurVal && !EEState.getGlobalAddressReverseMap().empty())
>> EEState.getGlobalAddressReverseMap().erase(CurVal);
>> @@ -225,20 +245,35 @@ void *ExecutionEngine::updateGlobalMappi
>>
>> // If we are using the reverse mapping, add it too.
>> if (!EEState.getGlobalAddressReverseMap().empty()) {
>> - AssertingVH<const GlobalValue> &V =
>> - EEState.getGlobalAddressReverseMap()[Addr];
>> - assert((!V || !GV) && "GlobalMapping already established!");
>> - V = GV;
>> + std::string &V = EEState.getGlobalAddressReverseMap()[CurVal];
>> + assert((!V.empty() || !Name.empty()) &&
>> + "GlobalMapping already established!");
>> + V = Name;
>> }
>> return OldVal;
>> }
>>
>> -void *ExecutionEngine::getPointerToGlobalIfAvailable(const GlobalValue
>> *GV) {
>> +uint64_t ExecutionEngine::getAddressToGlobalIfAvailable(StringRef S) {
>> MutexGuard locked(lock);
>> -
>> + uint64_t Address = 0;
>> ExecutionEngineState::GlobalAddressMapTy::iterator I =
>> - EEState.getGlobalAddressMap().find(GV);
>> - return I != EEState.getGlobalAddressMap().end() ? I->second : nullptr;
>> + EEState.getGlobalAddressMap().find(S);
>> + if (I != EEState.getGlobalAddressMap().end())
>> + Address = I->second;
>> + return Address;
>> +}
>> +
>> +
>> +void *ExecutionEngine::getPointerToGlobalIfAvailable(StringRef S) {
>> + MutexGuard locked(lock);
>> + if (void* Address = (void *) getAddressToGlobalIfAvailable(S))
>> + return Address;
>> + return nullptr;
>> +}
>> +
>> +void *ExecutionEngine::getPointerToGlobalIfAvailable(const GlobalValue
>> *GV) {
>> + MutexGuard locked(lock);
>> + return getPointerToGlobalIfAvailable(getMangledName(GV));
>> }
>>
>> const GlobalValue *ExecutionEngine::getGlobalValueAtAddress(void *Addr) {
>> @@ -247,15 +282,25 @@ const GlobalValue *ExecutionEngine::getG
>> // If we haven't computed the reverse mapping yet, do so first.
>> if (EEState.getGlobalAddressReverseMap().empty()) {
>> for (ExecutionEngineState::GlobalAddressMapTy::iterator
>> - I = EEState.getGlobalAddressMap().begin(),
>> - E = EEState.getGlobalAddressMap().end(); I != E; ++I)
>> + I = EEState.getGlobalAddressMap().begin(),
>> + E = EEState.getGlobalAddressMap().end(); I != E; ++I) {
>> + StringRef Name = I->first();
>> + uint64_t Addr = I->second;
>> EEState.getGlobalAddressReverseMap().insert(std::make_pair(
>> - I->second,
>> I->first));
>> + Addr, Name));
>> + }
>> }
>>
>> - std::map<void *, AssertingVH<const GlobalValue> >::iterator I =
>> - EEState.getGlobalAddressReverseMap().find(Addr);
>> - return I != EEState.getGlobalAddressReverseMap().end() ? I->second :
>> nullptr;
>> + std::map<uint64_t, std::string>::iterator I =
>> + EEState.getGlobalAddressReverseMap().find((uint64_t) Addr);
>> +
>> + if (I != EEState.getGlobalAddressReverseMap().end()) {
>> + StringRef Name = I->second;
>> + for (unsigned i = 0, e = Modules.size(); i != e; ++i)
>> + if (GlobalValue *GV = Modules[i]->getNamedValue(Name))
>> + return GV;
>> + }
>> + return nullptr;
>> }
>>
>> namespace {
>> @@ -511,7 +556,7 @@ void *ExecutionEngine::getPointerToGloba
>> return getPointerToFunction(F);
>>
>> MutexGuard locked(lock);
>> - if (void *P = EEState.getGlobalAddressMap()[GV])
>> + if (void* P = getPointerToGlobalIfAvailable(GV))
>> return P;
>>
>> // Global variable might have been added since interpreter started.
>> @@ -521,7 +566,7 @@ void *ExecutionEngine::getPointerToGloba
>> else
>> llvm_unreachable("Global hasn't had an address allocated yet!");
>>
>> - return EEState.getGlobalAddressMap()[GV];
>> + return getPointerToGlobalIfAvailable(GV);
>> }
>>
>> /// \brief Converts a Constant* into a GenericValue, including handling
>> of
>> @@ -1293,25 +1338,3 @@ void ExecutionEngine::EmitGlobalVariable
>> NumInitBytes += (unsigned)GVSize;
>> ++NumGlobals;
>> }
>> -
>> -ExecutionEngineState::ExecutionEngineState(ExecutionEngine &EE)
>> - : EE(EE), GlobalAddressMap(this) {
>> -}
>> -
>> -sys::Mutex *
>> -ExecutionEngineState::AddressMapConfig::getMutex(ExecutionEngineState
>> *EES) {
>> - return &EES->EE.lock;
>> -}
>> -
>> -void
>> ExecutionEngineState::AddressMapConfig::onDelete(ExecutionEngineState *EES,
>> - const GlobalValue
>> *Old) {
>> - void *OldVal = EES->GlobalAddressMap.lookup(Old);
>> - EES->GlobalAddressReverseMap.erase(OldVal);
>> -}
>> -
>> -void ExecutionEngineState::AddressMapConfig::onRAUW(ExecutionEngineState
>> *,
>> - const GlobalValue *,
>> - const GlobalValue *)
>> {
>> - llvm_unreachable("The ExecutionEngine doesn't know how to handle a"
>> - " RAUW on a value it has a global mapping for.");
>> -}
>>
>> Modified: llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp?rev=233747&r1=233746&r2=233747&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp
>> (original)
>> +++ llvm/trunk/unittests/ExecutionEngine/ExecutionEngineTest.cpp Tue Mar
>> 31 15:31:14 2015
>> @@ -54,6 +54,7 @@ TEST_F(ExecutionEngineTest, ForwardGloba
>> int32_t Mem1 = 3;
>> Engine->addGlobalMapping(G1, &Mem1);
>> EXPECT_EQ(&Mem1, Engine->getPointerToGlobalIfAvailable(G1));
>> + EXPECT_EQ(&Mem1, Engine->getPointerToGlobalIfAvailable("Global1"));
>> int32_t Mem2 = 4;
>> Engine->updateGlobalMapping(G1, &Mem2);
>> EXPECT_EQ(&Mem2, Engine->getPointerToGlobalIfAvailable(G1));
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150512/69836633/attachment.html>
More information about the llvm-commits
mailing list