Page MenuHomePhabricator

Make the plugin loader limit its symbol search to the specified module.
ClosedPublic

Authored by zturner on Aug 22 2014, 11:51 AM.

Details

Summary

Working on LLDB, I found that they have plugin loading code which is almost identical to that provided by LLVM's support libraries. There was one minor difference though, which is that on Apple platforms there is an RTLD_FIRST flag that you can pass to dlopen, which will limit symbol seaches to strictly the module that was dlopen'ed. LLDB developers claim that this functionality is required to support LLDB's needs [1, 2].

The attached patch modifies LLVM's DynamicLibrary class to use RTLD_FIRST on Apple platforms, and simulate the effect on non-apple platforms by checking that the address of the located symbol is in the module which was originally dlopen'ed.

Unfortunately I don't know really how to test this, and LLVM seems to have no code that makes use of this DynamicLibrary class.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 12852.Aug 22 2014, 11:51 AM
zturner retitled this revision from to Make the plugin loader limit its symbol search to the specified module..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a subscriber: Unknown Object (MLST).
rnk added a subscriber: rnk.Aug 26 2014, 3:31 PM
rnk added inline comments.
include/llvm/Support/DynamicLibrary.h
48 ↗(On Diff #12852)

It looks like DynamicLibrary is supposed to be a thin value type wrapper around native library handles. Adding this string makes it no longer trivially copyable and blows out its size. If we really need this, I think we should delete the copy ctor and eliminate the places where we pass it around by value.

Looks like the only use of lgetAddressOfSymbol is in clang/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp so I have no objections to changing it, but as Reid mentioned, we should probably make the DynamicLibrary type non-copyable.

I discussed this more with the LLDB people, and I think we're ok with
removing the filename check on Linux, but keeping the use of RTLD_FIRST on
Apple. I will update the patch accordingly.

zturner updated this revision to Diff 12992.Aug 27 2014, 9:38 AM
rafael accepted this revision.Aug 27 2014, 10:05 AM
rafael added a reviewer: rafael.

OK with a comment on why RTLD_FIRST.

I am a bit surprised that we need RTLD_GLOBAL, but that is independent.

This revision is now accepted and ready to land.Aug 27 2014, 10:05 AM
zturner closed this revision.Aug 27 2014, 10:15 AM
zturner updated this revision to Diff 12993.

Closed by commit rL216563 (authored by @zturner).

emaste added a subscriber: emaste.Sep 2 2014, 8:00 AM