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

[Momchil Velikov via Phabricator via libcxx-commits]

chill 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"
----------------
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}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111703