[PATCH] D33529: Allow Unix libraries loaded with RTLD_LOCAL to be searched.

[Vassil Vassilev via Phabricator via llvm-commits]

v.g.vassilev added inline comments.


================
Comment at: lib/Support/DynamicLibrary.cpp:85
+#if defined(LLVM_ON_WIN32) && !defined(NDEBUG)
       for (void *Handle : Handles)
         assert(!DLSym(Handle, Symbol) && "Symbol exists in non process handle");
----------------
marsupial wrote:
> v.g.vassilev wrote:
> > 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.
> 
> 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.


https://reviews.llvm.org/D33529