[lldb][Mach-O] Fix several bugs in x86_64 Mach-O corefile #146460
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reading, and one bug in the new RegisterContextUnifiedCore class. The PR I landed a few days ago to allow Mach-O corefiles to augment their registers with additional per-thread registers in metadata exposed a few bugs in the x86_64 corefile reader when running under different CI environments. It also showed a bug in my RegisterContextUnifiedCore class where I wasn't properly handling lookups of unknown registers (e.g. the LLDB_GENERIC_RA when debugging an intel target). The Mach-O x86_64 corefile support would say that it had fpu & exc registers available in every corefile, regardless of whether they were actually present. It would only read the bytes for the first register flavor in the LC_THREAD, the GPRs, but it read them incorrectly, so sometimes you got more register context than you'd expect. The LC_THREAD register context specifies a flavor and the number of uint32_t words; the ObjectFileMachO method would read that number of uint64_t's, exceeding the GPR register space, but it was followed by FPU and then EXC register space so it didn't crash. If you had a corefile with GPR and EXC register bytes, it would be written into the GPR and then FPU register areas, with zeroes filling out the rest of the context.
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) Changesreading, and one bug in the new RegisterContextUnifiedCore class. The PR I landed a few days ago to allow Mach-O corefiles to augment their registers with additional per-thread registers in metadata exposed a few bugs in the x86_64 corefile reader when running under different CI environments. It also showed a bug in my RegisterContextUnifiedCore class where I wasn't properly handling lookups of unknown registers (e.g. the LLDB_GENERIC_RA when debugging an intel target). The Mach-O x86_64 corefile support would say that it had fpu & exc registers available in every corefile, regardless of whether they were actually present. It would only read the bytes for the first register flavor in the LC_THREAD, the GPRs, but it read them incorrectly, so sometimes you got more register context than you'd expect. The LC_THREAD register context specifies a flavor and the number of uint32_t words; the ObjectFileMachO method would read that number of uint64_t's, exceeding the GPR register space, but it was followed by FPU and then EXC register space so it didn't crash. If you had a corefile with GPR and EXC register bytes, it would be written into the GPR and then FPU register areas, with zeroes filling out the rest of the context. Full diff: https://github.com/llvm/llvm-project/pull/146460.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 4394caf7f31e9..ddb662c374edb 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -184,46 +184,32 @@ class RegisterContextDarwin_x86_64_Mach : public RegisterContextDarwin_x86_64 {
SetError(GPRRegSet, Read, -1);
SetError(FPURegSet, Read, -1);
SetError(EXCRegSet, Read, -1);
- bool done = false;
- while (!done) {
+ while (offset < data.GetByteSize()) {
int flavor = data.GetU32(&offset);
if (flavor == 0)
- done = true;
- else {
- uint32_t i;
- uint32_t count = data.GetU32(&offset);
- switch (flavor) {
- case GPRRegSet:
- for (i = 0; i < count; ++i)
- (&gpr.rax)[i] = data.GetU64(&offset);
- SetError(GPRRegSet, Read, 0);
- done = true;
-
- break;
- case FPURegSet:
- // TODO: fill in FPU regs....
- // SetError (FPURegSet, Read, -1);
- done = true;
-
- break;
- case EXCRegSet:
- exc.trapno = data.GetU32(&offset);
- exc.err = data.GetU32(&offset);
- exc.faultvaddr = data.GetU64(&offset);
- SetError(EXCRegSet, Read, 0);
- done = true;
- break;
- case 7:
- case 8:
- case 9:
- // fancy flavors that encapsulate of the above flavors...
- break;
-
- default:
- done = true;
- break;
- }
+ break;
+ uint32_t count = data.GetU32(&offset);
+ switch (flavor) {
+ case GPRRegSet: {
+ uint32_t *gpr_data = reinterpret_cast<uint32_t *>(&gpr.rax);
+ for (uint32_t i = 0; i < count; ++i)
+ gpr_data[i] = data.GetU32(&offset);
+ SetError(GPRRegSet, Read, 0);
+ } break;
+ case FPURegSet:
+ // TODO: fill in FPU regs....
+ SetError(FPURegSet, Read, -1);
+ break;
+ case EXCRegSet:
+ exc.trapno = data.GetU32(&offset);
+ exc.err = data.GetU32(&offset);
+ exc.faultvaddr = data.GetU64(&offset);
+ SetError(EXCRegSet, Read, 0);
+ break;
+ default:
+ offset += count * 4;
+ break;
}
}
}
@@ -353,11 +339,11 @@ class RegisterContextDarwin_x86_64_Mach : public RegisterContextDarwin_x86_64 {
}
protected:
- int DoReadGPR(lldb::tid_t tid, int flavor, GPR &gpr) override { return 0; }
+ int DoReadGPR(lldb::tid_t tid, int flavor, GPR &gpr) override { return -1; }
- int DoReadFPU(lldb::tid_t tid, int flavor, FPU &fpu) override { return 0; }
+ int DoReadFPU(lldb::tid_t tid, int flavor, FPU &fpu) override { return -1; }
- int DoReadEXC(lldb::tid_t tid, int flavor, EXC &exc) override { return 0; }
+ int DoReadEXC(lldb::tid_t tid, int flavor, EXC &exc) override { return -1; }
int DoWriteGPR(lldb::tid_t tid, int flavor, const GPR &gpr) override {
return 0;
diff --git a/lldb/source/Plugins/Process/mach-core/RegisterContextUnifiedCore.cpp b/lldb/source/Plugins/Process/mach-core/RegisterContextUnifiedCore.cpp
index 16568f788c202..449f85bc3f52a 100644
--- a/lldb/source/Plugins/Process/mach-core/RegisterContextUnifiedCore.cpp
+++ b/lldb/source/Plugins/Process/mach-core/RegisterContextUnifiedCore.cpp
@@ -262,7 +262,9 @@ size_t RegisterContextUnifiedCore::GetRegisterCount() {
const RegisterInfo *
RegisterContextUnifiedCore::GetRegisterInfoAtIndex(size_t reg) {
- return &m_register_infos[reg];
+ if (reg < m_register_infos.size())
+ return &m_register_infos[reg];
+ return nullptr;
}
size_t RegisterContextUnifiedCore::GetRegisterSetCount() {
@@ -270,7 +272,9 @@ size_t RegisterContextUnifiedCore::GetRegisterSetCount() {
}
const RegisterSet *RegisterContextUnifiedCore::GetRegisterSet(size_t set) {
- return &m_register_sets[set];
+ if (set < m_register_sets.size())
+ return &m_register_sets[set];
+ return nullptr;
}
bool RegisterContextUnifiedCore::ReadRegister(
|
JDevlieghere
approved these changes
Jul 1, 2025
Co-authored-by: Jonas Devlieghere <[email protected]>
jasonmolenda
added a commit
to jasonmolenda/llvm-project
that referenced
this pull request
Jul 1, 2025
reading, and one bug in the new RegisterContextUnifiedCore class. The PR I landed a few days ago to allow Mach-O corefiles to augment their registers with additional per-thread registers in metadata exposed a few bugs in the x86_64 corefile reader when running under different CI environments. It also showed a bug in my RegisterContextUnifiedCore class where I wasn't properly handling lookups of unknown registers (e.g. the LLDB_GENERIC_RA when debugging an intel target). The Mach-O x86_64 corefile support would say that it had fpu & exc registers available in every corefile, regardless of whether they were actually present. It would only read the bytes for the first register flavor in the LC_THREAD, the GPRs, but it read them incorrectly, so sometimes you got more register context than you'd expect. The LC_THREAD register context specifies a flavor and the number of uint32_t words; the ObjectFileMachO method would read that number of uint64_t's, exceeding the GPR register space, but it was followed by FPU and then EXC register space so it didn't crash. If you had a corefile with GPR and EXC register bytes, it would be written into the GPR and then FPU register areas, with zeroes filling out the rest of the context. (cherry picked from commit e94c609)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
reading, and one bug in the new RegisterContextUnifiedCore class.
The PR I landed a few days ago to allow Mach-O corefiles to augment their registers with additional per-thread registers in metadata exposed a few bugs in the x86_64 corefile reader when running under different CI environments. It also showed a bug in my RegisterContextUnifiedCore class where I wasn't properly handling lookups of unknown registers (e.g. the LLDB_GENERIC_RA when debugging an intel target).
The Mach-O x86_64 corefile support would say that it had fpu & exc registers available in every corefile, regardless of whether they were actually present. It would only read the bytes for the first register flavor in the LC_THREAD, the GPRs, but it read them incorrectly, so sometimes you got more register context than you'd expect. The LC_THREAD register context specifies a flavor and the number of uint32_t words; the ObjectFileMachO method would read that number of uint64_t's, exceeding the GPR register space, but it was followed by FPU and then EXC register space so it didn't crash. If you had a corefile with GPR and EXC register bytes, it would be written into the GPR and then FPU register areas, with zeroes filling out the rest of the context.