[llvm] r218187 - [MCJIT] Make RTDyldMemoryManager::getSymbolAddress's behaviour more consistent.

Sat Sep 20 13:09:26 PDT 2014

Hi Lang,

I think your missing a 'return 0' at the end of getSymbolAddress ;-)

Cheers,
Juergen

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