[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"
----------------
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
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111703