[lldb] Merge/unify ABI-provided AArch64 unwind plans #139545
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
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThe macos and sysv ABIs return functionally equivalent unwind plans, so they can be implemented in the base AArch64 class. The only difference between them was that the macos plan provided a "pc=lr" rule whereas the sysv plan called SetReturnAddressRegister (which causes the unwind machinery to act as if that rule was present). This difference was enough to cause While merging the two functions, I couldn't stop myself from simplifying them to use the generic register number schemes -- which exposed another bug in CompareUnwindPlansForIdenticalInitialPCLocation, namely that it was expecting all unwind plans to use the LLDB numbering scheme. This patch fixes that as well. Full diff: https://github.com/llvm/llvm-project/pull/139545.diff 7 Files Affected:
diff --git a/lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp b/lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
index 7d8d0a4d3d671..58b8459b1806f 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
+++ b/lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
@@ -19,6 +19,7 @@
#include <optional>
using namespace lldb;
+using namespace lldb_private;
LLDB_PLUGIN_DEFINE(ABIAArch64)
@@ -200,3 +201,41 @@ void ABIAArch64::AugmentRegisterInfo(
lldb::eEncodingIEEE754, lldb::eFormatFloat);
}
}
+
+UnwindPlanSP ABIAArch64::CreateFunctionEntryUnwindPlan() {
+ UnwindPlan::Row row;
+
+ // Our previous Call Frame Address is the stack pointer
+ row.GetCFAValue().SetIsRegisterPlusOffset(LLDB_REGNUM_GENERIC_SP, 0);
+
+ // Our previous PC is in the LR, all other registers are the same.
+ row.SetRegisterLocationToRegister(LLDB_REGNUM_GENERIC_PC, LLDB_REGNUM_GENERIC_RA, true);
+
+ auto plan_sp = std::make_shared<UnwindPlan>(eRegisterKindGeneric);
+ plan_sp->AppendRow(std::move(row));
+ plan_sp->SetSourceName("arm64 at-func-entry default");
+ plan_sp->SetSourcedFromCompiler(eLazyBoolNo);
+ plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+ plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo);
+ return plan_sp;
+}
+
+UnwindPlanSP ABIAArch64::CreateDefaultUnwindPlan() {
+ UnwindPlan::Row row;
+ const int32_t ptr_size = 8;
+
+ row.GetCFAValue().SetIsRegisterPlusOffset(LLDB_REGNUM_GENERIC_FP, 2 * ptr_size);
+ row.SetUnspecifiedRegistersAreUndefined(true);
+
+ row.SetRegisterLocationToAtCFAPlusOffset(LLDB_REGNUM_GENERIC_FP, ptr_size * -2, true);
+ row.SetRegisterLocationToAtCFAPlusOffset(LLDB_REGNUM_GENERIC_PC, ptr_size * -1, true);
+
+ auto plan_sp = std::make_shared<UnwindPlan>(eRegisterKindGeneric);
+ plan_sp->AppendRow(std::move(row));
+ plan_sp->SetSourceName("arm64 default unwind plan");
+ plan_sp->SetSourcedFromCompiler(eLazyBoolNo);
+ plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+ plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo);
+ return plan_sp;
+}
+
diff --git a/lldb/source/Plugins/ABI/AArch64/ABIAArch64.h b/lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
index 52e42f1260a83..53702f4da580d 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
+++ b/lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
@@ -19,6 +19,9 @@ class ABIAArch64 : public lldb_private::MCBasedABI {
lldb::addr_t FixCodeAddress(lldb::addr_t pc) override;
lldb::addr_t FixDataAddress(lldb::addr_t pc) override;
+ lldb::UnwindPlanSP CreateFunctionEntryUnwindPlan() override;
+ lldb::UnwindPlanSP CreateDefaultUnwindPlan() override;
+
protected:
virtual lldb::addr_t FixAddress(lldb::addr_t pc, lldb::addr_t mask) {
return pc;
diff --git a/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp b/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
index f86ab8cbb1195..094e0523a4edf 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
+++ b/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
@@ -17,7 +17,6 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/PluginManager.h"
#include "lldb/Core/Value.h"
-#include "lldb/Symbol/UnwindPlan.h"
#include "lldb/Target/Process.h"
#include "lldb/Target/RegisterContext.h"
#include "lldb/Target/Target.h"
@@ -30,8 +29,6 @@
#include "lldb/Utility/Status.h"
#include "lldb/ValueObject/ValueObjectConstResult.h"
-#include "Utility/ARM64_DWARF_Registers.h"
-
using namespace lldb;
using namespace lldb_private;
@@ -344,48 +341,6 @@ ABIMacOSX_arm64::SetReturnValueObject(lldb::StackFrameSP &frame_sp,
return error;
}
-UnwindPlanSP ABIMacOSX_arm64::CreateFunctionEntryUnwindPlan() {
- uint32_t lr_reg_num = arm64_dwarf::lr;
- uint32_t sp_reg_num = arm64_dwarf::sp;
- uint32_t pc_reg_num = arm64_dwarf::pc;
-
- UnwindPlan::Row row;
-
- // Our previous Call Frame Address is the stack pointer
- row.GetCFAValue().SetIsRegisterPlusOffset(sp_reg_num, 0);
-
- // Our previous PC is in the LR, all other registers are the same.
- row.SetRegisterLocationToRegister(pc_reg_num, lr_reg_num, true);
-
- auto plan_sp = std::make_shared<UnwindPlan>(eRegisterKindDWARF);
- plan_sp->AppendRow(std::move(row));
- plan_sp->SetSourceName("arm64 at-func-entry default");
- plan_sp->SetSourcedFromCompiler(eLazyBoolNo);
- return plan_sp;
-}
-
-UnwindPlanSP ABIMacOSX_arm64::CreateDefaultUnwindPlan() {
- uint32_t fp_reg_num = arm64_dwarf::fp;
- uint32_t pc_reg_num = arm64_dwarf::pc;
-
- UnwindPlan::Row row;
- const int32_t ptr_size = 8;
-
- row.GetCFAValue().SetIsRegisterPlusOffset(fp_reg_num, 2 * ptr_size);
- row.SetUnspecifiedRegistersAreUndefined(true);
-
- row.SetRegisterLocationToAtCFAPlusOffset(fp_reg_num, ptr_size * -2, true);
- row.SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, ptr_size * -1, true);
-
- auto plan_sp = std::make_shared<UnwindPlan>(eRegisterKindDWARF);
- plan_sp->AppendRow(std::move(row));
- plan_sp->SetSourceName("arm64-apple-darwin default unwind plan");
- plan_sp->SetSourcedFromCompiler(eLazyBoolNo);
- plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
- plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo);
- return plan_sp;
-}
-
// AAPCS64 (Procedure Call Standard for the ARM 64-bit Architecture) says
// registers x19 through x28 and sp are callee preserved. v8-v15 are non-
// volatile (and specifically only the lower 8 bytes of these regs), the rest
diff --git a/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h b/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
index 94a60327c6181..c8851709f50ad 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
+++ b/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
@@ -27,10 +27,6 @@ class ABIMacOSX_arm64 : public ABIAArch64 {
bool GetArgumentValues(lldb_private::Thread &thread,
lldb_private::ValueList &values) const override;
- lldb::UnwindPlanSP CreateFunctionEntryUnwindPlan() override;
-
- lldb::UnwindPlanSP CreateDefaultUnwindPlan() override;
-
bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
// The arm64 ABI requires that stack frames be 16 byte aligned.
diff --git a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp b/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
index 6e07c0982be0e..aa9c20b6bb2cf 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
+++ b/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
@@ -30,8 +30,6 @@
#include "lldb/Utility/Status.h"
#include "lldb/ValueObject/ValueObjectConstResult.h"
-#include "Utility/ARM64_DWARF_Registers.h"
-
using namespace lldb;
using namespace lldb_private;
@@ -385,48 +383,6 @@ Status ABISysV_arm64::SetReturnValueObject(lldb::StackFrameSP &frame_sp,
return error;
}
-UnwindPlanSP ABISysV_arm64::CreateFunctionEntryUnwindPlan() {
- uint32_t lr_reg_num = arm64_dwarf::lr;
- uint32_t sp_reg_num = arm64_dwarf::sp;
-
- UnwindPlan::Row row;
-
- // Our previous Call Frame Address is the stack pointer, all other registers
- // are the same.
- row.GetCFAValue().SetIsRegisterPlusOffset(sp_reg_num, 0);
-
- auto plan_sp = std::make_shared<UnwindPlan>(eRegisterKindDWARF);
- plan_sp->AppendRow(std::move(row));
- plan_sp->SetReturnAddressRegister(lr_reg_num);
- plan_sp->SetSourceName("arm64 at-func-entry default");
- plan_sp->SetSourcedFromCompiler(eLazyBoolNo);
- plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
- plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo);
- return plan_sp;
-}
-
-UnwindPlanSP ABISysV_arm64::CreateDefaultUnwindPlan() {
- uint32_t fp_reg_num = arm64_dwarf::fp;
- uint32_t pc_reg_num = arm64_dwarf::pc;
-
- UnwindPlan::Row row;
- const int32_t ptr_size = 8;
-
- row.GetCFAValue().SetIsRegisterPlusOffset(fp_reg_num, 2 * ptr_size);
- row.SetUnspecifiedRegistersAreUndefined(true);
-
- row.SetRegisterLocationToAtCFAPlusOffset(fp_reg_num, ptr_size * -2, true);
- row.SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, ptr_size * -1, true);
-
- auto plan_sp = std::make_shared<UnwindPlan>(eRegisterKindDWARF);
- plan_sp->AppendRow(std::move(row));
- plan_sp->SetSourceName("arm64 default unwind plan");
- plan_sp->SetSourcedFromCompiler(eLazyBoolNo);
- plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
- plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo);
- return plan_sp;
-}
-
// AAPCS64 (Procedure Call Standard for the ARM 64-bit Architecture) says
// registers x19 through x28 and sp are callee preserved. v8-v15 are non-
// volatile (and specifically only the lower 8 bytes of these regs), the rest
diff --git a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.h b/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.h
index 2b8e608d4caab..213fbf7417b2c 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.h
+++ b/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.h
@@ -30,10 +30,6 @@ class ABISysV_arm64 : public ABIAArch64 {
SetReturnValueObject(lldb::StackFrameSP &frame_sp,
lldb::ValueObjectSP &new_value) override;
- lldb::UnwindPlanSP CreateFunctionEntryUnwindPlan() override;
-
- lldb::UnwindPlanSP CreateDefaultUnwindPlan() override;
-
bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
// The arm64 ABI requires that stack frames be 16 byte aligned.
diff --git a/lldb/source/Symbol/FuncUnwinders.cpp b/lldb/source/Symbol/FuncUnwinders.cpp
index faec24cde7fdd..8f3e3d0a9188a 100644
--- a/lldb/source/Symbol/FuncUnwinders.cpp
+++ b/lldb/source/Symbol/FuncUnwinders.cpp
@@ -365,33 +365,30 @@ FuncUnwinders::GetAssemblyUnwindPlan(Target &target, Thread &thread) {
LazyBool FuncUnwinders::CompareUnwindPlansForIdenticalInitialPCLocation(
Thread &thread, const std::shared_ptr<const UnwindPlan> &a,
const std::shared_ptr<const UnwindPlan> &b) {
- LazyBool plans_are_identical = eLazyBoolCalculate;
+ if (!a ||!b)
+ return eLazyBoolCalculate;
- RegisterNumber pc_reg(thread, eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC);
- uint32_t pc_reg_lldb_regnum = pc_reg.GetAsKind(eRegisterKindLLDB);
+ const UnwindPlan::Row *a_first_row = a->GetRowAtIndex(0);
+ const UnwindPlan::Row *b_first_row = b->GetRowAtIndex(0);
+ if (!a_first_row ||! b_first_row)
+ return eLazyBoolCalculate;
- if (a && b) {
- const UnwindPlan::Row *a_first_row = a->GetRowAtIndex(0);
- const UnwindPlan::Row *b_first_row = b->GetRowAtIndex(0);
+ RegisterNumber pc_reg(thread, eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC);
+ uint32_t a_pc_regnum = pc_reg.GetAsKind(a->GetRegisterKind());
+ uint32_t b_pc_regnum = pc_reg.GetAsKind(b->GetRegisterKind());
- if (a_first_row && b_first_row) {
- UnwindPlan::Row::AbstractRegisterLocation a_pc_regloc;
- UnwindPlan::Row::AbstractRegisterLocation b_pc_regloc;
+ UnwindPlan::Row::AbstractRegisterLocation a_pc_regloc;
+ UnwindPlan::Row::AbstractRegisterLocation b_pc_regloc;
- a_first_row->GetRegisterInfo(pc_reg_lldb_regnum, a_pc_regloc);
- b_first_row->GetRegisterInfo(pc_reg_lldb_regnum, b_pc_regloc);
+ a_first_row->GetRegisterInfo(a_pc_regnum, a_pc_regloc);
+ b_first_row->GetRegisterInfo(b_pc_regnum, b_pc_regloc);
- plans_are_identical = eLazyBoolYes;
+ if (a_first_row->GetCFAValue() != b_first_row->GetCFAValue())
+ return eLazyBoolNo;
+ if (a_pc_regloc != b_pc_regloc)
+ return eLazyBoolNo;
- if (a_first_row->GetCFAValue() != b_first_row->GetCFAValue()) {
- plans_are_identical = eLazyBoolNo;
- }
- if (a_pc_regloc != b_pc_regloc) {
- plans_are_identical = eLazyBoolNo;
- }
- }
- }
- return plans_are_identical;
+ return eLazyBoolYes;
}
std::shared_ptr<const UnwindPlan>
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
The macos and sysv ABIs return functionally equivalent unwind plans, so they can be implemented in the base AArch64 class. The only difference between them was that the macos plan provided a "pc=lr" rule whereas the sysv plan called SetReturnAddressRegister (which causes the unwind machinery to act as if that rule was present). This difference was enough to cause `CompareUnwindPlansForIdenticalInitialPCLocation` to return a different value and break the (temporarily reverted) TestUnwindFramelessFaulted test. While merging the two functions, I couldn't help myself from simplifying them to use the generic register number schemes -- which exposed another bug in CompareUnwindPlansForIdenticalInitialPCLocation, namely that it was expecting all unwind plans to use the LLDB numbering scheme. This patch fixes that as well.
jasonmolenda
approved these changes
May 14, 2025
There was a problem hiding this comment.
Nice unification, thanks. I'm sure the pc=lr rule was me not trusting the algorithms over in RegisterContextUnwind to do the right thing if the rule wasn't listed. Having just pushed all that code around for a day, I know this kind of thing is unneeded, but harmless if it's present.
Well.. as it stands now, it's kind of needed because without it, your test would break in CompareUnwindPlansForIdenticalInitialPCLocation, as it doesn't recognise that This is kind of why I have suggested in https://discourse.llvm.org/t/unhappiness-with-the-lldb-unwinder-register-passing-up-the-stack-interrup-trap-sigtramp-frames/86058/3 to standardise on the |
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 macos and sysv ABIs return functionally equivalent unwind plans, so they can be implemented in the base AArch64 class. The only difference between them was that the macos plan provided a "pc=lr" rule whereas the sysv plan called SetReturnAddressRegister (which causes the unwind machinery to act as if that rule was present). This difference was enough to cause
CompareUnwindPlansForIdenticalInitialPCLocationto return a different value and break the (temporarily reverted) TestUnwindFramelessFaulted test.While merging the two functions, I couldn't stop myself from simplifying them to use the generic register number schemes -- which exposed another bug in CompareUnwindPlansForIdenticalInitialPCLocation, namely that it was expecting all unwind plans to use the LLDB numbering scheme. This patch fixes that as well.