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

[Lang Hames]

Oops. Nice catch. Thanks for fixing it too Takumi.

The 'if' doesn't actually need to be there at all though - I've removed it
in r218220.

- Lang.


On Sat, Sep 20, 2014 at 1:09 PM, Juergen Ributzka <juergen at apple.com> wrote:

> 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/20140921/cb38fdc5/attachment.html>