This is an archive of the discontinued LLVM Phabricator instance.

Enable breakpoints and read/write GPRs for ppc64le
ClosedPublic

Authored by alexandreyy on Sep 27 2017, 11:24 AM.

Details

Summary

Add support for ppc64le to create breakpoints and read/write
general purpose registers.
Other features for ppc64le and functions to read/write
other registers are being implemented.

Event Timeline

alexandreyy created this revision.Sep 27 2017, 11:24 AM
gut added a subscriber: lldb-commits.

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.

source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
58–65

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?

67–69

Any reason to not use "uint64_t" instead of "struct Reg"?

source/Plugins/Process/Utility/RegisterContextPOSIX_ppc64le.h
55–61 ↗(On Diff #116848)

Any reason for this struct? Just members of the class?

64–66 ↗(On Diff #116848)

uint64_t instead?

clayborg added inline comments.Sep 28 2017, 8:12 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
72

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?

source/Plugins/Process/Utility/RegisterContextPOSIX_ppc64le.h
64–69 ↗(On Diff #116848)

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?

alexandreyy marked 6 inline comments as done.Sep 29 2017, 12:05 PM

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.

source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
58–65

I implemented this class based on the ARM code, copying that struct.
We don't really need it. I will remove that.

67–69

Changed to uint64_t.

72

Yes, we can. Better that way.
I will change that.

source/Plugins/Process/Utility/RegisterContextPOSIX_ppc64le.h
55–61 ↗(On Diff #116848)

File removed from patch.

64–66 ↗(On Diff #116848)

File removed from patch.

64–69 ↗(On Diff #116848)

File removed from patch.

alexandreyy marked 6 inline comments as done.

Remove duplicated structs and change register set types

Are these changes ok?
I am implementing the read/write functions for the other registers.
And I will add it later.

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.

source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
67–69

Changed register set type to GPR.

Hi,
Could you review/approve this patch, please?
I do not have commit permission.
I will add other register sets later.
Thanks! ;)

labath accepted this revision.Oct 5 2017, 10:29 AM
labath added a reviewer: eugene.
labath added a subscriber: eugene.

Looks fine to me. Sorry about the delay.

@eugene should be able to help you commit this.

Wrt. the extra register context discussion, I believe you will need to implement an extra class or two when you get around to debugging core files, but that should not be necessary right now. (And yes, linux now uses lldb-server for local debugging as well).

This revision is now accepted and ready to land.Oct 5 2017, 10:29 AM

Looks fine to me. Sorry about the delay.

@eugene should be able to help you commit this.

Wrt. the extra register context discussion, I believe you will need to implement an extra class or two when you get around to debugging core files, but that should not be necessary right now. (And yes, linux now uses lldb-server for local debugging as well).

Thanks for the review.
What are the classes/structures that I should implement?

This revision was automatically updated to reflect the committed changes.

Looks fine to me. Sorry about the delay.

@eugene should be able to help you commit this.

Wrt. the extra register context discussion, I believe you will need to implement an extra class or two when you get around to debugging core files, but that should not be necessary right now. (And yes, linux now uses lldb-server for local debugging as well).

Thanks for the review.
What are the classes/structures that I should implement?

Take a look at ProcessElfCore class. The things to implement should be fairly self-evident from the code there.

If you get stuck, I should be able to take a closer look at this next week.

clayborg edited edge metadata.Oct 9 2017, 8:32 AM

Sorry for the delay, I was out on vacation for a week.