This is an archive of the discontinued LLVM Phabricator instance.

Add initial support to PowerPC64 little endian (POWER8)
ClosedPublic

Authored by gut on Aug 16 2017, 12:59 PM.

Details

Summary

Now a ppc64le binary is correctly detected:
(lldb) target create "tst"
Current executable set to 'tst' (powerpc64le).
(lldb) disassemble -n main
tst`main:
tst[0x7b0] <+0>: addis 2, 12, 2
tst[0x7b4] <+4>: addi 2, 2, 30544
tst[0x7b8] <+8>: mflr 0

Wihout the patch, the endianess was incorrect:
(lldb) target create "tst"
Current executable set to 'tst' (powerpc64).
(lldb) disassemble -n main
tst`main:
tst[0x7b0] <+0>: .long 0x02004c3c ; unknown opcode
tst[0x7b4] <+4>: rlwimi 23, 3, 8, 8, 28
tst[0x7b8] <+8>: lhzu 16, 2172(2)
tst[0x7bc] <+12>: .long 0x100001f8 ; unknown opcode

Simple binary used is identified as:
$ file tst
tst: ELF 64-bit LSB shared object, 64-bit PowerPC or cisco 7500, version 1 (SYSV), dynamically linked, interpreter /lib64/ld64.so.2, for GNU/Linux 3.2.0, BuildID[sha1]=17a8fa2b24ce2837ba6625fabb34e6b29c6c5db7, not stripped

Diff Detail

Repository
rL LLVM

Event Timeline

gut created this revision.Aug 16 2017, 12:59 PM

This all looks correct to me on inspection.

However, this is the first time I look at LLDB code and am not sure what these components are used for. Is this the first patch in a planned series that will provide ppc6le support in LLDB? If this just gives the user an impression that ppc64le is supported when it really isn't, would it be better to just emit some kind of warning (if that's even possible)? Also, other targets seem to provide various CPU's for the architecture. Would it make sense to add Power8 and Power9 as separate CPU's?

Ultimately, I think this should have lldb-commits as a subscriber and a developer from that community should be a reviewer on the patch.

gut added a comment.Aug 17 2017, 4:12 AM

Is this the first patch in a planned series that will provide ppc6le support in LLDB?

Yes, we're looking forward to complete the port of LLDB to ppc64le.

If this just gives the user an impression that ppc64le is supported when it really isn't, would it be better to just emit some kind of warning (if that's even possible)?

It doesn't. Only with this patch LLDB doesn't even build on ppc64le yet. This patch merely enables the correct identification of the binary as ppc64le on lldb. It's an initial patch in order to show that we have a dedicated team to deal with this port and want to confirm how would LLVM community embrace a new architecture patch series. Are you open to such contributions?

Also, other targets seem to provide various CPU's for the architecture. Would it make sense to add Power8 and Power9 as separate CPU's?

I'd say POWER8 and POWER9 are the same CPU on an ABI point of view. POWER9 will have more instructions/features but it'll still be backward compatible to POWER8, and considered a ppc64le architecture.

Ultimately, I think this should have lldb-commits as a subscriber and a developer from that community should be a reviewer on the patch.

Is there anyone specifically I should ping? Or should I simply forward this message to that list?

Thanks

lbianc added a subscriber: lbianc.Aug 17 2017, 4:18 AM

Yes, we're looking forward to complete the port of LLDB to ppc64le.

Awesome. That's really nice to hear.

It doesn't. Only with this patch LLDB doesn't even build on ppc64le yet. This patch merely enables the correct identification of the binary as ppc64le on lldb. It's an initial patch in order to show that we have a dedicated team to deal with this port and want to confirm how would LLVM community embrace a new architecture patch series. Are you open to such contributions?

I am of course interested in seeing ppc64le support in LLDB and I'm sure the community is always happy to accept new targets (especially ones that are in-tree for LLVM).

I'd say POWER8 and POWER9 are the same CPU on an ABI point of view. POWER9 will have more instructions/features but it'll still be backward compatible to POWER8, and considered a ppc64le architecture.

There are no differences from the ABI point of view. I only asked this because I see it for other targets and I didn't know what the use of multiple CPU's is for LLDB (i.e. if the ISA matters or not).

Ultimately, I think this should have lldb-commits as a subscriber and a developer from that community should be a reviewer on the patch.

Is there anyone specifically I should ping? Or should I simply forward this message to that list?

I think you should subscribe that list to this patch and also send a message to that list explaining what you're interested in doing and asking for review volunteers.

gut added a comment.Aug 18 2017, 4:27 AM

Can I please get some review on this?

ps: check comments on phabricator as it was not being published on lldb-commits mailing list.

gut added a comment.Aug 22 2017, 12:00 PM
In D36804#845286, @gut wrote:

Can I please get some review on this?

ps: check comments on phabricator as it was not being published on lldb-commits mailing list.

Nobody? It's a very small change.

We hope to port LLDB to PowerPC64le if patches are welcome. Are you interested in such a port? It's an architecture already supported by LLVM.

krytarowski added a subscriber: krytarowski.

+ Linux and general LLDB maintainers

hfinkel added inline comments.
source/Core/ArchSpec.cpp
375 ↗(On Diff #111405)

I doubt this is needed. There's no Darwin support to speak of.

gut added inline comments.Aug 22 2017, 12:16 PM
source/Core/ArchSpec.cpp
375 ↗(On Diff #111405)

Oh, I see. The MachO object type is only used by MacOS, right? Then I can remove, you are right.

hfinkel accepted this revision.Aug 22 2017, 12:19 PM
hfinkel added inline comments.
source/Core/ArchSpec.cpp
375 ↗(On Diff #111405)

I'm pretty sure that's correct.

Otherwise, this LGTM.

This revision is now accepted and ready to land.Aug 22 2017, 12:19 PM
clayborg accepted this revision.Aug 22 2017, 2:05 PM
gut added a comment.Aug 23 2017, 9:41 AM

@labath could you please commit this? I don't have commit access.

Thanks everyone for the review.

labath edited edge metadata.Aug 30 2017, 2:23 AM
labath added a subscriber: eugene.

IIUC, the conclusion was that we don't need the mach-o entry. Could you submit a version of the patch without it. I can't commit this right now, but @eugene should be able to do that for you.

gut added a comment.Aug 30 2017, 3:58 AM

IIUC, the conclusion was that we don't need the mach-o entry. Could you submit a version of the patch without it. I can't commit this right now, but @eugene should be able to do that for you.

Yes, I can. Sorry, I didn't got that. Wait a sec...

gut updated this revision to Diff 113236.Aug 30 2017, 4:12 AM

Remove unused MachO detection for ppc64le

gut added a comment.Aug 30 2017, 4:14 AM

Ops, wrong diff. Sorry (first time using archanist...)

gut updated this revision to Diff 113237.Aug 30 2017, 4:16 AM

Remove unused MachO detection for ppc64le

(now sending all my changes on this update)

gut added a comment.Aug 30 2017, 4:17 AM

ok... now it looks fine!

Thanks @labath

clayborg accepted this revision.Aug 30 2017, 8:15 AM
This revision was automatically updated to reflect the committed changes.