[PATCH] D29955: Allow externally dlopen-ed libraries to be registered as permanent libraries.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In https://reviews.llvm.org/D29955#684287, @marsupial wrote:

> > Yeah, I didn't mean to create more work for you in https://reviews.llvm.org/D30107! The 'with-lock' helper seemed like the right solution here, and I have still have some questions/comments about https://reviews.llvm.org/D30107, hence the lgtm.
>
> It shouldn't be that much work integrating it...was just saying as the ** addPermanentLibraryWithLock** is a bit ugly and would go away anyway.
>
> > 
> > 
> >> Otherwise, doesn't this make it impossible to get symbols from an already loaded library unless all libs are searched?
> >>  That is, assuming **"libSomething"** is already loaded:
> >>  DynamicLibrary  DL = DynamicLibrary::addPermanentLibrary(dlopen(**"libSomething"**));
> >>  DL.getAddressOfSymbol("func"); // Fails
> > 
> > In that case, the client could check if DL is invalid, and then simply construct "DynamicLibrary(dlopen'd-value)". Perhaps the API could be cleaned up if we returned "Expected<DynamicLibrary>" from the static constructors, and got rid of the isValid method.
>
> Why not just have ** addPermanentLibrary** take an optional **bool*** indicating whether it was already loaded or not?
>  Whether it was loaded or not seems orthogonal to the task of adding a permanent library/getting a platform independent way to do lookups on it?


If you mean 'bool &', then your suggestion is actually very similar to mine (return 'Expected<DynamicLibrary>').

I agree that it's something we could improve, but think that it's something we can address as a follow-up (i.e I don't think the addPermanentLibrary API is broken as-is).

> Either way if https://reviews.llvm.org/D30107 is to be expanded per your comments this could also be discussed then rather than holding this up.

Sure.


https://reviews.llvm.org/D29955