[lldb] Fix auto advance PC in EmulateInstructionARM64 if PC >= 4G
#151460
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
The `EmulateInstructionARM64::EvaluateInstruction()` method used `uint32_t` to store the PC value, causing addresses greater than 2^32 to be truncated. As for now, the issue does not affect the mainline because the method is never called with both `eEmulateInstructionOptionAutoAdvancePC` and `eEmulateInstructionOptionIgnoreConditions` options set. However, it can trigger on a downstream that uses software stepping.
igorkudrin
requested review from
DavidSpickett,
adrian-prantl,
labath,
dcci and
jasonmolenda
|
@llvm/pr-subscribers-lldb Author: Igor Kudrin (igorkudrin) ChangesThe As for now, the issue does not affect the mainline because the method is never called with both Full diff: https://github.com/llvm/llvm-project/pull/151460.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
index 29f03fee47b0d..a8901beda3970 100644
--- a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -404,7 +404,7 @@ bool EmulateInstructionARM64::EvaluateInstruction(uint32_t evaluate_options) {
if (!success && !m_ignore_conditions)
return false;
- uint32_t orig_pc_value = 0;
+ uint64_t orig_pc_value = 0;
if (auto_advance_pc) {
orig_pc_value =
ReadRegisterUnsigned(eRegisterKindLLDB, gpr_pc_arm64, 0, &success);
@@ -418,7 +418,7 @@ bool EmulateInstructionARM64::EvaluateInstruction(uint32_t evaluate_options) {
return false;
if (auto_advance_pc) {
- uint32_t new_pc_value =
+ uint64_t new_pc_value =
ReadRegisterUnsigned(eRegisterKindLLDB, gpr_pc_arm64, 0, &success);
if (!success)
return false;
diff --git a/lldb/unittests/Instruction/ARM64/TestAArch64Emulator.cpp b/lldb/unittests/Instruction/ARM64/TestAArch64Emulator.cpp
index 4506c200dee3b..c021d994bb062 100644
--- a/lldb/unittests/Instruction/ARM64/TestAArch64Emulator.cpp
+++ b/lldb/unittests/Instruction/ARM64/TestAArch64Emulator.cpp
@@ -13,15 +13,124 @@
#include "lldb/Core/Disassembler.h"
#include "lldb/Target/ExecutionContext.h"
#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/RegisterValue.h"
#include "Plugins/Instruction/ARM64/EmulateInstructionARM64.h"
+#include "Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h"
+#include "Plugins/Process/Utility/lldb-arm64-register-enums.h"
using namespace lldb;
using namespace lldb_private;
struct Arch64EmulatorTester : public EmulateInstructionARM64 {
+ RegisterInfoPOSIX_arm64::GPR gpr;
+ uint8_t memory[64] = {0};
+ uint64_t memory_offset = 0;
+
Arch64EmulatorTester()
- : EmulateInstructionARM64(ArchSpec("arm64-apple-ios")) {}
+ : EmulateInstructionARM64(ArchSpec("arm64-apple-ios")) {
+ memset(&gpr, 0, sizeof(gpr));
+ EmulateInstruction::SetCallbacks(ReadMemoryCallback, WriteMemoryCallback,
+ ReadRegisterCallback,
+ WriteRegisterCallback);
+ }
+
+ static bool ReadRegisterCallback(EmulateInstruction *instruction, void *baton,
+ const RegisterInfo *reg_info,
+ RegisterValue ®_value) {
+ auto *tester = static_cast<Arch64EmulatorTester *>(instruction);
+ uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
+ if (reg >= gpr_x1_arm64 && reg <= gpr_x28_arm64) {
+ reg_value.SetUInt64(tester->gpr.x[reg - gpr_x0_arm64]);
+ return true;
+ }
+ if (reg >= gpr_w1_arm64 && reg <= gpr_w28_arm64) {
+ reg_value.SetUInt32(tester->gpr.x[reg - gpr_w0_arm64]);
+ return true;
+ }
+ switch (reg) {
+ case gpr_x0_arm64:
+ reg_value.SetUInt64(0);
+ return true;
+ case gpr_w0_arm64:
+ reg_value.SetUInt32(0);
+ return true;
+ case gpr_fp_arm64:
+ reg_value.SetUInt64(tester->gpr.fp);
+ return true;
+ case gpr_lr_arm64:
+ reg_value.SetUInt64(tester->gpr.lr);
+ return true;
+ case gpr_sp_arm64:
+ reg_value.SetUInt64(tester->gpr.sp);
+ return true;
+ case gpr_pc_arm64:
+ reg_value.SetUInt64(tester->gpr.pc);
+ return true;
+ case gpr_cpsr_arm64:
+ reg_value.SetUInt64(tester->gpr.cpsr);
+ return true;
+ default:
+ return false;
+ }
+ }
+
+ static bool WriteRegisterCallback(EmulateInstruction *instruction,
+ void *baton, const Context &context,
+ const RegisterInfo *reg_info,
+ const RegisterValue ®_value) {
+ auto *tester = static_cast<Arch64EmulatorTester *>(instruction);
+ uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
+ if (reg >= gpr_x1_arm64 && reg <= gpr_x28_arm64) {
+ tester->gpr.x[reg - gpr_x0_arm64] = reg_value.GetAsUInt64();
+ return true;
+ }
+ if (reg >= gpr_w1_arm64 && reg <= gpr_w28_arm64) {
+ tester->gpr.x[reg - gpr_w0_arm64] = reg_value.GetAsUInt32();
+ return true;
+ }
+ switch (reg) {
+ case gpr_fp_arm64:
+ tester->gpr.fp = reg_value.GetAsUInt64();
+ return true;
+ case gpr_lr_arm64:
+ tester->gpr.lr = reg_value.GetAsUInt64();
+ return true;
+ case gpr_sp_arm64:
+ tester->gpr.sp = reg_value.GetAsUInt64();
+ return true;
+ case gpr_pc_arm64:
+ tester->gpr.pc = reg_value.GetAsUInt64();
+ return true;
+ case gpr_cpsr_arm64:
+ tester->gpr.cpsr = reg_value.GetAsUInt64();
+ return true;
+ default:
+ return false;
+ }
+ }
+
+ static size_t ReadMemoryCallback(EmulateInstruction *instruction, void *baton,
+ const Context &context, addr_t addr,
+ void *dst, size_t length) {
+ auto *tester = static_cast<Arch64EmulatorTester *>(instruction);
+ assert(addr >= tester->memory_offset);
+ assert(addr - tester->memory_offset + length <= sizeof(tester->memory));
+ if (addr >= tester->memory_offset &&
+ addr - tester->memory_offset + length <= sizeof(tester->memory)) {
+ memcpy(dst, tester->memory + addr - tester->memory_offset, length);
+ return length;
+ }
+ return 0;
+ };
+
+ static size_t WriteMemoryCallback(EmulateInstruction *instruction,
+ void *baton, const Context &context,
+ addr_t addr, const void *dst,
+ size_t length) {
+ // TODO: implement when required
+ return 0;
+ };
static uint64_t AddWithCarry(uint32_t N, uint64_t x, uint64_t y, bool carry_in,
EmulateInstructionARM64::ProcState &proc_state) {
@@ -60,3 +169,18 @@ TEST_F(TestAArch64Emulator, TestOverflow) {
ASSERT_EQ(pstate.V, 1ULL);
ASSERT_EQ(pstate.C, 0ULL);
}
+
+TEST_F(TestAArch64Emulator, TestAutoAdvancePC) {
+ Arch64EmulatorTester emu;
+ emu.memory_offset = 0x1234567800;
+ emu.gpr.pc = 0x1234567800;
+ emu.gpr.x[8] = 0x1234567820;
+ memcpy(emu.memory, "\x08\x01\x40\xb9", 4); // ldr w8, [x8]
+ memcpy(emu.memory + 0x20, "\x11\x22\x33\x44", 4); // 0x44332211
+ ASSERT_TRUE(emu.ReadInstruction());
+ ASSERT_TRUE(
+ emu.EvaluateInstruction(eEmulateInstructionOptionAutoAdvancePC |
+ eEmulateInstructionOptionIgnoreConditions));
+ ASSERT_EQ(emu.gpr.pc, 0x1234567804);
+ ASSERT_EQ(emu.gpr.x[8], 0x44332211);
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| void *baton, const Context &context, | ||
| addr_t addr, const void *dst, | ||
| size_t length) { | ||
| // TODO: implement when required |
There was a problem hiding this comment.
Does this ever get called? If not you could add an assert here too.
Comment on lines
52
to
57
| case gpr_x0_arm64: | ||
| reg_value.SetUInt64(0); | ||
| return true; | ||
| case gpr_w0_arm64: | ||
| reg_value.SetUInt32(0); | ||
| return true; |
There was a problem hiding this comment.
Why are these tied to 0? AArch64's zero register is xzr and it's not the 0th register as it is for MIPS and RISC-V.
| TEST_F(TestAArch64Emulator, TestAutoAdvancePC) { | ||
| Arch64EmulatorTester emu; | ||
| emu.memory_offset = 0x1234567800; | ||
| emu.gpr.pc = 0x1234567800; |
There was a problem hiding this comment.
I see that this would be truncated if not for this fix, but please make it a full 64-bit value so that it is as obvious as it can be.
labath
approved these changes
Aug 4, 2025
| void *baton, const Context &context, | ||
| addr_t addr, const void *dst, | ||
| size_t length) { | ||
| // TODO: implement when required |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/20127 Here is the relevant piece of the build log for the reference |
|
Feels like someone stored a uint64_t into a void* or size_t that's 32-bit on Arm 32-bit. Thanks for reverting, I'm looking into it now. |
DavidSpickett
added a commit
that referenced
this pull request
Aug 6, 2025
… >= 4G (#151460)" This reverts commit 600976f. The test was crashing trying to access any element of the GPR struct. (gdb) disas Dump of assembler code for function _ZN7testing8internal11CmpHelperEQIyyEENS_15AssertionResultEPKcS4_RKT_RKT0_: 0x00450afc <+0>: push {r4, r5, r6, r7, r8, r9, r10, r11, lr} 0x00450b00 <+4>: sub sp, sp, #60 ; 0x3c 0x00450b04 <+8>: ldr r5, [sp, #96] ; 0x60 => 0x00450b08 <+12>: ldm r3, {r4, r7} 0x00450b0c <+16>: ldm r5, {r6, r9} 0x00450b10 <+20>: eor r7, r7, r9 0x00450b14 <+24>: eor r6, r4, r6 0x00450b18 <+28>: orrs r7, r6, r7 (gdb) p/x r3 $3 = 0x3e300f6e "However, load and store multiple instructions (LDM and STM) and load and store double-word (LDRD or STRD) must be aligned to at least a word boundary." https://developer.arm.com/documentation/den0013/d/Porting/Alignment >>> 0x3e300f6e % 4 2 Program received signal SIGBUS, Bus error. 0x00450b08 in testing::AssertionResult testing::internal::CmpHelperEQ<unsigned long long, unsigned long long>(char const*, char const*, unsigned long long const&, unsigned long long const&) () The struct is packed with 1 byte alignment, but it needs to start at an aligned address for us to ldm from it. So I've done that with alignas. Also fixed some compiler warnings in the test itself.
|
Actually was an alignment issue trying to load from a struct - 49d5dd3. |
Thank you for fixing this! |
|
Seems like this is tripping up UBSan: |
igorkudrin
added a commit
that referenced
this pull request
Aug 7, 2025
Thanks for reporting, fixed in 11e1d46 |
krishna2803
pushed a commit
to krishna2803/llvm-project
that referenced
this pull request
Aug 12, 2025
…lvm#151460) The `EmulateInstructionARM64::EvaluateInstruction()` method used `uint32_t` to store the PC value, causing addresses greater than 2^32 to be truncated. As for now, the issue does not affect the mainline because the method is never called with both `eEmulateInstructionOptionAutoAdvancePC` and `eEmulateInstructionOptionIgnoreConditions` options set. However, it can trigger on a downstream that uses software stepping.
krishna2803
pushed a commit
to krishna2803/llvm-project
that referenced
this pull request
Aug 12, 2025
… >= 4G (llvm#151460)" This reverts commit 64eba6e. It breaks on lldb-arm-ubuntu
krishna2803
pushed a commit
to krishna2803/llvm-project
that referenced
this pull request
Aug 12, 2025
… >= 4G (llvm#151460)" This reverts commit 600976f. The test was crashing trying to access any element of the GPR struct. (gdb) disas Dump of assembler code for function _ZN7testing8internal11CmpHelperEQIyyEENS_15AssertionResultEPKcS4_RKT_RKT0_: 0x00450afc <+0>: push {r4, r5, r6, r7, r8, r9, r10, r11, lr} 0x00450b00 <+4>: sub sp, sp, llvm#60 ; 0x3c 0x00450b04 <+8>: ldr r5, [sp, llvm#96] ; 0x60 => 0x00450b08 <+12>: ldm r3, {r4, r7} 0x00450b0c <+16>: ldm r5, {r6, r9} 0x00450b10 <+20>: eor r7, r7, r9 0x00450b14 <+24>: eor r6, r4, r6 0x00450b18 <+28>: orrs r7, r6, r7 (gdb) p/x r3 $3 = 0x3e300f6e "However, load and store multiple instructions (LDM and STM) and load and store double-word (LDRD or STRD) must be aligned to at least a word boundary." https://developer.arm.com/documentation/den0013/d/Porting/Alignment >>> 0x3e300f6e % 4 2 Program received signal SIGBUS, Bus error. 0x00450b08 in testing::AssertionResult testing::internal::CmpHelperEQ<unsigned long long, unsigned long long>(char const*, char const*, unsigned long long const&, unsigned long long const&) () The struct is packed with 1 byte alignment, but it needs to start at an aligned address for us to ldm from it. So I've done that with alignas. Also fixed some compiler warnings in the test itself.
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.
The
EmulateInstructionARM64::EvaluateInstruction()method useduint32_tto store the PC value, causing addresses greater than 2^32 to be truncated.As for now, the issue does not affect the mainline because the method is never called with both
eEmulateInstructionOptionAutoAdvancePCandeEmulateInstructionOptionIgnoreConditionsoptions set. However, it can trigger on a downstream that uses software stepping.