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