[libcxx-commits] [PATCH] D111703: [ARM] __cxa_end_cleanup should be called instead of _UnwindResume.

[Ryan Prichard via Phabricator via libcxx-commits]

rprichard added inline comments.


================
Comment at: libcxxabi/src/cxa_exception.cpp:383
     "	bl	__cxa_end_cleanup_impl\n"
-    "	pop	{r1, r2, r3, r4}\n"
-    "	bl	_Unwind_Resume\n"
-    "	bl	abort\n"
-    "	.popsection"
-);
+    "	pop	{r1, r2, r3, lr}\n"
+    "	b	_Unwind_Resume\n"
----------------
rprichard wrote:
> chill wrote:
> > danielkiss wrote:
> > > rprichard wrote:
> > > > chill wrote:
> > > > > Could/should be
> > > > > ```
> > > > > pop {r1, r2, r3, r4}
> > > > > mov lr, r4
> > > > > ```
> > > > > then it'll be compatible with Armv6-M and Armv8.0-M.base.
> > > > > 
> > > > @danielkiss @chill 
> > > > 
> > > > The comment above `__cxa_end_cleanup` says:
> > > > 
> > > > > According to ARM EHABI 8.4.1, __cxa_end_cleanup() should not clobber any
> > > > > register, thus we have to write this function in assembly so that we can save
> > > > > {r1, r2, r3}.
> > > > 
> > > > We're currently saving/restoring r1-r3 (volatiles), but with the Armv6-M fix, we're clobbering r4 (a local/callee-save register) in order to restore lr. That seems like wrong behavior?
> > > > 
> > > > i.e. `__cxa_end_cleanup` should ensure r4 has the original value when it calls `_Unwind_Resume`. Before this LLVM patch, it wasn't necessary to preserve r4, because `__cxa_end_cleanup_impl` would do so automatically.
> > > > 
> > > clobbering r4 is not in line with the aapcs. so it could be like this (not tested yet):
> > > 
> > > ```
> > >   push	{r1, r2, r3, lr}
> > >   push	{r4}
> > >   bl	__cxa_end_cleanup_impl
> > >   pop {r1, r2, r3, r4}
> > >   mov lr, r4
> > >   pop {r4}
> > > 
> > > ```
> > Maybe need to preserve stack alignment?
> > ```
> > push	{r4}
> > push	{r1, r2, r3, lr}
> > sub sp, #12
> > bl	__cxa_end_cleanup_impl
> > add sp, #12
> > pop {r1, r2, r3, r4}
> > mov lr, r4
> > pop {r4}
> > ```
> Is preserving 16-byte alignment at every instruction boundary important, e.g. for signals/interrupts?
> 
> Also, for LIBCXXABI_BAREMETAL, we have to clobber something for the address of _Unwind_Resume. @chill had suggested ip/r12 on https://reviews.llvm.org/D113181#3108664.
> 
> Maybe this would work (also untested):
> 
> ```
> 	push	{r1, r2, r3, r4}
> 	mov	r4, lr
> 	bl	__cxa_end_cleanup_impl
> 	mov	lr, r4
> #if defined(LIBCXXABI_BAREMETAL)
> 	ldr	r4,	=_Unwind_Resume
> 	mov	ip,	r4
> #endif
> 	pop	{r1, r2, r3, r4}
> #if defined(LIBCXXABI_BAREMETAL)
> 	bx	ip
> #else
> 	b	_Unwind_Resume
> #endif
> ```
> 
> Is preserving 16-byte alignment at every instruction boundary important, e.g. for signals/interrupts?

>From AAPCS, it looks like SP must always be aligned to 4 bytes, but must be 8-byte aligned at the start of a function ("public interface"). (grep https://developer.arm.com/documentation/ihi0042 for "Universal stack constraints" and "Stack constraints at a public interface".)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111703/new/

https://reviews.llvm.org/D111703