This is an archive of the discontinued LLVM Phabricator instance.

First cut of PowerPC(64) support in LLDB.
ClosedPublic

Authored by jhibbits on Oct 25 2014, 4:29 PM.

Details

Summary

This adds preliminary support for PowerPC/PowerPC64, for FreeBSD. There are
some issues still:

  • Breakpoints don't work well on powerpc64.
  • Shared libraries don't yet get loaded for a 32-bit process on powerpc64 host.
  • Backtraces don't work. This is due to PowerPC ABI using a backchain pointer in memory, instead of a dedicated frame pointer register for the backchain.
  • Breakpoints on functions without debug info may not work correctly for 32-bit powerpc.

Diff Detail

Repository
rL LLVM

Event Timeline

jhibbits updated this revision to Diff 15452.Oct 25 2014, 4:29 PM
jhibbits retitled this revision from to First cut of PowerPC(64) support in LLDB..
jhibbits updated this object.
jhibbits edited the test plan for this revision. (Show Details)
jhibbits added reviewers: zturner, emaste, tfiala.
jhibbits added a subscriber: Unknown Object (MLST).
zturner resigned from this revision.Oct 30 2014, 12:57 PM
zturner added reviewers: clayborg, jingham.
zturner removed a reviewer: zturner.

I don't think I'm the right person to review this change, but I've added Greg Clayton and Jim Ingham. If nothing else, they can help you find a better reviewer.

clayborg requested changes to this revision.Oct 30 2014, 1:04 PM
clayborg edited edge metadata.

Everything looks good except there is no need to add "eCore_ppc_powerpc" as we already have "eCore_ppc_generic", and no need to add "eCore_ppc64_powerpc64" as we already have "eCore_ppc64_generic". Feel free to keep the new "powerpc" and "powerpc64" string entries in the ArchSpec table, but I still use the eCore_ppc_generic and eCore_ppc64_generic core definitions.

This revision now requires changes to proceed.Oct 30 2014, 1:04 PM
In D5988#9, @clayborg wrote:

Everything looks good except there is no need to add "eCore_ppc_powerpc" as we already have "eCore_ppc_generic", and no need to add "eCore_ppc64_powerpc64" as we already have "eCore_ppc64_generic". Feel free to keep the new "powerpc" and "powerpc64" string entries in the ArchSpec table, but I still use the eCore_ppc_generic and eCore_ppc64_generic core definitions.

Thanks, Greg. I was also hesitant about the ArchSpec table, because of this duplication. The only reason I added these was to appease the static_assert that follows the table. Adding two new entries required adding two new enum items as well. If changing the assert to '>=' from '==' is sufficient, I'll go ahead with that and remove the new definitions. Otherwise, do you have any suggestion on how best to handle this?

Ah, I see what you mean about the static assert. In that case can you just use the "ppc" and "ppc64" entries? Or rename them to be "powerpc" and "powerpc64"? We want our triples to match what llvm uses for PowerPC so if llvm doesn't use "ppc" or "ppc64", just rename the entries in the "g_core_definitions" table, otherwise, use the "ppc" and "ppc64" entries.

jhibbits updated this revision to Diff 15591.Oct 30 2014, 3:51 PM
jhibbits edited edge metadata.

Remove the ArchSpec changes, and use 'powerpc*' instead of both 'ppc' and 'powerpc'.

Also handle an API change.

clayborg accepted this revision.Oct 30 2014, 4:17 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Oct 30 2014, 4:17 PM
jhibbits closed this revision.Oct 30 2014, 7:45 PM
jhibbits updated this revision to Diff 15601.

Closed by commit rL220944 (authored by @jhibbits).

emaste added inline comments.Oct 30 2014, 8:05 PM
source/Core/ArchSpec.cpp
22 ↗(On Diff #15591)

Leftover from earlier debugging?

source/Host/freebsd/Host.cpp
304 ↗(On Diff #15591)

Do we have a source for the magic constant?

312 ↗(On Diff #15591)

can we just return buf_sp here instead?

source/Plugins/ABI/CMakeLists.txt
2–3 ↗(On Diff #15591)

ppc should sort before ppc64

source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
264 ↗(On Diff #15591)

Should we have a log for this?

634 ↗(On Diff #15591)

Debug log or error return for these cases?

1032 ↗(On Diff #15591)

you should be able to use a register ID in kinds here rather than comparing strings, I'd think

source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
331 ↗(On Diff #15591)

Maybe a phabricator issue, but whitespace seems off here - tabs vs. spaces?

source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.cpp
51 ↗(On Diff #15591)

llvm_unreachable("powerpc::WriteFPR unimplemented"); perhaps?

source/Target/Thread.cpp
2284–2285 ↗(On Diff #15591)

ppc should sort first

source/lldb.cpp
232 ↗(On Diff #15591)

ppc and ppc64 ::Initialize but only pp64 ::Terminate

There's still lots of work left to do on this, which I'll work on for future patches. I'll fix the immediate problems, though, in a patch I'll submit tonight or tomorrow.

source/Core/ArchSpec.cpp
22 ↗(On Diff #15591)

Yup. Thought I had removed all the debug stuff. Guess I missed some

source/Host/freebsd/Host.cpp
304 ↗(On Diff #15591)

This constant is probably only good for powerpc. It's derived from SHARED_PAGE - sizeof(struct freebsd32_ps_strings). It's probably better to add a sysctl to the kernel to get the information of any given process. What do you think?

312 ↗(On Diff #15591)

This is a direct copy of GetAuxData() below. So they can both be updated the same. I'm not sure the rationale of GetAuxData() doing it, so figured the best solution for now is to just copy it.

source/Plugins/ABI/CMakeLists.txt
2–3 ↗(On Diff #15591)

Oops. Fixing in a new patch, with the others.

source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
264 ↗(On Diff #15591)

Log or TODO/FIXME, yes. I'll add something.

634 ↗(On Diff #15591)

Shamelessly copied from SysV-x86_64. The else{} block should be logged, though. Will add that.

1032 ↗(On Diff #15591)

That's a better way, yes. I did it this way because ABISysV-x86_64 does this. I'll fix it in a future patch. Still lots of work left before this is really complete.

source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
331 ↗(On Diff #15591)

Yup, this was one of the first changes I had made, and used tabs. Fixed in next patch.

source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.cpp
51 ↗(On Diff #15591)

Good idea. I'll include it in a future patch, for both GPR and FPR.

source/Target/Thread.cpp
2284–2285 ↗(On Diff #15591)

Oops, next patch.

source/lldb.cpp
232 ↗(On Diff #15591)

Fixing in a new patch.

loladiro added inline comments.
lldb/trunk/source/Plugins/Makefile
16

Missing ABI/SysV-ppc

jhibbits added inline comments.Nov 2 2014, 2:21 PM
lldb/trunk/source/Plugins/Makefile
16

Yeah, I noticed this, too. It's been fixed in a newer revision, along with other bugs.