Skip to content

[lldb][Mach-O corefiles] Don't init Target arch to corefile #136065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 21, 2025
Merged

[lldb][Mach-O corefiles] Don't init Target arch to corefile #136065

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41274f7
[lldb][Mach-O corefiles] Don't init Target arch to corefile
jasonmolenda Apr 17, 2025
6e1f6f0
Don't try to set the target arch to the exe module
jasonmolenda Apr 17, 2025
e7a9d47
rm sp
jasonmolenda Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,22 @@ 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. If we had a main binary
// already, we would have set the Target to that - so here we'll use
// the corefile's cputype/cpusubtype as the best guess.
if (!GetTarget().GetArchitecture().IsValid()) {
// 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);
Expand Down Expand Up @@ -569,6 +585,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) {
Expand All @@ -578,20 +595,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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you've already done exactly this computation at the beginning of LoadBinariesAndSetDYLD.

Why do you need to do it again here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a reason that's then that's all good, but it might be helpful to future us to say why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus, if we really need to do it twice, better to move this into a helper method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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();
Expand Down
Loading