Page MenuHomePhabricator

Improve PowerPC unwind support
ClosedPublic

Authored by jhibbits on Nov 9 2014, 10:45 AM.

Details

Summary

Taking advantage of the new 'CFAIsRegisterDereferenced' CFA register type, add
full stack unwind support to the PowerPC/PowerPC64 ABI. Also, add a new
register set for powerpc32-on-64, so the register sizes are correct. This also
requires modifying the ProcessMonitor to add support for non-uintptr_t-sized
register values.

Diff Detail

Repository
rL LLVM

Event Timeline

jhibbits updated this revision to Diff 15958.Nov 9 2014, 10:45 AM
jhibbits retitled this revision from to Improve PowerPC unwind support.
jhibbits updated this object.
jhibbits edited the test plan for this revision. (Show Details)
jhibbits added reviewers: emaste, jasonmolenda.
jhibbits added a subscriber: Unknown Object (MLST).
emaste edited edge metadata.Nov 9 2014, 1:00 PM

I'd like to think about the 32-on-64 implementation some more. Can you submit the unwind changes separately first?

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

I'd specifically call out this fix in the commit message

Unfortunately I can't. The unwind code depends on the 32-on-64 code in
order to give a valid back trace so becomes pointless without it. I guess I
could separate them and there would be no regression from the current
state, there just would be no gain.

jasonmolenda added inline comments.Nov 9 2014, 1:24 PM
source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
1015 ↗(On Diff #15958)

If you need this to be dynamically determined, when the ABISysV_ppc::CreateInstance method is called, it is passed an ArchSpec - that object has a GetAddressByteSize() method. I think it would be safe to cache the value in an ivar, or copy the ArchSpec. Greg might know of some reason why that's a bad idea -- but I think if a Target changes architecture, the ABI should be updated as well.

1016 ↗(On Diff #15958)

You should be calling SetCFARegisterNumberToDereference() here, shouldn't you?

source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp
1016 ↗(On Diff #15958)

Same thihng, this should be SetCFARegisterNumberToDereference().

emaste added inline comments.Nov 9 2014, 6:50 PM
source/Plugins/Process/Utility/RegisterContextFreeBSD_powerpc.cpp
236 ↗(On Diff #15958)

This seems like it ought to have a comment.

jhibbits updated this revision to Diff 15966.Nov 9 2014, 7:36 PM
jhibbits edited edge metadata.

Update per comments from Ed.

emaste added inline comments.Nov 10 2014, 6:14 AM
source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
320–323 ↗(On Diff #15966)

This will be dead code for one of the two cases (whichever has the same size as uintptr_t) no?

jhibbits updated this revision to Diff 15982.Nov 10 2014, 7:54 AM

Remove uintptr_t usage, it's handled by uint32_t and uint64_t.

LGTM

I'd like to revisit ReadRegOperation::Execute later on for endianness considerations (for i386 on x86_64 and ppc on ppc64), but am happy with this for now.

jhibbits closed this revision.Nov 12 2014, 7:14 AM
jhibbits updated this revision to Diff 16088.

Closed by commit rL221789 (authored by @jhibbits).