[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7fa7b81bcbd0: Combine multiple defs of arm64 register sets 
(authored by omjavaid).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D80105?vs=274382&id=275668#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80105

Files:
  lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
  
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.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/RegisterInfoAndSetInterface.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.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/ThreadElfCore.cpp

Index: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -82,7 +82,6 @@
 case llvm::Triple::FreeBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -111,7 +110,6 @@
 case llvm::Triple::NetBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::x86_64:
 reg_interface = new RegisterContextNetBSD_x86_64(arch);
@@ -128,7 +126,6 @@
 reg_interface = new RegisterInfoPOSIX_arm(arch);
 break;
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::mipsel:
   case llvm::Triple::mips:
@@ -159,7 +156,6 @@
 case llvm::Triple::OpenBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -180,7 +176,7 @@
   break;
 }
 
-if (!reg_interface) {
+if (!reg_interface && arch.GetMachine() != llvm::Triple::aarch64) {
   LLDB_LOGF(log, "elf-core::%s:: Architecture(%d) or OS(%d) not supported",
 __FUNCTION__, arch.GetMachine(), arch.GetTriple().getOS());
   assert(false && "Architecture or OS not supported");
@@ -189,7 +185,8 @@
 switch (arch.GetMachine()) {
 case llvm::Triple::aarch64:
   m_thread_reg_ctx_sp = std::make_shared(
-  *this, reg_interface, m_gpregset_data, m_notes);
+  *this, std::make_unique(arch),
+  m_gpregset_data, m_notes);
   break;
 case llvm::Triple::arm:
   m_thread_reg_ctx_sp = std::make_shared(
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
@@ -18,7 +18,7 @@
 public:
   RegisterContextCorePOSIX_arm64(
   lldb_private::Thread &thread,
-  lldb_private::RegisterInfoInterface *register_info,
+  std::unique_ptr register_info,
   const lldb_private::DataExtractor &gpregset,
   llvm::ArrayRef notes);
 
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -17,16 +17,16 @@
 using namespace lldb_private;
 
 RegisterContextCorePOSIX_arm64::RegisterContextCorePOSIX_arm64(
-Thread &thread, RegisterInfoInterface *register_info,
+Thread &thread, std::unique_ptr register_info,
 const DataExtractor &gpregset, llvm::ArrayRef notes)
-: RegisterContextPOSIX_arm64(thread, 0, register_info) {
+: RegisterContextPOSIX_arm64(thread, std::move(register_info)) {
   m_gpr_buffer = std:

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7fa7b81bcbd0: Combine multiple defs of arm64 register sets 
(authored by omjavaid).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80105

Files:
  lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
  
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.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/RegisterInfoAndSetInterface.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.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/ThreadElfCore.cpp

Index: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -82,7 +82,6 @@
 case llvm::Triple::FreeBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -111,7 +110,6 @@
 case llvm::Triple::NetBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::x86_64:
 reg_interface = new RegisterContextNetBSD_x86_64(arch);
@@ -128,7 +126,6 @@
 reg_interface = new RegisterInfoPOSIX_arm(arch);
 break;
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::mipsel:
   case llvm::Triple::mips:
@@ -159,7 +156,6 @@
 case llvm::Triple::OpenBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -180,7 +176,7 @@
   break;
 }
 
-if (!reg_interface) {
+if (!reg_interface && arch.GetMachine() != llvm::Triple::aarch64) {
   LLDB_LOGF(log, "elf-core::%s:: Architecture(%d) or OS(%d) not supported",
 __FUNCTION__, arch.GetMachine(), arch.GetTriple().getOS());
   assert(false && "Architecture or OS not supported");
@@ -189,7 +185,8 @@
 switch (arch.GetMachine()) {
 case llvm::Triple::aarch64:
   m_thread_reg_ctx_sp = std::make_shared(
-  *this, reg_interface, m_gpregset_data, m_notes);
+  *this, std::make_unique(arch),
+  m_gpregset_data, m_notes);
   break;
 case llvm::Triple::arm:
   m_thread_reg_ctx_sp = std::make_shared(
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
@@ -18,7 +18,7 @@
 public:
   RegisterContextCorePOSIX_arm64(
   lldb_private::Thread &thread,
-  lldb_private::RegisterInfoInterface *register_info,
+  std::unique_ptr register_info,
   const lldb_private::DataExtractor &gpregset,
   llvm::ArrayRef notes);
 
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -17,16 +17,16 @@
 using namespace lldb_private;
 
 RegisterContextCorePOSIX_arm64::RegisterContextCorePOSIX_arm64(
-Thread &thread, RegisterInfoInterface *register_info,
+Thread &thread, std::unique_ptr register_info,
 const DataExtractor &gpregset, llvm::ArrayRef notes)
-: RegisterContextPOSIX_arm64(thread, 0, register_info) {
+: RegisterContextPOSIX_arm64(thread, std::move(register_info)) {
   m_gpr_buffer = std::make_shared(gpregset.GetDataStart(),
   gpregset.GetByteSize());
   m_gpr.SetData(m_gpr_buffer);
   m_gpr.SetByteOrder

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Pavel Labath via Phabricator via lldb-commits]

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

ship it.


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

https://reviews.llvm.org/D80105



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

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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

This revision fixed issues highlighted in last review.

LGTM?


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

https://reviews.llvm.org/D80105

Files:
  lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
  
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.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/RegisterInfoAndSetInterface.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.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/ThreadElfCore.cpp

Index: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -82,7 +82,6 @@
 case llvm::Triple::FreeBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -111,7 +110,6 @@
 case llvm::Triple::NetBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::x86_64:
 reg_interface = new RegisterContextNetBSD_x86_64(arch);
@@ -128,7 +126,6 @@
 reg_interface = new RegisterInfoPOSIX_arm(arch);
 break;
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::mipsel:
   case llvm::Triple::mips:
@@ -159,7 +156,6 @@
 case llvm::Triple::OpenBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -180,7 +176,7 @@
   break;
 }
 
-if (!reg_interface) {
+if (!reg_interface && arch.GetMachine() != llvm::Triple::aarch64) {
   LLDB_LOGF(log, "elf-core::%s:: Architecture(%d) or OS(%d) not supported",
 __FUNCTION__, arch.GetMachine(), arch.GetTriple().getOS());
   assert(false && "Architecture or OS not supported");
@@ -189,7 +185,8 @@
 switch (arch.GetMachine()) {
 case llvm::Triple::aarch64:
   m_thread_reg_ctx_sp = std::make_shared(
-  *this, reg_interface, m_gpregset_data, m_notes);
+  *this, std::make_unique(arch),
+  m_gpregset_data, m_notes);
   break;
 case llvm::Triple::arm:
   m_thread_reg_ctx_sp = std::make_shared(
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
@@ -18,7 +18,7 @@
 public:
   RegisterContextCorePOSIX_arm64(
   lldb_private::Thread &thread,
-  lldb_private::RegisterInfoInterface *register_info,
+  std::unique_ptr register_info,
   const lldb_private::DataExtractor &gpregset,
   llvm::ArrayRef notes);
 
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -17,16 +17,16 @@
 using namespace lldb_private;
 
 RegisterContextCorePOSIX_arm64::RegisterContextCorePOSIX_arm64(
-Thread &thread, RegisterInfoInterface *register_info,
+Thread &thread, std::unique_ptr register_info,
 const DataExtractor &gpregset, llvm::ArrayRef notes)
-: RegisterContextPOSIX_arm64(thread, 0, register_info) {
+: RegisterContextPOSIX_arm64(thread, std::move(register_info)) {
   m_gpr_buffer = std::make_shared(gpregset.GetDataStart(),
   gpregset.GetByteSize());
   m_gpr.SetData(m_gpr_buffer);
   m_gpr.SetByteOrder(gpregset.GetByteOrder());
 
   m_fpregset = getRegset(
-  notes, register_info->GetTargetArchitecture().

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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



Comment at: 
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp:46
+: lldb_private::RegisterContext(thread, 0) {
+  m_register_info_up = std::move(register_info);
 

labath wrote:
> move this to the initializer list
ACK.



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:85
+gpr_w22, gpr_w23,  gpr_w24, gpr_w25, gpr_w26, gpr_w27, gpr_w28,
+LLDB_INVALID_REGNUM // register sets need to end with this flag
+};

labath wrote:
> I'd probably just delete this comment (or merge it with the leading comment 
> above the array definition), and then let clang-format lay this out 
> normally...
Let me check what clang-format emits.



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:19
 public:
+  enum { ARM64GPR = 0, ARM64FPR };
+

labath wrote:
> Why these names? I think [GF]PRegSet would be better for two reasons:
> - the same names with the same purpose already exist in 
> `NativeRegisterContextNetBSD_x86_64.h`
> - it seems like a better way to differentiate from the [GF]PR structs below 
> than adding a redundant ARM64 prefix.
Indeed ARM64 is redundant now that we have these enums in 
RegisterInfosPOSIX_arm64 . I will fix this in updated revision.


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

https://reviews.llvm.org/D80105



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

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Pavel Labath via Phabricator via lldb-commits]

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

This looks great. I just have a quick question about the GPR vs GPRegSet 
thingy...




Comment at: lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp:167
 assert(target_arch.GetTriple().getOS() == llvm::Triple::FreeBSD);
 switch (target_arch.GetMachine()) {
 case llvm::Triple::aarch64:

omjavaid wrote:
> labath wrote:
> > It would be good to merge these two switches. Then the reg(set)_interface 
> > variables would be local to each case label and it would not be so weird 
> > that we sometimes use one and sometimes the other.
> I have tried a couple of options but the no of different combinations 
> involved I feel current implementation should stay untill we incrementally 
> move remaining architectures to user RegisterInfosAndSet interface.
I like this solution.



Comment at: 
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp:46
+: lldb_private::RegisterContext(thread, 0) {
+  m_register_info_up = std::move(register_info);
 

move this to the initializer list



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:85
+gpr_w22, gpr_w23,  gpr_w24, gpr_w25, gpr_w26, gpr_w27, gpr_w28,
+LLDB_INVALID_REGNUM // register sets need to end with this flag
+};

I'd probably just delete this comment (or merge it with the leading comment 
above the array definition), and then let clang-format lay this out normally...



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:19
 public:
+  enum { ARM64GPR = 0, ARM64FPR };
+

Why these names? I think [GF]PRegSet would be better for two reasons:
- the same names with the same purpose already exist in 
`NativeRegisterContextNetBSD_x86_64.h`
- it seems like a better way to differentiate from the [GF]PR structs below 
than adding a redundant ARM64 prefix.


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

https://reviews.llvm.org/D80105



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

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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

This new revision incorporates all suggestions on previous version.

@labath What do you think? I have skipped merging switch statements and have 
picked all other suggestions you highlighted. If this is LGTM then i can rebase 
SVE patches on top of this?


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

https://reviews.llvm.org/D80105

Files:
  lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
  
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.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/RegisterInfoAndSetInterface.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.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/ThreadElfCore.cpp

Index: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -82,7 +82,6 @@
 case llvm::Triple::FreeBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -111,7 +110,6 @@
 case llvm::Triple::NetBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::x86_64:
 reg_interface = new RegisterContextNetBSD_x86_64(arch);
@@ -128,7 +126,6 @@
 reg_interface = new RegisterInfoPOSIX_arm(arch);
 break;
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::mipsel:
   case llvm::Triple::mips:
@@ -159,7 +156,6 @@
 case llvm::Triple::OpenBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -180,7 +176,7 @@
   break;
 }
 
-if (!reg_interface) {
+if (!reg_interface && arch.GetMachine() != llvm::Triple::aarch64) {
   LLDB_LOGF(log, "elf-core::%s:: Architecture(%d) or OS(%d) not supported",
 __FUNCTION__, arch.GetMachine(), arch.GetTriple().getOS());
   assert(false && "Architecture or OS not supported");
@@ -189,7 +185,8 @@
 switch (arch.GetMachine()) {
 case llvm::Triple::aarch64:
   m_thread_reg_ctx_sp = std::make_shared(
-  *this, reg_interface, m_gpregset_data, m_notes);
+  *this, std::make_unique(arch),
+  m_gpregset_data, m_notes);
   break;
 case llvm::Triple::arm:
   m_thread_reg_ctx_sp = std::make_shared(
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
@@ -18,7 +18,7 @@
 public:
   RegisterContextCorePOSIX_arm64(
   lldb_private::Thread &thread,
-  lldb_private::RegisterInfoInterface *register_info,
+  std::unique_ptr register_info,
   const lldb_private::DataExtractor &gpregset,
   llvm::ArrayRef notes);
 
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -17,16 +17,16 @@
 using namespace lldb_private;
 
 RegisterContextCorePOSIX_arm64::RegisterContextCorePOSIX_arm64(
-Thread &thread, RegisterInfoInterface *register_info,
+Thread &thread, std::unique_ptr register_info,
 const DataExtractor &gpregset, llvm::ArrayRef notes)
-: RegisterContextPOSIX_arm64(thread, 0, register_info) {
+: RegisterContextPOSIX_arm64(thread, std::move(register_info)) {
   m_gpr_buffer = std::make_shared(gpregset.GetDataStart(),

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp:167
 assert(target_arch.GetTriple().getOS() == llvm::Triple::FreeBSD);
 switch (target_arch.GetMachine()) {
 case llvm::Triple::aarch64:

labath wrote:
> It would be good to merge these two switches. Then the reg(set)_interface 
> variables would be local to each case label and it would not be so weird that 
> we sometimes use one and sometimes the other.
I have tried a couple of options but the no of different combinations involved 
I feel current implementation should stay untill we incrementally move 
remaining architectures to user RegisterInfosAndSet interface.



Comment at: 
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h:21
   lldb_private::Thread &thread, uint32_t concrete_frame_idx,
-  lldb_private::RegisterInfoInterface *register_info);
+  lldb_private::RegisterInfoAndSetInterface *register_info);
 

labath wrote:
> While we're changing interfaces, let's also make this 
> unique_ptr to document the ownership transfer. 
> (I might also drop the concrete_frame_idx argument, as that is always zero.)
Agreed. I will update this in updated revision.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:71-72
+
+  m_regset_interface_up = 
std::static_pointer_cast(
+  m_register_info_interface_up);
 }

labath wrote:
> The way I'd recommend dealing with this is to have a 
> `RegisterInfoPOSIX_arm64& GetRegisterInfo() const` method which performs a 
> cast on the `m_register_info_interface_up` pointer, and then have everything 
> call that. If you place that method in close proximity to this constructor, 
> then it should be clear that this operation is always safe. There's already 
> something similar being done in e.g. `NativeThreadLinux::GetProcess`.
yes this would be a lot cleaner than what I am doing currently. I am going to 
update this in next revision.



Comment at: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp:82
 
 switch (arch.GetTriple().getOS()) {
 case llvm::Triple::FreeBSD: {

labath wrote:
> The reg(set)_interface and register_context switches could be merged here too 
> in a similar way...
Here also incremental merger will be a better approach IMO.


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

https://reviews.llvm.org/D80105



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

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like this.




Comment at: lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp:167
 assert(target_arch.GetTriple().getOS() == llvm::Triple::FreeBSD);
 switch (target_arch.GetMachine()) {
 case llvm::Triple::aarch64:

It would be good to merge these two switches. Then the reg(set)_interface 
variables would be local to each case label and it would not be so weird that 
we sometimes use one and sometimes the other.



Comment at: 
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h:21
   lldb_private::Thread &thread, uint32_t concrete_frame_idx,
-  lldb_private::RegisterInfoInterface *register_info);
+  lldb_private::RegisterInfoAndSetInterface *register_info);
 

While we're changing interfaces, let's also make this 
unique_ptr to document the ownership transfer. (I 
might also drop the concrete_frame_idx argument, as that is always zero.)



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:71-72
+
+  m_regset_interface_up = 
std::static_pointer_cast(
+  m_register_info_interface_up);
 }

The way I'd recommend dealing with this is to have a `RegisterInfoPOSIX_arm64& 
GetRegisterInfo() const` method which performs a cast on the 
`m_register_info_interface_up` pointer, and then have everything call that. If 
you place that method in close proximity to this constructor, then it should be 
clear that this operation is always safe. There's already something similar 
being done in e.g. `NativeThreadLinux::GetProcess`.



Comment at: 
lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h:36
 
-private:
-  std::unique_ptr m_register_info_interface_up;
+  std::shared_ptr m_register_info_interface_up;
 };

With the above idea, it should no longer be needed to change this into a 
shared_ptr.



Comment at: 
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h:23-24
   RegisterContextPOSIX_arm64(
   lldb_private::Thread &thread, uint32_t concrete_frame_idx,
-  lldb_private::RegisterInfoInterface *register_info);
+  lldb_private::RegisterInfoAndSetInterface *register_info);
 

similar thoughts about unique_ptr and concrete_frame_idx. In fact, thinking 
ahead to the next patch, I think we could make this an 
`unique_ptr` even.



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoAndSetInterface.h:24
+  : RegisterInfoInterface(target_arch) {}
+  virtual ~RegisterInfoAndSetInterface() {}
+

s/virtual/override (or just omit the destructor completely)



Comment at: 
lldb/source/Plugins/Process/Utility/RegisterInfoAndSetInterface.h:35-37
+  virtual uint32_t GetRegisterOffset(uint32_t reg_index) const = 0;
+
+  virtual uint32_t GetRegisterSize(uint32_t reg_index) const = 0;

These aren't really register *set* related, are they? Could they go on the base 
interface instead?

Although, THB, the way I'd handle this is by having something a 
`ArrayRef RegInfos()` interface, and then having the callers do 
`reg_info_interface->RegInfos()[reg_index].offset`



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:16
 
-class RegisterInfoPOSIX_arm64 : public lldb_private::RegisterInfoInterface {
+enum { RegisterSetPOSIX_ARM64GPR = 0, RegisterSetPOSIX_ARM64FPR };
+

How about we put this inside the `RegisterInfoPOSIX_arm64` class, and drop 
`RegisterSetPOSIX_ARM64` from the name. (`RegisterInfoPOSIX_arm64::GPRegSet` ?) 
(there's already something similar inside `NativeRegisterContextNetBSD_x86_64`).



Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:21-29
+  struct RegInfo {
+uint32_t num_registers;
+uint32_t num_gpr_registers;
+uint32_t num_fpr_registers;
+
+uint32_t last_gpr;
+uint32_t first_fpr;

I'm not sure what's the advantage of making this a struct vs. just splating it 
into the containing class. Maybe that made more sense when originally when the 
class was doing a lot more work, but here I'd just replace this by a list of 
members.



Comment at: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp:82
 
 switch (arch.GetTriple().getOS()) {
 case llvm::Triple::FreeBSD: {

The reg(set)_interface and register_context switches could be merged here too 
in a similar way...


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

https://reviews.llvm.org/D80105



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

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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

In this updated I have segregated RegisterInfoInterface and 
RegisterInfoAndSetInterface as two mutually exclusive interfaces. 
RegisterInfoPosix_arm64 is currently the only class making use of set interface 
but I am going to extend this once we reach a consensus. All occurrences of  
RegisterInfoPosix_arm64 have been updated accordingly and for the case of  
NativeRegisterContextLinux_arm64 RegisterInfoInterface shared pointer is casted 
into a RegisterInfoAndSetInterface shared pointer. Once we have this 
implemented for other architecture there wont be a need for having a shared 
pointer of RegisterInfoAndSetInterface in all variants of 
NativeRegisterContextLinux_*

@labath what are your thoughts on this ?


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

https://reviews.llvm.org/D80105

Files:
  lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
  
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.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/RegisterInfoAndSetInterface.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.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/ThreadElfCore.cpp

Index: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -77,12 +77,13 @@
 ProcessElfCore *process = static_cast(GetProcess().get());
 ArchSpec arch = process->GetArchitecture();
 RegisterInfoInterface *reg_interface = nullptr;
+RegisterInfoAndSetInterface *regset_interface = nullptr;
 
 switch (arch.GetTriple().getOS()) {
 case llvm::Triple::FreeBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
+regset_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -111,7 +112,7 @@
 case llvm::Triple::NetBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
+regset_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::x86_64:
 reg_interface = new RegisterContextNetBSD_x86_64(arch);
@@ -128,7 +129,7 @@
 reg_interface = new RegisterInfoPOSIX_arm(arch);
 break;
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
+regset_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::mipsel:
   case llvm::Triple::mips:
@@ -159,7 +160,7 @@
 case llvm::Triple::OpenBSD: {
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64:
-reg_interface = new RegisterInfoPOSIX_arm64(arch);
+regset_interface = new RegisterInfoPOSIX_arm64(arch);
 break;
   case llvm::Triple::arm:
 reg_interface = new RegisterInfoPOSIX_arm(arch);
@@ -189,7 +190,7 @@
 switch (arch.GetMachine()) {
 case llvm::Triple::aarch64:
   m_thread_reg_ctx_sp = std::make_shared(
-  *this, reg_interface, m_gpregset_data, m_notes);
+  *this, regset_interface, m_gpregset_data, m_notes);
   break;
 case llvm::Triple::arm:
   m_thread_reg_ctx_sp = std::make_shared(
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
@@ -18,7 +18,7 @@
 public:
   RegisterContextCorePOSIX_arm64(
   lldb_private::Thread &thread,
-  lldb_private::RegisterInfoInterface *register_info,
+  lldb_private::RegisterInfoAndSetInterface *register_info,
   const lldb_private::DataExtractor &gpregset,
   llvm::ArrayRef notes);
 
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
===
--- lldb/source/Plugins/Process/elf-

[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid created this revision.
omjavaid added a reviewer: labath.
Herald added subscribers: danielkiss, atanasyan, kristof.beyls, emaste.

This patch aims to combine similar arm64 register set definitions defined in 
NativeRegisterContextLinux_arm64 and RegisterContextPOSIX_arm64.
I have implemented a register set interface out of RegisterInfoInterface class 
and moved arm64 register sets into RegisterInfosPOSIX_arm64 which is similar to 
Utility/RegisterContextLinux_* implemented by various other targets. This will 
help in managing register sets of new ARM64 architecture features in one place.

Built and tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabihf 
targets.


https://reviews.llvm.org/D80105

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h
  lldb/source/Plugins/Process/Utility/RegisterContextFreeBSD_mips64.h
  lldb/source/Plugins/Process/Utility/RegisterContextLinux_mips.h
  lldb/source/Plugins/Process/Utility/RegisterContextLinux_mips64.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/elf-core/RegisterContextPOSIXCore_arm64.cpp

Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -61,7 +61,7 @@
 return false;
 
   offset -= GetGPRSize();
-  if (IsFPR(reg) && offset + reg_info->byte_size <= sizeof(FPU)) {
+  if (IsFPR(reg) && offset + reg_info->byte_size <= GetFPUSize()) {
 Status error;
 value.SetFromMemoryData(reg_info, m_fpregset.GetDataStart() + offset,
 reg_info->byte_size, lldb::eByteOrderLittle, error);
Index: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
===
--- lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
+++ lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
@@ -13,8 +13,20 @@
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/lldb-private.h"
 
+enum { RegisterSetPOSIX_ARM64GPR = 0, RegisterSetPOSIX_ARM64FPR };
+
 class RegisterInfoPOSIX_arm64 : public lldb_private::RegisterInfoInterface {
 public:
+  struct RegInfo {
+uint32_t num_registers;
+uint32_t num_gpr_registers;
+uint32_t num_fpr_registers;
+
+uint32_t last_gpr;
+uint32_t first_fpr;
+uint32_t last_fpr;
+  };
+
   // based on RegisterContextDarwin_arm64.h
   struct GPR {
 uint64_t x[29]; // x0-x28
@@ -61,7 +73,19 @@
 
   uint32_t GetRegisterCount() const override;
 
+  uint32_t GetRegisterOffset(uint32_t reg_index) const override;
+
+  uint32_t GetRegisterSize(uint32_t reg_index) const override;
+
+  const lldb_private::RegisterSet *
+  GetRegisterSet(size_t reg_set) const override;
+
+  size_t GetRegisterSetCount() const override;
+
+  size_t GetRegisterSetFromRegisterIndex(uint32_t reg_index) const override;
+
 private:
+  RegInfo m_reg_info;
   const lldb_private::RegisterInfo *m_register_info_p;
   uint32_t m_register_info_count;
 };
Index: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
===
--- lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
+++ lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
@@ -65,6 +65,58 @@
   }
 }
 
+// Number of register sets provided by this context.
+enum {
+  k_num_gpr_registers = gpr_w28 - gpr_x0 + 1,
+  k_num_fpr_registers = fpu_fpcr - fpu_v0 + 1,
+  k_num_register_sets = 2
+};
+// clang-format off
+// ARM64 general purpose registers.
+static const uint32_t g_gpr_regnums_arm64[] = {
+gpr_x0,  gpr_x1,   gpr_x2,  gpr_x3,  gpr_x4,  gpr_x5,  gpr_x6,  gpr_x7,
+gpr_x8,  gpr_x9,   gpr_x10, gpr_x11, gpr_x12, gpr_x13, gpr_x14, gpr_x15,
+gpr_x16, gpr_x17,  gpr_x18, gpr_x19, gpr_x20, gpr_x21, gpr_x22, gpr_x23,
+gpr_x24, gpr_x25,  gpr_x26, gpr_x27, gpr_x28, gpr_fp,  gpr_lr,  gpr_sp,
+gpr_pc,  gpr_cpsr, gpr_w0,  gpr_w1,  gpr_w2,  gpr_w3,  gpr_w4,  gpr_w5,
+gpr_w6,  gpr_w7,   gpr_w8,  gpr_w9,  gpr_w10, gpr_w11, gpr_w12, gpr_w13,
+gpr_w14, gpr_w15,  gpr_w16, gpr_w17, gpr_w18, gpr_w19, gpr_w20, gpr_w21,
+gpr_w22, gpr_w23,  gpr_w24, gpr_w25, gpr_w26, gpr_w27, gpr_w28,
+LLDB_INVALID_REGNUM // register sets need to end with this flag
+};
+
+static_assert(((sizeof g_gpr_regnums_arm64 /