[lldb/aarch64] Fix PC register info augmentation #143499
Conversation
This fixes a regression caused by llvm#139817, where we would fail to unwind (using eh_frame) when using debug stubs (like qemu) which do not provide eh_frame register numbers. Unwinding fails because the unwinder tries to convert pc register number to the "eh_frame" scheme (and fails). Previously that worked because the code used a slightly different pattern which ended up comparing the number by converting it to the "generic" scheme (which we do provide). I don't think that's a bug in the unwinder -- we just need to make sure we provide the register number all the time. The associated test is not particularly useful, but I'm hoping it could be used to test other changes like this as well. Other register augmentation changes are tested in an end-to-end fashion (by injecting gdb-remote packets and then trying to read registers). That works well for testing the addition of subregisters, but it isn't particularly suitable for testing register numbers, as the only place (that I know of) where this manifests is during unwinding, and recreating an unwindable process in a test like this is fairly challenging.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThis fixes a regression caused by #139817, where we would fail to unwind (using eh_frame) when using debug stubs (like qemu) which do not provide eh_frame register numbers. Unwinding fails because the unwinder tries to convert pc register number to the "eh_frame" scheme (and fails). Previously that worked because the code used a slightly different pattern which ended up comparing the number by converting it to the "generic" scheme (which we do provide). I don't think that's a bug in the unwinder -- we just need to make sure we provide the register number all the time. The associated test is not particularly useful, but I'm hoping it could be used to test other changes like this as well. Other register augmentation changes are tested in an end-to-end fashion (by injecting gdb-remote packets and then trying to read registers). That works well for testing the addition of subregisters, but it isn't particularly suitable for testing register numbers, as the only place (that I know of) where this manifests is during unwinding, and recreating an unwindable process in a test like this is fairly challenging. Probably the best way to ensure coverage for this scenario would be to have lldb-server stop sending eh_frame register numbers (align it with other gdb-like stubs). Full diff: https://github.com/llvm/llvm-project/pull/143499.diff 5 Files Affected:
diff --git a/lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp b/lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
index b9e7c698cdec0..e40d2c5fc121a 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
+++ b/lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
@@ -12,6 +12,7 @@
#include "ABIMacOSX_arm64.h"
#include "ABISysV_arm64.h"
#include "Utility/ARM64_DWARF_Registers.h"
+#include "Utility/ARM64_ehframe_Registers.h"
#include "lldb/Core/PluginManager.h"
#include "lldb/Target/Process.h"
@@ -69,9 +70,9 @@ lldb::addr_t ABIAArch64::FixDataAddress(lldb::addr_t pc) {
std::pair<uint32_t, uint32_t>
ABIAArch64::GetEHAndDWARFNums(llvm::StringRef name) {
if (name == "pc")
- return {LLDB_INVALID_REGNUM, arm64_dwarf::pc};
+ return {arm64_ehframe::pc, arm64_dwarf::pc};
if (name == "cpsr")
- return {LLDB_INVALID_REGNUM, arm64_dwarf::cpsr};
+ return {arm64_ehframe::cpsr, arm64_dwarf::cpsr};
return MCBasedABI::GetEHAndDWARFNums(name);
}
diff --git a/lldb/unittests/ABI/AArch64/ABIAArch64Test.cpp b/lldb/unittests/ABI/AArch64/ABIAArch64Test.cpp
new file mode 100644
index 0000000000000..7ae175a0d3a70
--- /dev/null
+++ b/lldb/unittests/ABI/AArch64/ABIAArch64Test.cpp
@@ -0,0 +1,68 @@
+//===-- ABIAArch64Test.cpp ------------------------------------------------===//
+
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/ABI/AArch64/ABIMacOSX_arm64.h"
+#include "Plugins/ABI/AArch64/ABISysV_arm64.h"
+#include "Utility/ARM64_DWARF_Registers.h"
+#include "Utility/ARM64_ehframe_Registers.h"
+#include "lldb/Target/DynamicRegisterInfo.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/TargetSelect.h"
+#include "gtest/gtest.h"
+#include <vector>
+
+using namespace lldb_private;
+using namespace lldb;
+
+class ABIAArch64TestFixture : public testing::TestWithParam<llvm::StringRef> {
+public:
+ static void SetUpTestCase();
+ static void TearDownTestCase();
+
+ // virtual void SetUp() override { }
+ // virtual void TearDown() override { }
+
+protected:
+};
+
+void ABIAArch64TestFixture::SetUpTestCase() {
+ LLVMInitializeAArch64TargetInfo();
+ LLVMInitializeAArch64TargetMC();
+ ABISysV_arm64::Initialize();
+ ABIMacOSX_arm64::Initialize();
+}
+
+void ABIAArch64TestFixture::TearDownTestCase() {
+ ABISysV_arm64::Terminate();
+ ABIMacOSX_arm64::Terminate();
+ llvm::llvm_shutdown();
+}
+
+TEST_P(ABIAArch64TestFixture, AugmentRegisterInfo) {
+ ABISP abi_sp = ABI::FindPlugin(ProcessSP(), ArchSpec(GetParam()));
+ ASSERT_TRUE(abi_sp);
+ using Register = DynamicRegisterInfo::Register;
+
+ Register pc{ConstString("pc"), ConstString(), ConstString("GPR")};
+ std::vector<Register> regs{pc};
+
+ abi_sp->AugmentRegisterInfo(regs);
+
+ ASSERT_EQ(regs.size(), 1);
+ Register new_pc = regs[0];
+ EXPECT_EQ(new_pc.name, pc.name);
+ EXPECT_EQ(new_pc.set_name, pc.set_name);
+ EXPECT_EQ(new_pc.regnum_ehframe, arm64_ehframe::pc);
+ EXPECT_EQ(new_pc.regnum_dwarf, arm64_dwarf::pc);
+}
+
+INSTANTIATE_TEST_SUITE_P(ABIAArch64Tests, ABIAArch64TestFixture,
+ testing::Values("aarch64-pc-linux",
+ "arm64-apple-macosx"));
diff --git a/lldb/unittests/ABI/AArch64/CMakeLists.txt b/lldb/unittests/ABI/AArch64/CMakeLists.txt
new file mode 100644
index 0000000000000..592701fc40753
--- /dev/null
+++ b/lldb/unittests/ABI/AArch64/CMakeLists.txt
@@ -0,0 +1,9 @@
+add_lldb_unittest(ABIAArch64Tests
+ ABIAArch64Test.cpp
+ LINK_COMPONENTS
+ Support
+ AArch64
+ LINK_LIBS
+ lldbTarget
+ lldbPluginABIAArch64
+ )
diff --git a/lldb/unittests/ABI/CMakeLists.txt b/lldb/unittests/ABI/CMakeLists.txt
new file mode 100644
index 0000000000000..8ad7474e9444a
--- /dev/null
+++ b/lldb/unittests/ABI/CMakeLists.txt
@@ -0,0 +1,3 @@
+if ("AArch64" IN_LIST LLVM_TARGETS_TO_BUILD)
+ add_subdirectory(AArch64)
+endif()
diff --git a/lldb/unittests/CMakeLists.txt b/lldb/unittests/CMakeLists.txt
index d8f9cc747986e..6eaaa4f4c8c98 100644
--- a/lldb/unittests/CMakeLists.txt
+++ b/lldb/unittests/CMakeLists.txt
@@ -52,6 +52,7 @@ if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
add_subdirectory(API)
add_subdirectory(DAP)
endif()
+add_subdirectory(ABI)
add_subdirectory(Breakpoint)
add_subdirectory(Callback)
add_subdirectory(Core)
|
|
I'd appreciate if some one could take a look at this, as one of our internal debug stubs is currently broken. |
JDevlieghere
left a comment
JDevlieghere
left a comment
There was a problem hiding this comment.
LGTM. I don't have the functional expertise to review this, but this makes sense and if there ends up being feedback it can be address post-merge.
|
Thanks. |
|
Thanks for help, I didn't realize I regressed this with my NFC reorg. |
|
I thought we augmented the target.xml from the remote gdb stub via |
No, it's doing both. You see the slice registers because that's completely architecture specific. The register numbers are being added in the base class (MCBasedABI::AugmentRegisterInfo), which calls into this function (ABIAArch64::GetEHAndDWARFNums) |
This fixes a regression caused by llvm#139817, where we would fail to unwind (using eh_frame) when using debug stubs (like qemu) which do not provide eh_frame register numbers. Unwinding fails because the unwinder tries to convert pc register number to the "eh_frame" scheme (and fails). Previously that worked because the code used a slightly different pattern which ended up comparing the number by converting it to the "generic" scheme (which we do provide). I don't think that's a bug in the unwinder -- we just need to make sure we provide the register number all the time. The associated test is not particularly useful, but I'm hoping it could be used to test other changes like this as well. Other register augmentation changes are tested in an end-to-end fashion (by injecting gdb-remote packets and then trying to read registers). That works well for testing the addition of subregisters, but it isn't particularly suitable for testing register numbers, as the only place (that I know of) where this manifests is during unwinding, and recreating an unwindable process in a test like this is fairly challenging. Probably the best way to ensure coverage for this scenario would be to have lldb-server stop sending eh_frame register numbers (align it with other gdb-like stubs).
This fixes a regression caused by llvm#139817, where we would fail to unwind (using eh_frame) when using debug stubs (like qemu) which do not provide eh_frame register numbers. Unwinding fails because the unwinder tries to convert pc register number to the "eh_frame" scheme (and fails). Previously that worked because the code used a slightly different pattern which ended up comparing the number by converting it to the "generic" scheme (which we do provide). I don't think that's a bug in the unwinder -- we just need to make sure we provide the register number all the time. The associated test is not particularly useful, but I'm hoping it could be used to test other changes like this as well. Other register augmentation changes are tested in an end-to-end fashion (by injecting gdb-remote packets and then trying to read registers). That works well for testing the addition of subregisters, but it isn't particularly suitable for testing register numbers, as the only place (that I know of) where this manifests is during unwinding, and recreating an unwindable process in a test like this is fairly challenging. Probably the best way to ensure coverage for this scenario would be to have lldb-server stop sending eh_frame register numbers (align it with other gdb-like stubs).
5 participants
This fixes a regression caused by #139817, where we would fail to unwind (using eh_frame) when using debug stubs (like qemu) which do not provide eh_frame register numbers.
Unwinding fails because the unwinder tries to convert pc register number to the "eh_frame" scheme (and fails). Previously that worked because the code used a slightly different pattern which ended up comparing the number by converting it to the "generic" scheme (which we do provide).
I don't think that's a bug in the unwinder -- we just need to make sure we provide the register number all the time.
The associated test is not particularly useful, but I'm hoping it could be used to test other changes like this as well. Other register augmentation changes are tested in an end-to-end fashion (by injecting gdb-remote packets and then trying to read registers). That works well for testing the addition of subregisters, but it isn't particularly suitable for testing register numbers, as the only place (that I know of) where this manifests is during unwinding, and recreating an unwindable process in a test like this is fairly challenging.
Probably the best way to ensure coverage for this scenario would be to have lldb-server stop sending eh_frame register numbers (align it with other gdb-like stubs).