[LLDB] AArch64 Linux PAC unwinder support

[omjavaid]

Summary

This patch follows up on changes made by D99944 to test support for
unwinding stack when AArch64 pointer authentication feature is enabled.

We have already added logic to calculate code and data address masks which
will be utilized to mask off ignored bits from top of return address during the
stack trace calculation.

Default value of used address bits will be set to 52 which is maximum
possible AArch64 virtual address width. Addition logic has been added to
AArch64SVEReconfigure to detect pointer authentication feature for the
current process. If PAC is enabled, address bits in use are configured based on
PAC code/data mask.

This patch includes a test case to demonstrate successful back trace
calculation in presence of pointer authentication feature.

[DavidSpickett]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp

738 ↗

(On Diff #335484)

I'm inviting bikeshedding but this should be renamed if it's not just SVE.

AArch64ReconfigureRegisters?

748 ↗

(On Diff #335484)

Can you elaborate here on how we configure?

I'm thinking about the fact that there's 2 masks. So do we prefer one, what if they differ, can they differ?

I'm wondering what happens if you don't have PAC for data accesses, though I'm not sure how you'd test that. Linux enables both I assume.

751 ↗

(On Diff #335484)

Is it possible for the code mask to not have bit 48 set but the data does? I think no because PAC expands to the unused address bits so if you didn't have 52 bit addressing code and data would have bit 48 set.

Also I think you could rewrite this to remove repetition of the masking. (assuming I have understood the original code)

std::array<const char*> masks{"data", "code"};
for (auto regname: masks) {
    try to get the register
    if it's valid:
       check mask
       break
}

763 ↗

(On Diff #335484)

You don't need to re-assign this.

Overall I'd rather see LLDB_INVALID_ADDRESS used directly. The meaning is clearer to me.

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/Makefile

4

Shouldn't we use := here given that ?= is meant to only set if CFLAGS is not set? (https://www.gnu.org/software/make/manual/html_node/Setting.html)

No one's going to override this CFLAGS and if they did, the test would probably fail anyway.

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py

38

You could do this to replace the backtrace[i].

for i, name in enumerate(backtrace):

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c

3

Could you put a comment here like:
"When compiled with pac-ret-leaf <whatever settings>, the <link register/stack pointer...> will be signed..."

Just to make it obvious that there is extra protection even if the program file is plain C.

9

You could put the attribute here instead, the order the functions are defined in should be fine for the compiler.

[omjavaid]

Fixed review comments and rebased.

[DavidSpickett]

Did the changes in GDBRemoteRegisterContext.cpp (https://reviews.llvm.org/D99947?id=335484#inline-940747 ) get moved to another diff?

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py

41

Add a comment here to explain that _start isn't in main.c, so we don't check for its line number. (took me a bit to work that out)

[omjavaid]

In D99947#2718954, @DavidSpickett wrote:

Did the changes in GDBRemoteRegisterContext.cpp (https://reviews.llvm.org/D99947?id=335484#inline-940747 ) get moved to another diff?

We no longer need to configure Address bits in use after changes from D100521 and D99944 get merged. ABISysV_arm64 now contains the logic for masking of ignored bits from code and data addresses. Look through overrides of FixCodeAddress and FixDataAddress in D99944.

[DavidSpickett]

In D99947#2719403, @omjavaid wrote:

In D99947#2718954, @DavidSpickett wrote:

Did the changes in GDBRemoteRegisterContext.cpp (https://reviews.llvm.org/D99947?id=335484#inline-940747 ) get moved to another diff?

We no longer need to configure Address bits in use after changes from D100521 and D99944 get merged. ABISysV_arm64 now contains the logic for masking of ignored bits from code and data addresses. Look through overrides of FixCodeAddress and FixDataAddress in D99944.

Seems like you could merge them completely in that case. Make one commit that adds tests for core files and real targets.

(FWIW this LGTM with the comment for _start added)

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/Makefile

+C_SOURCES := main.c
+
+CFLAGS := -g -Os -march=armv8.5-a -mbranch-protection=pac-ret+leaf
+
+include Makefile.rules

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py

+"""
+Test that we can backtrace correctly when AArch64 PAC is enabled
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64UnwindPAC(TestBase):
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipIf(archs=no_match(["aarch64"]))
+    @skipIf(oslist=no_match(['linux']))
+    def test(self):
+        """Test that we can backtrace correctly when AArch64 PAC is enabled"""
+        self.build()
+
+        self.line = line_number('main.c', '// Frame func_c')
+
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+        lldbutil.run_break_set_by_file_and_line(
+            self, "main.c", self.line, num_expected_locations=1)
+        self.runCmd("run", RUN_SUCCEEDED)
+        self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=["stop reason = breakpoint 1."])
+
+        target = self.dbg.GetSelectedTarget()
+        process = target.GetProcess()
+        thread = process.GetThreadAtIndex(0)
+
+        backtrace = ["func_c", "func_b", "func_a", "main", "__libc_start_main", "_start"]
+        self.assertEqual(thread.GetNumFrames(), len(backtrace))
+        for frame_idx, frame in enumerate(thread.frames):
+            frame = thread.GetFrameAtIndex(frame_idx)
+            self.assertTrue(frame)
+            self.assertEqual(frame.GetFunctionName(), backtrace[frame_idx])
+            if (frame_idx < 4):
+                self.assertEqual(frame.GetLineEntry().GetLine(),
+                                 line_number("main.c", "Frame " + backtrace[frame_idx]))

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c

+// This program makes a multi tier nested function call to test AArch64
+// Pointer Authentication feature.
+
+// To enable PAC return address signing compile with following clang arguments:
+// -march=armv8.5-a -mbranch-protection=pac-ret+leaf
+
+#include <stdlib.h>
+
+static void __attribute__((noinline)) func_c(void) {
+  exit(0); // Frame func_c
+}
+
+static void __attribute__((noinline)) func_b(void) {
+  func_c(); // Frame func_b
+}
+
+static void __attribute__((noinline)) func_a(void) {
+  func_b(); // Frame func_a
+}
+
+int main(int argc, char *argv[]) {
+  func_a(); // Frame main
+  return 0;
+}