[Lldb-commits] [lldb] [LLDB][Minidump] Have Minidumps save off and properly read TLS data (PR #109477)

[Pavel Labath via lldb-commits]

https://github.com/labath commented:

It may be best to separate the loading and saving parts into separate PRs, as the two are functionality separate. (and I have -- separate -- questions about each part).

For loading, I'd like to discuss the back door you're adding DidAttach function. The thing I don't like there is that this is already a sort of a back door added in http://reviews.llvm.org/D9471 to support "processes which can load their own libraries". Here we have a process "that loads its own libraries" (that's how it was able to get away without a dynamic loader so far), *but* which does want the loader plugin to do some loading as well (What exactly is the thing that's missing due to this check? Is it that `m_loaded_modules` does not get populated?).

Instead of poking holes in that check, I'd like to see if we can reduce the scope of the original patch, as it seems to have been added in support of a very specific use case. So specific that I can't even think of what it was -- the patch is light on specifics, and I don't know of any gdb server that `qxfer:libraries` (as opposed to `qxfer:libraries-svr4`) *and* the "posix" dynamic loader plugin.

If we can't come up with a good solution with reasonable effort, then I'd be willing to even reduce its scope to zero (effectively revert that part of the patch), as we should not be carrying (untested?) code which we don't understand, particularly when it causes problems (I doubt the author of that patch tested TLS, but your PR shows that it does not work this way, and I also remember having problems with these lines of code in the past).

For the second part of the patch (saving minidumps) I'd like to better understand what it is that you're saving with this code. In particular, I'm confused as to how we can save TLS by iterating over the modules (alone). For the most general (general dynamic tls model) case, I think you'd need a (thread`x`module) nested loop since `dlopen`ed shared libraries may have their TLS allocated in a random piece of memory by malloc (TLS is [very](https://maskray.me/blog/2021-02-14-all-about-thread-local-storage) complicated). Instead of trying to piece it together generically (the thread register is just the beginning) it may be better to ask the dynamic linker plugin to provide the list of memory regions that (may) contain the TLS data.

https://github.com/llvm/llvm-project/pull/109477