Hi Lang,<div><br /></div><div>I think your missing a 'return 0' at the end of getSymbolAddress ;-)</div><div><br /></div><div>Cheers,</div><div>Juergen<br /><br /><span>On 09/20/14, <b class="name">Lang Hames </b> <lhames@gmail.com> wrote:</span><blockquote cite="mid:20140920174456.5C6AE2A6C028@llvm.org" class="iwcQuote" style="border-left: 1px solid #00F; padding-left: 13px; margin-left: 0;" type="cite"><div class="mimepart text plain">Author: lhames<br />Date: Sat Sep 20 12:44:56 2014<br />New Revision: 218187<br /><br />URL: <a href="http://llvm.org/viewvc/llvm-project?rev=218187&view=rev" target="l">http://llvm.org/viewvc/llvm-project?rev=218187&view=rev</a><br />Log:<br />[MCJIT] Make RTDyldMemoryManager::getSymbolAddress's behaviour more consistent.<br /><br />This patch modifies RTDyldMemoryManager::getSymbolAddress(Name)'s behavior to<br />make it consistent with how clients are using it: Name should be mangled, and<br />getSymbolAddress should demangle it on the caller's behalf before looking the<br />name up in the process. This patch also fixes the one client<br />(MCJIT::getPointerToFunction) that had been passing unmangled names (by having<br />it pass mangled names instead).<br /><br />Background:<br /><br />RTDyldMemoryManager::getSymbolAddress(Name) has always used a re-try mechanism<br />when looking up symbol names in the current process. Prior to this patch<br />getSymbolAddress first tried to look up 'Name' exactly as the user passed it in<br />and then, if that failed, tried to demangle 'Name' and re-try the look up. The<br />implication of this behavior is that getSymbolAddress expected to be called with<br />unmangled names, and that handling mangled names was a fallback for convenience.<br />This is inconsistent with how clients (particularly the RuntimeDyldImpl<br />subclasses, but also MCJIT) usually use this API. Most clients pass in mangled<br />names, and succeed only because of the fallback case. For clients passing in<br />mangled names, getSymbolAddress's old behavior was actually dangerous, as it<br />could cause unmangled names in the process to shadow mangled names being looked<br />up.<br /><br />For example, consider:<br /><br />foo.c:<br /><br />int _x = 7;<br />int x() { return _x; }<br /><br />foo.o:<br /><br />000000000000000c D __x<br />0000000000000000 T _x<br /><br />If foo.c becomes part of the process (E.g. via dlopen("libfoo.dylib")) it will<br />add symbols 'x' (the function) and '_x' (the variable) to the process. However<br />jit clients looking for the function 'x' will be using the mangled function name<br />'_x' (note how function 'x' appears in foo.o). When getSymbolAddress goes<br />looking for '_x' it will find the variable instead, and return its address and<br />in place of the function, leading to JIT'd code calling the variable and<br />crashing (if we're lucky).<br /><br />By requiring that getSymbolAddress be called with mangled names, and demangling<br />only when we're about to do a lookup in the process, the new behavior<br />implemented in this patch should eliminate any chance of names being shadowed<br />during lookup.<br /><br />There's no good way to test this at the moment: This issue only arrises when<br />looking up process symbols (not JIT'd symbols). Any test case would have to<br />generate a platform-appropriate dylib to pass to llvm-rtdyld, and I'm not<br />aware of any in-tree tool for doing this in a portable way.<br /><br /><br />Modified:<br />    llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp<br />    llvm/trunk/lib/ExecutionEngine/RTDyldMemoryManager.cpp<br /><br />Modified: llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp<br />URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp?rev=218187&r1=218186&r2=218187&view=diff" target="l">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp?rev=218187&r1=218186&r2=218187&view=diff</a><br />==============================================================================<br />--- llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp (original)<br />+++ llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp Sat Sep 20 12:44:56 2014<br />@@ -351,9 +351,13 @@ uint64_t MCJIT::getFunctionAddress(const<br /> void *MCJIT::getPointerToFunction(Function *F) {<br />   MutexGuard locked(lock);<br /> <br />+  Mangler Mang(TM->getSubtargetImpl()->getDataLayout());<br />+  SmallString<128> Name;<br />+  TM->getNameWithPrefix(Name, F, Mang);<br />+<br />   if (F->isDeclaration() || F->hasAvailableExternallyLinkage()) {<br />     bool AbortOnFailure = !F->hasExternalWeakLinkage();<br />-    void *Addr = getPointerToNamedFunction(F->getName(), AbortOnFailure);<br />+    void *Addr = getPointerToNamedFunction(Name, AbortOnFailure);<br />     addGlobalMapping(F, Addr);<br />     return Addr;<br />   }<br />@@ -364,17 +368,18 @@ void *MCJIT::getPointerToFunction(Functi<br />   // Make sure the relevant module has been compiled and loaded.<br />   if (HasBeenAddedButNotLoaded)<br />     generateCodeForModule(M);<br />-  else if (!OwnedModules.hasModuleBeenLoaded(M))<br />+  else if (!OwnedModules.hasModuleBeenLoaded(M)) {<br />     // If this function doesn't belong to one of our modules, we're done.<br />+    // FIXME: Asking for the pointer to a function that hasn't been registered,<br />+    //        and isn't a declaration (which is handled above) should probably<br />+    //        be an assertion.<br />     return nullptr;<br />+  }<br /> <br />   // FIXME: Should the Dyld be retaining module information? Probably not.<br />   //<br />   // This is the accessor for the target address, so make sure to check the<br />   // load address of the symbol, not the local address.<br />-  Mangler Mang(TM->getSubtargetImpl()->getDataLayout());<br />-  SmallString<128> Name;<br />-  TM->getNameWithPrefix(Name, F, Mang);<br />   return (void*)Dyld.getSymbolLoadAddress(Name);<br /> }<br /> <br /><br />Modified: llvm/trunk/lib/ExecutionEngine/RTDyldMemoryManager.cpp<br />URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/RTDyldMemoryManager.cpp?rev=218187&r1=218186&r2=218187&view=diff" target="l">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/RTDyldMemoryManager.cpp?rev=218187&r1=218186&r2=218187&view=diff</a><br />==============================================================================<br />--- llvm/trunk/lib/ExecutionEngine/RTDyldMemoryManager.cpp (original)<br />+++ llvm/trunk/lib/ExecutionEngine/RTDyldMemoryManager.cpp Sat Sep 20 12:44:56 2014<br />@@ -253,19 +253,20 @@ uint64_t RTDyldMemoryManager::getSymbolA<br />   // is called before ExecutionEngine::runFunctionAsMain() is called.<br />   if (Name == "__main") return (uint64_t)&jit_noop;<br /> <br />+  // Try to demangle Name before looking it up in the process, otherwise symbol<br />+  // '_<Name>' (if present) will shadow '<Name>', and there will be no way to<br />+  // refer to the latter.<br />+<br />   const char *NameStr = Name.c_str();<br />-  void *Ptr = sys::DynamicLibrary::SearchForAddressOfSymbol(NameStr);<br />-  if (Ptr)<br />-    return (uint64_t)Ptr;<br /> <br />-  // If it wasn't found and if it starts with an underscore ('_') character,<br />-  // try again without the underscore.<br />-  if (NameStr[0] == '_') {<br />-    Ptr = sys::DynamicLibrary::SearchForAddressOfSymbol(NameStr+1);<br />-    if (Ptr)<br />+  if (NameStr[0] == '_')<br />+    if (void *Ptr = sys::DynamicLibrary::SearchForAddressOfSymbol(NameStr + 1))<br />       return (uint64_t)Ptr;<br />-  }<br />-  return 0;<br />+<br />+  // If we Name did not require demangling, or we failed to find the demangled<br />+  // name, try again without demangling.<br />+  if (void *Ptr = sys::DynamicLibrary::SearchForAddressOfSymbol(NameStr))<br />+    return (uint64_t)Ptr;<br /> }<br /> <br /> void *RTDyldMemoryManager::getPointerToNamedFunction(const std::string &Name,<br /><br /><br />_______________________________________________<br />llvm-commits mailing list<br />llvm-commits@cs.uiuc.edu<br /><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="l">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br /></div></blockquote></div>