Page MenuHomePhabricator

Add SysV Abi for PPC64le
ClosedPublic

Authored by alexandreyy on Jan 3 2018, 7:57 AM.

Details

Summary

This patch implements the ABI Plugin for PPC64le. It was based on the
ABI for PPC64. It also enables LLDB to evaluate expressions using JIT.

Diff Detail

Repository
rL LLVM

Event Timeline

alexandreyy created this revision.Jan 3 2018, 7:57 AM
clayborg accepted this revision.Jan 3 2018, 10:57 AM
This revision is now accepted and ready to land.Jan 3 2018, 10:57 AM

Thanks, @clayborg.

@labath , Could you commit this patch?

labath added a comment.Jan 6 2018, 1:12 AM

Is the only difference between ppc64 and ppc64le ABIs in the endianness of the values?
If so, could we make one unified ABI which takes the endianness as an argument (in the constructor, or as a template argument, or deduces it from target endiannes, ...) ?

hfinkel added a subscriber: hfinkel.Jan 6 2018, 1:39 PM

Is the only difference between ppc64 and ppc64le ABIs in the endianness of the values?
If so, could we make one unified ABI which takes the endianness as an argument (in the constructor, or as a template argument, or deduces it from target endiannes, ...) ?

The ABIs have some other differences. The largest difference between the ABIs is how indirect-calls (and, thus, function pointers) work. There are some other more-minor differences, for example, some of the call-frame offsets are different. It still might be possible to unify the support (we certainly have one backend in LLVM for both), but it's a bit more involved than just switching the endianness.

labath added a comment.Jan 9 2018, 2:57 AM

Is the only difference between ppc64 and ppc64le ABIs in the endianness of the values?
If so, could we make one unified ABI which takes the endianness as an argument (in the constructor, or as a template argument, or deduces it from target endiannes, ...) ?

The ABIs have some other differences. The largest difference between the ABIs is how indirect-calls (and, thus, function pointers) work. There are some other more-minor differences, for example, some of the call-frame offsets are different. It still might be possible to unify the support (we certainly have one backend in LLVM for both), but it's a bit more involved than just switching the endianness.

Thanks for the explanation. These don't sound like major differences. Could you take a look the possibility of merging these two plugins? I scanned through the source code, and it looks like a large part of it is the same, so I'm hoping that we can make like 80% of this code go away with a unified plugin.
If you hit some major roadblock which would prevent doing that, then that's fine, but I would at least like to know what the blocker is.

(And sorry for the delays, I've been OOO last week.)

Is the only difference between ppc64 and ppc64le ABIs in the endianness of the values?
If so, could we make one unified ABI which takes the endianness as an argument (in the constructor, or as a template argument, or deduces it from target endiannes, ...) ?

The ABIs have some other differences. The largest difference between the ABIs is how indirect-calls (and, thus, function pointers) work. There are some other more-minor differences, for example, some of the call-frame offsets are different. It still might be possible to unify the support (we certainly have one backend in LLVM for both), but it's a bit more involved than just switching the endianness.

Thanks for the explanation. These don't sound like major differences. Could you take a look the possibility of merging these two plugins? I scanned through the source code, and it looks like a large part of it is the same, so I'm hoping that we can make like 80% of this code go away with a unified plugin.
If you hit some major roadblock which would prevent doing that, then that's fine, but I would at least like to know what the blocker is.

(And sorry for the delays, I've been OOO last week.)

Thanks, @labath .
The ABI plugin for PPC64be is not working: https://reviews.llvm.org/D5988 .
It was implemented based on the x86_64 plugin and needs to be fixed.
We could commit this patch and merge the plugins when both plugins work properly.

Thanks, @labath .
The ABI plugin for PPC64be is not working: https://reviews.llvm.org/D5988 .
It was implemented based on the x86_64 plugin and needs to be fixed.
We could commit this patch and merge the plugins when both plugins work properly.

Hmm.. that complicates things a bit. How badly is it broken, do you estimate? Would it be any better than if we just made your new plugin handle the the big-endian target as well (just a best effort, I'm not asking you to actually test it on that target)?

If the ppc64be FreeBSD target is really broken, and there is no desire to fix it, maybe we should just delete the relevant plugin.

@emaste, @jhibbits: do you know what's the state of ppc64 lldb support in freebsd?

Hmm.. that complicates things a bit. How badly is it broken, do you estimate? Would it be any better than if we just made your new plugin handle the the big-endian target as well (just a best effort, I'm not asking you to actually test it on that target)?

If the ppc64be FreeBSD target is really broken, and there is no desire to fix it, maybe we should just delete the relevant plugin.

@emaste, @jhibbits: do you know what's the state of ppc64 lldb support in freebsd?

It's broken because LLDB doesn't yet handle function descriptors, and I haven't yet made the effort (ENOTIME) to fix that. I think our eventual goal is to move even BE to ELFv2 on FreeBSD, to do away with function descriptors, and it should then Just Work, as powerpc (32-bit) works fine for most cases right now.

That said, if anyone knows enough to add function descriptor knowledge to LLDB, I think that's the only missing piece, or the biggest missing piece. I do want to finish the effort, since I started it 3.5 years ago, but I do need help with understanding that ABI detail and how it fits with LLDB.

@emaste, @jhibbits: do you know what's the state of ppc64 lldb support in freebsd?

It's broken because LLDB doesn't yet handle function descriptors, and I haven't yet made the effort (ENOTIME) to fix that. I think our eventual goal is to move even BE to ELFv2 on FreeBSD, to do away with function descriptors, and it should then Just Work, as powerpc (32-bit) works fine for most cases right now.

That said, if anyone knows enough to add function descriptor knowledge to LLDB, I think that's the only missing piece, or the biggest missing piece. I do want to finish the effort, since I started it 3.5 years ago, but I do need help with understanding that ABI detail and how it fits with LLDB.

Thanks for the explanation. How badly is it broken would you say? Do you think that using this ABI plugin for the big endian target (instead of the one from 2014) would make things any worse? Could you take a quick look to see if there is anything in this plugin that would be completely wrong for the big endian target ? (I couldn't find anything, apart from the obvious things like register constants in CreateDefaultUnwindPlan, but then again, I don't know the ABIs).

I'm trying to avoid making bad design decisions now because of legacy code that is known to be broken and has no clear maintainer nor a plan to fix it.

I think the stack frame structure is not correct for the PPC64be plugin.
But it is hard to say how bad it is broken and fix the plugin without testing it.
The register numbers and stack frame structures are different from the ABI 1.9 to the ABI 2.0.

Thanks, @labath .
The ABI plugin for PPC64be is not working: https://reviews.llvm.org/D5988 .
It was implemented based on the x86_64 plugin and needs to be fixed.
We could commit this patch and merge the plugins when both plugins work properly.

Hmm.. that complicates things a bit. How badly is it broken, do you estimate? Would it be any better than if we just made your new plugin handle the the big-endian target as well (just a best effort, I'm not asking you to actually test it on that target)?

If the ppc64be FreeBSD target is really broken, and there is no desire to fix it, maybe we should just delete the relevant plugin.

@emaste, @jhibbits: do you know what's the state of ppc64 lldb support in freebsd?

chmeee added a subscriber: chmeee.Jan 14 2018, 12:46 PM

@emaste, @jhibbits: do you know what's the state of ppc64 lldb support in freebsd?

It's broken because LLDB doesn't yet handle function descriptors, and I haven't yet made the effort (ENOTIME) to fix that. I think our eventual goal is to move even BE to ELFv2 on FreeBSD, to do away with function descriptors, and it should then Just Work, as powerpc (32-bit) works fine for most cases right now.

That said, if anyone knows enough to add function descriptor knowledge to LLDB, I think that's the only missing piece, or the biggest missing piece. I do want to finish the effort, since I started it 3.5 years ago, but I do need help with understanding that ABI detail and how it fits with LLDB.

Thanks for the explanation. How badly is it broken would you say? Do you think that using this ABI plugin for the big endian target (instead of the one from 2014) would make things any worse? Could you take a quick look to see if there is anything in this plugin that would be completely wrong for the big endian target ? (I couldn't find anything, apart from the obvious things like register constants in CreateDefaultUnwindPlan, but then again, I don't know the ABIs).

I'm trying to avoid making bad design decisions now because of legacy code that is known to be broken and has no clear maintainer nor a plan to fix it.

I don't see anything glaring. I also cross-referenced with the 64-bit BE ABI reference (http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html) and didn't see anything obvious that would hinder using your plugin as a replacement to mine (as mentioned before, symbol handling is the big problem right now, and that's not handled within the ABI plugin), if yours is fully tested.

labath requested changes to this revision.Jan 15 2018, 2:56 AM

Thank you for looking at this @chmeee. @alexandreyy, could you update this plugin to handle big-endian as well, and then delete the old plugin?

This revision now requires changes to proceed.Jan 15 2018, 2:56 AM

Thanks, @labath and @chmeee .
I will do the merge and send the update soon.

@emaste, @jhibbits: do you know what's the state of ppc64 lldb support in freebsd?

It's broken because LLDB doesn't yet handle function descriptors, and I haven't yet made the effort (ENOTIME) to fix that. I think our eventual goal is to move even BE to ELFv2 on FreeBSD, to do away with function descriptors, and it should then Just Work, as powerpc (32-bit) works fine for most cases right now.

That said, if anyone knows enough to add function descriptor knowledge to LLDB, I think that's the only missing piece, or the biggest missing piece. I do want to finish the effort, since I started it 3.5 years ago, but I do need help with understanding that ABI detail and how it fits with LLDB.

Thanks for the explanation. How badly is it broken would you say? Do you think that using this ABI plugin for the big endian target (instead of the one from 2014) would make things any worse? Could you take a quick look to see if there is anything in this plugin that would be completely wrong for the big endian target ? (I couldn't find anything, apart from the obvious things like register constants in CreateDefaultUnwindPlan, but then again, I don't know the ABIs).

I'm trying to avoid making bad design decisions now because of legacy code that is known to be broken and has no clear maintainer nor a plan to fix it.

I don't see anything glaring. I also cross-referenced with the 64-bit BE ABI reference (http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html) and didn't see anything obvious that would hinder using your plugin as a replacement to mine (as mentioned before, symbol handling is the big problem right now, and that's not handled within the ABI plugin), if yours is fully tested.

Merged ppc64le and ppc64 plugins.

Looks nice. Only nit is we probably don't need the m_endian member variable. See inlined comment.

source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.h
114 ↗(On Diff #130180)

Most other code uses "m_byte_order" as the name. That being said, you can always just ask the process since it is store in the lldb_private::ABI class so you really don't need to store it here if you don't want to, you could add an accessor:

lldb::ByteOrder GetByteOrder() const { return GetProcessSP()->GetByteOrder(); }

Removed m_endian variable.

Looks nice. Only nit is we probably don't need the m_endian member variable. See inlined comment.

Thanks. I have changed the code to get the byte order.

clayborg accepted this revision.Jan 17 2018, 12:04 PM
labath accepted this revision.Jan 18 2018, 2:58 AM

Thank you for taking the time to do this.
@chmeee, do you want to take a quick look at this?

This revision is now accepted and ready to land.Jan 18 2018, 2:58 AM
jhibbits accepted this revision.Jan 18 2018, 1:34 PM

Looks fine to me. We should eventually add tests for this, for both endians.

davide requested changes to this revision.Jan 18 2018, 1:59 PM
davide added a subscriber: davide.

Wait a minute. Is there any reason why we can't add tests now?

This revision now requires changes to proceed.Jan 18 2018, 1:59 PM

Well.. whether this plugin is tested depends on what machine you're running on. If you're running on a ppc machine, plenty of tests will exercise this code (TestReturnValue, and pretty much any test doing expression evaluation). Correct me if I am wrong, but I am assuming you guys are running the lldb testsuite as a part of ppc64le bringup.

Given that our entire test suite is based on compiling things and running them on the host, I don't think it's fair to ask a random contributor to come up with a new test strategy.

(Of course, I would still very much like to have tests which are not host-dependent).

Well.. whether this plugin is tested depends on what machine you're running on. If you're running on a ppc machine, plenty of tests will exercise this code (TestReturnValue, and pretty much any test doing expression evaluation). Correct me if I am wrong, but I am assuming you guys are running the lldb testsuite as a part of ppc64le bringup.

Given that our entire test suite is based on compiling things and running them on the host, I don't think it's fair to ask a random contributor to come up with a new test strategy.

(Of course, I would still very much like to have tests which are not host-dependent).

We are running the lldb suite as part of our development process.
And we know that there are still some issues with the PPC64le code.
But we are already working on that.
Later, we will send other patches to fix them.
Thanks!

Wait a minute. Is there any reason why we can't add tests now?

Thanks.
We are running the default tests of the lldb suite.
Currently, we have 48 failures that we are working on.
Could we proceed with this patch?
We will send fixes for these issues soon.

davide accepted this revision.Jan 19 2018, 7:31 AM

lgtm, feel free to go ahead.

This revision is now accepted and ready to land.Jan 19 2018, 7:31 AM

Wait a minute. Is there any reason why we can't add tests now?

Thanks.
We are running the default tests of the lldb suite.
Currently, we have 48 failures that we are working on.
Could we proceed with this patch?
We will send fixes for these issues soon.

@labath , Could you please commit this patch?

Closed by commit rL323100: Add SysV Abi for PPC64le (authored by labath, committed by ). · Explain WhyJan 22 2018, 3:32 AM
This revision was automatically updated to reflect the committed changes.