[Lldb-commits] [PATCH] D26093: Limit LLDB_EXPORT_ALL_SYMBOLS to additionally export only the lldb_private namespace symbols

[Todd Fiala via lldb-commits]

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