[libunwind] Fix unw_getcontext for ARMv6-m

[olista01]

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.

[compnerd]

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]

In D22292#482560, @compnerd wrote:

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).

[weimingz]

In D22292#482750, @rmaprath wrote:

In D22292#482560, @compnerd wrote:

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.

[rmaprath]

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

Cheers,

/ Asiri

[rmaprath]

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

[weimingz]

src/UnwindRegistersRestore.S

325 ↗

(On Diff #64318)

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

326 ↗

(On Diff #64318)

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

is that expected?

[weimingz]

src/UnwindRegistersRestore.S

326 ↗

(On Diff #64318)

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

[bcraig]

src/UnwindRegistersRestore.S

325 ↗

(On Diff #64318)

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]

src/UnwindRegistersRestore.S

331 ↗

(On Diff #64318)

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

336 ↗

(On Diff #64318)

Any point of this restoration? It is IP scratch.

src/UnwindRegistersSave.S

328 ↗

(On Diff #64318)

Thanks for the comment here!

[rengolin]

src/UnwindRegistersRestore.S

325 ↗

(On Diff #64318)

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]

• Don't save/restore r12
• Use LDM when restoring r8-r11

[olista01]

src/UnwindRegistersRestore.S

325–336 ↗

(On Diff #65037)

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]

• ADD must be ADDS for Thumb1 (previous patch was an old version uploaded by mistake)

[weimingz]

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

libunwind/trunk/src/UnwindRegistersRestore.S

 @
 @ void libunwind::Registers_arm::restoreCoreAndJumpTo()
 @
 @ On entry:
 @  thread_state pointer is in r0
 @
   .p2align 2
 DEFINE_LIBUNWIND_PRIVATE_FUNCTION(_ZN9libunwind13Registers_arm20restoreCoreAndJumpToEv)
-#if !defined(__ARM_ARCH_ISA_ARM)
-  ldr r2, [r0, #52]
-  ldr r3, [r0, #60]
+#if !defined(__ARM_ARCH_ISA_ARM) && __ARM_ARCH_ISA_THUMB == 1
+  @ r8-r11: ldm into r1-r4, then mov to r8-r11
+  adds r0, #0x20
+  ldm r0!, {r1-r4}
+  subs r0, #0x30
+  mov r8, r1
+  mov r9, r2
+  mov r10, r3
+  mov r11, r4
+  @ r12 does not need loading, it it the intra-procedure-call scratch register
+  ldr r2, [r0, #0x34]
+  ldr r3, [r0, #0x3c]
   mov sp, r2
   mov lr, r3         @ restore pc into lr
   ldm r0, {r0-r7}
 #else
   @ Use lr as base so that r0 can be restored.
   mov lr, r0
   @ 32bit thumb-2 restrictions for ldm:
   @ . the sp (r13) cannot be in the list

libunwind/trunk/src/UnwindRegistersSave.S

 @ Per EHABI #4.7 this only saves the core integer registers.
 @ EHABI #7.4.5 notes that in general all VRS registers should be restored
 @ however this is very hard to do for VFP registers because it is unknown
 @ to the library how many registers are implemented by the architecture.
 @ Instead, VFP registers are demand saved by logic external to unw_getcontext.
 @
   .p2align 2
 DEFINE_LIBUNWIND_FUNCTION(unw_getcontext)
-#if !defined(__ARM_ARCH_ISA_ARM)
-  stm r0, {r0-r7}
+#if !defined(__ARM_ARCH_ISA_ARM) && __ARM_ARCH_ISA_THUMB == 1
+  stm r0!, {r0-r7}
+  mov r1, r8
+  mov r2, r9
+  mov r3, r10
+  stm r0!, {r1-r3}
+  mov r1, r11
   mov r2, sp
   mov r3, lr
-  str r2, [r0, #52]
-  str r3, [r0, #56]
-  str r3, [r0, #60]  @ store return address as pc
+  str r1, [r0, #0]   @ r11
+  @ r12 does not need storing, it it the intra-procedure-call scratch register
+  str r2, [r0, #8]   @ sp
+  str r3, [r0, #12]  @ lr
+  str r3, [r0, #16]  @ store return address as pc
+  @ T1 does not have a non-cpsr-clobbering register-zeroing instruction.
+  @ It is safe to use here though because we are about to return, and cpsr is
+  @ not expected to be preserved.
+  movs r0, #0        @ return UNW_ESUCCESS
 #else
   @ 32bit thumb-2 restrictions for stm:
   @ . the sp (r13) cannot be in the list
   @ . the pc (r15) cannot be in the list in an STM instruction
   stm r0, {r0-r12}
   str sp, [r0, #52]
   str lr, [r0, #56]
   str lr, [r0, #60]  @ store return address as pc
-#endif
-#if __ARM_ARCH_ISA_THUMB == 1
-  @ T1 does not have a non-cpsr-clobbering register-zeroing instruction.
-  @ It is safe to use here though because we are about to return, and cpsr is
-  @ not expected to be preserved.
-  movs r0, #0        @ return UNW_ESUCCESS
-#else
   mov r0, #0         @ return UNW_ESUCCESS
 #endif
   JMP(lr)
 
 @
 @ static void libunwind::Registers_arm::saveVFPWithFSTMD(unw_fpreg_t* values)
 @
 @ On entry: