[libcxx-commits] [PATCH] D143010: [libc++abi][AIX] Skip non-C++ EH aware frames when retrieving exception object

[Xing Xue via Phabricator via libcxx-commits]

xingxue marked 6 inline comments as done.
xingxue added inline comments.


================
Comment at: libcxxabi/src/aix_state_tab_eh.inc:653
+  while (currentPc != 0) {
+    uint32_t* p = reinterpret_cast<uint32_t*>(currentPc);
+
----------------
cebowleratibm wrote:
> unnecessary cast?
Right, removed the cast, thanks!


================
Comment at: libcxxabi/src/aix_state_tab_eh.inc:665
-  // frame for this function.
-  force_a_stackframe();
-
----------------
cebowleratibm wrote:
> cebowleratibm wrote:
> > I'm not convinced this can be removed.  The implementation still moves up at least as many frames as it did before.
> 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.
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.


================
Comment at: libcxxabi/src/aix_state_tab_eh.inc:665
+
+    // Move up one stack frame.
+    currentStack = reinterpret_cast<uintptr_t*>(currentStack[0]);
----------------
xingxue wrote:
> cebowleratibm wrote:
> > cebowleratibm wrote:
> > > I'm not convinced this can be removed.  The implementation still moves up at least as many frames as it did before.
> > 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.
> 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.
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.


================
Comment at: libcxxabi/src/aix_state_tab_eh.inc:672
+  _LIBCXXABI_TRACE_STATETAB0("skipNonCxxEHAwareFrames() reached the end of stack frames, terminating\n");
+  std::terminate();
+}
----------------
cebowleratibm wrote:
> 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.
Changed to `std::abort()` as suggested, thanks!


================
Comment at: libcxxabi/src/aix_state_tab_eh.inc:685
   // Get the SP of this function, i.e., __xlc_exception_handle().
-  uintptr_t *lastStack;
-  asm("mr %0, 1" : "=r"(lastStack));
-  // Get the SP of the caller of __xlc_exception_handle().
-  uintptr_t *callerStack = reinterpret_cast<uintptr_t*>(lastStack[0]);
-  // Get the SP of the caller of the caller.
+  uintptr_t* lastStack = reinterpret_cast<uintptr_t*>(__builtin_frame_address(0));
+  // Move one frame up to the frame of the caller of __xlc_exception_handle().
----------------
cebowleratibm wrote:
> Why the change to __builtin_frame_address?  Does this simply read r1 as before?
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.


================
Comment at: libcxxabi/src/aix_state_tab_eh.inc:695
+  // Get the SP of the caller of the C++ EH aware caller.
   uintptr_t *callerStack2 = reinterpret_cast<uintptr_t*>(callerStack[0]);
+  // Retrieve the exception object in the stack slot saved by the personality.
----------------
cebowleratibm wrote:
> 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.
Changed as suggested, thanks!


================
Comment at: libcxxabi/test/vendor/ibm/aix_xlclang_passing_excp_obj_32.pass.sh.S:29
+#
+# t1.cpp:
+#
----------------
cebowleratibm wrote:
> Should you also provide the t2.cpp source to demonstrate the `wrap__xlc_exception_handle` is a forwarder to `__xlc_exception_handle`?
Good catch, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143010/new/

https://reviews.llvm.org/D143010