<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>Hi Jordy,</div><div><br></div><div>Thanks for working on it, comments below.</div><div><br></div><div>On Aug 2, 2011, at 9:38 PM, Jordy Rose wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>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.<br></div></blockquote><div><br></div><div>How about having DynamicLibrary just be a thin wrapper over the handle (no dlopen/dlclose), a RAII DynamicLibrary does not seem very useful.</div><div>LoadLibraryPermanently can get an optional DynamicLibrary* in order to return a DynamicLibrary instance, on which you would then call its getAddressOfSymbol.</div><div><br></div><div>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).</div><div><br></div><div>Also a nitpick</div><div><br></div><div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ /// Returns true if the object refers to a valid library.</font></div><div><font class="Apple-style-span" color="#000000">+ bool valid() { return Data != NULL; }</font></div></blockquote></div><div><div><br></div></div><div>'isValid' seems to be more common in the llvm/clang codebase.</div><div><br></div></div><div>What do you think ?</div><div><br></div><div>-Argyrios</div><br><blockquote type="cite"><div><br>The use case for this is for Clang plugins loaded with -load, which can have standard entry points like clang_registerASTActions.<br><br>The advantages over llvm::Registry:<br>- it is easier on Windows (or so I've heard)<br>- it allows us to be lazy (e.g. no need to register plugin AST actions if we're running a standard action)<br><br>(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.)<br><br>Is this approach reasonable?<br>Thanks,<br>Jordy<br><br>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.<br></div></blockquote><div><br></div></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ /// Returns true if the object refers to a valid library.</font></div><div><font class="Apple-style-span" color="#000000">+ bool valid() { return Data != NULL; }</font></div></blockquote></div><div><div><br></div></div><div>'isValid' seems to be more common in the llvm/clang codebase.</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ /// Creates a reference to a dynamic library. The library must already have</font></div><div><font class="Apple-style-span" color="#000000">+ /// been loaded using LoadLibraryPermanently().</font></div><div><font class="Apple-style-span" color="#000000">+ explicit DynamicLibrary(const char *filename);</font></div><div><font class="Apple-style-span" color="#000000">+ ~DynamicLibrary();</font></div></blockquote></div><div><div><br></div></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+DynamicLibrary::DynamicLibrary(const char *filename) {</font></div><div><font class="Apple-style-span" color="#000000">+ assert(filename && "DynamicLibrary is only for modules loaded by filename.");</font></div><div><font class="Apple-style-span" color="#000000">+ Data = dlopen(filename, RTLD_LAZY|RTLD_GLOBAL);</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+#ifndef NDEBUG</font></div><div><font class="Apple-style-span" color="#000000">+ if (Data != 0) {</font></div><div><font class="Apple-style-span" color="#000000">+ assert(OpenedHandles &&</font></div><div><font class="Apple-style-span" color="#000000">+ "DynamicLibrary instances are only for already-loaded modules.");</font></div><div><font class="Apple-style-span" color="#000000">+ std::vector<void *>::const_iterator i =</font></div><div><font class="Apple-style-span" color="#000000">+ find(OpenedHandles->begin(), OpenedHandles->end(), Data);</font></div><div><font class="Apple-style-span" color="#000000">+ assert((i != OpenedHandles->end()) &&</font></div><div><font class="Apple-style-span" color="#000000">+ "DynamicLibrary instances are only for already-loaded modules.");</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+#endif // NDEBUG</font></div><div><font class="Apple-style-span" color="#000000">+}</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+DynamicLibrary::~DynamicLibrary() {</font></div><div><font class="Apple-style-span" color="#000000">+ if (valid())</font></div><div><font class="Apple-style-span" color="#000000">+ dlclose(Data);</font></div><div><font class="Apple-style-span" color="#000000">+}</font></div></blockquote></div><div><div><br></div></div><div><br></div></body></html>