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