[llvm-commits] [PATCH] Look up symbols in a specific library with DynamicLibrary

[Argyrios Kyrtzidis]

Hi Jordy,

Thanks for working on it, comments below.

On Aug 2, 2011, at 9:38 PM, Jordy Rose wrote:

> This patch adds the ability to look up symbols in a specific library by instantiating llvm::sys::DynamicLibrary. I've tried to make it minimally intrusive and preserve the existing interface (LoadLibraryPermanently and SearchForAddressOfSymbol). Requiring that DynamicLibrary instances must represent already-loaded libraries also means not having to deal with potentially unloaded libraries, though since both dlfcn handles and HMODULEs are ref-counted, that shouldn't be too hard to deal with if we ever lift this restriction.

How about having DynamicLibrary just be a thin wrapper over the handle (no dlopen/dlclose), a RAII DynamicLibrary does not seem very useful.
LoadLibraryPermanently can get an optional DynamicLibrary* in order to return a DynamicLibrary instance, on which you would then call its getAddressOfSymbol.

I'd also suggest that the library filename is kept associated with its handle so that there is a static getLoadedLibrary method returning a DynamicLibrary instance (and LoadLibraryPermanently could check with getLoadedLibrary before doing a dlopen).

Also a nitpick

> +    /// Returns true if the object refers to a valid library.
> +    bool valid() { return Data != NULL; }


'isValid' seems to be more common in the llvm/clang codebase.

What do you think ?

-Argyrios

> 
> The use case for this is for Clang plugins loaded with -load, which can have standard entry points like clang_registerASTActions.
> 
> The advantages over llvm::Registry:
> - it is easier on Windows (or so I've heard)
> - it allows us to be lazy (e.g. no need to register plugin AST actions if we're running a standard action)
> 
> (I'm not pushing for a change in how ASTConsumer plugins are registered. The impetus for this is coming out of a discussion with Ted, Argyrios, and Anna on pluggable analyzer checkers.)
> 
> Is this approach reasonable?
> Thanks,
> Jordy
> 
> P.S. I would especially appreciate if someone could look over my Windows changes; I have very little Windows experience and don't have a Windows dev machine available.

> +    /// Returns true if the object refers to a valid library.
> +    bool valid() { return Data != NULL; }


'isValid' seems to be more common in the llvm/clang codebase.

> +    /// Creates a reference to a dynamic library. The library must already have
> +    /// been loaded using LoadLibraryPermanently().
> +    explicit DynamicLibrary(const char *filename);
> +    ~DynamicLibrary();


> +
> +DynamicLibrary::DynamicLibrary(const char *filename) {
> +  assert(filename && "DynamicLibrary is only for modules loaded by filename.");
> +  Data = dlopen(filename, RTLD_LAZY|RTLD_GLOBAL);
> +
> +#ifndef NDEBUG
> +  if (Data != 0) {
> +    assert(OpenedHandles &&
> +      "DynamicLibrary instances are only for already-loaded modules.");
> +    std::vector<void *>::const_iterator i =
> +      find(OpenedHandles->begin(), OpenedHandles->end(), Data);
> +    assert((i != OpenedHandles->end()) &&
> +      "DynamicLibrary instances are only for already-loaded modules.");
> +  }
> +#endif // NDEBUG
> +}
> +
> +DynamicLibrary::~DynamicLibrary() {
> +  if (valid())
> +    dlclose(Data);
> +}



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110805/49325551/attachment.html>