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

[David Blaikie]

On Sat, Sep 20, 2014 at 10:44 AM, 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.


Unit tests added in r218626. As it happens it wasn't /quite/ as easy to
test as we'd discussed - this function specifically looks for dynamic
library symbols, so I couldn't seem to tickle it into finding symbols in
the current executable - but perhaps I was doing it wrong? If you know of a
way to do that, it might be nicer than inserting artificial symbols into
the global symbol table thingy... (maybe it isn't nicer - I'm not sure).

But, as noted in my commit message, how can fallback in either direction be
the right behavior here? It seems like so long as there are clients passing
both mangled and unmangled names, some lookups will incorrectly find the
wrong symbol, won't they? (if I pass "_x" as an unmangled symbol and the
API gives me the address of the unmangled "x", the code is just as broken
as the mangled scenario you gave in the commit - right?)


>
>
> 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/20140929/70d966c0/attachment.html>