[Lldb-commits] [PATCH] D79699: Add ptrace register access for AArch64 SVE registers

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I haven't looked at all the sve details, but they seem very similar to the core file stuff, so I think they should be fine. I have some other comments though...



================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:240
+  // Update SVE registers in case there is change in configuration.
+  ConfigureRegisterContext();
+
----------------
What's the relationship of this function and the `InvalidateAllRegisters` function we added (for arm64 benefit) a couple of reviews back? Can we just have one of them?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h:22
+#define INCLUDE_LINUX_PTRACE_DEFINITIONS_FOR_SVE_ARM64
+#include "Plugins/Process/Utility/LinuxPTraceDefines_arm64sve.h"
+#endif
----------------
At this point, that header is sufficiently different from the asm/ptrace.h implementation (names of all structs and fields are different) that I don't think it makes sense to use it as an optional replacement. It makes it very likely things will not build in one of the modes.

This should either use the system version (and then only build when the system has the appropriate headers), or we should use the in-house version unconditionally (which may require renaming/namespacing the rest of the file so that it does not conflict with the real `asm/ptrace.h`).


================
Comment at: lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py:22
+
+    @skipIf
+    def test_sve_registers_configuration(self):
----------------
It would be better to have this actually detect the availability of the feature. A skip-always test is not very convincing.

IIRC, some of the tests for fancy x86 registers have the inferior detect the availability of the feature (via the cpuid instruction or something), and then exit with a predefined exit code. Then the test can check for that and skip itself if the feature is not available. Would something like that work here?


================
Comment at: lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py:75
+
+    mydir = TestBase.compute_mydir(__file__)
+    @no_debug_info_test
----------------
you don't need to set this twice


================
Comment at: lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/main.c:2-4
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #5.00000000\n\t");
+  return 0; // Set a break point here.
----------------
In our newer register tests we use the pattern where the debugger sets the register values, which are then read back through the inferior (and vice versa). I thing this is a good thing as it avoids the possibility of making symmetrical bugs in the read/write code, which will then appear to modify the register successfully, but they won't actually change the value observed by the inferior process.


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

https://reviews.llvm.org/D79699