[llvm] r233747 - [ExecutionEngine] Fix MCJIT::addGlobalMapping.
Josh Klontz
josh.klontz at gmail.com
Thu May 7 10:34:25 PDT 2015
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/20150507/fac57a55/attachment.html>
More information about the llvm-commits
mailing list