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

[xingxue]

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.

• The state variable and state table data is the LSDA found from the traceback table of the function during unwinding (see https://reviews.llvm.org/D100132).

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

[cebowleratibm]

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.

[xingxue]

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]

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]

Fixed format according to git-clang-format.

[cebowleratibm]

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.

[ldionne]

@MaskRay Does this look reasonable to you?

[MaskRay]

In D100504#3419067, @ldionne wrote:

@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

See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

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

drop parentheses https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

1534

drop parentheses https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

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]

libcxxabi/src/cxa_personality.cpp

1949

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

[MaskRay]

libcxxabi/src/cxa_personality.cpp

1493

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

[hubert.reinterpretcast]

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]

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

[xingxue]

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]

Fixed problems in pre-merg check.

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

[xingxue]

Hi @MaskRay, Do you have further comments?

[MaskRay]

libcxxabi/src/aix_state_tab_eh.inc

64

67

71

#ifdef NDEBUG.

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

[xingxue]

Addressed comments:

• Fixed formatting.

[xingxue]

libcxxabi/src/aix_state_tab_eh.inc

64

Fixed, thanks!

67

Fixed, thanks!

71

Fixed, thanks!

[xingxue]

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]

but you need some from #libc_abi

[xingxue]

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

[ldionne]

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.

[xingxue]

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