This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Fix unw_getcontext for ARMv6-m
ClosedPublic

Authored by olista01 on Jul 12 2016, 5:06 PM.

Details

Summary

[libunwind][ARM] Add support for Thumb1 targets

The Thumb1 version of the code for saving and restoring the unwind
context has a few bugs which prevent it from working:

  • It uses the STM instruction without writeback, which is not valid for Thumb1 (It was introduced in Thumb2).
  • It only saves/restores the low 8 registers, the sp and the lr, so if a program uses r8-r12 they will not be correctly restored when throwing an exception.

There aren't currently any Thumb1 build-bots to test this, but we have
been successfully running the libc++abi and libc++ test suites on
Cortex-M0 models, as well as some other test suites that use C++
exceptions on a downstream version of libunwind with this patch applied.

Diff Detail

Event Timeline

weimingz updated this revision to Diff 63759.Jul 12 2016, 5:06 PM
weimingz retitled this revision from to [libunwind] Fix unw_getcontext for ARMv6-m.
weimingz updated this object.
weimingz added reviewers: rengolin, rmaprath.
weimingz added a subscriber: cfe-commits.

Can you explain why the write back is needed? You are doing the write back on r0, but then adjusting it back. So it is unclear why this change is needed. Could you provide some more context. A test case would be even better.

rmaprath edited edge metadata.Jul 13 2016, 4:10 AM

Can you explain why the write back is needed? You are doing the write back on r0, but then adjusting it back. So it is unclear why this change is needed. Could you provide some more context. A test case would be even better.

This is because STM without write-back is only defined in Thumb-2 (Thumb-1 version must always have write-back set).

Now, I scanned through our downstream sources and realized we have some local patches in this area that addresses this (in a bit more general way - and possibly some other fixes).

@weimingz: If you can hold off for a bit, I can put these patches for review and sort these out for good (need to clean up the patches first). If you want to get this fixed asap, you should add a check for __ARM_ARCH_ISA_THUMB == 1 in addition to !defined(__ARM_ARCH_ISA_ARM).

Can you explain why the write back is needed? You are doing the write back on r0, but then adjusting it back. So it is unclear why this change is needed. Could you provide some more context. A test case would be even better.

This is because STM without write-back is only defined in Thumb-2 (Thumb-1 version must always have write-back set).

Now, I scanned through our downstream sources and realized we have some local patches in this area that addresses this (in a bit more general way - and possibly some other fixes).

@weimingz: If you can hold off for a bit, I can put these patches for review and sort these out for good (need to clean up the patches first). If you want to get this fixed asap, you should add a check for __ARM_ARCH_ISA_THUMB == 1 in addition to !defined(__ARM_ARCH_ISA_ARM).

Cool. We can hold off. Please upload the patches.

Update: Almost there, should be able to put it up for review tomorrow.

Cheers,

/ Asiri

Update: We've hit a minor issue with the patch, we're confident it can be sorted by next week. I will ask @olista01 to put the final patch for review next week, as I'm on holidays. Hope this is OK.

Cheers,

/ Asiri

olista01 commandeered this revision.Jul 18 2016, 7:50 AM
olista01 updated this revision to Diff 64318.
olista01 added a reviewer: weimingz.
olista01 updated this object.
olista01 edited edge metadata.
weimingz added inline comments.Jul 18 2016, 12:03 PM
src/UnwindRegistersRestore.S
325

originally, r8-r12 were not restored. Was that some existing bug?

326

originally, r0-r7 get loaded from [r0]
now, r8-r12 get loaded from [r0+0x20]

is that expected?

weimingz added inline comments.Jul 18 2016, 12:06 PM
src/UnwindRegistersRestore.S
326

Please ignore the comment on line 326. I thought the “x" icon will delete it.

bcraig added a subscriber: bcraig.Jul 18 2016, 12:25 PM
bcraig added inline comments.
src/UnwindRegistersRestore.S
325

I'm not sure why r12 is getting messed with here. It is intended as a GOT/PLT scratch register. r8+ isn't ubiquitously available in Thumb state according to the ATPCS. The AAPCS says that r4-r8, r10, r11, and SP must be preserved. r9 can optionally be preserved.

compnerd added inline comments.Jul 18 2016, 5:50 PM
src/UnwindRegistersRestore.S
331

Is there a benefit to doing the explicit loading rather than a add + ldm?

336

Any point of this restoration? It is IP scratch.

src/UnwindRegistersSave.S
328

Thanks for the comment here!

rengolin added inline comments.Jul 21 2016, 4:47 AM
src/UnwindRegistersRestore.S
325

So, you're replacing a Thumb2+ARM implementation with a Thumb1+ARM one. This is all fine and dandy, but a Thumb2 implementation would be much smaller and cleaner (thus also faster).

Maybe we should have three implementations?

olista01 updated this revision to Diff 65037.Jul 22 2016, 1:58 AM
  • Don't save/restore r12
  • Use LDM when restoring r8-r11
olista01 added inline comments.Jul 22 2016, 1:58 AM
src/UnwindRegistersRestore.S
325–339

Maybe we should have three implementations?

Previously, we had two implementations:

  • The first one is used for all thumb-only targets, but is only valid thumb2 code, and doesn't save the high registers
  • The second one is ARM code used for all targets that have ARM (despite the comment about thumb2)

Now we have:

  • The first one is used for Thumb1-only targets
  • The second one is used for targets that have Thumb2 or ARM, and assembles as either ISA (with a preference for ARM, if that is available)
olista01 updated this revision to Diff 65039.Jul 22 2016, 2:17 AM
  • ADD must be ADDS for Thumb1 (previous patch was an old version uploaded by mistake)
weimingz edited edge metadata.Jul 22 2016, 10:45 AM

LGTM.
Maybe @renato can review it too.
Thanks Oliver and Saleem.

compnerd accepted this revision.Jul 23 2016, 10:32 AM
compnerd added a reviewer: compnerd.
This revision is now accepted and ready to land.Jul 23 2016, 10:32 AM
This revision was automatically updated to reflect the committed changes.