labath added inline comments.
================
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
----------------
omjavaid wrote:
> labath wrote:
> > omjavaid wrote:
> > > omjavaid wrote:
> > > > 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.
> > > asm/ptrace.h also describes legacy structs for fpsimd access, hardware
> > > breakpoint structs and other related ptrace access. Newer versions of
> > > ptrace only append SVE specific structs and macros. In house LLDB's SVE
> > > macros file only include SVE specific register names. I dont see any
> > > conflict since I have also added a guard for multiple definitions in
> > > LinuxPTraceDefines_arm64sve.h as it protects against builds of
> > > AArch64/Linux where ThreadElfCore also includes sigcontext and ptrace.h
> > > for accessing elf-core related struct definitions.
> > I'm not saying we shouldn't include asm/ptrace.h at all. It obviously
> > contains a lot of useful stuff, and we wouldn't want to duplicate all of
> > that.
> >
> > What I am saying is that for the stuff that we already have duplicated, we
> > just be using that unconditionally. Doing otherwise just needlessly forks
> > the build matrix and creates opportunities for things to not work in one
> > mode or the other. We're already committed to ensuring things work with the
> > in-house definitions so why bothering to ensure that things work both with
> > them _and_ with the system ones.
> Right now SVE in-house macros and the ones in asm/ptrace.h are identical. If
> we include asm/ptrace.h and it already has SVE macros we cannot avoid that
> inclusion so in case we want to force use of in-house macros we ll have to
> rename the in-house macros and headers with LLVM_ or LLDB_ prefix to make
> sure we avoid duplication. If you have strong opinion in favor of renaming i
> can go ahead and do that.
I do, actually. Given that this code is also used for the core files, which can
be used from any host, and the fact that we have already started making changes
to it because of that (`_uNN` vs `uintNN_t`, the msvc compatibility patch,
etc), I think we just just treating this as a part of our codebase and not some
asm/ptrace addon.
I wouldn't tack an additional LLDB_ onto the names though. What I'd do is
replace all of these macros with regular C(++) symbols. Parameter-less macros
can be constants and function-like macros can just be functions. They can keep
the original all-caps spelling to make it clear where they are coming from, but
in order to distinguish them from the system symbols we can make a tiny
modification -- instead of naming everyting `SVE_FOO`, we name the thing just
`FOO`, but put it inside a `lldb_private::SVE` namespace -- that way the
symbols can be referred to as `SVE::FOO`.
================
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.
----------------
omjavaid wrote:
> labath wrote:
> > omjavaid wrote:
> > > 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.
> > I have a feeling we're misunderstanding each other. I am talking about the
> > tests in `test/Shell/Register` (e.g. `x86-64-xmm16-read.test`,
> > `x86-64-xmm16-write.test`). This test definitely doesn't to that. The code
> > in 140-142 is just reading back what was written by lines 135-137, not
> > values set by the inferior.
> The problem in case of SVE is that size of vector is unknown untill we run
> therefore we cannot hard write value that will be written and read back.
> Also this API based test actually verifies that vector granule register value
> and Z/P register sizes make sense. It then prepares a value to be written to
> Z, P or FFr registers based on currently configured vector length and the
> read that same value back. I could not find a way to do that using shell
> based tests.
Right. While it would definitely be nice if this test was next to the other
ones, rewriting this as a lit test is not what I was going for. I think these
are sufficiently special to handle them this way.
What I was referring to is the general principle of setting the registers in
the inferior (via `asm` or something) and then reading them from the test. That
principle applies no matter which style is the test written in.
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
