Page MenuHomePhabricator

[libc++abi][AIX] add personality and helper functions for the state table EH
ClosedPublic

Authored by xingxue on Apr 14 2021, 12:52 PM.

Details

Summary

This patch adds the personality and helper functions for the state table based EH used by IBM legacy compilers xlC and xlclang++ on AIX.

  • A high level description of the state table based EH is provided in the code comments.
  • Function scan_state_tab() is added to scan the state table. It is invoked by the state table personality routine __xlcxx_personality_v0() and returns scan_results like scan_eh_tab() does.
  • A couple of EH helper functions used by xlC and xlclang++ generated code are also added, e.g., __xlc_catch_matchv2() which checks whether the thrown object matches the catch handler's exception type.
  • Debugging macros _LIBCXXABI_TRACE_STATETAB, _LIBCXXABI_TRACE_STATETAB0, and _LIBCXXABI_TRACING_STATETAB are added to dump state table scanning traces if environment variable LIBCXXABI_PRINT_STATTAB is set.
  • No issues were found when running tests of the libunwind and libcxxabi test suites. LIT test cases have been added for code specific to AIX EH.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
cebowleratibm requested changes to this revision.Feb 22 2022, 10:17 AM

Looks good. I didn't find any semantic issues but I did have a number of suggestions on further refinements for clarity of the code. A vast improvement over the proprietary XL code btw! Thanks Xing.

libcxxabi/src/cxa_personality.cpp
1312

Clarification suggestions:
suggest "...also has an autolocal state variable"
suggest "...and is found through the traceback table..."

1392

"inline destructor" is an XL proprietary term. What we're really saying is there's a label in the state table entry and the runtime should simply install the "landing pad".

I would suggest: "landingPadHandler" or similar and also replace the term "inline destructor" in any of the comments.

1442

There's an implicit expectation that size_t and intptr_t are the same size. Is it better to use

uintptr_t elementCount
```?
1722

Suggest removing "xlC" from reference here since xlC frames are handled by the code block above.

Suggest further comment clarification: It's important to note that xlclang++ compiled frames use cxa-abi EH calls and we know that any catch block will include a catch(...) so we can safely say handler found without running a catch match. Unfortunately this makes the search phase less useful but it's all we can do and no worse than the current runtime EH for xlclang++ programs.

1791

The code reads strange here: "compute_obj_addr", what object is in the state table entry?

Perhaps a comment like: A conditional state table entry holds the address of a local that holds the next state to unwind to.

1794

typo "onditionalStateChange"

1826

compute_obj_addr seems to be overloaded for "computeMagicalAddress" and the caller happens to know what the address means based on a bunch of flags.

Either only call compute_obj_addr when there is an actual object address returned or change the name of the function to something like "compute_addr_from_table" so that the name doesn't imply it always returns an object address.

This revision now requires changes to proceed.Feb 22 2022, 10:17 AM
xingxue updated this revision to Diff 412104.Mar 1 2022, 7:53 AM
xingxue marked 7 inline comments as done.
xingxue added a reviewer: ldionne.

Addressed comments.

  • Updated code comments;
  • Renamed variable inlineDestructor to cleanupLabel. Replaced the term "inline destructor" with "cleanup code";
  • Renamed function compute_obj_addr to compute_addr_from_table.
xingxue added inline comments.Mar 1 2022, 7:54 AM
libcxxabi/src/cxa_personality.cpp
1312

Changes as suggested, thanks!

1392

Changed inline_destructor to cleanupLabel as discussed off line. Term "inline destructor" has been changed in comments as well.

1442

uintptr_t is a type that is guaranteed to hold pointer void*, whether it is 32-bit or 64-bit. elementCount is a count so I think size_t is better.

1722

Modified comments as suggested, thanks!

1791

Added the comment as suggested, thanks!

1794

Fixed, thanks for catching it!

1826

Changed compute_obj_addr to compute_addr_from_table.

xingxue updated this revision to Diff 412406.Mar 2 2022, 7:08 AM

Fixed format according to git-clang-format.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 7:08 AM
cebowleratibm accepted this revision.Mar 29 2022, 1:13 PM

I've had a thorough review and access to the proprietary XL "state table EH" code and I believe this patch is ready to land, nonetheless, I do not have the authority to grant permission to commit this patch. This patch should not affect targets other than AIX and Xing has already run thorough proprietary test coverage against this implementation. Essentially this is a tidied-up version of the EH runtime support we already provide with the libc++ we distribute for AIX.

@MaskRay Does this look reasonable to you?

@MaskRay Does this look reasonable to you?

Sorry for the belated response. I was on trips for the past two weeks and spent little time on reading patches.

I know nearly nothing about AIX. scan_state_tab and the personality routine have similar structures with the Itanium implementation, and the structure looks good to me.
There are many many extra stuff specific to AIX. I think we need to leave that to the domain expert.
Left some code styles comments inline.
From the structure of the file cxa_personality.cpp, I think it is cleaner to move the large AIX #ifdef #endif to a separate file.
The separate file can be a .cpp file, or if the impl needs to reuse many static functions, an .inc file (llvm and compiler-rt have a dozen .inc files. I can see that libc++/libc++abi haven't adopted this type of files yet).

libcxxabi/src/cxa_personality.cpp
1355
1357

Add static? Function naming generally uses snake_case in this file.

1382

Prefer using to typedef

1415
1465

Just use constexpr. The non-template variable of non-volatile const-qualified type having namespace-scope has internal linkage even in the absence of static. Also fix the style of gRegisterNum

1468

Try avoiding __ (reserved identifiers) for functions not specified by ABI.

Pick a snake_case function name.

1469

reinterpret_cast<void*> can be removed. This applies to many places in the file.

1489

*reinterpret_cast does not need parentheses. This applies to many places in the file.

1493

I don't think the personality impl is supposed to use try catch. You may use other mechanisms to avoid try catch.

1507
1514

The coding standard prefers pre-increment ++p.

ditto below

1525
1534
1579

The function is called only once and is not necessarily in a separate function.

For the Itanium impl, I think some if conditions can be reworked to simplify overall complexity.

You can try placing the whole body into __xlcxx_personality_v0 and check whether the total number of if branches can be decreased.

1581
1883

Delete the comment since the _LIBCXXABI_TRACE_STATETAB self explains.

1944

delete blank line

MaskRay added inline comments.Apr 9 2022, 9:50 PM
libcxxabi/src/cxa_personality.cpp
1949

xlclang++ may generate calls to __Deleted_Virtual may be clearer.

MaskRay added inline comments.Apr 9 2022, 10:05 PM
libcxxabi/src/cxa_personality.cpp
1493

You can use something like st_terminate in cxa_vector.cpp to catch exceptions in destructors.

libcxxabi/src/cxa_personality.cpp
1493

@MaskRay, is there a specific reason you can give for why try/catch should not be used here?

It may be worth noting that the expected build compiler for this code uses a personality routine that is not the one being implemented here. It may also be worth noting that this is less part of the personality routine and more an action that could be thought of as taking place in the corresponding application frame's context (except that, instead of transferring control to code in that frame, the encoded action is called from the personality routine).

That a search phase triggered by throwing an exception during a stack unwinding action finds this try/catch seems compatible with the behaviour seen with other implementations: Unwinding for the new exception happens until the stack unwinding action actually exits via an exception.

It is not the case that entry into this personality routine necessarily means that a new search phase will find a frame with a handler. It is a desirable property that this try/catch is present for the new search phase to consider that there is a handler (and thus perform unwinding).

As for the st_terminate suggestion, the current usage of st_terminate seems to be the source of https://github.com/llvm/llvm-project/issues/55032. That is, entering a handler prior to the call to terminate, as the code here does, is desirable.

xingxue updated this revision to Diff 427622.May 6 2022, 7:11 AM
xingxue marked 17 inline comments as done.

Addressed comments. Moved the implementation for AIX state table based EH into a separate file aix_state_tab_eh.inc as suggested.

xingxue added inline comments.May 6 2022, 7:13 AM
libcxxabi/src/cxa_personality.cpp
1355

Changed as suggested, thanks!

1357

Added static and changed function name to state_tab_dbg. Thanks!

1382

Changed to use using, thanks!

1415

Used anonymous namespace for 3 local structures, thanks!

1465

Changed to use constexpr and renamed gRegisterNum to REG_EXCP_OBJ.

1468

Renamed __Invoke__Destructor and __Invoke__Delete to invoke_destructor and invoke_delete respectively.

1469

The compiler issues a warning if reinterpret_cast<void*> is removed.

warning: format specifies type 'void *' but the argument has type '__cxxabiv1::__state_table_eh::FSMEntry *' [-Wformat-pedantic]
1489

Fixed this and other occurrences, thanks!

1507

get_frame_addr calls _Unwind_GetIP and _Unwind_GetGR with context as an argument. Adding const will cause mismatches because _Unwind_GetIP and _Unwind_GetGR do not have the const qualifier for argument context.

1514

Fixed this and other occurrences, thanks for pointing out!

1525

Fixed, thanks!

1534

Fixed, thanks!

1579

Function scan_state_tab is indeed called only once. It is separated from __xlcxx_personality_v0 so that the structure of the state table personality is the same as that of the Itanium personality. This makes the code easier to compare and understand.

I tried to place the whole body into __xlcxx_personality_v0 and the number of if branches is decreased by 2. These if branches are ones in the original __xlcxx_personality_v0 code because it now returns directly from the code in scan_state_tab without going through __xlcxx_personality_v0.

With that, I am inclined to keep the structure as is.

1581

Fixed, thanks!

1883

Done, thanks!

1944

Done.

1949

Changed as suggested, thanks!

xingxue updated this revision to Diff 429322.May 13 2022, 12:17 PM

Fixed problems in pre-merg check.

  • Avoid using GCC extension named variadic macros;
  • Avoid using C99 feature flexible array members.

Hi @MaskRay, Do you have further comments?

MaskRay added inline comments.May 19 2022, 4:51 PM
libcxxabi/src/aix_state_tab_eh.inc
64
67
71

#ifdef NDEBUG.

It is not nested, and should have spaces between # and ifdef.

xingxue updated this revision to Diff 430970.May 20 2022, 7:51 AM

Addressed comments:

  • Fixed formatting.
xingxue marked 3 inline comments as done.May 20 2022, 7:53 AM
xingxue added inline comments.
libcxxabi/src/aix_state_tab_eh.inc
64

Fixed, thanks!

67

Fixed, thanks!

71

Fixed, thanks!

xingxue marked 3 inline comments as done.EditedMay 24 2022, 7:10 AM

Hi @MaskRay, Do you have further comments? If not, would you be able to approve this patch? My colleague @cebowleratibm, who is an AIX compiler and runtime expert, has done a thorough review of AIX specific contents and approved it (see comment https://reviews.llvm.org/D100504#3414984). This implementation has been run through the proprietary test coverage and included in the IBM Open XL C/C++ for AIX V17.1 compiler released in Sept. 2021.

MaskRay accepted this revision.May 24 2022, 10:54 PM

but you need some from #libc_abi

Thanks very much, @MaskRay!
@ldionne Do you have further comments? If not, would you be able to approve for #libc_abi?

ldionne accepted this revision as: Restricted Project.May 30 2022, 9:56 AM

Deferring to @MaskRay for approval. The AIX-specific contact surface with libc++abi has been reduced to the minimum via the #include, which I like.

This revision is now accepted and ready to land.May 30 2022, 9:56 AM

@MaskRay, @cebowleratibm, @ldionne, @hubert.reinterpretcast, @jasonliu: Thank you so much for your help, time and effort in reviewing, and constructive suggestions. All of these make the patch much better than it was. Thanks again!