omjavaid marked 3 inline comments as done.
omjavaid added inline comments.
================
Comment at:
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:240
+ // Update SVE registers in case there is change in configuration.
+ ConfigureRegisterContext();
+
----------------
labath wrote:
> 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?
Both InvalidateAllRegisters and ConfigureRegisterContext are inter related.
The idea is to maintain a cache of SVE register configurations created after
thread stop on first call to ReadRegister/WriteRegister, using
ConfigureRegisterContext. This configuration cache is maintained unless we do
register write or we issue a step or resume.
InvalidateAllRegisters is called before resume and it will invalidate all
cached register data including SVE configuration thus forcing us to do
ConfigureRegisterContext on stop.
There is also a case where we need to reconfigure register context when we
write vector granule register and need to update register sizes and offsets and
sync them up between LLDB client and server. That case is handled by the child
revisions of this patch.
================
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
----------------
labath wrote:
> 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`).
Ack.
================
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.
----------------
labath wrote:
> 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.
In this test we are also doing the same please have a look at
TestSVERegisters.py:140-142.
The code in main.c is used to make sure we enable SVE mode. AArch64 SVE mode is
enabled by Linux kernel on first execution of SVE code.
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
