<div dir="ltr">Oops. Nice catch. Thanks for fixing it too Takumi.<div><br></div><div>The 'if' doesn't actually need to be there at all though - I've removed it in r218220.</div><div><br></div><div>- Lang.<br><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Sep 20, 2014 at 1:09 PM, Juergen Ributzka <span dir="ltr"><<a href="mailto:juergen@apple.com" target="_blank">juergen@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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<div><div class="h5"><br><br><span>On 09/20/14, <b>Lang Hames </b> <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:</span><blockquote style="border-left:1px solid #00f;padding-left:13px;margin-left:0" type="cite"><div>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="_blank">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="_blank">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="_blank">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><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></div></blockquote></div></div></div>
</blockquote></div><br></div>