[LLDB][Minidump] Have Minidumps save off and properly read TLS data #109477
Conversation
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesThis patch adds the support to Full diff: https://github.com/llvm/llvm-project/pull/109477.diff 7 Files Affected:
diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
index 51e4b3e6728f23..38e9a61817ab55 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -115,7 +115,8 @@ void DynamicLoaderPOSIXDYLD::DidAttach() {
// don't rebase if the module already has a load address
Target &target = m_process->GetTarget();
Address addr = obj->GetImageInfoAddress(&target);
- if (addr.GetLoadAddress(&target) != LLDB_INVALID_ADDRESS)
+ addr_t load_addr = addr.GetLoadAddress(&target);
+ if (load_addr != LLDB_INVALID_ADDRESS)
rebase_exec = false;
}
} else {
@@ -124,7 +125,7 @@ void DynamicLoaderPOSIXDYLD::DidAttach() {
}
// if the target executable should be re-based
- if (rebase_exec) {
+ if (rebase_exec || IsCoreFile()) {
ModuleList module_list;
module_list.Append(executable_sp);
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
index 32ffba763c08e3..2a8f9fe9b9d93d 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -21,6 +21,7 @@
#include "lldb/Interpreter/CommandReturnObject.h"
#include "lldb/Interpreter/OptionArgParser.h"
#include "lldb/Interpreter/OptionGroupBoolean.h"
+#include "lldb/Target/DynamicLoader.h"
#include "lldb/Target/JITLoaderList.h"
#include "lldb/Target/MemoryRegionInfo.h"
#include "lldb/Target/SectionLoadList.h"
@@ -34,6 +35,7 @@
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Threading.h"
+#include "Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h"
#include "Plugins/ObjectFile/Placeholder/ObjectFilePlaceholder.h"
#include "Plugins/Process/Utility/StopInfoMachException.h"
@@ -333,6 +335,23 @@ ArchSpec ProcessMinidump::GetArchitecture() {
return ArchSpec(triple);
}
+DynamicLoader *ProcessMinidump::GetDynamicLoader() {
+ if (m_dyld_up.get() == nullptr)
+ m_dyld_up.reset(DynamicLoader::FindPlugin(
+ this, DynamicLoaderPOSIXDYLD::GetPluginNameStatic()));
+ return m_dyld_up.get();
+}
+
+DataExtractor ProcessMinidump::GetAuxvData() {
+ std::optional<llvm::ArrayRef<uint8_t>> auxv =
+ m_minidump_parser->GetStream(StreamType::LinuxAuxv);
+ if (!auxv)
+ return DataExtractor();
+
+ return DataExtractor(auxv->data(), auxv->size(), ByteOrder::eByteOrderLittle,
+ GetAddressByteSize(), GetAddressByteSize());
+}
+
void ProcessMinidump::BuildMemoryRegions() {
if (m_memory_regions)
return;
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.h b/lldb/source/Plugins/Process/minidump/ProcessMinidump.h
index f2ea0a2b61d14e..332e2637cc7ee7 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.h
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.h
@@ -53,11 +53,12 @@ class ProcessMinidump : public PostMortemProcess {
Status DoLoadCore() override;
- DynamicLoader *GetDynamicLoader() override { return nullptr; }
+ // Returns AUXV structure found in the core file
+ lldb_private::DataExtractor GetAuxvData() override;
- llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
+ DynamicLoader *GetDynamicLoader() override;
- SystemRuntime *GetSystemRuntime() override { return nullptr; }
+ llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
Status DoDestroy() override;
diff --git a/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp b/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
index e879c493156593..f305d1b7031d82 100644
--- a/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
+++ b/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
@@ -44,6 +44,17 @@ static void writeRegister(const void *reg_src, uint8_t *context,
memcpy(reg_dest.data(), reg_src, reg_dest.size());
}
+// TODO: Fix the registers in this file!
+// writeRegister checks x86_64 registers without base registers. This causes
+// an overlap in the register enum values. So we were truncating fs_base.
+// We should standardize to the x86_64_with_base registers.
+static void writeBaseRegister(const void *reg_src, uint8_t *context,
+ const RegisterInfo ®) {
+ auto bytes = reg.mutable_data(context);
+ llvm::MutableArrayRef<uint8_t> reg_dest = bytes.take_front(8);
+ memcpy(reg_dest.data(), reg_src, reg_dest.size());
+}
+
lldb::DataBufferSP lldb_private::minidump::ConvertMinidumpContext_x86_64(
llvm::ArrayRef<uint8_t> source_data,
RegisterInfoInterface *target_reg_interface) {
@@ -105,11 +116,12 @@ lldb::DataBufferSP lldb_private::minidump::ConvertMinidumpContext_x86_64(
writeRegister(&context->r15, result_base, reg_info[lldb_r15_x86_64]);
}
+ // See comment on base regsiter
if ((context_flags & LLDBSpecificFlag) == LLDBSpecificFlag) {
- writeRegister(&context->fs_base, result_base,
- reg_info[x86_64_with_base::lldb_fs_base]);
- writeRegister(&context->gs_base, result_base,
- reg_info[x86_64_with_base::lldb_gs_base]);
+ writeBaseRegister(&context->fs_base, result_base,
+ reg_info[x86_64_with_base::lldb_fs_base]);
+ writeBaseRegister(&context->gs_base, result_base,
+ reg_info[x86_64_with_base::lldb_gs_base]);
}
// TODO parse the floating point registers
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index aca08972811470..8ddb2be409f855 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6528,6 +6528,74 @@ static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
CreateCoreFileMemoryRange(region));
}
+static void AddRegisterSections(Process &process, ThreadSP &thread_sp,
+ CoreFileMemoryRanges &ranges,
+ lldb::addr_t range_end) {
+ lldb::RegisterContextSP reg_ctx = thread_sp->GetRegisterContext();
+ if (!reg_ctx)
+ return;
+
+ const RegisterInfo *reg_info = reg_ctx->GetRegisterInfo(
+ lldb::RegisterKind::eRegisterKindGeneric, LLDB_REGNUM_GENERIC_TP);
+ if (!reg_info)
+ return;
+
+ lldb_private::RegisterValue reg_value;
+ bool success = reg_ctx->ReadRegister(reg_info, reg_value);
+ if (!success)
+ return;
+
+ const uint64_t fail_value = UINT64_MAX;
+ bool readSuccess = false;
+ const lldb::addr_t reg_value_addr =
+ reg_value.GetAsUInt64(fail_value, &readSuccess);
+ if (!readSuccess || reg_value_addr == fail_value)
+ return;
+
+ MemoryRegionInfo register_region;
+ Status err = process.GetMemoryRegionInfo(reg_value_addr, register_region);
+ if (err.Fail())
+ return;
+
+ // We already saved off this truncated stack range.
+ if (register_region.GetRange().GetRangeEnd() == range_end)
+ return;
+
+ // We don't need to worry about duplication because the CoreFileMemoryRanges
+ // will unique them
+ AddRegion(register_region, true, ranges);
+}
+
+static void AddModuleSections(Process &process, CoreFileMemoryRanges &ranges,
+ std::set<addr_t> &stack_ends) {
+ ModuleList &module_list = process.GetTarget().GetImages();
+ Target *target = &process.GetTarget();
+ for (size_t idx = 0; idx < module_list.GetSize(); idx++) {
+ ModuleSP module_sp = module_list.GetModuleAtIndex(idx);
+ if (!module_sp)
+ continue;
+
+ ObjectFile *obj = module_sp->GetObjectFile();
+ if (!obj)
+ continue;
+ Address addr = obj->GetImageInfoAddress(target);
+ addr_t load_addr = addr.GetLoadAddress(target);
+ if (load_addr == LLDB_INVALID_ADDRESS)
+ continue;
+
+ MemoryRegionInfo tls_storage_region;
+ Status err = process.GetMemoryRegionInfo(load_addr, tls_storage_region);
+ if (err.Fail())
+ continue;
+
+ // We already saved off this truncated stack range.
+ if (stack_ends.count(tls_storage_region.GetRange().GetRangeEnd()) > 0)
+ continue;
+
+ AddRegion(tls_storage_region, true, ranges);
+ }
+}
+
static void SaveOffRegionsWithStackPointers(Process &process,
const SaveCoreOptions &core_options,
const MemoryRegionInfos ®ions,
@@ -6559,11 +6627,14 @@ static void SaveOffRegionsWithStackPointers(Process &process,
// off in other calls
sp_region.GetRange().SetRangeBase(stack_head);
sp_region.GetRange().SetByteSize(stack_size);
- stack_ends.insert(sp_region.GetRange().GetRangeEnd());
+ const addr_t range_end = sp_region.GetRange().GetRangeEnd();
+ stack_ends.insert(range_end);
// This will return true if the threadlist the user specified is empty,
// or contains the thread id from thread_sp.
- if (core_options.ShouldThreadBeSaved(thread_sp->GetID()))
+ if (core_options.ShouldThreadBeSaved(thread_sp->GetID())) {
AddRegion(sp_region, try_dirty_pages, ranges);
+ AddRegisterSections(process, thread_sp, ranges, range_end);
+ }
}
}
}
@@ -6672,9 +6743,13 @@ Status Process::CalculateCoreFileSaveRanges(const SaveCoreOptions &options,
std::set<addr_t> stack_ends;
// For fully custom set ups, we don't want to even look at threads if there
// are no threads specified.
- if (core_style != lldb::eSaveCoreCustomOnly || options.HasSpecifiedThreads())
+ if (core_style != lldb::eSaveCoreCustomOnly ||
+ options.HasSpecifiedThreads()) {
SaveOffRegionsWithStackPointers(*this, options, regions, ranges,
stack_ends);
+ // Save off the load sections for the TLS data.
+ AddModuleSections(*this, ranges, stack_ends);
+ }
switch (core_style) {
case eSaveCoreUnspecified:
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
index ccdb6653cf16f8..80ebc884d3dbf5 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
@@ -522,3 +522,77 @@ def minidump_deleted_on_save_failure(self):
finally:
self.assertTrue(self.dbg.DeleteTarget(target))
+
+ @skipUnlessPlatform(["linux"])
+ @skipUnlessArch("x86_64")
+ def minidump_saves_fs_base_region(self):
+ """Test that verifies the minidump file saves region for fs_base"""
+
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ try:
+ target = self.dbg.CreateTarget(exe)
+ process = target.LaunchSimple(
+ None, None, self.get_process_working_directory()
+ )
+ self.assertState(process.GetState(), lldb.eStateStopped)
+ thread = process.GetThreadAtIndex(0)
+ custom_file = self.getBuildArtifact("core.reg_region.dmp")
+ options = lldb.SBSaveCoreOptions()
+ options.SetOutputFile(lldb.SBFileSpec(custom_file))
+ options.SetPluginName("minidump")
+ options.SetStyle(lldb.eSaveCoreCustomOnly)
+ options.AddThread(thread)
+ error = process.SaveCore(options)
+ self.assertTrue(error.Success())
+
+ registers = thread.GetFrameAtIndex(0).GetRegisters()
+ fs_base = registers.GetFirstValueByName("fs_base").GetValueAsUnsigned()
+ self.assertTrue(fs_base != 0)
+ core_target = self.dbg.CreateTarget(None)
+ core_proc = core_target.LoadCore(one_region_file)
+ core_region_list = core_proc.GetMemoryRegions()
+ live_region_list = process.GetMemoryRegions()
+ live_region = lldb.SBMemoryRegionInfo()
+ live_region_list.GetMemoryRegionForAddress(fs_base, live_region)
+ core_region = lldb.SBMemoryRegionInfo()
+ error = core_region_list.GetMemoryRegionForAddress(fs_base, core_region)
+ self.assertTrue(error.Success())
+ self.assertEqual(live_region, core_region)
+
+ finally:
+ self.assertTrue(self.dbg.DeleteTarget(target))
+ self.assertTrue(self.dbg.DeleteTarget(core_target))
+ if os.path.isfile(custom_file):
+ os.unlink(custom_file)
+
+ @skipUnlessPlatform(["linux"])
+ @skipUnlessArch("x86_64")
+ def minidump_saves_fs_base_region(self):
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ try:
+ target = self.dbg.CreateTarget(exe)
+ process = target.LaunchSimple(
+ None, None, self.get_process_working_directory()
+ )
+ self.assertState(process.GetState(), lldb.eStateStopped)
+ thread = process.GetThreadAtIndex(0)
+ tls_file = self.getBuildArtifact("core.tls.dmp")
+ options = lldb.SBSaveCoreOptions()
+ options.SetOutputFile(lldb.SBFileSpec(tls_file))
+ options.SetPluginName("minidump")
+ options.SetStyle(lldb.eSaveCoreCustomOnly)
+ options.AddThread(thread)
+ error = process.SaveCore(options)
+ self.assertTrue(error.Success())
+ core_target = self.dbg.CreateTarget(None)
+ core_proc = core_target.LoadCore(tls_file)
+ frame = core_proc.GetThreadAtIndex(0).GetFrameAtIndex(0)
+ tls_val = frame.FindValue("lf")
+ self.assertEqual(tls_val.GetValueAsUnsigned(), 42)
+
+ except:
+ self.assertTrue(self.dbg.DeleteTarget(target))
+ if os.path.isfile(tls_file):
+ os.unlink(tls_file)
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/main.cpp b/lldb/test/API/functionalities/process_save_core_minidump/main.cpp
index fa34a371f20647..15daa68e9a648c 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/main.cpp
+++ b/lldb/test/API/functionalities/process_save_core_minidump/main.cpp
@@ -1,6 +1,7 @@
#include <cassert>
#include <iostream>
#include <thread>
+thread_local size_t lf = 42;
void g() { assert(false); }
|
There was a problem hiding this comment.
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 (threadxmodule) nested loop since dlopened shared libraries may have their TLS allocated in a random piece of memory by malloc (TLS is very 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.
|
@labath I really like the idea to add the TLS saving to the dynamic loader, I'll add that, As for the dynamic loader, I don't like how I've modified it. When a minidump is loaded, the link map isn't properly relocated, so TLS data is searched for relative to module lowest in memory. So for my example, we were looking for TLS data in This codepath results in none of the modules being loaded, or rebased, and I don't why this should apply without checking all modules. |
Yeah, I don't like that code either, which is why I'm leaning more and more towards deleting it. Judging by the patch which introduced it, I believe this check is basically a proxy for "have the modules been loaded by the process class", and the intention more-or-less was to make the dynamic loader plugin a no-op in this case. What the other of the patch probably did not realize (just like I did not realize it when I was implementing module loading in ProcessMinidump) is that this makes thread local data access impossible (in fairness, it's quite possible that, at that point, thread-local access was not implemented yet). |
Great context. In my mind, I think every core file format will have to handle loading it's own modules that are from the core file. We will still want the dynamic loader plugin to do it's own book keeping. I haven't tested it, but I think we should allow the DYLD to do it's own book keeping but just not handle loading/unloading modules owned by the CoreProcess. This will likely require some extension of Process, but I think this a good evolution of the patch you linked |
|
Yeah, the thing which makes the core files different from live processes is that core files (at least minidumps, the situation is a bit fuzzier for elf cores) often have a better idea of where the files are located. OTOH, the dynamic loader plugin is a (I'm deliberately not using "the" because I could think of other places as well) for the thread-local storage lookup code, so we do the two to work together. Regarding the "allow the DYLD to do it's own book keeping but just not handle loading/unloading modules owned by the CoreProcess" idea, how exactly would that differ from what your change in |
I'm mentally struggling with this as there is a distinction I think between loading in a core file and actually loading of a module. ProcessMinidump handles loading modules defined in the module list, but the DYLD will need to relocate them. I want to support this workflow but not force it for all Cores (Which I am doing). I wonder if we can force have the process force relocation which is what I'm doing with the |
|
The way I see it, there should be no distinction between "loading in a core file" and "actually loading" a module. "Loading of a module", in the sense that I'm using it here, consists of two steps:
Both of these steps are necessary for a module to work properly, but it shouldn't matter who's doing the loading (except that probably thread local sections will not work for a module which was not loaded by the dynamic loader). I think there's nothing wrong with giving the dynamic loader a chance to load any additional modules (for all processes even, not just core files). Normally, it should compute the same address (if any) that was produced by the process class, but if it happens to compute a different one, so be it (it probably means one of them has a bug). Your mention of a relocation makes it sound like this is already happening though (process computing one load address and then the DYLD "relocating" it). Is that true? If so, then I think we ought to figure out what's causing the difference (and which one is correct). |
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
Outdated
Show resolved
Hide resolved
|
On Pavel's suggestion, I broke the DYLD changes into their own patch #110885 |
| @@ -1,6 +1,7 @@ | ||
| #include <cassert> | ||
| #include <iostream> | ||
| #include <thread> | ||
| thread_local size_t lf = 42; |
There was a problem hiding this comment.
To be sure thread-locals work properly, I'd suggest also creating a shared library (which is dlopened at runtime) and creating a non-main thread, which will be used to access the value of the variable.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
It's not clear to me what's up with the idea of moving the saving code to the dynamic loader plugin. Is that something you plan to work on?
The main thing that bothers me is the AddLinkMapSections function. Although it superficially generic, it uses the GetImageInfoAddress, which is only implemented by ObjectFileELF, and only called from the elf-core and posix-dyld plugins (i.e. code which already knows its dealing with elf object files), so I doubt it will be useful elsewhere. Even the name "link map" is very elf-specific, as there's nothing quite like that in other platforms/object file formats.
| @@ -83,7 +83,11 @@ ModuleSP DynamicLoader::GetTargetExecutable() { | ||
| ModuleSpec module_spec(executable->GetFileSpec(), | ||
| executable->GetArchitecture()); | ||
| auto module_sp = std::make_shared<Module>(module_spec); | ||
|
|
||
| // If we're a coredump and we already have a main executable, we don't | ||
| // need to reload the module list that target already has |
There was a problem hiding this comment.
Why is that? What is this trying to achieve/prevent?
There was a problem hiding this comment.
In the Testcase in MinidumpUUID, the one module which has a matching filename that isn't an actual match needs this check in order to work.
The situation is the DYLD will itself create a stub depending on some conditions, but this is after the process has already made a stub. So we need some way for the CoreProcess to supercede the dynamic loader clearing all loaded modules again, and so I added this
There was a problem hiding this comment.
Ok, I see. I'm not entirely happy by this, but it sort of makes sense, so I'm willing to look the other way. :P
lldb/source/Target/Process.cpp
Outdated
| @@ -6528,6 +6528,75 @@ static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages, | ||
| CreateCoreFileMemoryRange(region)); | ||
| } | ||
|
|
||
| static void AddSegmentRegisterSections(Process &process, ThreadSP &thread_sp, |
There was a problem hiding this comment.
This name is very x86 specific (other architectures don't have "segment registers"). If this is to be generic code, then I guess what you're really trying to do is to save memory regions which are pointed to by the "tp" register.
There was a problem hiding this comment.
In the DYLD, do you think the x86 specificity is a problem? We know the DYLD handles it with fs/gs_base. So do you still think this is a concern?
There was a problem hiding this comment.
Slightly less of a concern, but it still isn't right as this is still used for architectures without segment registers. It's also completely avoidable -- you could just call this "AddThreadLocalMemoryRegions" or something like that..
I completely forgot the recommendation to let the Dynamic loader handle this. I will convert this today. |
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
Outdated
Show resolved
Hide resolved
| @@ -83,7 +83,11 @@ ModuleSP DynamicLoader::GetTargetExecutable() { | ||
| ModuleSpec module_spec(executable->GetFileSpec(), | ||
| executable->GetArchitecture()); | ||
| auto module_sp = std::make_shared<Module>(module_spec); | ||
|
|
||
| // If we're a coredump and we already have a main executable, we don't | ||
| // need to reload the module list that target already has |
There was a problem hiding this comment.
Ok, I see. I'm not entirely happy by this, but it sort of makes sense, so I'm willing to look the other way. :P
lldb/source/Target/Process.cpp
Outdated
| @@ -6528,6 +6528,75 @@ static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages, | ||
| CreateCoreFileMemoryRange(region)); | ||
| } | ||
|
|
||
| static void AddSegmentRegisterSections(Process &process, ThreadSP &thread_sp, |
There was a problem hiding this comment.
Slightly less of a concern, but it still isn't right as this is still used for architectures without segment registers. It's also completely avoidable -- you could just call this "AddThreadLocalMemoryRegions" or something like that..
… figure out the TLS sections
…lvm#109477) This patch adds the support to `Process.cpp` to automatically save off TLS sections, either via loading the memory region for the module, or via reading `fs_base` via generic register. Then when Minidumps are loaded, we now specify we want the dynamic loader to be the `POSIXDYLD` so we can leverage the same TLS accessor code as `ProcessELFCore`. Being able to access TLS Data is an important step for LLDB generated minidumps to have feature parity with ELF Core dumps.
|
This broke minidumps in lldb: #119598 |
|
Hi @Jlalond I am looking at commits that warrant a release note for LLDB 20, and I think this and the rest of your Minidump changes might qualify. If you want to do that yourself you can send a PR adding a bullet point to https://github.com/llvm/llvm-project/blob/main/llvm/docs/ReleaseNotes.md#changes-to-lldb, describing the change. There are a bunch (I listed some in https://discourse.llvm.org/t/lldb-20-request-for-release-notes/84034/1) so I assume you are working towards an end goal, so if you could summarise them that'd be great but if you don't think it's there yet, we can leave it for next release. You also worked on SBSaveCore (#100443), same question there. Enough to warrant a release note? (I don't use this stuff myself, so I won't attempt to write it up, I'll just get it wrong :) ) |
|
Hey @DavidSpickett, sure I'm drafting that up now. What time frame should we include commits from? I ask because one of my original patches added 64b support, and paging. I assume we could include anyway as I don't see it in the last release notes. I'll also make a discourse account so this is easier in the future :) |
|
From the Discourse thread:
If there was unfinished work in 19 that is now finished by later PRs that's fair to include too.
Yeah sounds fine, I figured you'd want to group it all together so if it tells the story better, include it. |
Minidump files contain explicit information about load addresses of modules, so it can load them itself. This works on other platforms, but fails on darwin because DynamicLoaderDarwin nukes the loaded module list on initialization (which happens after the core file plugin has done its work). This used to work until llvm#109477, which enabled the dynamic loader plugins for minidump files in order to get them to provide access to TLS. Clearing the load list makes sense, but I think we could do it earlier in the process, so that both Process and DynamicLoader plugins get a chance to load modules. This patch does that by calling the function early in the launch/attach/load core flows. This fixes TestDynamicValue.py:test_from_core_file on darwin.
Minidump files contain explicit information about load addresses of modules, so it can load them itself. This works on other platforms, but fails on darwin because DynamicLoaderDarwin nukes the loaded module list on initialization (which happens after the core file plugin has done its work). This used to work until llvm#109477, which enabled the dynamic loader plugins for minidump files in order to get them to provide access to TLS. Clearing the load list makes sense, but I think we could do it earlier in the process, so that both Process and DynamicLoader plugins get a chance to load modules. This patch does that by calling the function early in the launch/attach/load core flows. This fixes TestDynamicValue.py:test_from_core_file on darwin.
Minidump files contain explicit information about load addresses of modules, so it can load them itself. This works on other platforms, but fails on darwin because DynamicLoaderDarwin nukes the loaded module list on initialization (which happens after the core file plugin has done its work). This used to work until #109477, which enabled the dynamic loader plugins for minidump files in order to get them to provide access to TLS. Clearing the load list makes sense, but I think we could do it earlier in the process, so that both Process and DynamicLoader plugins get a chance to load modules. This patch does that by calling the function early in the launch/attach/load core flows. This fixes TestDynamicValue.py:test_from_core_file on darwin.
Minidump files contain explicit information about load addresses of modules, so it can load them itself. This works on other platforms, but fails on darwin because DynamicLoaderDarwin nukes the loaded module list on initialization (which happens after the core file plugin has done its work). This used to work until llvm#109477, which enabled the dynamic loader plugins for minidump files in order to get them to provide access to TLS. Clearing the load list makes sense, but I think we could do it earlier in the process, so that both Process and DynamicLoader plugins get a chance to load modules. This patch does that by calling the function early in the launch/attach/load core flows. This fixes TestDynamicValue.py:test_from_core_file on darwin.
This patch adds the support to
Process.cppto automatically save off TLS sections, either via loading the memory region for the module, or via readingfs_basevia generic register. Then when Minidumps are loaded, we now specify we want the dynamic loader to be thePOSIXDYLDso we can leverage the same TLS accessor code asProcessELFCore. Being able to access TLS Data is an important step for LLDB generated minidumps to have feature parity with ELF Core dumps.