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

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm added inline comments.


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


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


================
Comment at: libcxxabi/src/aix_state_tab_eh.inc:672
+  _LIBCXXABI_TRACE_STATETAB0("skipNonCxxEHAwareFrames() reached the end of stack frames, terminating\n");
+  std::terminate();
+}
----------------
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.


================
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().
----------------
Why the change to __builtin_frame_address?  Does this simply read r1 as before?


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143010