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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
