[libcxx-commits] [PATCH] D100504: [libc++abi][AIX] initial patch for EH on AIX

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm added a comment.

More comments.



================
Comment at: libcxxabi/src/cxa_personality.cpp:1380
+                          // the address is direct. (static local)
+  stateChange = 0x800,    // State table entry is an indirect state
+                          // change, dereference the address in
----------------
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.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1582
+
+      void* addr = compute_obj_addr(currEntry, state);
+
----------------
`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.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1629
+    return;
+  } else if (_UA_CLEANUP_PHASE & actions) {
+    // Start walking the table.
----------------
suggest "else if" -> "if"


================
Comment at: libcxxabi/src/cxa_personality.cpp:1640
+
+      void* addr = compute_obj_addr(currEntry, state);
+
----------------
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.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1654
+                  // we changed a state.
+      } else if (currEntry->flags & FSMEntryFlag::stateChange) {
+        state->state = *((int*)(addr));
----------------
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.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1668
+          _LIBCXXABI_TRACE_STATETAB("Invoke dtor for object=%p\n", addr);
+          __Invoke__Destructor(currEntry, addr);
+        }
----------------
The IBM XL proprietary unwinder adds an extra indirect when vBaseFlag is set.  I don't see that logic in compute_obj_addr.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1674
+      } else if (currEntry->count == FSMEntryCount::beginCatch) {
+        // Branch to the landing pad to see if the exception can be
+        // caught.
----------------
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)


================
Comment at: libcxxabi/src/cxa_personality.cpp:1687
+        // End current catch block and branch to the landing pad.
+        _LIBCXXABI_TRACE_STATETAB("FSMEntryCount::endCatch: handler "
+                                  "%p/%p, return _URC_HANDLER_FOUND\n",
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100504