[Lldb-commits] [PATCH] D38323: Enable breakpoints and read/write GPRs for ppc64le

[Alexandre Yukio Yamashita via Phabricator via lldb-commits]

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

In https://reviews.llvm.org/D38323#883429, @clayborg wrote:

> Looks fine. One main questions for new linux archs in particular: is linux using the lldb-server to debug these days even when debugging locally? If so, then this patch only needs to implement both a native register content and not the lldb_private::RegisterContext subclass.


When I debug locally, the lldb launches the lldb-server. 
However, the remote debug is not working yet. The server crashes when I try to launch a process.

It shows that error:
lldb-server: /lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp:134: lldb_private::Status lldb_private::process_gdb_remote::GDBRemoteCommunicationServerPlatform::LaunchGDBServer(const lldb_private::Args&, std::__cxx11::string, lldb::pid_t&, uint16_t&, std::__cxx11::string&): Assertion `ok' failed.

I will investigate that later.



================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h:57-64
+  struct RegInfo {
+    uint32_t num_registers;
+    uint32_t num_gpr_registers;
+
+    uint32_t last_gpr;
+
+    uint32_t gpr_flags;
----------------
clayborg wrote:
> Any reason for this struct? Just make each member a member variable of the containing class? Is this passed an API somewhere and thus needed?
I implemented this class based on the ARM code, copying that struct.
We don't really need it. I will remove that.


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h:66-68
+  struct Reg {
+    uint8_t bytes[8];
+  };
----------------
clayborg wrote:
> Any reason to not use "uint64_t" instead of "struct Reg"?
Changed to uint64_t.


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h:71
+  RegInfo m_reg_info;
+  Reg m_gpr_ppc64le[48]; // 64-bit general purpose registers.
+
----------------
clayborg wrote:
> Could we share the structure that backs all registers from the RegisterInfos_ppc64le.h file here? Or are there multiple flavors of registers for different OSs and this is just big enough for both?
Yes, we can. Better that way.
I will change that. 


================
Comment at: source/Plugins/Process/Utility/RegisterContextPOSIX_ppc64le.h:55-61
+  struct RegInfo {
+    uint32_t num_registers;
+    uint32_t num_gpr_registers;
+
+    uint32_t last_gpr;
+
+    uint32_t gpr_flags;
----------------
clayborg wrote:
> Any reason for this struct? Just members of the class?
File removed from patch.


================
Comment at: source/Plugins/Process/Utility/RegisterContextPOSIX_ppc64le.h:64-66
+  struct Reg {
+    uint8_t bytes[8];
+  };
----------------
clayborg wrote:
> clayborg wrote:
> > uint64_t instead?
> Could we share the structure that backs all registers from the RegisterInfos_ppc64le.h file here? Or are there multiple flavors of registers for different OSs and this is just big enough for both?
File removed from patch.


================
Comment at: source/Plugins/Process/Utility/RegisterContextPOSIX_ppc64le.h:64-69
+  struct Reg {
+    uint8_t bytes[8];
+  };
+
+  // 64-bit general purpose registers.
+  Reg m_gpr_ppc64le[48]; // 64-bit general purpose registers.
----------------
alexandreyy wrote:
> clayborg wrote:
> > clayborg wrote:
> > > uint64_t instead?
> > Could we share the structure that backs all registers from the RegisterInfos_ppc64le.h file here? Or are there multiple flavors of registers for different OSs and this is just big enough for both?
> File removed from patch.
File removed from patch.


https://reviews.llvm.org/D38323