This is an archive of the discontinued LLVM Phabricator instance.

[libc++abi][AIX] Skip non-C++ EH aware frames when retrieving exception object
ClosedPublic

Authored by xingxue on Jan 31 2023, 1:24 PM.

Details

Summary

The personality routine for the legacy AIX xlclang++ compiler uses the stack slot reserved for compilers to pass the exception object to the landing pad. The landing pad retrieves the exception object with a call to the runtime function __xlc_exception_handle(). The current implementation incorrectly assumes that __xlc_exception_handle() should go up one stack frame to get to the stack frame of the caller of __xlc_exception_handle(), which is supposedly the stack frame of the function containing the landing pad. However, this does not always work, e.g., the xlclang++ compiler sometimes generates a wrapper of __xlc_exception_handle() and calls the wrapper from the landing pad for optimization purposes. This patch changes the implementation to unwind the stack from __xlc_exception_handle() and skip frames not associated with functions that are C++ EH-aware until a frame associated with a C++ EH-aware function is found and then retrieving the exception object with the expectation that the located frame is the one that the personality routine installed as it transferred control to the landing pad.

Diff Detail

Event Timeline

xingxue created this revision.Jan 31 2023, 1:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 1:25 PM
xingxue requested review of this revision.Jan 31 2023, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 1:25 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cebowleratibm added inline comments.Feb 1 2023, 7:30 AM
libcxxabi/src/aix_state_tab_eh.inc
653

unnecessary cast?

665

I'm not convinced this can be removed. The implementation still moves up at least as many frames as it did before.

672

is terminate the preferred solution? The exception won't be handled but we can't really say that the user code failed to catch the exception, rather an assertion failure condition has occurred in the runtime itself. My inclination was to call std::abort.

687–688

Why the change to __builtin_frame_address? Does this simply read r1 as before?

687–688

Suggest: just update callerStack rather than introduce a new variable. callerStack is dead afterwards as-is but the close names are just asking to introduce a future bug.

libcxxabi/test/vendor/ibm/aix_xlclang_passing_excp_obj_32.pass.sh.S
30

Should you also provide the t2.cpp source to demonstrate the wrap__xlc_exception_handle is a forwarder to __xlc_exception_handle?

cebowleratibm added inline comments.Feb 1 2023, 7:32 AM
libcxxabi/src/aix_state_tab_eh.inc
665

Suggest: add a print statement each time we skip a frame. In general this should only happen when the XL compiler injects a wrapper and it's probably of interest to the trace that we're bypassing wrapper frames enroute to the frame that contains the landing pad.

daltenty added inline comments.Feb 1 2023, 10:59 AM
libcxxabi/src/aix_state_tab_eh.inc
657

Is there any possibility of encountering a function without traceback?

xingxue marked 6 inline comments as done.Feb 1 2023, 1:13 PM
xingxue added inline comments.
libcxxabi/src/aix_state_tab_eh.inc
653

Right, removed the cast, thanks!

665

With the new implementation, __xlc_excpetion_handle() calls skipNonCxxEHAwareFrames(), which in turn calls std::abort(). force_a_stackframe() is no longer needed because __xlc_excpetion_handle() won't be a leaf function even if skipNonCxxEHAwareFrames() is inlined. I will add a comment in the code.

665

I was dumping this info in the prototype but it caused timeouts when running a large application because the xlclang++ compiler generates so many calls to __xlc_exception_handle(), noting we've already added more code to skip the non-C++ EH frames there. To print the function name, we need to parse the traceback table to reach the function name field, in addition to printing it out, etc. I will look into implementing a verbose mode to only dump certain info later.

672

Changed to std::abort() as suggested, thanks!

687–688

Yes, __builtin_frame_address(0) is the same as read r1 as before. However, we now need the return address. We can use inline asm code for that too but the inline asm code needs 3 instructions and the instructions are different for 32- and 64-bit. So, __builtin_return_address(0) is used instead. I replaced the inline asm code for the frame address with __builtin_frame_address(0) to be consistent.

687–688

Changed as suggested, thanks!

libcxxabi/test/vendor/ibm/aix_xlclang_passing_excp_obj_32.pass.sh.S
30

Good catch, thanks!

xingxue updated this revision to Diff 494052.Feb 1 2023, 1:17 PM
xingxue marked 6 inline comments as done.

Addressed comments:

  • removed an unnecessary cast
  • call std::abort() instead of std::terminate() when unwinding reaches the end of stack frames
  • update callerStack instead of using another variable callerStack2
  • added missing reference to t2.cpp source in the 32-bit test case
xingxue marked an inline comment as done.Feb 1 2023, 1:26 PM
xingxue added inline comments.
libcxxabi/src/aix_state_tab_eh.inc
657

When compiled with option -qtbtable=none, the resulting functions do not have a traceback table. As a result, the stack frame cannot be unwound so exception handling is disabled (see man page of -qtbtable).

hubert.reinterpretcast edited the summary of this revision. (Show Details)Feb 1 2023, 2:28 PM

I made changes to the patch description (which I expect we will be using for the commit message).

libcxxabi/src/aix_state_tab_eh.inc
671
libcxxabi/src/aix_state_tab_eh.inc
667

Suggestion:
Get the value of the LR (saved, prior to incrementing the SP, by the prolog of the function just inspected) from the frame.

xingxue marked an inline comment as done.Feb 1 2023, 4:31 PM

I made changes to the patch description (which I expect we will be using for the commit message).

Thanks, @hubert.reinterpretcast!

libcxxabi/src/aix_state_tab_eh.inc
665

If we are in a debugger or have a core file, we might be able to map back to the function using just the program counter value.

xingxue updated this revision to Diff 494286.Feb 2 2023, 6:34 AM
xingxue marked 3 inline comments as done.

Addressed comments:

  • changed a comment as suggested to be more descriptive
  • fixed the text of a trace print
libcxxabi/src/aix_state_tab_eh.inc
667

Changed, thanks for the suggestion!

671

Good catch, thanks!

xingxue updated this revision to Diff 494319.Feb 2 2023, 8:22 AM

Changed function name from skipNonCxxEHAwareFrames to skip_non_cxx_eh_aware_frames to be consistent with function naming in snake_case in this file.

daltenty accepted this revision.Feb 2 2023, 8:29 AM

LGTM, with a minor tweak

libcxxabi/src/aix_state_tab_eh.inc
673

Prefer just un-namespaced abort, since we don't have the standard library headers in the libcxxabi context.

685
xingxue marked 2 inline comments as done.Feb 2 2023, 9:05 AM
xingxue added inline comments.
libcxxabi/src/aix_state_tab_eh.inc
673

Good point, thanks!

685

Changed as suggested, thanks!

xingxue updated this revision to Diff 494333.Feb 2 2023, 9:07 AM
xingxue marked 2 inline comments as done.

Addressed comment:

  • Call abort() instead of std::abort().
xingxue updated this revision to Diff 494336.Feb 2 2023, 9:17 AM

Diff with context.

This patch is specific to the support for AIX legacy xlclang++ compiler generated code. It has been reviewed and approved by AIX compiler/runtime experts. If there are no objections, I will commit it tomorrow.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 15 2023, 8:15 AM
This revision was automatically updated to reflect the committed changes.