Different JITs and other clients of LLVM may have different needs in how symbol resolution should occur.
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?
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?
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.
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.
Can we keep it civil?
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.
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.
No, its what I think you are for.
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).
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')