This is an archive of the discontinued LLVM Phabricator instance.

[ARM] __cxa_end_cleanup should be called instead of _UnwindResume.
ClosedPublic

Authored by danielkiss on Oct 13 2021, 3:27 AM.

Details

Summary

ARM EHABI[1] specifies the cxa_end_cleanup to be called after cleanup.
It will call the UnwindResume.
cxa_begin_cleanup will be called from libcxxabi while cxa_end_cleanup is never called.
This will trigger a termination when a foreign exception is processed while UnwindResume is called
because the global state will be wrong due to the missing
cxa_end_cleanup call.

Additional test here: D109856
[1] https://github.com/ARM-software/abi-aa/blob/main/ehabi32/ehabi32.rst#941compiler-helper-functions

Diff Detail

Event Timeline

danielkiss created this revision.Oct 13 2021, 3:27 AM
danielkiss requested review of this revision.Oct 13 2021, 3:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2021, 3:27 AM
logan added inline comments.Oct 13 2021, 7:36 AM
libcxxabi/src/cxa_exception.cpp
346

s/origian/original/

llvm/lib/CodeGen/DwarfEHPrepare.cpp
233

I think in addition to isTargetEHABICompatible check, you should also check (Pers == EHPersonality::GNU_CXX || Pers == EhPersonality::GNU_CXX_SjLj) because only __gxx_personality_v0 and __gxx_personality_sj0 will call __cxa_begin_cleanup and push the exception object into the global stack.

logan added inline comments.Oct 13 2021, 7:36 AM
libcxxabi/src/cxa_exception.cpp
345

Add and its frame information after identify the caller.

llvm/test/CodeGen/ARM/eh-resume.ll
29

Please add another test for __gcc_personality_v0, which should not use __cxa_end_cleanup.

danielkiss marked 4 inline comments as done.
danielkiss added inline comments.
llvm/lib/CodeGen/DwarfEHPrepare.cpp
233

Right, thanks I forget about it.

logan added inline comments.Oct 13 2021, 9:19 AM
llvm/test/CodeGen/ARM/eh-resume2.ll
26

Add some comment to explain the difference between this test case and the one in eh-resume.ll.

danielkiss marked an inline comment as done.
danielkiss marked an inline comment as done.
logan accepted this revision.Oct 13 2021, 10:47 AM
This revision is now accepted and ready to land.Oct 13 2021, 10:47 AM
logan added inline comments.Oct 13 2021, 3:00 PM
libcxxabi/src/cxa_exception.cpp
345

BTW, please add a period to end the sentence after "... its frame information".

Also, please re-flow the text to fit in 80 columns.

logan added inline comments.Oct 13 2021, 7:34 PM
llvm/lib/CodeGen/DwarfEHPrepare.cpp
82

Maybe it makes more sense to define this function as a member function of the Triple class?

logan added inline comments.Oct 13 2021, 7:39 PM
llvm/lib/CodeGen/DwarfEHPrepare.cpp
239

__cxa_end_cleanup does not expect the exception object. Maybe we can exploit that to eliminate unnecessary instructions that set up r0?

Moved isTargetEHABICompatible up to the Triple class.
as __cxa_end_cleanup does not take the exception object ( managed it internally ) so less code to be generated.

logan added inline comments.Oct 24 2021, 9:49 AM
llvm/lib/CodeGen/DwarfEHPrepare.cpp
82

I think renaming this as doesRewindFunctionNeedExceptionObject would be better (since it is less tied to _Unwind_Resume) and change the implementation as:

/// Determines whether the exception object must be passed to the rewind
/// function.  For example, `_Unwind_Resume` expects an exception object but
/// `__cxa_end_cleanup` does not.
bool doesRewindFunctionNeedExceptionObject() const {
  auto *FTy = RewindFunction.getFunctionType();
  return FTy->getNumParams() > 0;
}
danielkiss marked 2 inline comments as done.
danielkiss marked 2 inline comments as done.
logan added a comment.Oct 25 2021, 9:06 AM

We are quite close now. Here are my final comments.

llvm/lib/CodeGen/DwarfEHPrepare.cpp
237

Add:

RewindFunctionCallingConv = TLI.getLibcallCallingConv(RTLIB::CXA_END_CLEANUP);
240

Add:

RewindFunctionCallingConv = TLI.getLibcallCallingConv(RTLIB::UNWIND_RESUME);
248–253

Refine as:

llvm::SmallVector<Value *, 1> RewindFunctionArgs;

if (doesRewindFunctionNeedExceptionObject())
  RewindFunctionArgs.push_back(GetExceptionObject(RI));

// Call the rewind function.
CallInst *CI = CallInst::Create(RewindFunction, RewindFunctionArgs, "",
                                UnwindBB);
CI->setCallingConv(RewindFunctionCallingConv);

(Rationale: Avoid repeating CallInst::Create(...) and CI->setCallingConv(...) for two cases.)

260–261

Move these two lines into if (doesRewindFunctionNeedExceptionObject()) (Line 271).

Add:

llvm::SmallVector<Value *, 1> RewindFunctionArgs;
279

Add:

RewindFunctionArgs.push_back(PN);
280–282

Move these out of the if body:

// Call the function.
CallInst *CI = CallInst::Create(RewindFunction, RewindFunctionArgs, "",
                                UnwindBB);
CI->setCallingConv(RewindFunctionCallingConv);
MaskRay added inline comments.
llvm/include/llvm/ADT/Triple.h
735 ↗(On Diff #381951)

Can Darwin or Windows use EHABI related environment? If not, remove !isOSDarwin() && !isOSWindows().
If possible, check isOSBinFormatELF() instead

mstorsjo added inline comments.
llvm/include/llvm/ADT/Triple.h
735 ↗(On Diff #381951)

At least windows can’t, afaik (I haven’t heard about anybody setting that up, and neither llvm nor libunwind support it).

danielkiss marked 8 inline comments as done.Oct 26 2021, 3:52 AM
danielkiss added inline comments.
llvm/lib/CodeGen/DwarfEHPrepare.cpp
237

Moved after the if (!RewindFunction) { because it need to be done if the RewindFunction is already set.
RewindFunctionCallingConv may not worth a class member..

248–253

Thanks for the rational, refactored in the spirit of it.
GetExceptionObject has sideeffects so I kept the original code structure.

logan added a comment.Oct 26 2021, 7:07 AM

Looks good to me. Please also fix the pre-merge lint checks.

logan accepted this revision.Oct 26 2021, 7:07 AM
danielkiss marked 2 inline comments as done.

Looks good to me. Please also fix the pre-merge lint checks.

Thanks for the reviews! Reformatted with newer clang-format.

logan requested changes to this revision.Oct 26 2021, 8:12 AM
logan added inline comments.
llvm/lib/CodeGen/DwarfEHPrepare.cpp
228–230

Thinking this further, I think we can no longer cache RewindFunction as a data member, since there may be functions with different personality functions in the same module. If we cache it in this way, we may get errors when some functions expect _Unwind_Resume and the others expect __cxa_end_cleanup (in the same module).

This revision now requires changes to proceed.Oct 26 2021, 8:12 AM

no longer cache RewindFunction.

danielkiss marked an inline comment as done.Oct 26 2021, 8:56 AM
logan accepted this revision.Oct 26 2021, 9:57 AM

Looks good to me.

This revision is now accepted and ready to land.Oct 26 2021, 9:57 AM
This revision was landed with ongoing or failed builds.Oct 27 2021, 1:40 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2021, 1:40 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript

I think this has caused a failure on our v7 bot: https://lab.llvm.org/buildbot/#/builders/186/builds/2047

llvm/test/CodeGen/XCore/exception.ll (doesn't show up on the armv7 quick bot because it doesn't build the XCore target)

chill added a subscriber: chill.Oct 27 2021, 5:12 AM
chill added inline comments.
libcxxabi/src/cxa_exception.cpp
382–384

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 added a subscriber: momchil.velikov.EditedOct 28 2021, 7:53 AM

@DavidSpickett, @chill Thanks, both fixed, patch relanded.
march is not enough to set Triplet.Environment and it falls back to the default, which is based on the host.

thakis added a subscriber: thakis.Oct 28 2021, 8:19 AM

This breaks tests: http://45.33.8.238/linux/59368/step_12.txt

Please take a look and revert for now if it takes a while to fix.

Independent of the test failure: We (chromium) update clang and libcxxabi independently of each other. That works fine with this change, yes?

logan added a comment.Oct 28 2021, 9:23 AM

Independent of the test failure: We (chromium) update clang and libcxxabi independently of each other. That works fine with this change, yes?

The libcxxabi must be integrated before the clang integration (because the __cxa_end_cleanup trampoline doesn't have unwind frame information).

https://lab.llvm.org/buildbot/#/builders/77/builds/10938 is broken after the patch
STEP: run lit tests [arm/aosp_coral-userdebug/AOSP.MASTER] (it's green but this is UI issue)

Only this test, we found offender for the rest:

FAIL: AddressSanitizer-arm-android :: TestCases/intercept-rethrow-exception.cpp (372 of 1466)
******************** TEST 'AddressSanitizer-arm-android :: TestCases/intercept-rethrow-exception.cpp' FAILED ********************
Script:
--
: 'RUN: at line 6';     /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/sanitizer_common/android_commands/android_compile.py  /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/bin/clang  --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only  --target=armv7-linux-androideabi24 --sysroot=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64  -B/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -stdlib=libc++ -fuse-ld=lld  -shared-libasan -fexceptions -O0 /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/intercept-rethrow-exception.cpp -o /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/intercept-rethrow-exception.cpp.tmp
: 'RUN: at line 7';    /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/intercept-rethrow-exception.cpp.tmp
--
Exit Code: 134
Command Output (stderr):
--
+ : 'RUN: at line 6'
+ /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/sanitizer_common/android_commands/android_compile.py /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only --target=armv7-linux-androideabi24 --sysroot=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -B/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -stdlib=libc++ -fuse-ld=lld -shared-libasan -fexceptions -O0 /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/intercept-rethrow-exception.cpp -o /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/intercept-rethrow-exception.cpp.tmp
+ : 'RUN: at line 7'
+ /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/intercept-rethrow-exception.cpp.tmp
Aborted 
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/intercept-rethrow-exception.cpp.script: line 2: 22341 Aborted                 (core dumped) /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/intercept-rethrow-exception.cpp.tmp
--
********************

@vitalybuka Thanks for the report. The problem could be that the libcxxabi is taken from the sysroot which contains the old code. so the compiler and the runtime is out of sync. what do think?

@vitalybuka Thanks for the report. The problem could be that the libcxxabi is taken from the sysroot which contains the old code. so the compiler and the runtime is out of sync. what do think?

@eugenis That's very likely. We test Android with a version of NDK against compiler HEAD. I will disabled a test on android unless someone have a better idea?

rprichard added inline comments.
libcxxabi/src/cxa_exception.cpp
382–384

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

Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 5:17 PM
danielkiss added inline comments.Mar 4 2022, 12:34 PM
libcxxabi/src/cxa_exception.cpp
382–384

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}
chill added inline comments.Mar 4 2022, 3:12 PM
libcxxabi/src/cxa_exception.cpp
382–384

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}
rprichard added inline comments.Mar 4 2022, 3:44 PM
libcxxabi/src/cxa_exception.cpp
382–384

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
rprichard added inline comments.Mar 4 2022, 4:02 PM
libcxxabi/src/cxa_exception.cpp
382–384

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

danielkiss added inline comments.Mar 8 2022, 5:31 AM
libcxxabi/src/cxa_exception.cpp
382–384

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

This looks good to me.