Page MenuHomePhabricator

[Arm][Unwind][libc++abi] Add _Unwind_ForcedUnwind to EHABI.
Needs ReviewPublic

Authored by danielkiss on Oct 16 2020, 11:04 AM.

Details

Reviewers
ajwong
compnerd
ldionne
steven_wu
phosek
Group Reviewers
Restricted Project
Restricted Project
Summary

_Unwind_ForcedUnwind is not mandated by the EHABI but for compatibilty
reasons adding so the interface to higher layers would be the same.

Diff Detail

Event Timeline

danielkiss requested review of this revision.Oct 16 2020, 11:04 AM
danielkiss created this revision.
danielkiss created this object with visibility "danielkiss (Daniel Kiss)".
danielkiss changed the visibility from "danielkiss (Daniel Kiss)" to "Public (No Login Required)".

_Unwind_Stop_Fn shall not have different signature for EHABI.

danielkiss planned changes to this revision.Oct 17 2020, 1:54 PM

Add a simple test and fixes.
This version passes on arm32 - armv7l-unknown-linux-gnueabihf -

https://bugs.llvm.org/show_bug.cgi?id=47142 this effects this. the missing unwind info make forceunwind fragile.

Hi Daniel,

We tested this patch in Chrome OS and it resolved our build issues :) . We do have issues on ARM32 with some crashes but we believe that is a separate problem. And we'll continue to look into those.
But I feel it should be ok for you to submit this or request a review from more qualified folks.

danielkiss retitled this revision from [WIP][Arm][Unwind] Add _Unwind_ForcedUnwind to EHABI. to [Arm][Unwind] Add _Unwind_ForcedUnwind to EHABI..Dec 16 2020, 8:31 AM
danielkiss added reviewers: ajwong, Restricted Project.

@manojgupta Thanks for the feedback, let me know if you have updates.

libunwind/src/Unwind-EHABI.cpp
720

Because there was no forceunwind and only in that case the reserved1 is written.

libunwind/test/forceunwind.pass.cpp
41

typo.

libunwind/test/libunwind/test/config.py
46 ↗(On Diff #298857)

done already in upstream.

rebased, fixed my own comments.

I think ARMEH _Unwind_ForcedUnwind can probably just reuse the generic implementation.

@danielkiss Hi, I've added libc++abi forced unwinding tests in D95200. Does this implementation make the tests pass?

(Why does ARMEH uses a typedef so that struct _Unwind_Exception* does not compile? )

ikudrin added a subscriber: ikudrin.Feb 3 2021, 2:13 AM

@MaskRay Thanks for the test case.

I think ARMEH _Unwind_ForcedUnwind can probably just reuse the generic implementation.

The personality handling is a bit different. Might not be much nicer with that many ifdefs.

@danielkiss Hi, I've added libc++abi forced unwinding tests in D95200. Does this implementation make the tests pass?

It passes now.

(Why does ARMEH uses a typedef so that struct _Unwind_Exception* does not compile? )

I think the original typedef struct _Unwind_Control_Block comes from the spec:
https://github.com/ARM-software/abi-aa/blob/f52e1ad3f81254497a83578dc102f6aac89e52d0/ehabi32/ehabi32.rst#82language-independent-unwinding-types-and-functions
_Unwind_Control_Block could be just _Unwind_Exception.
IMHO there is no nice way to achieve the struct _Unwind_Exception* compile.

danielkiss added inline comments.Feb 4 2021, 11:07 AM
libunwind/include/unwind.h
73

this is not nice, but right know I don't see any other way around. please let me know if you have any better idea.

danielkiss added a reviewer: Restricted Project.Feb 4 2021, 11:35 AM
danielkiss retitled this revision from [Arm][Unwind] Add _Unwind_ForcedUnwind to EHABI. to [Arm][Unwind][libc++abi] Add _Unwind_ForcedUnwind to EHABI..

Reenables tests ( revert of a4fa667dee60 )

jgorbe added a subscriber: jgorbe.May 3 2021, 7:03 PM
jroelofs added inline comments.
libunwind/include/unwind.h
73

What's the issue that this solves?

libunwind/src/Unwind-EHABI.cpp
725

splitting hairs, but should long here be intptr_t?

libunwind/src/UnwindLevel1-gcc-ext.c
33

might be more clear to combine with if/else chain below and not rely on the PRIVATE_1 => private_1 => private_[0] expansion.

libunwind/test/forceunwind.pass.cpp
41

IIRC this needs special handling on baremetal, since there is no dladdr there.

danielkiss updated this revision to Diff 343181.May 5 2021, 1:58 PM

Rebased, retested on ToT.

Hi Daniel,

We tested this patch in Chrome OS and it resolved our build issues :) . We do have issues on ARM32 with some crashes but we believe that is a separate problem. And we'll continue to look into those.
But I feel it should be ok for you to submit this or request a review from more qualified folks.

Now we are confirmed the crash is a separate, unrelated problem.

ARM EHABI is very different from the Itanium implementation. Someone should move the implementation to a separate file.

ARM EHABI is very different from the Itanium implementation. Someone should move the implementation to a separate file.

Sorry, do you mean the content of the libunwind/include/unwind.h ?

libunwind/include/unwind.h
73

struct _Unwind_Exception* is used mostly but the definition of _Unwind_Control_Block comes from the Arm EHABI. with the typedef struct _Unwind_Exception* won't work.

libunwind/src/Unwind-EHABI.cpp
725

right, will fix.

libunwind/src/UnwindLevel1-gcc-ext.c
33

agree, I'll update it.

libunwind/test/forceunwind.pass.cpp
41

for now I add a filter for the test it should run only on linux as other tests here.

jroelofs added inline comments.May 7 2021, 8:09 AM
libunwind/test/forceunwind.pass.cpp
41

sounds good

danielkiss updated this revision to Diff 343702.May 7 2021, 9:39 AM
danielkiss marked 5 inline comments as done.

@jroelofs Thanks for the review, all addressed.

ARM EHABI is very different from the Itanium implementation. Someone should move the implementation to a separate file.

Sorry, do you mean the content of the libunwind/include/unwind.h ?

I mean all the #if defined(_LIBCXXABI_ARM_EHABI) branches make the file difficult to read. reduce sharing and move the implementation to a separate file.
Many sanitizer files are organized this way.

ARM EHABI is very different from the Itanium implementation. Someone should move the implementation to a separate file.

Sorry, do you mean the content of the libunwind/include/unwind.h ?

I mean all the #if defined(_LIBCXXABI_ARM_EHABI) branches make the file difficult to read. reduce sharing and move the implementation to a separate file.
Many sanitizer files are organized this way.

I will create another patch for the that without functional changes.