[lldb][Mach-O corefiles] Don't init Target arch to corefile #136065
Conversation
This patch is making three changes, when loading a Mach-O corefile: 1. At the start of `DoLoadCore`, if a binary was provided in addition to the corefile, initialize the Target's ArchSpec. 2. Before ProcessMachCore does its "exhaustive search" fallback, looking through the corefile contents for a userland dyld or mach kernel, we must make sure the Target has an ArchSpec, or methods that check the address word size, or initialize a DataExtractor based on the Target arch will not succeed. 3. Add logging when setting the Target's arch listing exactly what that setting was based on -- the corefile itself, or the main binary. Jonas landed a change last August (started with a patch from me) which removed the Target ArchSpec initialization at the start of DoLoadCore, in a scenario where the corefile had arch armv7 and the main binary had arch armv7em (Cortex-M), and there was python code in the main binary's dSYM which sets the operating system threads provider based on the Target arch. It did different things for armv7 or armv7em, and so it would fail. Jonas' patch removed any ArchSpec setting at the start of DoLoadCore, so we wouldn't have an incorrect arch value, but that broke the exhaustive search for kernel binaries, because we didn't have an address word size or endianness. This patch should navigate the needs of both use cases. I spent a good bit of time trying to construct a test to capture all of these requirements -- but it turns out to be a good bit difficult, encompassing both a genuine kernel corefiles and a microcontroller firmware corefiles. rdar://146821929
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) ChangesThis patch is making three changes, when loading a Mach-O corefile:
Jonas landed a change last August (started with a patch from me) which removed the Target ArchSpec initialization at the start of DoLoadCore, in a scenario where the corefile had arch armv7 and the main binary had arch armv7em (Cortex-M), and there was python code in the main binary's dSYM which sets the operating system threads provider based on the Target arch. It did different things for armv7 or armv7em, and so it would fail. Jonas' patch removed any ArchSpec setting at the start of DoLoadCore, so we wouldn't have an incorrect arch value, but that broke the exhaustive search for kernel binaries, because we didn't have an address word size or endianness. This patch should navigate the needs of both use cases. I spent a good bit of time trying to construct a test to capture all of these requirements -- but it turns out to be a good bit difficult, encompassing both a genuine kernel corefiles and a microcontroller firmware corefiles. rdar://146821929 Full diff: https://github.com/llvm/llvm-project/pull/136065.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
index 281f3a0db8f69..17362b2d2855d 100644
--- a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -426,6 +426,32 @@ void ProcessMachCore::LoadBinariesViaExhaustiveSearch() {
std::vector<addr_t> dylds_found;
std::vector<addr_t> kernels_found;
+ // To do an exhaustive search, we'll need to create data extractors
+ // to get correctly sized/endianness fields, and if the Target still
+ // doesn't have an architecture set, we need to seed it from either
+ // the main binary (if we were given one) or the corefile cputype/cpusubtype.
+ if (!GetTarget().GetArchitecture().IsValid()) {
+ Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Target));
+ ModuleSP exe_module_sp = GetTarget().GetExecutableModule();
+ if (exe_module_sp && exe_module_sp->GetArchitecture().IsValid()) {
+ LLDB_LOGF(log,
+ "ProcessMachCore::%s: Was given binary + corefile, setting "
+ "target ArchSpec to binary to start",
+ __FUNCTION__);
+ GetTarget().SetArchitecture(exe_module_sp->GetArchitecture());
+ } else {
+ // The corefile's architecture is our best starting point.
+ ArchSpec arch(m_core_module_sp->GetArchitecture());
+ if (arch.IsValid()) {
+ LLDB_LOGF(log,
+ "ProcessMachCore::%s: Setting target ArchSpec based on "
+ "corefile mach-o cputype/cpusubtype",
+ __FUNCTION__);
+ GetTarget().SetArchitecture(arch);
+ }
+ }
+ }
+
const size_t num_core_aranges = m_core_aranges.GetSize();
for (size_t i = 0; i < num_core_aranges; ++i) {
const VMRangeToFileOffset::Entry *entry = m_core_aranges.GetEntryAtIndex(i);
@@ -569,6 +595,7 @@ Status ProcessMachCore::DoLoadCore() {
error = Status::FromErrorString("invalid core module");
return error;
}
+ Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Target));
ObjectFile *core_objfile = m_core_module_sp->GetObjectFile();
if (core_objfile == nullptr) {
@@ -578,20 +605,47 @@ Status ProcessMachCore::DoLoadCore() {
SetCanJIT(false);
+ // If we have an executable binary in the Target already,
+ // use that to set the Target's ArchSpec.
+ //
+ // Don't initialize the ArchSpec based on the corefile's cputype/cpusubtype
+ // here, the corefile creator may not know the correct subtype of the code
+ // that is executing, initialize the Target to that, and if the
+ // main binary has Python code which initializes based on the Target arch,
+ // get the wrong subtype value.
+ ModuleSP exe_module_sp = GetTarget().GetExecutableModule();
+ if (exe_module_sp && exe_module_sp->GetArchitecture().IsValid()) {
+ LLDB_LOGF(log,
+ "ProcessMachCore::%s: Was given binary + corefile, setting "
+ "target ArchSpec to binary to start",
+ __FUNCTION__);
+ GetTarget().SetArchitecture(exe_module_sp->GetArchitecture());
+ }
+
CreateMemoryRegions();
LoadBinariesAndSetDYLD();
CleanupMemoryRegionPermissions();
- ModuleSP exe_module_sp = GetTarget().GetExecutableModule();
+ exe_module_sp = GetTarget().GetExecutableModule();
if (exe_module_sp && exe_module_sp->GetArchitecture().IsValid()) {
+ LLDB_LOGF(log,
+ "ProcessMachCore::%s: have executable binary in the Target "
+ "after metadata/scan. Setting Target's ArchSpec based on "
+ "that.",
+ __FUNCTION__);
GetTarget().SetArchitecture(exe_module_sp->GetArchitecture());
} else {
// The corefile's architecture is our best starting point.
ArchSpec arch(m_core_module_sp->GetArchitecture());
- if (arch.IsValid())
+ if (arch.IsValid()) {
+ LLDB_LOGF(log,
+ "ProcessMachCore::%s: Setting target ArchSpec based on "
+ "corefile mach-o cputype/cpusubtype",
+ __FUNCTION__);
GetTarget().SetArchitecture(arch);
+ }
}
AddressableBits addressable_bits = core_objfile->GetAddressableBits();
|
| CreateMemoryRegions(); | ||
|
|
||
| LoadBinariesAndSetDYLD(); | ||
|
|
||
| CleanupMemoryRegionPermissions(); | ||
|
|
||
| ModuleSP exe_module_sp = GetTarget().GetExecutableModule(); | ||
| exe_module_sp = GetTarget().GetExecutableModule(); |
There was a problem hiding this comment.
It seems like you've already done exactly this computation at the beginning of LoadBinariesAndSetDYLD.
Why do you need to do it again here?
There was a problem hiding this comment.
If there's a reason that's then that's all good, but it might be helpful to future us to say why.
There was a problem hiding this comment.
Plus, if we really need to do it twice, better to move this into a helper method.
There was a problem hiding this comment.
Before we call LoadBinariesAndSetDYLD(), the only way we have an executable module is if the user provided it on the commandline along with the corefile. After that method call, we may have found it by metadata in the corefile or by an exhaustive search for a properly-aligned dyld or kernel. I'm open to having this in a method, but it's also a tiny method "if we have an executable binary, set the Target's arch to that ArchSpec" - the logging I added here is the largest part.
The real opportunity for reducing a copy of code would be in ProcessMachCore::LoadBinariesViaExhaustiveSearch where it does the "if we have an executable module, set arch, else use the corefile's arch" but we already know we don't have an executable module. We checked it before we got here in DoLoadCore, and if we'd added an executable module at this point, we wouldn't be doing the exhaustive search. This bit could be reduced to simply "if target has no arch, set it to the corefile's arch", I thought of that last night.
|
Beyond the question about the duplicate work, the approach seems reasonable to me. |
| CreateMemoryRegions(); | ||
|
|
||
| LoadBinariesAndSetDYLD(); | ||
|
|
||
| CleanupMemoryRegionPermissions(); | ||
|
|
||
| ModuleSP exe_module_sp = GetTarget().GetExecutableModule(); | ||
| exe_module_sp = GetTarget().GetExecutableModule(); |
There was a problem hiding this comment.
Plus, if we really need to do it twice, better to move this into a helper method.
in the ExhaustiveSearch - we would have done that already in DoLoadCore if it was possible. Use the less-preferable corefile binary's cputype/cpusubtype to seed Target so we can search.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This patch is making three changes, when loading a Mach-O corefile:
At the start of
DoLoadCore, if a binary was provided in addition to the corefile, initialize the Target's ArchSpec.Before ProcessMachCore does its "exhaustive search" fallback, looking through the corefile contents for a userland dyld or mach kernel, we must make sure the Target has an ArchSpec, or methods that check the address word size, or initialize a DataExtractor based on the Target arch will not succeed.
Add logging when setting the Target's arch listing exactly what that setting was based on -- the corefile itself, or the main binary.
Jonas landed a change last August (started with a patch from me) which removed the Target ArchSpec initialization at the start of DoLoadCore, in a scenario where the corefile had arch armv7 and the main binary had arch armv7em (Cortex-M), and there was python code in the main binary's dSYM which sets the operating system threads provider based on the Target arch. It did different things for armv7 or armv7em, and so it would fail.
Jonas' patch removed any ArchSpec setting at the start of DoLoadCore, so we wouldn't have an incorrect arch value, but that broke the exhaustive search for kernel binaries, because we didn't have an address word size or endianness.
This patch should navigate the needs of both use cases.
I spent a good bit of time trying to construct a test to capture all of these requirements -- but it turns out to be a good bit difficult, encompassing both a genuine kernel corefiles and a microcontroller firmware corefiles.
rdar://146821929