[llvm] r233747 - [ExecutionEngine] Fix MCJIT::addGlobalMapping.
Josh Klontz
josh.klontz at gmail.com
Tue May 12 11:01:42 PDT 2015
https://llvm.org/bugs/show_bug.cgi?id=23497
Thanks,
Josh
On Tue, May 12, 2015 at 1:48 PM, Lang Hames <lhames at gmail.com> wrote:
> 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/a78aa57f/attachment.html>
More information about the llvm-commits
mailing list