This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

_Unwind_ForcedUnwind is not mandated by the EHABI but for compatibilty
reasons adding so the interface to higher layers would be the same.
Dropping EHABI specific _Unwind_Stop_Fn definition since it is not defined by EHABI.

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
40

typo.

libunwind/test/libunwind/test/config.py
46

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
29

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
40

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
29

agree, I'll update it.

libunwind/test/forceunwind.pass.cpp
40

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
40

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.

MaskRay added a comment.EditedJul 21 2021, 10:19 AM

Thanks for the libunwind split D106461 :)

danielkiss planned changes to this revision.Jul 21 2021, 11:41 AM

Thanks for the libunwind split D106461 :)

No problem, I'm going to rebase this one.

danielkiss edited the summary of this revision. (Show Details)

Rebase

MaskRay accepted this revision.Aug 3 2021, 1:59 PM

I understand Itanium _Unwind_ForcedUnwind. The ARM EH impl has a similar structure. LG

libunwind/include/unwind.h
151

delete excess space

libunwind/src/Unwind-EHABI.cpp
698

This can trigger clang -Wunused-but-set-variable (from 2021-04 onwards)

Don't define functionName in the #ifdef NDEBUG branch

711

C++ doesn't need the elaborated type specifier struct

1092

Delete the comment

libunwind/test/forceunwind.pass.cpp
27

Not needed

This revision is now accepted and ready to land.Aug 3 2021, 1:59 PM
danielkiss updated this revision to Diff 364463.Aug 5 2021, 7:36 AM
danielkiss marked 3 inline comments as done.

Thanks for the review, comments are addressed.

libunwind/src/Unwind-EHABI.cpp
698

This pattern need to be fixed all over libunwind. I'll patch it separately.

libunwind/test/forceunwind.pass.cpp
27

line 38 won't compile without it.

danielkiss added inline comments.
libunwind/src/Unwind-EHABI.cpp
698

fix is here: D107835

This revision was landed with ongoing or failed builds.Aug 11 2021, 1:16 AM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 11 2021, 1:16 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
manojgupta added a comment.EditedAug 24 2021, 12:26 PM

@danielkiss

We have been trying to debug 1 issue on ARM32 Chrome OS devices related to pthread_cancel.
After some debugging, I found that glibc uses char[8] for the exception class [1] but libunwind is using uin64_t.
This causes crashes because of ABI mismatch when pthread_cancel calls Unwind routines.

e.g. look at the following code in libunwind which calls unwind_stop from glibc:

(*stop)(1, action, exception_object->exception_class, exception_object,
       (_Unwind_Context *)(cursor), stop_parameter);

This mismatches the glibc's expected prototype for for unwind_stop [2] in argument 3 (exception class should be char * vs uint64_t).

Here is some data I got from a sample program.

(gdb) bt
#0  unwind_stop (version=1, actions=10, exc_class=0x0, exc_obj=0x0, context=0xf74ff628, stop_parameter=0xf74fe648)
    at unwind.c:44
#1  0xf7f77bdc in unwind_phase2_forced (uc=0xf74fe7d0, cursor=0xf74fe648, exception_object=0xf74ff628, 
    stop=0xf7fab3f1 <unwind_stop>, stop_parameter=0xf74fed78)
    at /build/elm/tmp/portage/sys-libs/llvm-libunwind-13.0_pre428724-r2/work/llvm-libunwind-13.0_pre428724/libunwind/src/Unwind-EHABI.cpp:716
#2  0xf7f77df2 in _Unwind_ForcedUnwind (exception_object=0xf74ff628, stop=0xf7fab3f1 <unwind_stop>, 
    stop_parameter=0xf74fed78)
    at /build/elm/tmp/portage/sys-libs/llvm-libunwind-13.0_pre428724-r2/work/llvm-libunwind-13.0_pre428724/libunwind/src/Unwind-EHABI.cpp:1100
#3  0xf7fab4d4 in __GI___pthread_unwind (buf=<optimized out>) at unwind.c:121
#4  0xf7fa22b0 in __do_cancel () at ./pthreadP.h:313

unwind_stop parameters show context=0xf74ff628, it should be the next value shown for stop_parameter instead i.e. 0xf74fe648.

(gdb) ptype exc_class
type = char *

this shows char * was expected in glibc instead of uint64_t.

[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/arm/unwind.h;h=ffdea8047d5c6202bdc775548c27b640c36430ae;hb=HEAD#l194
[2] https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/unwind.c;h=8f157e49f4a088ac64722e85ff24514fff7f3c71;hb=HEAD#l40

@danielkiss

We have been trying to debug 1 issue on ARM32 Chrome OS devices related to pthread_cancel.
After some debugging, I found that glibc uses char[8] for the exception class [1] but libunwind is using uin64_t.
This causes crashes because of ABI mismatch when pthread_cancel calls Unwind routines.

Thanks for the report, I submitted a patch to fix the type D109047