Page MenuHomePhabricator

Allow clients to specify search order of DynamicLibraries.
ClosedPublic

Authored by marsupial on May 24 2017, 5:47 PM.

Details

Summary

Different JITs and other clients of LLVM may have different needs in how symbol resolution should occur.

Diff Detail

Event Timeline

marsupial created this revision.May 24 2017, 5:47 PM
marsupial updated this revision to Diff 100191.May 24 2017, 5:52 PM

I'm not actually sure I'm in favor of this but wanted another opinion.
To me DynamicLibrary::addPermanentLibrary doesn't really seem all that useful and actually complicates things a bit.

What is the reasoning of it existence?
If adding libraries with RTLD_LOCAL is important, could that not be accomplished with an additional argument to DynamicLibrary::getPermanentLibrary?

v.g.vassilev edited edge metadata.

@marsupial, the reasoning for this is that we do not want to expose the symbols to our experiments software stacks because they have other uses of llvm. dlopening cling as libcling allows to hide symbols avoiding potential versioning and other issues.

What is the reasoning of it existence?
If adding libraries with RTLD_LOCAL is important, could that not be accomplished with an additional argument to DynamicLibrary::getPermanentLibrary?

We had this discussion in another review. The dlopen has to be outside, because DynamicLibrary::getPermanentLibrary symbol is not available until you call dlopen :)

v.g.vassilev added inline comments.May 25 2017, 4:08 AM
lib/Support/DynamicLibrary.cpp
103–104

I am a little confused by this section here. This seems well optimized for binaries which are statically linked.

IIUC we disallow searching symbols from the dlopened from the process libraries? Why?

marsupial added inline comments.May 25 2017, 7:28 AM
lib/Support/DynamicLibrary.cpp
103–104

Yes it's a bit uglier than needed...I can fix that.

The point is ROOT seems to be the only user of addPermanentLibrary which is the only way to inject a handle of RTLD_LOCAL.

As such searching the Process handle will in cases other than ROOT will have already searched all libs that are in Handles. Which means you've slowed things down for nonexistent symbols 2x for all users to accommodate your special needs.

I can understand that you have a desire outside normal usage in LLVM but think it's better to have an argument to explicitly search any RTLD_LOCAL in Handles rather than require it for everyone.

v.g.vassilev added inline comments.May 25 2017, 7:42 AM
lib/Support/DynamicLibrary.cpp
103–104

As such searching the Process handle will in cases other than ROOT will have already searched all libs that are in Handles. Which means you've slowed things down for nonexistent symbols 2x for all users to accommodate your special needs.

Hm... It looks like you are arguing both ways here: you sold the initial version of the patch as 'improving' the situation for cling and its use-cases. Now you are essentially saying that you don't care if you broke it as this would slowdown 'hypothetical' users who resolve symbols from the process and have dlopened libraries. Oddly, I am not aware of any such complaints from those users as the previous version of the code was doing exactly this.

Over the years ROOT has been one of the (if not the only) few use cases of this code path. We have extended this several times and your refactoring is breaking our use-case. To give you context: ROOT is used in many domains outside high-energy physics estimating over 10K users, excluding the CERN LHC program.

marsupial added inline comments.May 25 2017, 7:59 AM
lib/Support/DynamicLibrary.cpp
103–104

Can we keep it civil?
I 'sold' nothing as improving cling nor said I don't care if it breaks anything.

I'm offering a solution that will fix the problem for you, not at the cost for every other LLVM user and have merely asked for someone outside of ROOT to comment on the current situation.

Other users of the JIT does symbol resolution this way and they also have users with tangible numbers.

marsupial updated this revision to Diff 100250.May 25 2017, 8:34 AM
marsupial retitled this revision from Allow Unix libraries loaded with RTLD_LOCAL to be searched. to Allow clients to specify search order of DynamicLibraries (in the JIT as well)..
marsupial edited the summary of this revision. (Show Details)
marsupial updated this revision to Diff 100512.May 26 2017, 5:35 PM
marsupial retitled this revision from Allow clients to specify search order of DynamicLibraries (in the JIT as well). to Allow clients to specify search order of DynamicLibraries..
marsupial edited the summary of this revision. (Show Details)
marsupial updated this revision to Diff 100513.May 26 2017, 5:59 PM

Formatting.

marsupial updated this revision to Diff 100514.May 26 2017, 6:38 PM

Add tests for symbol resolution order.

pcanal added a subscriber: pcanal.May 27 2017, 2:37 AM

The new version looks promising. We are going to give a try. Thanks.

That looks very good. Can we add a test case with dlopening a library with RTLD_LOCAL.

I plan to test whether this patch is enough for us later this week.

That looks very good. Can we add a test case with dlopening a library with RTLD_LOCAL.

I'd like to avoid adding the test at this moment.
I have subsequent patches to make DynamicLibrary a more usable class without adding them permanently.
In that is the ability to open with RTLD_LOCAL as well as tests for it.

marsupial updated this revision to Diff 100636.May 29 2017, 1:31 PM

Restore default ordering after llvm_shutdown called.

pcanal added inline comments.May 29 2017, 1:45 PM
include/llvm/Support/DynamicLibrary.h
96

Did you mean "Search all loaded libraries as the SO_Linker would" ?

marsupial added inline comments.May 29 2017, 1:59 PM
include/llvm/Support/DynamicLibrary.h
96

No, its what I think you are for.
The process does not have priority.

BTW, In case it wasn't clear: using this on Windows can cause real problems as a lot of C++ runtime can be exported via the binary (though I understand you want to preserve behavior ROOT has on Unix).

pcanal added inline comments.May 29 2017, 2:07 PM
include/llvm/Support/DynamicLibrary.h
96

No, its what I think you are for.

My bad. I mis-parsed the sentence, maybe adding a comma would have helped me: "Search all loaded libraries, then as the SO_Linker would".

Another point you might consider clarifying in the doc is whether the search of the libraries in SO_LoadedFirst and SO_LoadedLast is done in load order ('oldest first') or reverse load order ('newest first') [which is the closest to 'dependency order')

Allow forward and backward search of loaded modules.

Currently I am busy with other things and I would be able to test this beginning on July when I plan to upgrade LLVM again. Sorry...

marsupial updated this revision to Diff 105577.Jul 6 2017, 7:27 PM

Typo & reword documentation.

@v.g.vassilev @pcanal :
Can you provide a concrete example of a test in ROOT that this patch does not solve?

v.g.vassilev accepted this revision.Jul 7 2017, 3:38 PM

@marsupial, thanks for this. With a little tweaking I can say it fixes our use case.

@lhames, that seems to address what we discussed on irc. This covers our use case.

This revision is now accepted and ready to land.Jul 7 2017, 3:38 PM
This revision was automatically updated to reflect the committed changes.