<div dir="ltr">Deal.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 27, 2014 at 12:40 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I'm fine with that, although I would prefer to address it by fixing DynamicLibrary in LLVM to support this behavior in LLVM if we find that it breaks something.  Sharing as much code as possible should be an explicit goal IMO, and especially duplicating entire classes should be shunned quite strongly.  Nevertheless, if any fallout should arise, I will make sure to deal with it until it's not broken.</div>
<div class="HOEnZb"><div class="h5">
<div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 27, 2014 at 12:35 PM, Todd Fiala <span dir="ltr"><<a href="mailto:tfiala@google.com" target="_blank">tfiala@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div dir="ltr">I'm personally okay with this on the Linux side, with the caveat as always that if we find an issue with this, we back out the change.<span><font color="#888888"><div><br></div><div>-Todd</div>
</font></span></div><div><div><div class="gmail_extra"><br><br>
<div class="gmail_quote">On Wed, Aug 27, 2014 at 11:51 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<div dir="ltr">Combined with the fact that plugins are essentially a dead codepath (going by Enrico's earlier comment), I think it's probably ok to just remove this and use LLVM's default DynamicLibrary loader.  Any objections?</div>


<div><div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 27, 2014 at 11:49 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br>



<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">The documentation of dlsym() says this:<div><br></div><div>------ </div><div><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px">The </span><i style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px">dlsym</i><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px">() function shall search for the named symbol in all objects loaded automatically <b>as a result of loading the object referenced by </b></span><i style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px"><b>handle</b></i><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px"> (see </span><a href="http://pubs.opengroup.org/onlinepubs/009695399/functions/dlopen.html" style="color:rgb(102,102,255);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px" target="_blank"><i>dlopen</i>()</a><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px">). Load ordering is used in </span><i style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px">dlsym</i><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px">() operations upon the global symbol object. The symbol resolution algorithm used shall be dependency order as described in </span><a href="http://pubs.opengroup.org/onlinepubs/009695399/functions/dlopen.html" style="color:rgb(102,102,255);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px" target="_blank"><i>dlopen</i>()</a><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px">.</span><br>




</div><div><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px">------</span></div><div><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px"><br>




</span></div><div>So, it appears that this is perhaps a non-issue.  In other words, if you dlopen plugin A and get back handle A, and then you dlopen plugin B and get back handle B, there is no chance that dlsym against B would find a symbol in plugin A.  It will search only B and B's direct and indirect dependencies.<span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px"><br>




</span></div><div><br></div><div>There is a remote possibility that a plugin A could link against <i>another plugin B.  </i>However, as long as A provides a LLDBPluginInitialize method, it will always be found first as described by the section on "dependency ordering" in the documentation of dlopen().</div>




<div><br></div><div>----- </div><div><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13px">Dependency ordering uses a breadth-first order <b>starting with a given object</b>, then all of its dependencies, then any dependents of those, iterating until all dependencies are satisfied.</span><br>




</div></div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 27, 2014 at 11:32 AM, Greg Clayton <span dir="ltr"><<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>></span> wrote:<br>




<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br>
> On Aug 26, 2014, at 5:09 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
><br>
> I think my understanding of what it does is correct, but maybe Greg or someone can confirm.  Basically, it tries to dlopen() the module at the path specified, and then search for the symbols LLDBPluginInitialize and LLDBPluginTerminate.  If it finds them, it calls them.  If it doesn't, the plugin load fails.  According to the documentation of dladdr(), it appears that the process for locating this symbol involves first searching the module specified in the argument to dlopen(), and then searching any dependent modules.  If it is found in any of these, it succeeds.  This optimization (using RTLD_FIRST and the filename comparison), causes this search to fail if the symbol is found in a dependent module, but not the original module.<br>





<br>
</div>Bingo. We don't want to call any other version of LLDBPluginInitialize or LLDBPluginTerminate from any other plug-in. It isn't clear that the dynamic linker sticks to dependent modules, I would need to check on that. It might search all loaded shared libraries in the current process. Not sure if that differs between Mac and Linux. Probably not.<br>





<div><br>
><br>
> I will try to verify that this is correct with someone who knows more than me about Linux, Mac, and dynamic linking on these platforms, but if correct then it doesn't seem like there is any risk to removing this.  That said, I'm interested in who is actually use these plugins.  The best way to find out if it's going to break something is to talk to the people who depend on this code.<br>





<br>
</div>Again, we _only_ want to call LLDBPluginInitialize or LLDBPluginTerminate from the exact shared library we are trying to ask for the symbols from, not from anywhere else.<br>
<span><font color="#888888"><br>
Greg<br>
</font></span><div><div>><br>
><br>
> On Tue, Aug 26, 2014 at 5:01 PM, Todd Fiala <<a href="mailto:tfiala@google.com" target="_blank">tfiala@google.com</a>> wrote:<br>
> Ah ok.<br>
><br>
> It's worth figuring out what it does (really) before we consider removing it.<br>
><br>
><br>
><br>
> On Tue, Aug 26, 2014 at 4:53 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> That part is a good question, which I don't totally understand.  There's a function Debugger::LoadPlugin() though, which accepts a path to a plugin to load.  It's called there.  This also appears to be exposed through the "plugin load" command.<br>





><br>
><br>
> On Tue, Aug 26, 2014 at 4:32 PM, Todd Fiala <<a href="mailto:tfiala@google.com" target="_blank">tfiala@google.com</a>> wrote:<br>
> I guess the thing to do is make sure we're certain we understand the behavior, which is perhaps best captured in a test.  (i.e. test it with the RTLD_FIRST behavior where it does something, then verify it does something different without the flag.  Then, once we agree it is not useful behavior for us, look at removing it).<br>





><br>
> By valid plugin, you're referring to shared libraries, right?  (What plugins are we referring to here, at what load point?)<br>
><br>
><br>
> On Tue, Aug 26, 2014 at 4:29 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> Just as a counterpoint, unless I'm misunderstanding this code, I don't see it actually having a noticeable impact on stability.  The search limiting will only be a factor in a case where you attempt to load something that *isn't a valid plugin*.  It's already an error path.  In fact, this code worked fine before the change was made, and was only made to imitate what appears to have been an optimization that was Mac-specific.  The change for Mac doesn't seem to have been strictly necessary either, but just an optimization.  It's actually not an optimization for Linux, because the dynamic loader will still search every module on linux, it will just fail anyway.<br>





><br>
><br>
> On Tue, Aug 26, 2014 at 4:21 PM, Todd Fiala <<a href="mailto:tfiala@google.com" target="_blank">tfiala@google.com</a>> wrote:<br>
> Probably the way I'd look at this right now is that support in Linux is a bit dicey and we're doing our best to stabilize (starting with single path for remote/local debugging, and making that stable and fast).  In an effort to stabilize, I'd prefer to limit how much code change we do on the Linux end until we have a more stable product.<br>





><br>
> So while we could potentially take that out, I'd rather avoid making changes just because it might be simpler, as it might also add yet another error scenario on the Linux side.  Right now I value similarity to MacOSX execution over code reduction.  Once we're a lot more stable on the Linux side, I'd be much more interested in revisiting with some actual use cases to see diffs in performance and scope of usage.<br>





><br>
> Just my 2 cents...<br>
><br>
><br>
> On Tue, Aug 26, 2014 at 3:47 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> The review is up on the LLVM side.  One point which was raised, and which I agree with, is that the presence of the string makes the class much heavier.  This string is only needed to mimic MacOSX's RTLD_FIRST behavior on other posix platforms.  However, going back through the history of when this was added, I never actually saw a use case from anyone saying "we *need* this on Linux".  See the full original thread at [1].  But the TL;DR is that the flag is nice to have on MacOSX, and the filename comparison was added to Linux to maintain parity.<br>





><br>
> If nobody actually knows of a specific example of why this is necessary on Linux, can we just remove this behavior on Linux?  My understanding is that the only thing which will change by removing this for Linux is the following: Imagine a plugin X is loaded, and X has a library dependency on Y and Z.  X doesn't contain the plugin Initialize or Terminate symbol, but Y or Z does.  With the filename comparison code, LoadPlugin would fail, and without it, it would succeed and use the symbol found in Y or Z.  I can understand that with the comparison the algorithm is a bit better, but it seems such an extremely unusual edge case that I don't think it's a big deal to remove it from the Linux side.<br>





><br>
> Thoughts?<br>
><br>
> [1] - <a href="http://thread.gmane.org/gmane.comp.debugging.lldb.devel/300/focus=302" target="_blank">http://thread.gmane.org/gmane.comp.debugging.lldb.devel/300/focus=302</a><br>
><br>
><br>
> On Thu, Aug 21, 2014 at 5:27 PM, Greg Clayton <<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>> wrote:<br>
> Sounds good to me. Hopefully if they don't want that they might accept an extra boolean argument that can specify to only look in the current shared library and then we can switch over to using LLVM's DynamicLibrary.<br>





><br>
> > On Aug 21, 2014, at 4:22 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> ><br>
> > This seems like the only case we ever want, so I'm going to post a patch to LLVM's DynamicLibrary class to use RTLD_FIRST on Apple, and a similar method of checking the module filespec on other platforms, and see if they accept it.  If so, I will convert our Plugin code to use LLVM's DynamicLibrary and then delete our DynamicLibrary<br>





> ><br>
> ><br>
> > On Thu, Aug 21, 2014 at 4:08 PM, Greg Clayton <<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>> wrote:<br>
> ><br>
> > > On Aug 21, 2014, at 3:31 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> > ><br>
> > > Can someone explain this flag to me?<br>
> ><br>
> > It says "only look in this binary, don't look in any others. We are looking for a plug-in initialization function and we don't want to get one back from another dylib.<br>
> ><br>
> > As Enrico said, the email from a while back details this:<br>
> ><br>
> > <a href="http://comments.gmane.org/gmane.comp.debugging.lldb.devel/305" target="_blank">http://comments.gmane.org/gmane.comp.debugging.lldb.devel/305</a><br>
> ><br>
> > >  I've read the documentation, but it's still not clear to me.  If you ask dlsym() to search some module X, why would it ever search modules other than X?<br>
> ><br>
> > I don't know but it does.<br>
> ><br>
> > ><br>
> > > The reason I ask about this is that llvm support library already has a DynamicLibrary class whose purpose almost exactly matches what we're using the Host::DynamicLibrary related functions for.  However, it doesn't use the RTLD_FIRST flag, and so I'm not sure what the implications are of us using it and deleting our own DynamicLibrary code.<br>





> ><br>
> > It would be nice if we could specify this flag so we either find the symbol from libx.dylib or we don't. We don't want to find the symbol in liby.dylib and call it in our case.<br>
> ><br>
><br>
><br>
><br>
> _______________________________________________<br>
> lldb-dev mailing list<br>
> <a href="mailto:lldb-dev@cs.uiuc.edu" target="_blank">lldb-dev@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev</a><br>
><br>
><br>
><br>
><br>
> --<br>
> Todd Fiala |   Software Engineer |     <a href="mailto:tfiala@google.com" target="_blank">tfiala@google.com</a> |     <a href="tel:650-943-3180" value="+16509433180" target="_blank">650-943-3180</a><br>
><br>
><br>
><br>
><br>
><br>
> --<br>
> Todd Fiala |   Software Engineer |     <a href="mailto:tfiala@google.com" target="_blank">tfiala@google.com</a> |     <a href="tel:650-943-3180" value="+16509433180" target="_blank">650-943-3180</a><br>
><br>
><br>
><br>
><br>
><br>
> --<br>
> Todd Fiala |   Software Engineer |     <a href="mailto:tfiala@google.com" target="_blank">tfiala@google.com</a> |     <a href="tel:650-943-3180" value="+16509433180" target="_blank">650-943-3180</a><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><table cellspacing="0" cellpadding="0" style="color:rgb(136,136,136);font-family:'Times New Roman'"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small">


<td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Todd Fiala |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td>


<td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tfiala@google.com" style="color:rgb(17,85,204)" target="_blank"><span style="background-color:rgb(255,255,204);color:rgb(34,34,34);background-repeat:initial initial">tfiala@google.com</span></a> |</td>


<td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"><font color="#1155cc"> <a>650-943-3180</a></font></td></tr></tbody></table><br></div>
</div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><table cellspacing="0" cellpadding="0" style="color:rgb(136,136,136);font-family:'Times New Roman'"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small">
<td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Todd Fiala |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td>
<td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tfiala@google.com" style="color:rgb(17,85,204)" target="_blank"><span style="background-color:rgb(255,255,204);color:rgb(34,34,34);background-repeat:initial initial">tfiala@google.com</span></a> |</td>
<td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"><font color="#1155cc"> <a>650-943-3180</a></font></td></tr></tbody></table><br></div>
</div>