In case it's not obvious, the exact bit of code I'm referring to is here:
https://reviews.llvm.org/rL262657#C38049635NL269

My solution was to change:

#ifdef APPLE

to:

#if defined(APPLE) || defined(_WIN32)

Hi James,

My (very limited) understanding of windows name mangling is that it is more complex than Darwin or Linux. See e.g. https://msdn.microsoft.com/en-us/library/56h2zst2.aspx

We probably (eventually) want something like:

#if (APPLE)
// strip leading underscore
#elif (_WIN32)
win32demangle(sym);
#endif

I think your change is a step in the right direction though. Do you have a patch that you'd like to share?

Do you have commit access?

Cheers,
Lang.

Proposed fix for getSymbolAddressInProcess on Windows
You're right about the mangling on Windows being more complicated - I'd forgotten about that. If we need to handle all those calling conventions, there is a function in the LLVMSymbolizer implementation (demanglePE32ExternCFunc) which looks like it could be perfect for the job. Unfortunately it doesn't appear to be publicly accessible, but perhaps that code could be moved somewhere more useful at some point - maybe a static in the Mangler class?

Also, my original concern was that in some cases you may not want the name demangled on Windows. Because while it's typical for a function to be exported from a DLL with a demangled name, that's not necessarily always the case - I think it's just a convention really. But perhaps mangled exports are rare enough that we wouldn't need to care about those cases here.

In the meantime, I've attached a patch with my current fix, which I suppose is better than nothing.

I should mention, though, that Monday was my first time looking at the LLVM code base, and it's probably been close to a decade since I last did any real C/C++ work, so I'm a bit out of my depth here. Don't assume that I know what I'm talking about. ;)

I've had a chance to investigate this further, and I'm now of the opinion that there's not much point in trying to support further demangling of symbol names on Windows (I mean other than underscore demangling proposed above).

As far as I can tell, the DynamicLibrary::SearchForAddressOfSymbol method will only lookup functions from a fixed set of DLLs - those that are imported by the lli interpreter. The majority of those are Windows APIs, and when called from from C code compiled with clang, those APIs end up as dllimport references. Those dllimport references then translate to indirect calls in the assembly, where the mangled symbol is a reference to the import table entry rather than the function itself. So demangling that symbol to lookup the address of the function is pointless, and in fact will crash, because it's really expecting a memory address.

The end result is that SearchForAddressOfSymbol is only going to work with function calls that are statically linked (i.e. no dllimport), and which are also found in the fixed set of DLLs imported by the lli interpreter. And I think that just leaves you with C runtime library functions which are probably all cdecl anyway (and thus handled by the simple underscore demangling), or possibly C++ runtime library references which aren't meant to be demangled.

That's not to say that SearchForAddressOfSymbol couldn't be extended to support dllimport references, but that seems like a separate issue to me, and may well be considered out of scope for lli.

Hi James,

Thanks so much for the investigation!

"As far as I can tell, the DynamicLibrary::SearchForAddressOfSymbol method will only lookup functions from a fixed set of DLLs"

I think it should also search anything loaded dynamically via DynamicLibrary::LoadLibraryPermanently (assuming that is supported on Windows).

"The end result is that SearchForAddressOfSymbol is only going to work with function calls that are statically linked (i.e. no dllimport), and which are also found in the fixed set of DLLs imported by the lli interpreter. And I think that just leaves you with C runtime library functions which are probably all cdecl anyway (and thus handled by the simple underscore demangling), or possibly C++ runtime library references which aren't meant to be demangled."

Perhaps we need to re-think the semantics of DynamicLibrary SearchForAddressOfSymbol on Windows. On *nix (including Linux and Darwin) SearchForAddressOfSymbol is meant to be analogous to the dlsym library function: it searches for symbols based on their C symbol name. If DLLs on windows don't have a universal mangling rule then "search based on C symbol name" is difficult (or perhaps impossible) to implement with the existing interface. I'll ask some of the Windows experts in IRC tomorrow.

For now, would you be happy with me committing your existing fix?

I think it should also search anything loaded dynamically via
DynamicLibrary::LoadLibraryPermanently (assuming that is supported on
Windows).

That does work on Windows, but the only way I could see that being called from lli is via the -load command line option, and the documentation for -load says it "causes lli to load the plugin (shared object) named pluginfilename and use it for optimization." So while technically that is a way to get an additional DLL into the search set, that doesn't sound to me like it was intended to be used that way. Am I misreading that?

In any event, if you are going to have to pass in extra command line parameters to get lli to execute your input, I would think it might make more sense (on Windows at least) to link in the import library for the DLL (via the -extra-archive option) rather than the DLL itself. That would in theory avoid the whole name demangling issue altogether, since the import library already has all the information required to map mangled names to their corresponding DLL and export names.

That said, the MCJIT::findSymbol implementation doesn't appear to support import libs at the moment, so that's a non-starter for now. But longer term that seems like it might be a better approach to take.

Perhaps we need to re-think the semantics of DynamicLibrary
SearchForAddressOfSymbol on Windows. On *nix (including Linux and Darwin)
SearchForAddressOfSymbol is meant to be analogous to the dlsym library
function: it searches for symbols based on their C symbol name.

I think I need to get a better understanding of how things works on the *nix side before I can voice a useful opinion on this. I'm still not really sure what exactly is expected of lli on a given set of inputs. But I suspect Windows is always going to end up being more complicated because of its need for an import library when linking to a DLL (while I believe *nix just uses the .so directly).

For now, would you be happy with me committing your existing fix?

Yep, I'm happy for you to commit if nobody else has objected. It may not be the perfect long term solution, but it at least gets the basic C libs working.

Hi James,

Yep, I'm happy for you to commit if nobody else has objected. It may not be the
perfect long term solution, but it at least gets the basic C libs working.

Sounds good to me. Committed in r279016, with minor modifications (I added a check for !defined(WIN64), since the leading '' mangling appears to only apply to 32-bit apps). Hope that's ok - if that change affects your use-case let me know and we'll dig deeper.

Thanks again for working on this!

• Lang.

Re-opening due to http://bb.pgr.jp/builders/ninja-clang-i686-msc19-R/builds/5963

Looks like this breaks calls to chkstk. I'm investigating now, and will revert shortly if I don't see an obvious fix.

Reverted in r279029: this breaks win32-elf tests (see the triple for the failing tests at http://bb.pgr.jp/builders/ninja-clang-i686-msc19-R/builds/5963).

The right solution to this is probably to deprecate the win32-elf hack, but it's not clear how many people are using this. This will require some more thought...

First off, I must apologise for giving you a patch that broke the build. I should have caught that.

Having looked into the cause of the failure, though, I think that area of the code is still technically broken, even with the patch reverted. The problem is we're trying to demangle the symbol names based on the architecture of the lli build, when it should be based on the target triple.

For example, I can produce the same kind of error on Linux by calling lli with the target triple set like this:

lli -mtriple=x86_64-unknown-linux-gnu-macho

And I suspect you might get it to fail similarly on a Mac with a target triple looking something like this:

lli -mtriple=x86_64-apple-darwin-gnu-elf

It's just a matter of overriding the target object format (coff/elf/macho) in such a way that the mangling mode is different from the build default. This may not be a problem in practice, and maybe you want to deprecate that sort of thing eventually, but it does suggest that something isn't quite right with the current approach.

Now the simplest solution could just be to go back to the old getSymbolAddressInProcess implementation which attempted to resolve the name both with and without the underscore prefix removed. That way it doesn't have to care what kind of mangling the target architecture is performing.

Obviously that's not ideal though - it's inefficient, and could potentially cause symbols to be resolved incorrectly (which I believe is why you changed this code in the first place). However, it's worth mentioning that this is already a problem in the LinkingSymbolResolver::findSymbol implementation which uses the same kind of hack.

That brings me to the second possible solution. The LinkingSymbolResolver has access to the target machine's DataLayout, and from that it can lookup the GlobalPrefix with which the symbol would have been mangled (if any). In theory it should then be perfectly capable of demangling the name without the need for any of this guess work.

We'd still then need to extend the JITSymbolResolver interface to pass in the mangling prefix so it would also know how to demangle accurately. Otherwise we could change the semantics of the resolver so we can pass in the symbol name already demangled. Either way would potentially have backwards compatibility issues, but this seems a better approach in the long-term.

What do you think?

First off, I must apologise for giving you a patch that broke the build. I should have caught that.

No worries. If you're working on Windows it's likely that this failure wouldn't have shown up when you tested. That's a common situation for all LLVM developers. We rely on the buildbots to let us know when we've broken configs we don't have access to.

The problem is we're trying to demangle the symbol names based on the architecture of the lli build, when it should be based on the target triple.

Your analysis of the problem is spot on.

The underlying issue, which is intended behavior, is the use of mangled names for lookup. This allows JIT symbol resolution rules to match those in the static linker, which is ideal for guaranteeing that JIT'd code behavior matches that of ahead-of-time compiled behavior. As you've identified however, this falls flat in getSymbolAddressInProcess if the JIT'd code's mangling rules don't match the host platform.

We could thread a DataLayout reference through symbol lookup, but I want to avoid the overhead if possible. I think we can solve this by redefining getSymbolAddressInProcess to take a demangled name (It's supposed to be a wrapper around dlsym anyway, so that's a saner API), and having the user wrap the symbol name in an explicit demangler call. So instead of:

if (auto Sym = SearchJIT(Name))
return Sym;
return getSymbolAddressInProcess(Name);

We would have:

if (auto Sym = SearchJIT(Name))
return Sym;
return getSymbolAddressInProcess(Demangle(DL, Name));

Any interest in writing a patch for this? :)

I'm out on vacation this week, but happy to review patches this or fix it myself when I get back next week.

Cheers,
Lang.

Just so you know, I have started looking into this, but it's turned out to be a little more complicated than I initially thought. If you get back next week and I haven't made any progress, I don't mind if you just want to get on with it and fix it yourself. It'll have been a good learning experience for me either way.

But from what I've been able to figure out so far, the places that are calling getSymbolAddressInProcess don't always have access to the DataLayout, so they wouldn't necessarily be in a position to demangle the name at that point either. That has made me think we'll probably need to handle the demangling further up the call stack.

Anyway, once I'm a little more certain of myself, I'd be happy to try putting together a patch. It might just take me a while to get that far.

Proposed fix for MCJIT::findSymbol mangling inconsistency
After spending some time looking through the code, I found a number of inconsistencies in the way the symbol name mangling was handled, and I thought it might be easier if we tried to address some of those issues first, before tackling my original problem.

In particular, I've been looking at the LinkingSymbolResolver::findSymbol method, which appears to me to accept a mangled name. It's called from MCJIT::GetPointerToNamedFunction, which is called from GetPointerToFunction, with a name that has been explicitly mangled (at least since rev:218187). It's also called from RuntimeDlydImpl::resolveExternalSymbols with the names in the ExternalSymbolRelocations list, which I suspect have always been mangled.

So assuming it takes a mangled name, we then have that mangled name passed to the MCJIT::findSymbol method, but findSymbol seems to want an unmangled name. I'm basing that assumption on the fact that MCJIT::findSymbol passes its name to MCJIT::findExistingSymbol, which immediately tries to mangle it. It also calls MCJIT::findModuleForSymbol, which searches the Module::ValSymTab table, which seems to use unmangled names too. There's more to it than that, but I think that was the extent of the original behaviour.

So we have the LinkingSymbolResolver::findSymbol method taking a mangled name, and passing it to the MCJIT::findSymbol method which expects an unmangled name. At least that was the case until rev:191780, when the code was "fixed" by calling the MCJIT::findSymbol a second time with the name demangled if the first call failed. This obviously wasn't an ideal solution, since the method was unnecessarily being called twice, and there are any number of edge cases that will cause it to produce the wrong results.

To makes matters worse, MCJIT::findSymbol was later extended to support external archives, but that code was written with the assumption that findSymbol took a mangled name, although it still relies on the findExistingSymbol method which takes an unmangled name. Miraculously it manages to succeed a lot of the time, because the first findSymbol call with the mangled name proceeds far enough to decode the archive, and while the findExistingSymbol call will fail on that pass, the second findSymbol call with the demangled name triggers another findExistingSymbol call which does ultimately succeed!

In addition to that, MCJIT::findSymbol was also extended to support the LazyFunctionCreator, and again that seems to expect a mangled name (at least that's how it was called from getPointerToNamedFunction, which was where it was first used). So that case should work correctly on the first (mangled) MCJIT::findSymbol call, but the second (demangled) call may erroneously match functions that it wasn't supposed to match.

Anyway, to try and resolve these inconsistencies, my suggestion is we change the semantics of MCJIT::findSymbol so it is always expected to take a mangled name. This means we can remove the mangling from the MCJIT::findExistingSymbol method, and we can also get rid of the double call hack in the LinkingSymbolResolver::findSymbol method (since the first call to MCJIT::findSymbol with the mangled name should now always succeed).

The MCJIT::findModuleForSymbol method still requires an unmangled name, so that will need some additional demangling code to make it work (I think probably best added to the method itself, rather than the code that calls it). But everything else in the MCJIT::findSymbol method should work OK with the mangled name as is.

To summarise: the findSymbol, findExistingSymbol and findModuleForSymbol methods in MCJIT would now all take a mangled name rather than an unmangled name. For the latter two, this change in semantics will hopefully not be a problem, since they are protected methods that don't appear to be used anywhere else (although external projects that have subclassed MCJIT could be effected). For findSymbol, all external access seems to be through the getSymbolAddress method, so we can probably retain backwards compatibility just by making it mangle its input name before passing that on to findSymbol.

I've attached a patch with these proposed changes, but I wouldn't necessarily expect it to be accepted as is. This is just a starting point for debate and a summary of my understanding of the code so far.

Hi James,

Sorry for the delay in reviewing this, and thanks for being brave enough to venture into this thicket - I didn't realise how bad it was.

I'll check the patch out and have a review for you by the end of the week.

• Lang.

Proposed fix for getSymbolAddressInProcess on Windows
The attached patch is another attempt to solve the original demangling problem in getSymbolAddressInProcess. I'd initially thought this could be achieved by handling the demangling further up the call stack where the DataLayout was already accessible. But this would require the JITSymbolResolver::findSymbol method to take an unmangled name, and that's just not practical for all the JITSymbolResolver implementations.

In particular, the LinkingSymbolResolver::findSymbol implementation absolutely has to receive a mangled name in order for it to work correctly (hopefully this should be clear from the analysis above). And it can't just take a demangled name and remangle it, because it doesn't have enough information to do so.

That leaves us back with trying to make the DataLayout accessible to the RTDyldMemoryManager class, and I figured the easiest way to do that was just by adding a setDataLayout method to the class. If that method isn't called, it falls back to using a reasonable default based on the current process target triple.

For now, the only place that explicitly sets the DataLayout is the lli interpreter, so it can work correctly with non-standard target triples (the cause of the test failures in the first patch). There may be other places that would benefit from setting the DataLayout explicitly, but I suspect most will be fine with the default (or at least no worse off than they were before).

It's also worth mentioning that a lot of places call the static getSymbolAddressInProcess method directly, and those will always fallback to using the default DataLayout. If there's a need to support a non-default DataLayout in those case, we'll probably want to add another static with an explicit DataLayout parameter, but I didn't think that was worth doing until there was a real demand for it.

I should also note that this patch is entirely independent from the MCJIT patch. This one just fixes the demangling in the RTDyldMemoryManager, whereas the MCJIT patch fixes a more diverse set of mangling issues which don't necessarily have anything to do with dynamic linking (in retrospect I should probably have raised that as a separate issue - I can still do that if you'd prefer).

And don't worry about the delay. I'm on an indefinite holiday. Nothing in my life happens with any great sense of urgency. :)

Hi James,

I'm just running through your previous patch - all tests pass, but I want to make sure I really understand the changes.

Regarding getSymbolAddressInProcess, I think demangling should be the caller's job. That way getSymbolAddressInProcess matches the behavior of dlsym. We'll need to add a demangle function that callers can use, and that could take an optional datalayout (if we want to support 'best-guess' demangling) or a mandatory one (if we determine that all callers can be reasonably expected to have access to a data layout).

• Lang.

Ok. The MCJIT::findSymbol mangling inconsistency fix looks good to me. I've committed it in r281238. Thanks again for that!

I don't have time to work on the getSymbolAddressInProcess problems just yet, but I'm happy to review patches. I'll try to turn the next ones around a little more quickly - I appreciate your patience.

I'll be in #llvm on irc.oftc.net as 'lhames' too if you want to chat about any of this stuff in real time too.

Thanks again for all the help. :)

• Lang.

Alternative fix for getSymbolAddressInProcess on Windows
Another attempt to fix the getSymbolAddressInProcess mangling issue, this time making the caller handle the demangling. I don't know if you got my message on IRC, but you should be aware that if we take this approach, there's going to be some inconsistency in the semantics of the various RTDyldMemoryManager methods, because we still need the findSymbol and getSymbolAddress methods to take a mangled name while getSymbolAddressInProcess will now be getting a demangled name.

I wasn't sure where best to put the demangling code, so for now I've added it to the Mangler class. Also not sure what the best practice is for string types in this situation, but I went with StringRefs for both the input and the return value. This seemed like it would be most efficient and easy to use, but I suspect there could be some situations where it could be unsafe if called with a temporary.

Another uncertainty I have is with the code in getSymbolAddressInProcess that tests for a bunch of hardcoded function names, since the name being tested is now demangled. The Linux ones are assumedly fine because mangling wouldn't apply there, and I think the ARM ones are OK too. My concern is with the Mingw/Cygwin test for "__main" - I've changed that to "_main" on the assumption that "__main" was the mangled form, but I don't have Cygwin or Mingw so can't be sure of that.

Also haven't been able to test all the Kaleidoscope examples because the existing code is already a bit broken. Will file a separate bug report for those issues.

Finally, I wasn't sure what to do with the LookupWithMangledAndDemangledSymbol unit test since most of what it was testing no longer applies. For now I've updated it so that the test should pass, but we may just want to get rid of it altogether.

Alternative fix for getSymbolAddressInProcess on Windows
I've since managed to get the Cygwin build to work and discovered that "__main" (with two underscores) is actually the unmangled form of the symbol, so that code was fine as it was. Have updated the patch with that change removed.

Another attempt to fix the getSymbolAddressInProcess mangling issue, this time making the caller handle the demangling.

Thanks again! :)

I don't know if you got my message on IRC, but you should be aware that if we take this approach, there's going to be some inconsistency in the semantics of the various RTDyldMemoryManager methods

I'm afraid I missed it. That's ok: We should just document the inconsistency, or perhaps deprecate getSymbolAddressInProcess and create a new free function instead.

I wasn't sure where best to put the demangling code, so for now I've added it to the Mangler class.

I don't have strong feelings here. The Mangler class is the right place to put this if demangling is unambiguous, and would be useful to other people. Otherwise we can introduce a JITDemangler class.

Rafael - I think you did the most recent work on the Mangler, do you have any thoughts?

Also not sure what the best practice is for string types in this situation, but I went with StringRefs for both the input and the return value. This seemed like it would be most efficient and easy to use, but I suspect there could be some situations where it could be unsafe if called with a temporary.

So long as demangling just involves dropping a prefix/suffix (which in practice I think is always true) I think StringRef is fine. I'll just add documentation to the functions explaining the lifetime guarantees.

Also haven't been able to test all the Kaleidoscope examples because the existing code is already a bit broken. Will file a separate bug report for those issues.

Did you file a bug for that? I'd be happy to look into the issue.

Finally, I wasn't sure what to do with the LookupWithMangledAndDemangledSymbol unit test since most of what it was testing no longer applies. For now I've updated it so that the test should pass, but we may just want to get rid of it altogether.

Yes - I think this can go away if/when your changes go in.

Pending Rafael's comments (and me adding some comments) I think this looks great. Thanks James.

Also haven't been able to test all the Kaleidoscope examples
Did you file a bug for that? I'd be happy to look into the issue.

The main problem I had with Kaleidoscope was that nothing worked on Windows, but there was already a bug raised for that (25493), and my proposed solution was the same as the fix you committed and then reverted in January (rev 258665). Do you have a specific plan in mind for fixing that still, or would you be interested in alternative proposals?

Another issue I had was with the Remote-JIT example not working on Linux (it's not supported at all on Windows, but that is a known limitation I think). I raised bug 30417 for the Linux issue along with a proposed patch.

I think there was at least one other problem with the Windows code, but until bug 25493 is addressed there didn't seem much point in trying to fix anything else.

I also encountered an issue with the interpreter's address resolution which I raised as bug 30584 along with a proposed patch. Not sure if that is also your department.

I stumbled upon this bug in a simple scenario:

On Windows i386 I compiled a simple C program with -S -emit-llvm and piped it directly into lli.exe. This resulted in errors like

LLVM ERROR: Program used external function '_puts' which could not be resolved!

or

LLVM ERROR: Program used external function '___stdio_common_vsprintf' which could not be resolved!

In other words, all symbols were prefixed with additional underscore. However, applying a patch from this PR fixed it. Hoping this will be committed soon.