Page MenuHomePhabricator

[lldb] [Process/FreeBSDRemote] Introduce powerpc support
ClosedPublic

Authored by mgorny on Feb 3 2021, 5:33 AM.

Details

Summary

Introduce a minimal support for the 32-bit powerpc platform. This
includes support for GPR and FPR registers. I also needed to add
software breakpoint opcode for PPC32/PPC64 (big endian), and to fix
offsets in RegisterInfos_powerpc.h (used only by FreeBSD register
context to be globally unique rather than relative to each struct).

Diff Detail

Event Timeline

mgorny created this revision.Feb 3 2021, 5:33 AM
mgorny updated this revision to Diff 321966.Feb 6 2021, 2:10 PM
mgorny edited the summary of this revision. (Show Details)

Fixed breakpoints and FPR reading. Now it's actually tested ;-).

krytarowski accepted this revision.Feb 8 2021, 2:33 AM
This revision is now accepted and ready to land.Feb 8 2021, 2:33 AM
emaste accepted this revision.Feb 8 2021, 9:29 AM
jrtc27 added a subscriber: jrtc27.Feb 8 2021, 11:48 AM
jrtc27 added inline comments.
lldb/source/Host/common/NativeProcessProtocol.cpp
525–526

Why are these two different? Should it not always be trap ie tw 31,0,0? If not that should be explained here. These names also aren't great as it's unclear which ppc64 is using unless you read the code below (I'd expect either ppc and ppc64 or ppc and ppcle as the two "axes", but ppc and ppc64le are on a diagonal in the 2x2 grid).

krytarowski added inline comments.Feb 8 2021, 12:09 PM
lldb/source/Host/common/NativeProcessProtocol.cpp
525–526

On PPC we assume Big-Endian unless specified otherwise, so no need to specify ppcbe or ppc64be.

jrtc27 added inline comments.Feb 8 2021, 12:16 PM
lldb/source/Host/common/NativeProcessProtocol.cpp
525–526

I realise why there is no be suffix, that's not what I was asking. Currently there are four options (though ppcle isn't implemented in FreeBSD):

BigLittle
32ppcppcle
64ppc64ppc64le

If the difference between the two encodings is solely endianness then they should be called g_ppc_opcode and g_ppcle_opcode. If the difference between the two encodings is solely machine word size then they should be called g_ppc_opcode and g_ppc64_opcode`. But g_ppc_opcode vs g_ppc64le_opcode` has *both* differences in the name, which tells you nothing about *why* they are different, and thus does not obviously state which encoding ppc64 uses, if either of them.

As for the encodings themselves, they obviously differ in endianness, but there is also a difference in the second/third byte where g_ppc_opcode[1] is 0xc0 but g_ppc64le_opcode[2] is 0xe0. That does not make sense to me, but if it's there for a reason it needs a comment.

mgorny added inline comments.Feb 8 2021, 12:45 PM
lldb/source/Host/common/NativeProcessProtocol.cpp
525–526

I don't really know what's the difference between the 0xc0 and 0xe0 opcode. Both can be found in various places in LLDB sources. I've copied this one from source/Target/Platform.cpp). Maybe 0xc0 works on Big Endian ppc as well, I've copied 0xe0 because it was used by the relevant code before. I'll simplify the code as suggested and try 0xc0.

mgorny added inline comments.Feb 8 2021, 12:46 PM
lldb/source/Host/common/NativeProcessProtocol.cpp
525–526

Sorry, I got the two exchanged. I meant I'm going to try 0xe0.

jrtc27 added inline comments.Feb 8 2021, 12:49 PM
lldb/source/Host/common/NativeProcessProtocol.cpp
525–526

FWIW 0xc is tw 30, 0, 0 whereas 0xe is tw 31, 0, 0 i.e. the canonical unconditional trap instruction, which trap is an alias for.

krytarowski added inline comments.Feb 8 2021, 12:59 PM
lldb/source/Host/common/NativeProcessProtocol.cpp
525–526

NetBSD uses:

#define PTRACE_BREAKPOINT ((const uint8_t[]) { 0x7f, 0xe0, 0x00, 0x08 }).

https://nxr.netbsd.org/xref/src/sys/arch/powerpc/include/ptrace.h#76

jrtc27 added inline comments.Feb 8 2021, 1:05 PM
lldb/source/Host/common/NativeProcessProtocol.cpp
525–526

Looking things up in more detail, the first value is a bit mask of conditions under which to trap (signed <, signed >, =, unsigned <, unsigned >), so if the same register is given twice it'll trap if and only if the third bit is set in the TO (trap operand?) field. Thus 30 and 31 should be entirely equivalent here, and I imagine it's best to use 31 (i.e. what ppc64le already uses) due to being the canonical version.

mgorny updated this revision to Diff 322208.Feb 8 2021, 1:40 PM

Improve consistency of trap opcodes.

mgorny marked 6 inline comments as done.Feb 8 2021, 1:41 PM

@jrtc27 , does this look good?

jrtc27 accepted this revision.Feb 8 2021, 1:42 PM

@jrtc27 , does this look good?

Yep, assuming it still works.

krytarowski accepted this revision.Feb 8 2021, 11:42 PM

@jrtc27 , does this look good?

Yep, assuming it still works.

Yes, it does.

@labath, do you have any comments?

labath accepted this revision.Feb 9 2021, 12:03 PM

Seems legit.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2021, 12:29 PM