[Lldb-commits] [PATCH] D120485: [lldb][Process/FreeBSD] Add support for address masks on aarch64

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Thanks for enabling the test, great to see support for this outside Linux.



================
Comment at: lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp:817
+  return address_mask;
+}
+
----------------
I was going to suggest deduping this and the Linux function but then I found a problem with the Linux one that needs fixing. https://reviews.llvm.org/D122411#change-cwOvKZ9JBiEU

We could probably move everything within the `if thread_sp` into a function to reuse it but I can do that after I get the Linux one correct.


================
Comment at: lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp:104
+
+
 Status NativeRegisterContextFreeBSD_arm64::ReadGPR() {
----------------
Remove the extra newline here.


================
Comment at: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py:37-38
+        backtrace = ["func_c", "func_b", "func_a", "main"] + backtrace_tail
+        # This doesn't work as we get a ___lldb_unnamed_symbol on FreeBSD
+        #self.assertEqual(thread.GetNumFrames(), len(backtrace))
         for frame_idx, frame in enumerate(thread.frames):
----------------
Might as well delete these lines then. Perhaps a comment before `backtrace = ` "# Between Linux and FreeBSD the last frames differ". (first frames? I get them mixed up sometimes)


================
Comment at: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py:42
+            lldb_unnamed = "___lldb_unnamed_symbol"
+            if frame.GetFunctionName()[:len(lldb_unnamed)] == lldb_unnamed:
+                break
----------------
You could do:
```
if frame.GetFunctionName().startswith(""___lldb_unnamed_symbol"):
```

If I understand correctly, on FreeBSD this unamed symbol marks the end of the backtrace basically? If so please comment before the if to say so.


================
Comment at: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c:19
+
+  return (hwcap & HWCAP_PACA) != 0;
+}
----------------
Do you have any worry that `HWCAP_PACA` might not be defined or has it been around long enough at this point to be ok? I don't know what toolchains you usually build with.

For Linux we've sometimes done #ifndef something #define it to what we know the constant should be, but other times we just use it, with mixed results.


================
Comment at: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c:22
+#else
+static bool pac_supported(void) {
+  return true;
----------------
Add a comment like:
```
// We expect Linux to check for PAC up front using /proc/cpuinfo.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120485