[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7e017de0ad62: AArch64 SVE register infos and core file 
support (authored by omjavaid).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77047

Files:
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  
lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve-fpsimd.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve-full.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -35,69 +35,69 @@
 _mips_regions = 5
 _ppc64le_regions = 2
 
-
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("AArch64")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_aarch64(self):
 """Test that lldb can read the process information from an aarch64 linux core file."""
-self.do_test("linux-aarch64", self._aarch64_pid, self._aarch64_regions, "a.out")
+self.do_test("linux-aarch64", self._aarch64_pid,
+ self._aarch64_regions, "a.out")
 
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("X86")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_i386(self):
 """Test that lldb can read the process information from an i386 linux core file."""
 self.do_test("linux-i386", self._i386_pid, self._i386_regions, "a.out")
 
 @skipIfLLVMTargetMissing("Mips")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_mips_o32(self):
 """Test that lldb can read the process information from an MIPS O32 linux core file."""
 self.do_test("linux-mipsel-gnuabio32", self._mips_o32_pid,
-self._mips_regions, "linux-mipsel-gn")
+ self._mips_regions, "linux-mipsel-gn")
 
 @skipIfLLVMTargetMissing("Mips")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_mips_n32(self):
 """Test that lldb can read the process information from an MIPS N32 linux core file """
 self.do_test("linux-mips64el-gnuabin32", self._mips64_n32_pid,
-self._mips_regions, "linux-mips64el-")
+ self._mips_regions, "linux-mips64el-")
 
 @skipIfLLVMTargetMissing("Mips")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_mips_n64(self):
 """Test that lldb can read the process information from an MIPS N64 linux core file """
 self.do_test("linux-mips64el-gnuabi64", self._mips64_n64_pid,
-self._mips_regions, "linux-mips64el-")
+  

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 279151.
omjavaid added a comment.

This update makes minor adjustments before merge in light of final comments.


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

https://reviews.llvm.org/D77047

Files:
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  
lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve-fpsimd.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve-full.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -35,69 +35,69 @@
 _mips_regions = 5
 _ppc64le_regions = 2
 
-
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("AArch64")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_aarch64(self):
 """Test that lldb can read the process information from an aarch64 linux core file."""
-self.do_test("linux-aarch64", self._aarch64_pid, self._aarch64_regions, "a.out")
+self.do_test("linux-aarch64", self._aarch64_pid,
+ self._aarch64_regions, "a.out")
 
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("X86")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_i386(self):
 """Test that lldb can read the process information from an i386 linux core file."""
 self.do_test("linux-i386", self._i386_pid, self._i386_regions, "a.out")
 
 @skipIfLLVMTargetMissing("Mips")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_mips_o32(self):
 """Test that lldb can read the process information from an MIPS O32 linux core file."""
 self.do_test("linux-mipsel-gnuabio32", self._mips_o32_pid,
-self._mips_regions, "linux-mipsel-gn")
+ self._mips_regions, "linux-mipsel-gn")
 
 @skipIfLLVMTargetMissing("Mips")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_mips_n32(self):
 """Test that lldb can read the process information from an MIPS N32 linux core file """
 self.do_test("linux-mips64el-gnuabin32", self._mips64_n32_pid,
-self._mips_regions, "linux-mips64el-")
+ self._mips_regions, "linux-mips64el-")
 
 @skipIfLLVMTargetMissing("Mips")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_mips_n64(self):
 """Test that lldb can read the process information from an MIPS N64 linux core file """
 self.do_test("linux-mips64el-gnuabi64", self._mips64_n64_pid,
-self._mips_regions, "linux-mips64el-")
+ self._mips_regions, "linux-mips64el-")
 
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMis

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

I think this is as good as we can get right now. Thanks for sticking through. 
Two quick comments inline.




Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:152-153
+
+  if (sve_reg_num != LLDB_INVALID_REGNUM &&
+  offset != LLDB_INVALID_INDEX32) {
+assert(offset < m_sveregset.GetByteSize());

Are there any FPR registers which we should not be able to retrieve from the 
SVE state? I'm wondering if this should be turned into an assert.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:167-169
+uint64_t byte_size = 1;
+uint8_t zeros = 0;
+const uint8_t *src = &zeros;

It looks like this is only used in the `IsSVEZ(reg)` block below. You could 
move the declarations there and avoid initializations with bogus values.


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked an inline comment as done.
omjavaid added inline comments.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:63-66
+m_sve_note_payload.resize(m_sveregset.GetByteSize());
+::memcpy(GetSVEBuffer(), m_sveregset.GetDataStart(),
+ m_sveregset.GetByteSize());
+::memcpy(&m_sve_header, m_sveregset.GetDataStart(), sizeof(m_sve_header));

labath wrote:
> omjavaid wrote:
> > labath wrote:
> > > What's up with this copying? We already have the data in the 
> > > `m_sveregset` DataExtractor. What's the reason for copying it into the 
> > > `m_sve_note_payload` vector? Also, `m_sve_header` seems like it could 
> > > just be a `reinterpret_cast`ed pointer into that buffer instead of a 
> > > copy. (Maybe not even a pointer, just a utility function which performs 
> > > the cast when called).
> > > 
> > > Actually, when I think about casts and data extractors, I am reminded of 
> > > endianness. This will access those fields using host endianness, which is 
> > > most likely not what we want to do. So, probably the best/simplest 
> > > solution would be to indeed declare a `user_sve_header` struct, but don't 
> > > populate it via memcpy, but rather via the appropriate DataExtractor 
> > > extraction methods. Since the only field of the user_sve_header used 
> > > outside this function is the `vl` field, perhaps the struct could be a 
> > > local variable and only the vector length would be persisted. That would 
> > > be consistent with how the `flags` field is decoded and stored into the 
> > > `m_sve_state` field.
> > > 
> > > (If the struct fields are always little-endian (if that's true, I thought 
> > > arm has BE variants) then you could also stick to the `reinterpret_cast` 
> > > idea, but change the types of the struct fields to 
> > > `llvm::support::ulittleXX_t` to read them as LE independently of the 
> > > host.)
> > Agreed m_sve_note_payload is redundant. Most of the implementation was 
> > mirrored from ptrace implementation and I was lazy enough and did not 
> > remove the redundancies which are not needed in case of core dump. 
> > 
> > About endianess of SVE data I dont think it will create any problem as the 
> > data is transferred in endian invariant format. According to the standard 
> > here:
> > 
> > * Whenever SVE scalable register values (Zn, Pn, FFR) are exchanged in 
> > memory
> >   between userspace and the kernel, the register value is encoded in memory 
> > in
> >   an endianness-invariant layout, with bits [(8 * i + 7) : (8 * i)] encoded 
> > at
> >   byte offset i from the start of the memory representation.  This affects 
> > for
> >   example the signal frame (struct sve_context) and ptrace interface
> >   (struct user_sve_header) and associated data.
> Ok, so if I parse that correctly, this is basically saying that the register 
> values are always little-endian. Fair enough. However, lldb will be reading 
> these things using the host endianness, so the values will be incorrect on a 
> big-endian host.
> 
> Also, the are you sure that this includes the metadata in the 
> `user_sve_header` field? The documentation does mention the struct 
> explicitly, but then it also very explicitly speaks about **register** 
> values. The data in the struct is not a register value, and I can find no 
> evidence of byte-swapping in the linux kernel 
> (https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/ptrace.c#L769).
@labath Thanks for being great help in this review. I think you were right 
about the user_sve_header being susceptible to endianity problems. I have 
plugged the issues now.


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 278617.
omjavaid added a comment.

This update plugs the holes susceptible to bugs by endianity variations.  I 
have used DataExtractor GetU16 to read flags and vl from user_sve_header. Also 
removed the memset and replaced it with SetFromMemoryData function.

Moreover a new test is added which tests SVE FPSIMD mode core file.


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

https://reviews.llvm.org/D77047

Files:
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  
lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve-fpsimd.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve-full.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -35,69 +35,69 @@
 _mips_regions = 5
 _ppc64le_regions = 2
 
-
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("AArch64")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_aarch64(self):
 """Test that lldb can read the process information from an aarch64 linux core file."""
-self.do_test("linux-aarch64", self._aarch64_pid, self._aarch64_regions, "a.out")
+self.do_test("linux-aarch64", self._aarch64_pid,
+ self._aarch64_regions, "a.out")
 
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("X86")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_i386(self):
 """Test that lldb can read the process information from an i386 linux core file."""
 self.do_test("linux-i386", self._i386_pid, self._i386_regions, "a.out")
 
 @skipIfLLVMTargetMissing("Mips")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_mips_o32(self):
 """Test that lldb can read the process information from an MIPS O32 linux core file."""
 self.do_test("linux-mipsel-gnuabio32", self._mips_o32_pid,
-self._mips_regions, "linux-mipsel-gn")
+ self._mips_regions, "linux-mipsel-gn")
 
 @skipIfLLVMTargetMissing("Mips")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_mips_n32(self):
 """Test that lldb can read the process information from an MIPS N32 linux core file """
 self.do_test("linux-mips64el-gnuabin32", self._mips64_n32_pid,
-self._mips_regions, "linux-mips64el-")
+ self._mips_regions, "linux-mips64el-")
 
 @skipIfLLVMTargetMissing("Mips")
-@skipIfReproducer # lldb::FileSP used in typemap cannot be instrumented.
+@skipIfReproducer  # lldb::FileSP used in typemap cannot be instrumented.
 def test_mips_n64(self):
 """Test that lldb can read the process information from an MIPS N64 linux core file """
 self.do_test("linu

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:63-66
+m_sve_note_payload.resize(m_sveregset.GetByteSize());
+::memcpy(GetSVEBuffer(), m_sveregset.GetDataStart(),
+ m_sveregset.GetByteSize());
+::memcpy(&m_sve_header, m_sveregset.GetDataStart(), sizeof(m_sve_header));

omjavaid wrote:
> labath wrote:
> > What's up with this copying? We already have the data in the `m_sveregset` 
> > DataExtractor. What's the reason for copying it into the 
> > `m_sve_note_payload` vector? Also, `m_sve_header` seems like it could just 
> > be a `reinterpret_cast`ed pointer into that buffer instead of a copy. 
> > (Maybe not even a pointer, just a utility function which performs the cast 
> > when called).
> > 
> > Actually, when I think about casts and data extractors, I am reminded of 
> > endianness. This will access those fields using host endianness, which is 
> > most likely not what we want to do. So, probably the best/simplest solution 
> > would be to indeed declare a `user_sve_header` struct, but don't populate 
> > it via memcpy, but rather via the appropriate DataExtractor extraction 
> > methods. Since the only field of the user_sve_header used outside this 
> > function is the `vl` field, perhaps the struct could be a local variable 
> > and only the vector length would be persisted. That would be consistent 
> > with how the `flags` field is decoded and stored into the `m_sve_state` 
> > field.
> > 
> > (If the struct fields are always little-endian (if that's true, I thought 
> > arm has BE variants) then you could also stick to the `reinterpret_cast` 
> > idea, but change the types of the struct fields to 
> > `llvm::support::ulittleXX_t` to read them as LE independently of the host.)
> Agreed m_sve_note_payload is redundant. Most of the implementation was 
> mirrored from ptrace implementation and I was lazy enough and did not remove 
> the redundancies which are not needed in case of core dump. 
> 
> About endianess of SVE data I dont think it will create any problem as the 
> data is transferred in endian invariant format. According to the standard 
> here:
> 
> * Whenever SVE scalable register values (Zn, Pn, FFR) are exchanged in memory
>   between userspace and the kernel, the register value is encoded in memory in
>   an endianness-invariant layout, with bits [(8 * i + 7) : (8 * i)] encoded at
>   byte offset i from the start of the memory representation.  This affects for
>   example the signal frame (struct sve_context) and ptrace interface
>   (struct user_sve_header) and associated data.
Ok, so if I parse that correctly, this is basically saying that the register 
values are always little-endian. Fair enough. However, lldb will be reading 
these things using the host endianness, so the values will be incorrect on a 
big-endian host.

Also, the are you sure that this includes the metadata in the `user_sve_header` 
field? The documentation does mention the struct explicitly, but then it also 
very explicitly speaks about **register** values. The data in the struct is not 
a register value, and I can find no evidence of byte-swapping in the linux 
kernel 
(https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/ptrace.c#L769).


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 278435.
omjavaid added a comment.

In this update I have addressed issues highlighted in last review iteration.

@labath Does this now look LGTM?


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

https://reviews.llvm.org/D77047

Files:
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.core

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -323,6 +323,48 @@
 
 self.expect("register read --all")
 
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-sve.core")
+
+values = {}
+values["fp"] = "0xfc1ff4f0"
+values["lr"] = "0x00400170"
+values["sp"] = "0xfc1ff4d0"
+values["pc"] = "0x0040013c"
+values["v0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["v1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["s0"] = "7.5"
+values["s1"] = "11.5"
+values["s2"] = "0"
+values["s3"] = "15.5"
+values["d0"] = "65536.0158538818"
+values["d1"] = "1572864.25476074"
+values["d2"] = "0"
+values["d3"] = "25165828.0917969"
+values["vg"] = "0x0004"
+values["z0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["z1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["z2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["z3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["p0"] = "{0x11 0x11 0x11 0x11}"
+values["p1"] = "{0x11 0x11 0x11 0x11}"
+values["p2"] = "{0x00 0x00 0x00 0x00}"
+values["p3"] = "{0x11 0x11 0x11 0x11}"
+values["p4"] = "{0x00 0x00 0x00 0x00}"
+
+for regname, value in values.items():
+self.expect("register read {}".format(regname),
+substrs=["{} = {}".format(regname, value)])
+
+self.expect("register read --all")
+
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("ARM")
 def test_arm_core(self):
Index: lldb/source/Plugins/Process/elf-core/RegisterUt

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked 9 inline comments as done.
omjavaid added inline comments.



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:289
+
+lldb_private::RegisterInfo *new_reg_info_p = reg_info_ref.data();
+

labath wrote:
> I think all uses of `new_reg_info_p` could just be replaced by `reg_info_ref`
Ack.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:63-66
+m_sve_note_payload.resize(m_sveregset.GetByteSize());
+::memcpy(GetSVEBuffer(), m_sveregset.GetDataStart(),
+ m_sveregset.GetByteSize());
+::memcpy(&m_sve_header, m_sveregset.GetDataStart(), sizeof(m_sve_header));

labath wrote:
> What's up with this copying? We already have the data in the `m_sveregset` 
> DataExtractor. What's the reason for copying it into the `m_sve_note_payload` 
> vector? Also, `m_sve_header` seems like it could just be a 
> `reinterpret_cast`ed pointer into that buffer instead of a copy. (Maybe not 
> even a pointer, just a utility function which performs the cast when called).
> 
> Actually, when I think about casts and data extractors, I am reminded of 
> endianness. This will access those fields using host endianness, which is 
> most likely not what we want to do. So, probably the best/simplest solution 
> would be to indeed declare a `user_sve_header` struct, but don't populate it 
> via memcpy, but rather via the appropriate DataExtractor extraction methods. 
> Since the only field of the user_sve_header used outside this function is the 
> `vl` field, perhaps the struct could be a local variable and only the vector 
> length would be persisted. That would be consistent with how the `flags` 
> field is decoded and stored into the `m_sve_state` field.
> 
> (If the struct fields are always little-endian (if that's true, I thought arm 
> has BE variants) then you could also stick to the `reinterpret_cast` idea, 
> but change the types of the struct fields to `llvm::support::ulittleXX_t` to 
> read them as LE independently of the host.)
Agreed m_sve_note_payload is redundant. Most of the implementation was mirrored 
from ptrace implementation and I was lazy enough and did not remove the 
redundancies which are not needed in case of core dump. 

About endianess of SVE data I dont think it will create any problem as the data 
is transferred in endian invariant format. According to the standard here:

* Whenever SVE scalable register values (Zn, Pn, FFR) are exchanged in memory
  between userspace and the kernel, the register value is encoded in memory in
  an endianness-invariant layout, with bits [(8 * i + 7) : (8 * i)] encoded at
  byte offset i from the start of the memory representation.  This affects for
  example the signal frame (struct sve_context) and ptrace interface
  (struct user_sve_header) and associated data.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:84-86
+  const uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
+  if (reg == LLDB_INVALID_REGNUM)
+return false;

labath wrote:
> This is already checked by ReadRegister and the false -> uint32_t conversion 
> is dodgy. An assertion would completely suffice here.
Ack.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:140
+  offset -= GetGPRSize();
+  if (IsFPR(reg) && offset < m_fpregset.GetByteSize()) {
+value.SetFromMemoryData(reg_info, m_fpregset.GetDataStart() + offset,

labath wrote:
> This `IsFPR` check is now redundant.
Ack,



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:152-153
+sve_reg_num = reg_info->value_regs[0];
+  else if (reg == GetRegNumFPCR() || reg == GetRegNumFPSR())
+sve_reg_num = reg;
+  if (sve_reg_num != LLDB_INVALID_REGNUM) {

labath wrote:
> These two registers are special-cased both here and in the 
> `CalculateSVEOffset`. Given that both functions also do a switch over the sve 
> "states", it makes following the code quite challenging. What if we moved the 
> handling of these registers completely up-front, and removed their handling 
> from `CalculateSVEOffset` completely?
> I'm thinking of something like:
> ```
> if (reg == GetRegNumFPCR() && m_sve_state != SVEState::Disabled) {
>   src = GetSVEBuffer() + GetFPCROffset(); // Or maybe just inline 
> GetFPCROffset here
> } else if (reg == GetRegNumFPSR() && m_sve_state != SVEState::Disabled) {
>   src = ...;
> } if (IsFPR(reg)) {
>   // as usual, only you can assume that FP[CS]R are already handled if SVE is 
> enabled
> }
> ```
Ok will try to update in next revision.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:161-162
+  } else if (IsSVEVG(reg)) {
+sve_vg = GetSVERegVG();
+src = (uint8_t *)&sve_vg;
+  }

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

At this point, I think I (finally) have a good understanding of both how this 
patch works and interacts with the rest of the world. I have one more batch of 
comments, but hopefully none are too controversial, and I really do hope this 
is the last iteration.




Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:289
+
+lldb_private::RegisterInfo *new_reg_info_p = reg_info_ref.data();
+

I think all uses of `new_reg_info_p` could just be replaced by `reg_info_ref`



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:63-66
+m_sve_note_payload.resize(m_sveregset.GetByteSize());
+::memcpy(GetSVEBuffer(), m_sveregset.GetDataStart(),
+ m_sveregset.GetByteSize());
+::memcpy(&m_sve_header, m_sveregset.GetDataStart(), sizeof(m_sve_header));

What's up with this copying? We already have the data in the `m_sveregset` 
DataExtractor. What's the reason for copying it into the `m_sve_note_payload` 
vector? Also, `m_sve_header` seems like it could just be a `reinterpret_cast`ed 
pointer into that buffer instead of a copy. (Maybe not even a pointer, just a 
utility function which performs the cast when called).

Actually, when I think about casts and data extractors, I am reminded of 
endianness. This will access those fields using host endianness, which is most 
likely not what we want to do. So, probably the best/simplest solution would be 
to indeed declare a `user_sve_header` struct, but don't populate it via memcpy, 
but rather via the appropriate DataExtractor extraction methods. Since the only 
field of the user_sve_header used outside this function is the `vl` field, 
perhaps the struct could be a local variable and only the vector length would 
be persisted. That would be consistent with how the `flags` field is decoded 
and stored into the `m_sve_state` field.

(If the struct fields are always little-endian (if that's true, I thought arm 
has BE variants) then you could also stick to the `reinterpret_cast` idea, but 
change the types of the struct fields to `llvm::support::ulittleXX_t` to read 
them as LE independently of the host.)



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:84-86
+  const uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
+  if (reg == LLDB_INVALID_REGNUM)
+return false;

This is already checked by ReadRegister and the false -> uint32_t conversion is 
dodgy. An assertion would completely suffice here.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:140
+  offset -= GetGPRSize();
+  if (IsFPR(reg) && offset < m_fpregset.GetByteSize()) {
+value.SetFromMemoryData(reg_info, m_fpregset.GetDataStart() + offset,

This `IsFPR` check is now redundant.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:152-153
+sve_reg_num = reg_info->value_regs[0];
+  else if (reg == GetRegNumFPCR() || reg == GetRegNumFPSR())
+sve_reg_num = reg;
+  if (sve_reg_num != LLDB_INVALID_REGNUM) {

These two registers are special-cased both here and in the 
`CalculateSVEOffset`. Given that both functions also do a switch over the sve 
"states", it makes following the code quite challenging. What if we moved the 
handling of these registers completely up-front, and removed their handling 
from `CalculateSVEOffset` completely?
I'm thinking of something like:
```
if (reg == GetRegNumFPCR() && m_sve_state != SVEState::Disabled) {
  src = GetSVEBuffer() + GetFPCROffset(); // Or maybe just inline GetFPCROffset 
here
} else if (reg == GetRegNumFPSR() && m_sve_state != SVEState::Disabled) {
  src = ...;
} if (IsFPR(reg)) {
  // as usual, only you can assume that FP[CS]R are already handled if SVE is 
enabled
}
```



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:161-162
+  } else if (IsSVEVG(reg)) {
+sve_vg = GetSVERegVG();
+src = (uint8_t *)&sve_vg;
+  } else if (IsSVE(reg)) {

This also looks endian-incorrect. Maybe `value = GetSVERegVG(); return true;` ?



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:164
+  } else if (IsSVE(reg)) {
+if (m_sve_state != SVEState::Disabled) {
+  if (m_sve_state == SVEState::FPSIMD) {

A switch over possible `m_sve_state` values would likely be cleaner.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:174
+  ::memcpy(sve_reg_non_live.data(),
+   (const uint8_t *)GetSVEBuffer() + offset, 16);
+}

I guess it would be better to do the cast inside GetSVEBuffer. (and please make 
that a c++ reinterpret_cast).

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 278124.
omjavaid added a comment.

Minor fix removing GetRegNumP0 which is no longer needed.


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

https://reviews.llvm.org/D77047

Files:
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.core

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -323,6 +323,48 @@
 
 self.expect("register read --all")
 
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-sve.core")
+
+values = {}
+values["fp"] = "0xfc1ff4f0"
+values["lr"] = "0x00400170"
+values["sp"] = "0xfc1ff4d0"
+values["pc"] = "0x0040013c"
+values["v0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["v1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["s0"] = "7.5"
+values["s1"] = "11.5"
+values["s2"] = "0"
+values["s3"] = "15.5"
+values["d0"] = "65536.0158538818"
+values["d1"] = "1572864.25476074"
+values["d2"] = "0"
+values["d3"] = "25165828.0917969"
+values["vg"] = "0x0004"
+values["z0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["z1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["z2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["z3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["p0"] = "{0x11 0x11 0x11 0x11}"
+values["p1"] = "{0x11 0x11 0x11 0x11}"
+values["p2"] = "{0x00 0x00 0x00 0x00}"
+values["p3"] = "{0x11 0x11 0x11 0x11}"
+values["p4"] = "{0x00 0x00 0x00 0x00}"
+
+for regname, value in values.items():
+self.expect("register read {}".format(regname),
+substrs=["{} = {}".format(regname, value)])
+
+self.expect("register read --all")
+
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("ARM")
 def test_arm_core(self):
Index: lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
===

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 278113.
omjavaid added a comment.

This update address issues highlighted in last iteration and also minimizes the 
use of SVE_PT macros by using RegisterInfo.byte_size and byte_offset where 
possible.

@labath What do you think ?


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

https://reviews.llvm.org/D77047

Files:
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.core

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -323,6 +323,48 @@
 
 self.expect("register read --all")
 
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-sve.core")
+
+values = {}
+values["fp"] = "0xfc1ff4f0"
+values["lr"] = "0x00400170"
+values["sp"] = "0xfc1ff4d0"
+values["pc"] = "0x0040013c"
+values["v0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["v1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["s0"] = "7.5"
+values["s1"] = "11.5"
+values["s2"] = "0"
+values["s3"] = "15.5"
+values["d0"] = "65536.0158538818"
+values["d1"] = "1572864.25476074"
+values["d2"] = "0"
+values["d3"] = "25165828.0917969"
+values["vg"] = "0x0004"
+values["z0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["z1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["z2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["z3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["p0"] = "{0x11 0x11 0x11 0x11}"
+values["p1"] = "{0x11 0x11 0x11 0x11}"
+values["p2"] = "{0x00 0x00 0x00 0x00}"
+values["p3"] = "{0x11 0x11 0x11 0x11}"
+values["p4"] = "{0x00 0x00 0x00 0x00}"
+
+for regname, value in values.items():
+self.expect("register read {}".format(regname),
+substrs=["{} = {}".format(regname, value)])
+
+self.expect("register read --all")
+
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("ARM")

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked an inline comment as done.
omjavaid added inline comments.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+if (IsSVEZ(reg_num))
+  sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+else if (reg_num == GetRegNumFPSR())

labath wrote:
> omjavaid wrote:
> > labath wrote:
> > > omjavaid wrote:
> > > > labath wrote:
> > > > > omjavaid wrote:
> > > > > > labath wrote:
> > > > > > > I'm confused by this function. If I understant the SVE_PT macros 
> > > > > > > and the logic in 
> > > > > > > `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` 
> > > > > > > correctly, then they both seem to encode the same information. 
> > > > > > > And it seems to me that this function should just be 
> > > > > > > `reg_infos[reg_num].offset - some_constant`, which is the same 
> > > > > > > thing that we're doing for the arm FP registers when SVE is 
> > > > > > > disabled, and also for most other architectures too.
> > > > > > > 
> > > > > > > Why is that not the case? Am I missing something? If they are not 
> > > > > > > encoding the same thing, could they be made to encode the same 
> > > > > > > thing?
> > > > > > This function calculates offset of a particular register in core 
> > > > > > note data. SVE data in core dump is similar to what PTrace emits 
> > > > > > and offsets into this data is not linear. SVE macros are used to 
> > > > > > access those offsets based on register numbers and currently 
> > > > > > selected vector length.
> > > > > > Also for the purpose of ease we have linear offsets in SVE register 
> > > > > > infos and it helps us simplify register data once it makes way to 
> > > > > > GDBRemoteRegisterContext on the client side.
> > > > > Could you give an example of the non-linearity of the core dump data? 
> > > > > (Some registers, and their respective core file and gdb-remote 
> > > > > offsets)
> > > > In case of core file we create a buffer m_sveregset which stores SVE 
> > > > core note information
> > > > m_sveregset =
> > > >   getRegset(notes, 
> > > > m_register_info_up->GetTargetArchitecture().GetTriple(),
> > > > AARCH64_SVE_Desc);
> > > > 
> > > > At this point we do not know what was the vector length and at what 
> > > > offsets in the data our registers are located. We read top bytes of 
> > > > size use_sve_header and read vector length. Based on this information 
> > > > we configure vector length in Register infos. While the SVE payload 
> > > > starts with user_sve_header then there are some allignment bytes 
> > > > followed by vector length based Z registers followed by P and FFR, then 
> > > > there are some more allginment bytes followd by FPCR and FPSR. Macros 
> > > > provided by Linux help us jump to the desired offset by providing 
> > > > register number and vq into the core note or Ptrace payload.
> > > > 
> > > > In case of client side storage we store GPRs at linear offset followed 
> > > > by Vector Granule register. Then there are SVE registers Z, P, FFR, 
> > > > FPSR and FPCR. Offsets of V, D and S registers in FPR regset overlap 
> > > > with corresponding first bytes of Z registers and will be same as 
> > > > corresponding Z register. While both FP/SVE FPSR share same register 
> > > > offset, size and register number.
> > > > 
> > > > Here is an excerpt from 
> > > > https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.rst
> > > > 
> > > > SVE_PT_REGS_FPSIMD
> > > > SVE registers are not live (GETREGSET) or are to be made non-live 
> > > > (SETREGSET).
> > > > The payload is of type struct user_fpsimd_state, with the same meaning 
> > > > as for NT_PRFPREG, starting at offset SVE_PT_FPSIMD_OFFSET from the 
> > > > start of user_sve_header.
> > > > Extra data might be appended in the future: the size of the payload 
> > > > should be obtained using SVE_PT_FPSIMD_SIZE(vq, flags).
> > > > vq should be obtained using sve_vq_from_vl(vl).
> > > > 
> > > > or
> > > > 
> > > > SVE_PT_REGS_SVE
> > > > SVE registers are live (GETREGSET) or are to be made live (SETREGSET).
> > > > The payload contains the SVE register data, starting at offset 
> > > > SVE_PT_SVE_OFFSET from the start of user_sve_header, and with size 
> > > > SVE_PT_SVE_SIZE(vq, flags);
> > > Given this
> > > > SVE payload starts with ... followed by vector length based Z registers 
> > > > followed by P and FFR,
> > > and this
> > > > In case of client side storage we store GPRs ... Then there are SVE 
> > > > registers Z, P, FFR, FPSR and FPCR
> > > I would expect that for each of the Z, P and FFR registers, the 
> > > expression `offset_in_core(reg) - offset_in_gdb_remote(reg)` is always 
> > > the same constant (and is basically equal to `SVE_PT_SVE_ZREG_OFFSET(vq, 
> > > 0) - reg_info[

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+if (IsSVEZ(reg_num))
+  sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+else if (reg_num == GetRegNumFPSR())

omjavaid wrote:
> labath wrote:
> > omjavaid wrote:
> > > labath wrote:
> > > > omjavaid wrote:
> > > > > labath wrote:
> > > > > > I'm confused by this function. If I understant the SVE_PT macros 
> > > > > > and the logic in 
> > > > > > `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` correctly, 
> > > > > > then they both seem to encode the same information. And it seems to 
> > > > > > me that this function should just be `reg_infos[reg_num].offset - 
> > > > > > some_constant`, which is the same thing that we're doing for the 
> > > > > > arm FP registers when SVE is disabled, and also for most other 
> > > > > > architectures too.
> > > > > > 
> > > > > > Why is that not the case? Am I missing something? If they are not 
> > > > > > encoding the same thing, could they be made to encode the same 
> > > > > > thing?
> > > > > This function calculates offset of a particular register in core note 
> > > > > data. SVE data in core dump is similar to what PTrace emits and 
> > > > > offsets into this data is not linear. SVE macros are used to access 
> > > > > those offsets based on register numbers and currently selected vector 
> > > > > length.
> > > > > Also for the purpose of ease we have linear offsets in SVE register 
> > > > > infos and it helps us simplify register data once it makes way to 
> > > > > GDBRemoteRegisterContext on the client side.
> > > > Could you give an example of the non-linearity of the core dump data? 
> > > > (Some registers, and their respective core file and gdb-remote offsets)
> > > In case of core file we create a buffer m_sveregset which stores SVE core 
> > > note information
> > > m_sveregset =
> > >   getRegset(notes, 
> > > m_register_info_up->GetTargetArchitecture().GetTriple(),
> > > AARCH64_SVE_Desc);
> > > 
> > > At this point we do not know what was the vector length and at what 
> > > offsets in the data our registers are located. We read top bytes of size 
> > > use_sve_header and read vector length. Based on this information we 
> > > configure vector length in Register infos. While the SVE payload starts 
> > > with user_sve_header then there are some allignment bytes followed by 
> > > vector length based Z registers followed by P and FFR, then there are 
> > > some more allginment bytes followd by FPCR and FPSR. Macros provided by 
> > > Linux help us jump to the desired offset by providing register number and 
> > > vq into the core note or Ptrace payload.
> > > 
> > > In case of client side storage we store GPRs at linear offset followed by 
> > > Vector Granule register. Then there are SVE registers Z, P, FFR, FPSR and 
> > > FPCR. Offsets of V, D and S registers in FPR regset overlap with 
> > > corresponding first bytes of Z registers and will be same as 
> > > corresponding Z register. While both FP/SVE FPSR share same register 
> > > offset, size and register number.
> > > 
> > > Here is an excerpt from 
> > > https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.rst
> > > 
> > > SVE_PT_REGS_FPSIMD
> > > SVE registers are not live (GETREGSET) or are to be made non-live 
> > > (SETREGSET).
> > > The payload is of type struct user_fpsimd_state, with the same meaning as 
> > > for NT_PRFPREG, starting at offset SVE_PT_FPSIMD_OFFSET from the start of 
> > > user_sve_header.
> > > Extra data might be appended in the future: the size of the payload 
> > > should be obtained using SVE_PT_FPSIMD_SIZE(vq, flags).
> > > vq should be obtained using sve_vq_from_vl(vl).
> > > 
> > > or
> > > 
> > > SVE_PT_REGS_SVE
> > > SVE registers are live (GETREGSET) or are to be made live (SETREGSET).
> > > The payload contains the SVE register data, starting at offset 
> > > SVE_PT_SVE_OFFSET from the start of user_sve_header, and with size 
> > > SVE_PT_SVE_SIZE(vq, flags);
> > Given this
> > > SVE payload starts with ... followed by vector length based Z registers 
> > > followed by P and FFR,
> > and this
> > > In case of client side storage we store GPRs ... Then there are SVE 
> > > registers Z, P, FFR, FPSR and FPCR
> > I would expect that for each of the Z, P and FFR registers, the expression 
> > `offset_in_core(reg) - offset_in_gdb_remote(reg)` is always the same 
> > constant (and is basically equal to `SVE_PT_SVE_ZREG_OFFSET(vq, 0) - 
> > reg_info[Z0].byte_offset`). So we could just add/subtract that constant to 
> > the gdb-remote byte_offset field instead of painstakingly decomposing the 
> > register number only for the linux macros to reconstruct it back again. Is 
> > that

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked an inline comment as done.
omjavaid added inline comments.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+if (IsSVEZ(reg_num))
+  sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+else if (reg_num == GetRegNumFPSR())

labath wrote:
> omjavaid wrote:
> > labath wrote:
> > > omjavaid wrote:
> > > > labath wrote:
> > > > > I'm confused by this function. If I understant the SVE_PT macros and 
> > > > > the logic in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` 
> > > > > correctly, then they both seem to encode the same information. And it 
> > > > > seems to me that this function should just be 
> > > > > `reg_infos[reg_num].offset - some_constant`, which is the same thing 
> > > > > that we're doing for the arm FP registers when SVE is disabled, and 
> > > > > also for most other architectures too.
> > > > > 
> > > > > Why is that not the case? Am I missing something? If they are not 
> > > > > encoding the same thing, could they be made to encode the same thing?
> > > > This function calculates offset of a particular register in core note 
> > > > data. SVE data in core dump is similar to what PTrace emits and offsets 
> > > > into this data is not linear. SVE macros are used to access those 
> > > > offsets based on register numbers and currently selected vector length.
> > > > Also for the purpose of ease we have linear offsets in SVE register 
> > > > infos and it helps us simplify register data once it makes way to 
> > > > GDBRemoteRegisterContext on the client side.
> > > Could you give an example of the non-linearity of the core dump data? 
> > > (Some registers, and their respective core file and gdb-remote offsets)
> > In case of core file we create a buffer m_sveregset which stores SVE core 
> > note information
> > m_sveregset =
> >   getRegset(notes, 
> > m_register_info_up->GetTargetArchitecture().GetTriple(),
> > AARCH64_SVE_Desc);
> > 
> > At this point we do not know what was the vector length and at what offsets 
> > in the data our registers are located. We read top bytes of size 
> > use_sve_header and read vector length. Based on this information we 
> > configure vector length in Register infos. While the SVE payload starts 
> > with user_sve_header then there are some allignment bytes followed by 
> > vector length based Z registers followed by P and FFR, then there are some 
> > more allginment bytes followd by FPCR and FPSR. Macros provided by Linux 
> > help us jump to the desired offset by providing register number and vq into 
> > the core note or Ptrace payload.
> > 
> > In case of client side storage we store GPRs at linear offset followed by 
> > Vector Granule register. Then there are SVE registers Z, P, FFR, FPSR and 
> > FPCR. Offsets of V, D and S registers in FPR regset overlap with 
> > corresponding first bytes of Z registers and will be same as corresponding 
> > Z register. While both FP/SVE FPSR share same register offset, size and 
> > register number.
> > 
> > Here is an excerpt from 
> > https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.rst
> > 
> > SVE_PT_REGS_FPSIMD
> > SVE registers are not live (GETREGSET) or are to be made non-live 
> > (SETREGSET).
> > The payload is of type struct user_fpsimd_state, with the same meaning as 
> > for NT_PRFPREG, starting at offset SVE_PT_FPSIMD_OFFSET from the start of 
> > user_sve_header.
> > Extra data might be appended in the future: the size of the payload should 
> > be obtained using SVE_PT_FPSIMD_SIZE(vq, flags).
> > vq should be obtained using sve_vq_from_vl(vl).
> > 
> > or
> > 
> > SVE_PT_REGS_SVE
> > SVE registers are live (GETREGSET) or are to be made live (SETREGSET).
> > The payload contains the SVE register data, starting at offset 
> > SVE_PT_SVE_OFFSET from the start of user_sve_header, and with size 
> > SVE_PT_SVE_SIZE(vq, flags);
> Given this
> > SVE payload starts with ... followed by vector length based Z registers 
> > followed by P and FFR,
> and this
> > In case of client side storage we store GPRs ... Then there are SVE 
> > registers Z, P, FFR, FPSR and FPCR
> I would expect that for each of the Z, P and FFR registers, the expression 
> `offset_in_core(reg) - offset_in_gdb_remote(reg)` is always the same constant 
> (and is basically equal to `SVE_PT_SVE_ZREG_OFFSET(vq, 0) - 
> reg_info[Z0].byte_offset`). So we could just add/subtract that constant to 
> the gdb-remote byte_offset field instead of painstakingly decomposing the 
> register number only for the linux macros to reconstruct it back again. Is 
> that not so?
The standard never talks about Z, P and FFR being contagious that is what I 
learnt by reading macros. There standard states this:

If the registers are present,

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+if (IsSVEZ(reg_num))
+  sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+else if (reg_num == GetRegNumFPSR())

omjavaid wrote:
> labath wrote:
> > omjavaid wrote:
> > > labath wrote:
> > > > I'm confused by this function. If I understant the SVE_PT macros and 
> > > > the logic in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` 
> > > > correctly, then they both seem to encode the same information. And it 
> > > > seems to me that this function should just be 
> > > > `reg_infos[reg_num].offset - some_constant`, which is the same thing 
> > > > that we're doing for the arm FP registers when SVE is disabled, and 
> > > > also for most other architectures too.
> > > > 
> > > > Why is that not the case? Am I missing something? If they are not 
> > > > encoding the same thing, could they be made to encode the same thing?
> > > This function calculates offset of a particular register in core note 
> > > data. SVE data in core dump is similar to what PTrace emits and offsets 
> > > into this data is not linear. SVE macros are used to access those offsets 
> > > based on register numbers and currently selected vector length.
> > > Also for the purpose of ease we have linear offsets in SVE register infos 
> > > and it helps us simplify register data once it makes way to 
> > > GDBRemoteRegisterContext on the client side.
> > Could you give an example of the non-linearity of the core dump data? (Some 
> > registers, and their respective core file and gdb-remote offsets)
> In case of core file we create a buffer m_sveregset which stores SVE core 
> note information
> m_sveregset =
>   getRegset(notes, 
> m_register_info_up->GetTargetArchitecture().GetTriple(),
> AARCH64_SVE_Desc);
> 
> At this point we do not know what was the vector length and at what offsets 
> in the data our registers are located. We read top bytes of size 
> use_sve_header and read vector length. Based on this information we configure 
> vector length in Register infos. While the SVE payload starts with 
> user_sve_header then there are some allignment bytes followed by vector 
> length based Z registers followed by P and FFR, then there are some more 
> allginment bytes followd by FPCR and FPSR. Macros provided by Linux help us 
> jump to the desired offset by providing register number and vq into the core 
> note or Ptrace payload.
> 
> In case of client side storage we store GPRs at linear offset followed by 
> Vector Granule register. Then there are SVE registers Z, P, FFR, FPSR and 
> FPCR. Offsets of V, D and S registers in FPR regset overlap with 
> corresponding first bytes of Z registers and will be same as corresponding Z 
> register. While both FP/SVE FPSR share same register offset, size and 
> register number.
> 
> Here is an excerpt from 
> https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.rst
> 
> SVE_PT_REGS_FPSIMD
> SVE registers are not live (GETREGSET) or are to be made non-live (SETREGSET).
> The payload is of type struct user_fpsimd_state, with the same meaning as for 
> NT_PRFPREG, starting at offset SVE_PT_FPSIMD_OFFSET from the start of 
> user_sve_header.
> Extra data might be appended in the future: the size of the payload should be 
> obtained using SVE_PT_FPSIMD_SIZE(vq, flags).
> vq should be obtained using sve_vq_from_vl(vl).
> 
> or
> 
> SVE_PT_REGS_SVE
> SVE registers are live (GETREGSET) or are to be made live (SETREGSET).
> The payload contains the SVE register data, starting at offset 
> SVE_PT_SVE_OFFSET from the start of user_sve_header, and with size 
> SVE_PT_SVE_SIZE(vq, flags);
Given this
> SVE payload starts with ... followed by vector length based Z registers 
> followed by P and FFR,
and this
> In case of client side storage we store GPRs ... Then there are SVE registers 
> Z, P, FFR, FPSR and FPCR
I would expect that for each of the Z, P and FFR registers, the expression 
`offset_in_core(reg) - offset_in_gdb_remote(reg)` is always the same constant 
(and is basically equal to `SVE_PT_SVE_ZREG_OFFSET(vq, 0) - 
reg_info[Z0].byte_offset`). So we could just add/subtract that constant to the 
gdb-remote byte_offset field instead of painstakingly decomposing the register 
number only for the linux macros to reconstruct it back again. Is that not so?


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked an inline comment as done.
omjavaid added inline comments.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+if (IsSVEZ(reg_num))
+  sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+else if (reg_num == GetRegNumFPSR())

labath wrote:
> omjavaid wrote:
> > labath wrote:
> > > I'm confused by this function. If I understant the SVE_PT macros and the 
> > > logic in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` 
> > > correctly, then they both seem to encode the same information. And it 
> > > seems to me that this function should just be `reg_infos[reg_num].offset 
> > > - some_constant`, which is the same thing that we're doing for the arm FP 
> > > registers when SVE is disabled, and also for most other architectures too.
> > > 
> > > Why is that not the case? Am I missing something? If they are not 
> > > encoding the same thing, could they be made to encode the same thing?
> > This function calculates offset of a particular register in core note data. 
> > SVE data in core dump is similar to what PTrace emits and offsets into this 
> > data is not linear. SVE macros are used to access those offsets based on 
> > register numbers and currently selected vector length.
> > Also for the purpose of ease we have linear offsets in SVE register infos 
> > and it helps us simplify register data once it makes way to 
> > GDBRemoteRegisterContext on the client side.
> Could you give an example of the non-linearity of the core dump data? (Some 
> registers, and their respective core file and gdb-remote offsets)
In case of core file we create a buffer m_sveregset which stores SVE core note 
information
m_sveregset =
  getRegset(notes, m_register_info_up->GetTargetArchitecture().GetTriple(),
AARCH64_SVE_Desc);

At this point we do not know what was the vector length and at what offsets in 
the data our registers are located. We read top bytes of size use_sve_header 
and read vector length. Based on this information we configure vector length in 
Register infos. While the SVE payload starts with user_sve_header then there 
are some allignment bytes followed by vector length based Z registers followed 
by P and FFR, then there are some more allginment bytes followd by FPCR and 
FPSR. Macros provided by Linux help us jump to the desired offset by providing 
register number and vq into the core note or Ptrace payload.

In case of client side storage we store GPRs at linear offset followed by 
Vector Granule register. Then there are SVE registers Z, P, FFR, FPSR and FPCR. 
Offsets of V, D and S registers in FPR regset overlap with corresponding first 
bytes of Z registers and will be same as corresponding Z register. While both 
FP/SVE FPSR share same register offset, size and register number.

Here is an excerpt from 
https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.rst

SVE_PT_REGS_FPSIMD
SVE registers are not live (GETREGSET) or are to be made non-live (SETREGSET).
The payload is of type struct user_fpsimd_state, with the same meaning as for 
NT_PRFPREG, starting at offset SVE_PT_FPSIMD_OFFSET from the start of 
user_sve_header.
Extra data might be appended in the future: the size of the payload should be 
obtained using SVE_PT_FPSIMD_SIZE(vq, flags).
vq should be obtained using sve_vq_from_vl(vl).

or

SVE_PT_REGS_SVE
SVE registers are live (GETREGSET) or are to be made live (SETREGSET).
The payload contains the SVE register data, starting at offset 
SVE_PT_SVE_OFFSET from the start of user_sve_header, and with size 
SVE_PT_SVE_SIZE(vq, flags);


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+if (IsSVEZ(reg_num))
+  sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+else if (reg_num == GetRegNumFPSR())

omjavaid wrote:
> labath wrote:
> > I'm confused by this function. If I understant the SVE_PT macros and the 
> > logic in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` correctly, 
> > then they both seem to encode the same information. And it seems to me that 
> > this function should just be `reg_infos[reg_num].offset - some_constant`, 
> > which is the same thing that we're doing for the arm FP registers when SVE 
> > is disabled, and also for most other architectures too.
> > 
> > Why is that not the case? Am I missing something? If they are not encoding 
> > the same thing, could they be made to encode the same thing?
> This function calculates offset of a particular register in core note data. 
> SVE data in core dump is similar to what PTrace emits and offsets into this 
> data is not linear. SVE macros are used to access those offsets based on 
> register numbers and currently selected vector length.
> Also for the purpose of ease we have linear offsets in SVE register infos and 
> it helps us simplify register data once it makes way to 
> GDBRemoteRegisterContext on the client side.
Could you give an example of the non-linearity of the core dump data? (Some 
registers, and their respective core file and gdb-remote offsets)


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked 6 inline comments as done.
omjavaid added a comment.

In D77047#2125220 , @labath wrote:

> There's always a chance for ODR violations, particularly with header files 
> defining static objects. This looks better though what I really wanted was to 
> keep the non-sve register info array (`g_register_infos_arm64_le`) completely 
> out of the path of the sve header. Like, by using three headers, one defining 
> `g_register_infos_arm64_le` (and stuff specific to that like the exception 
> registers), one defining the sve array, and the third one which would contain 
> the things common to both. Nonetheless, I think we can now move on to aspects 
> of this patch. Please see my inline comments.


I am working to fix this with D80105  by 
defining dynamic register infos array based on register sets which will be 
requirement for Pointer Authentication and MTE. But I wanted to first get done 
with SVE and come back to register infos refactoring later.




Comment at: 
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:282-290
+  if (!m_per_vq_reg_infos.count(sve_vq)) {
+
+m_per_vq_reg_infos.insert(std::make_pair(
+sve_vq, std::vector(
+g_register_infos_arm64_sve_le,
+g_register_infos_arm64_sve_le + m_register_info_count)));
+

labath wrote:
> I'd probably try relying on the fact that `operator[]` constructs an empty 
> vector, and that the empty vector is not a valid register info value. 
> Something like:
> ```
> std::vector &new_reg_info = m_per_vq_reg_infos[sve_vq];
> if (new_reg_info.empty()) {
>   new_reg_info = llvm::makeArrayRef(g_register_infos_arm64_sve_le, 
> m_register_info_count);
>   ...
> }
> m_register_info_p = new_reg_info.data();
> ```
> 
> That would make this less of a mouthful and also avoid looking up the same 
> key in the map three or four times.
ACK



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:17-22
+enum class SVE_STATE {
+  SVE_STATE_UNKNOWN,
+  SVE_STATE_DISABLED,
+  SVE_STATE_FPSIMD,
+  SVE_STATE_FULL
+};

labath wrote:
> If this is going to be a class enum (which I think is a good idea), then 
> there's no need to repeat the type name in the actual enumerators. It also 
> seems like this could/should follow the lldb naming conventions 
> (SveState::Disabled, etc).
ACK



Comment at: lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h:261
+  k_num_fpr_registers_arm64 = k_last_fpr_arm64 - k_first_fpr_arm64 + 1,
+
+  k_first_sve_arm64 = exc_far_arm64,

labath wrote:
> I guess these changes should be reverted now?
ACK



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+if (IsSVEZ(reg_num))
+  sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+else if (reg_num == GetRegNumFPSR())

labath wrote:
> I'm confused by this function. If I understant the SVE_PT macros and the 
> logic in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` correctly, 
> then they both seem to encode the same information. And it seems to me that 
> this function should just be `reg_infos[reg_num].offset - some_constant`, 
> which is the same thing that we're doing for the arm FP registers when SVE is 
> disabled, and also for most other architectures too.
> 
> Why is that not the case? Am I missing something? If they are not encoding 
> the same thing, could they be made to encode the same thing?
This function calculates offset of a particular register in core note data. SVE 
data in core dump is similar to what PTrace emits and offsets into this data is 
not linear. SVE macros are used to access those offsets based on register 
numbers and currently selected vector length.
Also for the purpose of ease we have linear offsets in SVE register infos and 
it helps us simplify register data once it makes way to 
GDBRemoteRegisterContext on the client side.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17
 
+#include "Plugins/Process/Utility/LinuxPTraceDefines_arm64sve.h"
+

labath wrote:
> Please group the header next to other `Plugins/Process/Utility` header
ACK



Comment at: 
lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py:326-327
 
+#@skipIf(triple='^mips')
+#@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):

labath wrote:
> What's up with these?
Sorry Typo


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

https://reviews.llvm.org/D77047



___
lldb-commits mai

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Some more comments (and questions) from me. Sorry about the back-and-forth. 
It's taking me a while to wrap my head around this, and as I start to 
understand things, new questions arise...




Comment at: 
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:282-290
+  if (!m_per_vq_reg_infos.count(sve_vq)) {
+
+m_per_vq_reg_infos.insert(std::make_pair(
+sve_vq, std::vector(
+g_register_infos_arm64_sve_le,
+g_register_infos_arm64_sve_le + m_register_info_count)));
+

I'd probably try relying on the fact that `operator[]` constructs an empty 
vector, and that the empty vector is not a valid register info value. Something 
like:
```
std::vector &new_reg_info = m_per_vq_reg_infos[sve_vq];
if (new_reg_info.empty()) {
  new_reg_info = llvm::makeArrayRef(g_register_infos_arm64_sve_le, 
m_register_info_count);
  ...
}
m_register_info_p = new_reg_info.data();
```

That would make this less of a mouthful and also avoid looking up the same key 
in the map three or four times.



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:17-22
+enum class SVE_STATE {
+  SVE_STATE_UNKNOWN,
+  SVE_STATE_DISABLED,
+  SVE_STATE_FPSIMD,
+  SVE_STATE_FULL
+};

If this is going to be a class enum (which I think is a good idea), then 
there's no need to repeat the type name in the actual enumerators. It also 
seems like this could/should follow the lldb naming conventions 
(SveState::Disabled, etc).



Comment at: lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h:261
+  k_num_fpr_registers_arm64 = k_last_fpr_arm64 - k_first_fpr_arm64 + 1,
+
+  k_first_sve_arm64 = exc_far_arm64,

I guess these changes should be reverted now?



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+if (IsSVEZ(reg_num))
+  sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+else if (reg_num == GetRegNumFPSR())

I'm confused by this function. If I understant the SVE_PT macros and the logic 
in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` correctly, then they 
both seem to encode the same information. And it seems to me that this function 
should just be `reg_infos[reg_num].offset - some_constant`, which is the same 
thing that we're doing for the arm FP registers when SVE is disabled, and also 
for most other architectures too.

Why is that not the case? Am I missing something? If they are not encoding the 
same thing, could they be made to encode the same thing?



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17
 
+#include "Plugins/Process/Utility/LinuxPTraceDefines_arm64sve.h"
+

Please group the header next to other `Plugins/Process/Utility` header



Comment at: 
lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py:326-327
 
+#@skipIf(triple='^mips')
+#@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):

What's up with these?


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 277701.
omjavaid added a comment.

This update addresses issues raised in last iteration. Also have removed 
dependence on lldb-arm64-register-enums.h.


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

https://reviews.llvm.org/D77047

Files:
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.core

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -269,7 +269,7 @@
 self.dbg.DeleteTarget(target)
 
 @skipIf(triple='^mips')
-@skipIfLLVMTargetMissing("AArch64")
+#@skipIfLLVMTargetMissing("AArch64")
 def test_aarch64_regs(self):
 # check 64 bit ARM core files
 target = self.dbg.CreateTarget(None)
@@ -323,6 +323,47 @@
 
 self.expect("register read --all")
 
+#@skipIf(triple='^mips')
+#@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-sve.core")
+
+values = {}
+values["fp"] = "0xfc1ff4f0"
+values["lr"] = "0x00400170"
+values["sp"] = "0xfc1ff4d0"
+values["pc"] = "0x0040013c"
+values["v0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["v1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["s0"] = "7.5"
+values["s1"] = "11.5"
+values["s2"] = "0"
+values["s3"] = "15.5"
+values["d0"] = "65536.0158538818"
+values["d1"] = "1572864.25476074"
+values["d2"] = "0"
+values["d3"] = "25165828.0917969"
+values["vg"] = "0x0004"
+values["z0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["z1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["z2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["z3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["p0"] = "{0x11 0x11 0x11 0x11}"
+values["p1"] = "{0x11 0x11 0x11 0x11}"
+values["p2"] = "{0x00 0x00 0x00 0x00}"
+values["p3"] = "{0x11 0x11 0x11 0x11}"
+values["p4"] = "{0x00 0x00 0x00 0x00}"
+
+

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I didn't notice this before, but I see now that there's more register number 
overlap in `lldb-arm64-register-enums.h`. Having one overlapping enum is bad 
enough, but two seems really too much? Can we avoid the second enum somehow, at 
least? Perhaps by switching `RegisterContextCorePOSIX_arm64` to use the other 
enum definitions for its work?




Comment at: 
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:256-257
+  uint32_t offset) {
+  // Register info mode denotes SVE vector length in context of AArch64.
+  // Register info mode once set to zero permanently selects default static
+  // AArch64 register info and cannot be changed to SVE. Also if an invalid

This comment looks outdated.



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:280
+
+  m_sve_enabled = true;
+

IIUC, `m_sve_enabled` is only true if m_vector_reg_vq is not zero. Could we 
delete this variable and just make a function to make that comparison?



Comment at: 
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:288-291
+  if (m_per_vq_reg_infos.count(sve_vq)) {
+m_dynamic_register_infos = m_per_vq_reg_infos.at(sve_vq);
+m_register_info_p = &m_dynamic_register_infos[0];
+return m_vector_reg_vq;

It doesn't look like `m_dynamic_register_infos` is needed -- you could just 
make `m_register_info_p` point directly into the map.



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:91
 
+  uint32_t ConfigureVectorRegisterInfos(uint32_t mode, uint32_t offset = 0);
+

The `offset` argument is not set anywhere.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17
 
+#ifndef SVE_PT_REGS_SVE
+#define INCLUDE_LINUX_PTRACE_DEFINITIONS_FOR_SVE_ARM64

I guess this does not make sense now that the header is standalone


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 276963.
omjavaid added a comment.

This update incorporates suggestion from last iteration and also rebases after 
merger to registersets and register infos in RegisterInfosPOSIX_arm64.
Also I have posted a separate patch D83541  to 
make lldb/source/Plugins/Process/Linux/LinuxPTraceDefines_arm64sve.h platform 
independent in order to use them with elf-core.

@labath what do you think ?


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

https://reviews.llvm.org/D77047

Files:
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.core

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -269,7 +269,7 @@
 self.dbg.DeleteTarget(target)
 
 @skipIf(triple='^mips')
-@skipIfLLVMTargetMissing("AArch64")
+#@skipIfLLVMTargetMissing("AArch64")
 def test_aarch64_regs(self):
 # check 64 bit ARM core files
 target = self.dbg.CreateTarget(None)
@@ -323,6 +323,47 @@
 
 self.expect("register read --all")
 
+#@skipIf(triple='^mips')
+#@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-sve.core")
+
+values = {}
+values["fp"] = "0xfc1ff4f0"
+values["lr"] = "0x00400170"
+values["sp"] = "0xfc1ff4d0"
+values["pc"] = "0x0040013c"
+values["v0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["v1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["s0"] = "7.5"
+values["s1"] = "11.5"
+values["s2"] = "0"
+values["s3"] = "15.5"
+values["d0"] = "65536.0158538818"
+values["d1"] = "1572864.25476074"
+values["d2"] = "0"
+values["d3"] = "25165828.0917969"
+values["vg"] = "0x0004"
+values["z0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["z1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["z2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["z3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added a comment.

Thanks. This looks is starting to look good now. I didn't get a chance to 
review all of the changes, but here's a few more things that I noticed.




Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:72-76
+  uint32_t SetRegisterInfoMode(uint32_t mode, uint32_t offset = 0);
+
+  uint32_t GetRegisterInfoMode() const;
+
+  bool RegisterInfoModeIsValid(uint32_t mode) {

Now that this is arm-specific, I am wondering if there isn't a better name for 
this than "register info 'mode'". Maybe it could be just "enable/disable SVE" ? 
Specifically, if the validity of the "mode" is checked by the caller, then we 
can avoid this `RegisterInfoModeIsValid` business...



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:72-89
+if (IsSVEZReg(reg_num))
+  return (reg_num - sve_z0_arm64) * 16;
+else if (reg_num == fpu_fpsr_arm64)
+  return 32 * 16;
+else if (reg_num == fpu_fpcr_arm64)
+  return (32 * 16) + 4;
+  } else if (m_sve_state == SVE_STATE::SVE_STATE_FULL) {

omjavaid wrote:
> labath wrote:
> > no else after return
> Ack.
Well.. that's one way of handing that.. :) It's not necessarily bad, but I was 
imagining you would just delete all the "else"s...



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:21
 RegisterContextCorePOSIX_arm64::RegisterContextCorePOSIX_arm64(
 Thread &thread, RegisterInfoInterface *register_info,
 const DataExtractor &gpregset, llvm::ArrayRef notes)

It would be also nice if this constructor accepted a `RegisterInfoPOSIX_arm64` 
instead of plain `RegisterInfoInterface`. That would make it much harder to 
misuse this class. For that we'd need to slightly refactor the construction 
code in ThreadElfCore. For example, we could swap the order of the "os" and 
"machine" switches and then join the two "machine" switches that construct 
RegisterInfoInterfaces and RegisterContexts.

Maybe you could do that as a separate patch?



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17-22
+#include 
+
+#ifndef SVE_PT_REGS_SVE
+#define INCLUDE_LINUX_PTRACE_DEFINITIONS_FOR_SVE_ARM64
+#include "Plugins/Process/Linux/LinuxPTraceDefines_arm64sve.h"
+#endif

This is including platform-specific headers, but elf cores should be readable 
from any host. I guess you should just re-define the relevant constants 
somewhere. (I'm not sure what they are.)



Comment at: lldb/source/Utility/ARM64_DWARF_Registers.h:54-146
+  // 34-45 reserved
+
+  // 64-bit SVE Vector granule pseudo register
+  vg = 46,
+
+  // VG ́8-bit SVE first fault register
+  ffr = 47,

All of these numbers are from the official spec, right? Maybe you could just 
commit this straight away so it does not clutter this review.


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 275037.
omjavaid added a comment.

@labath I have updated diff after resolving issues highlighted. Also I have 
removed dependence on generic interfaces.


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

https://reviews.llvm.org/D77047

Files:
  lldb/source/Plugins/Process/Linux/LinuxPTraceDefines_arm64sve.h
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/source/Utility/ARM64_DWARF_Registers.h
  lldb/source/Utility/ARM64_ehframe_Registers.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.core

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -269,7 +269,7 @@
 self.dbg.DeleteTarget(target)
 
 @skipIf(triple='^mips')
-@skipIfLLVMTargetMissing("AArch64")
+#@skipIfLLVMTargetMissing("AArch64")
 def test_aarch64_regs(self):
 # check 64 bit ARM core files
 target = self.dbg.CreateTarget(None)
@@ -323,6 +323,47 @@
 
 self.expect("register read --all")
 
+#@skipIf(triple='^mips')
+#@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-sve.core")
+
+values = {}
+values["fp"] = "0xfc1ff4f0"
+values["lr"] = "0x00400170"
+values["sp"] = "0xfc1ff4d0"
+values["pc"] = "0x0040013c"
+values["v0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["v1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["s0"] = "7.5"
+values["s1"] = "11.5"
+values["s2"] = "0"
+values["s3"] = "15.5"
+values["d0"] = "65536.0158538818"
+values["d1"] = "1572864.25476074"
+values["d2"] = "0"
+values["d3"] = "25165828.0917969"
+values["vg"] = "0x0004"
+values["z0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["z1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["z2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["z3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["p0"] = "{0x11 0x11 0x11 0x11}"
+values["p1"] = "{0x11 0x11 0x11 

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked 6 inline comments as done.
omjavaid added a comment.

In D77047#2125220 , @labath wrote:

> There's always a chance for ODR violations, particularly with header files 
> defining static objects. This looks better though what I really wanted was to 
> keep the non-sve register info array (`g_register_infos_arm64_le`) completely 
> out of the path of the sve header. Like, by using three headers, one defining 
> `g_register_infos_arm64_le` (and stuff specific to that like the exception 
> registers), one defining the sve array, and the third one which would contain 
> the things common to both. Nonetheless, I think we can now move on to aspects 
> of this patch. Please see my inline comments.


I am working to fix this with D80105  by 
defining dynamic register infos array based on register sets which will be 
requirement for Pointer Authentication and MTE. But I wanted to first get done 
with SVE and come back to register infos refactoring later.




Comment at: 
lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h:38-40
+  uint32_t SetRegisterInfoMode(uint32_t mode) {
+return m_register_info_interface_up->SetRegisterInfoMode(mode);
+  }

labath wrote:
> labath wrote:
> > I can't find any callers of this function. Who is calling it?
> It still seems to me that this function is not used (in this patch at least).
Ack. This function is used by chiild revisions which adds ptrace support. I ll 
move it there.



Comment at: 
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp:261
+return set_index < k_num_register_sets;
+  else
+return set_index < (k_num_register_sets - 1);

labath wrote:
> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
Ack.



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoInterface.h:64-68
+  virtual uint32_t SetRegisterInfoMode(uint32_t mode, uint32_t offset = 0) {
+return 0;
+  }
+
+  virtual uint32_t GetRegisterInfoMode() const { return 0; }

labath wrote:
> I don't believe these functions should be present on the generic interface. 
> The only caller and the only implementation of them is already 
> arm64-specific, so it should be possible to arrange it such that they don't 
> need to go through the base class interface. For instance the 
> `RegisterContextCorePOSIX_arm64` constructor could accept an 
> `RegisterInfoPOSIX_arm64` instance to indicate what it needs to work with, 
> and then downcast `m_register_info_up` (via a helper function?) when it needs 
> to call these. Solutions without downcasting are possible too, though may 
> require a bit more refactoring.
Agreed. I ll make an arrangement for this in updated rev.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:72-89
+if (IsSVEZReg(reg_num))
+  return (reg_num - sve_z0_arm64) * 16;
+else if (reg_num == fpu_fpsr_arm64)
+  return 32 * 16;
+else if (reg_num == fpu_fpcr_arm64)
+  return (32 * 16) + 4;
+  } else if (m_sve_state == SVE_STATE::SVE_STATE_FULL) {

labath wrote:
> no else after return
Ack.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:53-55
+  void ConfigureRegisterContext();
+
+  uint32_t CalculateSVEOffset(uint32_t reg_num) const;

labath wrote:
> Do these need to be public? If so, why?
These should be private. I ll update this in updated rev.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:76
+
+  mutable SVE_STATE m_sve_state;
+  struct user_sve_header m_sve_header;

labath wrote:
> Why is this mutable?
It was pulled in from NativeRegisterContextLinux_arm64 and got set mutable by 
mistake I ll correct in updated rev.


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

There's always a chance for ODR violations, particularly with header files 
defining static objects. This looks better though what I really wanted was to 
keep the non-sve register info array (`g_register_infos_arm64_le`) completely 
out of the path of the sve header. Like, by using three headers, one defining 
`g_register_infos_arm64_le` (and stuff specific to that like the exception 
registers), one defining the sve array, and the third one which would contain 
the things common to both. Nonetheless, I think we can now move on to aspects 
of this patch. Please see my inline comments.




Comment at: 
lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h:38-40
+  uint32_t SetRegisterInfoMode(uint32_t mode) {
+return m_register_info_interface_up->SetRegisterInfoMode(mode);
+  }

labath wrote:
> I can't find any callers of this function. Who is calling it?
It still seems to me that this function is not used (in this patch at least).



Comment at: 
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp:261
+return set_index < k_num_register_sets;
+  else
+return set_index < (k_num_register_sets - 1);

http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoInterface.h:64-68
+  virtual uint32_t SetRegisterInfoMode(uint32_t mode, uint32_t offset = 0) {
+return 0;
+  }
+
+  virtual uint32_t GetRegisterInfoMode() const { return 0; }

I don't believe these functions should be present on the generic interface. The 
only caller and the only implementation of them is already arm64-specific, so 
it should be possible to arrange it such that they don't need to go through the 
base class interface. For instance the `RegisterContextCorePOSIX_arm64` 
constructor could accept an `RegisterInfoPOSIX_arm64` instance to indicate what 
it needs to work with, and then downcast `m_register_info_up` (via a helper 
function?) when it needs to call these. Solutions without downcasting are 
possible too, though may require a bit more refactoring.



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:72-89
+if (IsSVEZReg(reg_num))
+  return (reg_num - sve_z0_arm64) * 16;
+else if (reg_num == fpu_fpsr_arm64)
+  return 32 * 16;
+else if (reg_num == fpu_fpcr_arm64)
+  return (32 * 16) + 4;
+  } else if (m_sve_state == SVE_STATE::SVE_STATE_FULL) {

no else after return



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:53-55
+  void ConfigureRegisterContext();
+
+  uint32_t CalculateSVEOffset(uint32_t reg_num) const;

Do these need to be public? If so, why?



Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:76
+
+  mutable SVE_STATE m_sve_state;
+  struct user_sve_header m_sve_header;

Why is this mutable?


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 274390.
omjavaid added a comment.

In this updated I have removed overlapping parts of RegisterInfos_arm64.h and 
RegisterInfos_arm64_sve.h which in turn removes any possibility of duplicate 
definitions.

Both register infos define separate static register info arrays namely 
g_register_infos_arm64_le and g_register_infos_arm64_sve_le. Definition of FPR 
registers is also different and their invalidate/contains reg lists are also 
different. For the context of all native register context which do not yet 
support SVE g_register_infos_arm64_le will be used as they only include 
RegisterInfos_arm64.h.

@labath do you think there is still chance of any ODR violations?


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

https://reviews.llvm.org/D77047

Files:
  lldb/include/lldb/Target/RegisterContext.h
  lldb/source/Plugins/Process/Linux/LinuxPTraceDefines_arm64sve.h
  lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoInterface.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/source/Utility/ARM64_DWARF_Registers.h
  lldb/source/Utility/ARM64_ehframe_Registers.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.core

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -269,7 +269,7 @@
 self.dbg.DeleteTarget(target)
 
 @skipIf(triple='^mips')
-@skipIfLLVMTargetMissing("AArch64")
+#@skipIfLLVMTargetMissing("AArch64")
 def test_aarch64_regs(self):
 # check 64 bit ARM core files
 target = self.dbg.CreateTarget(None)
@@ -323,6 +323,47 @@
 
 self.expect("register read --all")
 
+#@skipIf(triple='^mips')
+#@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-sve.core")
+
+values = {}
+values["fp"] = "0xfc1ff4f0"
+values["lr"] = "0x00400170"
+values["sp"] = "0xfc1ff4d0"
+values["pc"] = "0x0040013c"
+values["v0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["v1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["s0"] = "7.5"
+values["s1"] = "11.5"
+values["s2"] = "0"
+values["s3"] = "15.5"
+values["d0"] = "65536.0158538818"
+values["d1"] = "1572864.25476074"
+values["d2"] = "0"
+values["d3"] = "25165828.0917969"
+values["vg"] = "0x0004"
+values["z0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm worried about the confusing relationship between 
`RegisterInfos_arm64_sve.h` and `RegisterInfos_arm64.h` and the overlap in the 
register numbers it enforces. when we discussed this last time, my 
understanding was that the sve registers could be inserted _before_ the debug 
registers (similar to what happened with D24255 
). Now I see that may not be possible because 
some of these registers are used in the darwin arm64 register context. However, 
that makes the whole idea much less appealing.

Would it be possible to organize these headers like this:

- create a new header which holds common register definitions: this would be 
register numbers for the gpr and fpr registers (and their respective 
`_contains` arrays).
- have `RegisterInfos_arm64_sve.h` include this header and provide additional 
register definitions, and define its RegisterInfo array to match
- have `RegisterInfos_arm64.h` to the same

What I am hoping to achieve this way is to remove the 
`DECLARE_REGISTER_INFOS_ARM64_SVE_STRUCT` condition macro (dragons (ODR 
violations) lie there), and the duplicate enum values associated with it.


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 271532.
omjavaid added a comment.

This update remove skipping over register infos requirement by overlapping sve 
register numbers with debug register numbers which are not required by Linux 
register context.


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

https://reviews.llvm.org/D77047

Files:
  lldb/include/lldb/Target/RegisterContext.h
  lldb/source/Plugins/Process/Linux/LinuxPTraceDefines_arm64sve.h
  lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoInterface.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/source/Utility/ARM64_DWARF_Registers.h
  lldb/source/Utility/ARM64_ehframe_Registers.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.core

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -269,7 +269,7 @@
 self.dbg.DeleteTarget(target)
 
 @skipIf(triple='^mips')
-@skipIfLLVMTargetMissing("AArch64")
+#@skipIfLLVMTargetMissing("AArch64")
 def test_aarch64_regs(self):
 # check 64 bit ARM core files
 target = self.dbg.CreateTarget(None)
@@ -323,6 +323,47 @@
 
 self.expect("register read --all")
 
+#@skipIf(triple='^mips')
+#@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-sve.core")
+
+values = {}
+values["fp"] = "0xfc1ff4f0"
+values["lr"] = "0x00400170"
+values["sp"] = "0xfc1ff4d0"
+values["pc"] = "0x0040013c"
+values["v0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["v1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["s0"] = "7.5"
+values["s1"] = "11.5"
+values["s2"] = "0"
+values["s3"] = "15.5"
+values["d0"] = "65536.0158538818"
+values["d1"] = "1572864.25476074"
+values["d2"] = "0"
+values["d3"] = "25165828.0917969"
+values["vg"] = "0x0004"
+values["z0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["z1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["z2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 263934.
omjavaid added a comment.

New update contains minor update needed to accommodate SVE PTrace macros header 
from ARM.


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

https://reviews.llvm.org/D77047

Files:
  lldb/include/lldb/Target/RegisterContext.h
  lldb/source/Plugins/Process/Linux/LinuxPTraceDefines_arm64sve.h
  lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoInterface.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h
  lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/source/Utility/ARM64_DWARF_Registers.h
  lldb/source/Utility/ARM64_ehframe_Registers.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.core

Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-sve.c
@@ -0,0 +1,24 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("ptrue p0.s\n\t");
+  asm volatile("fcpy  z0.s, p0/m, #7.5\n\t");
+  asm volatile("ptrue p1.s\n\t");
+  asm volatile("fcpy  z1.s, p1/m, #11.5\n\t");
+  asm volatile("ptrue p3.s\n\t");
+  asm volatile("fcpy  z3.s, p3/m, #15.5\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -311,6 +311,47 @@
 
 self.expect("register read --all")
 
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_sve_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-sve.core")
+
+values = {}
+values["fp"] = "0xfc1ff4f0"
+values["lr"] = "0x00400170"
+values["sp"] = "0xfc1ff4d0"
+values["pc"] = "0x0040013c"
+values["v0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["v1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["s0"] = "7.5"
+values["s1"] = "11.5"
+values["s2"] = "0"
+values["s3"] = "15.5"
+values["d0"] = "65536.0158538818"
+values["d1"] = "1572864.25476074"
+values["d2"] = "0"
+values["d3"] = "25165828.0917969"
+values["vg"] = "0x0004"
+values["z0"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40 0x00 0x00 0xf0 0x40}"
+values["z1"] = "{0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41 0x00 0x00 0x38 0x41}"
+values["z2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["z3"] = "{0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41 0x00 0x00 0x78 0x41}"
+values["p0"] = "{0x11 0x11 0x11 0x11}"
+values["p1"] = "{0x11 0x11 0x11 0x11}"
+values["p2"] = "{0x00 0x00 0x00 0x00}"
+values["p3"] = "{

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

(sorry about all the editing -- I'm trying to figure out the order of these 
patches)


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

https://reviews.llvm.org/D77047



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits