This is an archive of the discontinued LLVM Phabricator instance.

[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
jasonliu added inline comments.Apr 21 2021, 1:04 PM
libcxxabi/src/cxa_personality.cpp
1769

This should be marked as "always inline" to reduce the call overhead.

In sanitizer runtime, we create separate files and duplicate some needed functions when a platform has sufficient different implementation. See compiler-rt/lib/sanitizer_common/*.cpp and compiler-lib/lib/lsan/lsan_*.cpp for examples.

I have simplified the Itanium LSDA a lot. From my experience the code sharing with arm EHABI actually made it really different to refactor.

@MaskRay Thanks for pointing out the sanitizer files! We will take a look. Yeah, scan_eh_tab and gxx_personality_v0 are much simpler now.

xingxue updated this revision to Diff 340532.Apr 26 2021, 8:33 AM

Changes:

  • Split off changes for enabling calculating relative addresses with the DW_EH_PE_datarel encoding using a base (https://reviews.llvm.org/D101298).
  • Reverted to making __xlcxx_personality_v1 an alias of __gxx_personality_v0.
xingxue marked an inline comment as done.Apr 26 2021, 8:36 AM
xingxue added inline comments.
libcxxabi/src/cxa_personality.cpp
1769

Reverted to making __xlcxx_personality_v1 an alias of __gxx_personality_v0.

xingxue marked an inline comment as done.Apr 26 2021, 8:36 AM
cebowleratibm requested changes to this revision.Jun 20 2021, 8:07 PM

Are there any tests planned for this patch? We would have to commit XL binaries, or I think a better approach would be to write C code with a user defined eh fsm table.

libcxxabi/src/cxa_personality.cpp
1253

I think this inclusion is better at the top of the file.

1271

Isn't this on line 1248?

1282

I find it a bit awkward to see the matching #ifdef
suggest: #endif // NDEBUG

1302

Suggestion: use flexible array member.

1313

I don't see a compelling reason to use macros for these values. I suggest a typed enum for FSM::magic that defines the FSM_MAGIC values as eumerators.

1317

I don't see a compelling reason to use macros for these values. I suggest a typed enum that defines these values as enumerators, then union that with a size_t member. I think this makes the contract FSMEntry::count more clear.

1323

suggest: FSMEntry::flags should be typed enum and define these flag values as enumerators.

1353

is it appropriate to assert that count won't be [-5, -1] or can those count values legitimately show up here?

1407

I don't think a line break is needed here.

This revision now requires changes to proceed.Jun 20 2021, 8:07 PM
jasonliu added inline comments.Jun 21 2021, 7:58 AM
libcxxabi/src/cxa_personality.cpp
1271

Yes. It wasn't there before my comment. Xing addressed my comment. Thanks.

libcxxabi/src/cxa_personality.cpp
1781

I think a call to placement-new should be added.

xingxue updated this revision to Diff 366648.Aug 16 2021, 1:24 PM

Addressed comments:

  • Move the include of AIX headers to the top of the file.
  • Use flexible array in struct FSM.
  • Use typed enum in place of macros.
  • Use placement-new in function __cxa_allocate_exception() to get object newexception constructed properly.

Other:

  • The personality for the state table terminates unwinding when it finds a catch handler compiled by the legacy xlC compiler.
  • Move EH helper functions for xlclang++ compiled code to the same code section.
xingxue marked 11 inline comments as done.Aug 16 2021, 1:35 PM
xingxue added inline comments.
libcxxabi/src/cxa_personality.cpp
1271

Including of headers for AIX is moved to the top of the file.

1282

Changed as suggested. Thanks!

1302

Use flexible array as suggested, thanks!

1313

Changed to use typed enum.

1317

Changed to use typed enum.

1323

Changed to use typed enum.

1353

When __Invoke__Destructor() is called, fsmEntry->count is greater than 0. See line 1662.

1781

Good catch, thanks!

Early comments. Continuing to work through the review.

libcxxabi/src/cxa_personality.cpp
1251

Because the "state table" we're referring to is an IBM proprietary implementation there ought to be an abstract description of what it is and which compilers generate it.

1282

suggest namespacing all of the types that pertain to the "state table" implementation.

1291

I suggest making this:

union {
  FSMEntryCount flag;
  intptr_t count;
}

which makes overlapping use of the memory more clear.

1293

should the flags be of type FSMEntryFlag? Noting FSMEntryFlag has type int16_t

Review in still on-going.

libcxxabi/src/cxa_personality.cpp
1313

why number0, 2 3? reads as though there a missing number1

1340

Suggest adding a comment.

The address of the object for the cleanup action is based on the StateVariable::thisValue

1341

Also suggest adding a comment but I don't have a suggestion at this time. It looks like it tells the unwinder an extra indirect is required to arrive at the virtual base to evaluate the address of the object for the cleanup action.

1342

Although I know this wasn't documented in the internal IBM source I think we should provide a comment

// The table entry offset is the start address of the object.

1348

this is only used in InvokeDestructor. I recommend a static const local. The preprocessor is not needed here.

1404

The XL code adds in the value in the frame pointer register, which you're no longer doing here. Can you explain the difference? (same question for the else path below)

1512

Noting a behaviour difference to confirm: I expect the legacy IBM code would have quietly skipped over the frame. This return would force fail the unwind.

1517

Likewise, the legacy IBM unwinder would quietly skip over the frame.

1535

Minor suggestion: "Processing" -> "Searching" or similar so that the log more clearly denotes the search phase.

1550

The missing catch matching logic is ominous. Suggest adding a comment that we know that any XL compiled catch handler always appended a catch(...) handler so we know that any catch will match the exception.

1557

Are we better to delay the call to terminate and begin the cleanup phase or is it better to call terminate before any cleanup actions are performed?

1560

I also suggest adding a union type for the conditional state table entry so that the code reads cleaner rather than chasing seemingly random fields in the code. (we happen to know the offset field holds a pointer to the state before the conditional state?)

More comments.

libcxxabi/src/cxa_personality.cpp
1336

IMO "stateChange" is poorly named. These flags exist for handling conditional state changes so that we know which path was taken at runtime. Suggest "conditionalStateChange" and "oldConditionalStateChange"

Ex: "(b ? (T1(), foo()) : (T2(), foo())), throw 42;" This causes us to encounter a conditional state change so that we know if T1 or T2 need to be destroyed. I think the comments should be more helpful.

1538

addr is only used on the FSMEntryFlag::stateChange code path. I think it's better to push this down into that path so that we're not evaluating garbage addresses on the other paths, even if they're not used it's just something to go wrong in the future.

It's also unnecessary to call the general compute_obj_addr on this path as we know exactly how to find the next state from the conditional state table entry. Suggest a new function compute_next_state or similar and use this function on both the search and cleanup phases.

1585

suggest "else if" -> "if"

1596

similar comment to what I had for the search phase: some of the paths don't have meaningful address value computations. I think it's better to split those paths above the call to compute_obj_addr so that we only evaluate it on the paths where it's meaningful.

1610

I suggest changing the "else if" to an "if" because the preceding block always exits the if-stmt. Applies to multiple "else if"s here.

1624

The IBM XL proprietary unwinder adds an extra indirect when vBaseFlag is set. I don't see that logic in compute_obj_addr.

1630

The comment is a bit misleading. We know that the catch will handle the exception and rethrow it if there's no catch match (I don't think this is valid behaviour, however, we're simply replicating what the XL behaviour was)

1643

Aside from the trace statements I believe the beginCatch, endCatch and inlineDestructor paths do exactly the same thing: retrieve the landing pad address from the entry catchBlock and install the handler. I think it's best to share the code path for these 3 cases.

xingxue updated this revision to Diff 406538.Feb 7 2022, 1:11 PM
xingxue marked 32 inline comments as done.
xingxue retitled this revision from [libc++abi][AIX] initial patch for EH on AIX to [libc++abi][AIX] add personality and helper functions for the state table EH.
xingxue edited the summary of this revision. (Show Details)
xingxue edited reviewers, added: compnerd; removed: jasonliu.

Addressed comments.

  • Used unions where the same memory is used for different purposes to make overlapping use of the memory more clear.
  • Renamed variables to be more descriptive.
  • Added a high level description of the state table based EH.
  • Added comments in various locations.
  • Fixed a problem where the frame pointer is needed to calculate the address.
  • Fixed the address calculation of virtual base object for calling destructor or deletor to be based on the IBM object model used by the proprietary xlC compiler because these paths are only used for xlC generated code.
  • Commoned up some code to reduce redundancy.
xingxue added inline comments.Feb 7 2022, 1:12 PM
libcxxabi/src/cxa_personality.cpp
1251

Added a high level description of the state table.

1282

Added namespace __state_table_eh.

1291

Changed to:

union {
  // The flag for actions (when the value is negative).
  FSMEntryCount actionFlag;
  // The element count (when the value is positive or zero).
  intptr_t elementCount;
};

Also changed to use a union for the offset field.

union {
  // Offset of the object within its stack frame or containing object.
  intptr_t offset;
  // Address of a global object.
  intptr_t address;
  // Address of the next state if it is an old conditional state change entry.
  intptr_t nextStatePtr;
};
1293

Good point, changed to use type FSMEntryFlag. Thanks!

1313

These are mapped from the legacy runtime (FSM_MAGIC, FSM_MAGIC2, and FSM_MAGIC3) except that 'FSM_MAGIC' should have been mirrored to number rather than number0. Changed from number0 to number.

1336

Changed the naming and added comments as suggested, thanks!

1340

Added the comment as suggested, thanks!

1341

Added the comment "The object is of a virtual base class.".

1342

Changed as suggested. Also added description of the state table fields.

1348

Changed as suggested, thanks.

1404

Good catch, fixed. Thanks and sorry for my oversight.

1512

The state table is corrupted when this branch is entered so it makes sense to hard stop rather than quietly ignoring the frame IMHO.

1517

Same as the comment above.

1535

The debugging statement at the beginning of this function (line 1499) prints out the current action, search or cleanup. Then "Processing state=" is printed in a loop that processes the states.

1538

Changed to call compute_obj_addr only in paths where addr is used.

1550

Added the comment as suggested, thanks!

1557

The legacy runtime EH has one phase and performs cleanups while unwinding the stack. I am inclined to be consistent with that behavior because this code is dealing with the frame generated by the legacy compilers. xlC or xlclang++. We can do it the other way if you think that is better.

1560

Fixed. Also added a union member nextStatePtr in structure FSMEntry.

1585

Changed as suggested.

1596

Changed as suggested, thanks!

1610

Changes as suggested, thanks.

1624

Good catch! The IBM XL proprietary compiler xlC uses the IBM object model which needs an extra indirect and the CXA model used by xlclang++ does not. Fortunately, the xlclang++ compiler generates inline destructor and therefore, we can safely assume it is an xlC generated frame when it gets here. Changing to have an extra indirect.

1630

Added a table above to describe the actions.

1643

Changed as suggested, thanks!

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
1259

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

1339

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

1389

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

uintptr_t elementCount
```?
1669

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.

1738

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.

1741

typo "onditionalStateChange"

1773

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
1259

Changes as suggested, thanks!

1339

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

1389

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.

1669

Modified comments as suggested, thanks!

1738

Added the comment as suggested, thanks!

1741

Fixed, thanks for catching it!

1773

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
1302
1304

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

1329

Prefer using to typedef

1362
1412

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

1415

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

Pick a snake_case function name.

1416

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

1436

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

1440

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

1454
1461

The coding standard prefers pre-increment ++p.

ditto below

1472
1481
1526

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.

1528
1830

Delete the comment since the _LIBCXXABI_TRACE_STATETAB self explains.

1891

delete blank line

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

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
1440

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

libcxxabi/src/cxa_personality.cpp
1440

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

Changed as suggested, thanks!

1304

Added static and changed function name to state_tab_dbg. Thanks!

1329

Changed to use using, thanks!

1362

Used anonymous namespace for 3 local structures, thanks!

1412

Changed to use constexpr and renamed gRegisterNum to REG_EXCP_OBJ.

1415

Renamed __Invoke__Destructor and __Invoke__Delete to invoke_destructor and invoke_delete respectively.

1416

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]
1436

Fixed this and other occurrences, thanks!

1454

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.

1461

Fixed this and other occurrences, thanks for pointing out!

1472

Fixed, thanks!

1481

Fixed, thanks!

1526

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.

1528

Fixed, thanks!

1830

Done, thanks!

1891

Done.

1896

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
63 ↗(On Diff #429322)
66 ↗(On Diff #429322)
70 ↗(On Diff #429322)

#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
63 ↗(On Diff #429322)

Fixed, thanks!

66 ↗(On Diff #429322)

Fixed, thanks!

70 ↗(On Diff #429322)

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!