[Lldb-commits] [PATCH] D99947: [LLDB] AArch64 Linux PAC unwinder support

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:738
 
 bool GDBRemoteRegisterContext::AArch64SVEReconfigure() {
   if (!m_reg_info_sp)
----------------
I'm inviting bikeshedding but this should be renamed if it's not just SVE.

AArch64ReconfigureRegisters?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:748
+  // process. If PAC is enabled, address bits in use are configured based on
+  // PAC code/data mask.
+  if (!GetThread().GetProcess()->GetAddressBitsInUse()) {
----------------
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.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:751
+    uint32_t address_bits_in_use = 52;
+    const RegisterInfo *reg_info = m_reg_info_sp->GetRegisterInfo("code_mask");
+    if (reg_info) {
----------------
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
}
```



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:763
+        if (reg_info) {
+          fail_value = LLDB_INVALID_ADDRESS;
+          reg_num = reg_info->kinds[eRegisterKindLLDB];
----------------
You don't need to re-assign this.

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


================
Comment at: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/Makefile:3
+
+CFLAGS ?= -g -Os -march=armv8.5-a -mbranch-protection=pac-ret+leaf
+
----------------
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.


================
Comment at: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py:37
+        self.assertEqual(thread.GetNumFrames(), len(backtrace))
+        for i in range(len(backtrace)):
+            frame = thread.GetFrameAtIndex(i)
----------------
You could do this to replace the backtrace[i].
```
for i, name in enumerate(backtrace):
```


================
Comment at: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c:2
+#include <stdlib.h>
+
+static void func_a (void) __attribute__((noinline));
----------------
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.


================
Comment at: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c:8
+static void
+func_c (void)
+{
----------------
You could put the attribute here instead, the order the functions are defined in should be fine for the compiler.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99947/new/

https://reviews.llvm.org/D99947