tfiala added a comment.

In https://reviews.llvm.org/D26093#582583, @beanz wrote:

> This patch ends up hiding the problem, not fixing it.


Yes.

> If it unblocks something it might be ok, but it doesn't actually fix the 
> problem.

It allows debugging of lldb with lldb on Ubuntu.  This broke recently, and the 
CL here re-enables the ability to do that.

> The underlying problem is that liblldb and lldb-mi have their own copies of 
> LLVM with their own copies of the GlobalParser static 
> (llvm/lib/Support/CommandLine.cpp), and the corresponding static for the 
> debug cl::opt (llvm/lib/Support/Debug.cpp).
> 
> When the library exports the LLVM symbols, the dynamic loader loads lldb-mi 
> and it makes the cl::opt constructor point at the same instance of the 
> GlobalParser (following link-once ODR rules).
> 
> When the library doesn't export the LLVM symbols, the cl::opt variables 
> inside liblldb resolve to the GlobalParser inside liblldb (because it is 
> local), and the cl::opt variables inside lldb-mi resolve inside lldb-mi 
> because the loader cannot see the GlobalParser from liblldb because the 
> symbol isn't exported. This makes two instances of the GlobalParser exist at 
> the same time.

Right.  I think I said all that in comments above, but perhaps that is a 
succinct summary :-)

> This does work around the problem Todd was seeing. Unfortunately, it doesn't 
> actually fix the problem, and if lldb-mi is using cl::opt across the library 
> boundary it will cause subtle and difficult to diagnose bugs.

As lldb-mi doesn't use CommandLine to the best of my understanding by searching 
for cl usages in the code, I don't see that as critical.  The current usage of 
CommandLine via global static constructors in Debug.cpp seems like a poor 
choice, as it forces all users of llvm with NDEBUG not defined to get some 
command line behavior that they may never use.  Maybe some aspect of that can 
be made lazy?

Note that this change neither introduces the issue of having two islands of 
CommandLine data --- that is already there in standard builds of lldb-mi --- it 
simply prevents the "please allow me to debug lldb with lldb" case from getting 
clobbered by the dynamic linker coalescing which the old implementation of 
LLDB_EXPORT_ALL_SYMBOLS did not avoid.

Unless there is strong opposition, I'd like to put this in.


https://reviews.llvm.org/D26093



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to