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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
