[libcxx-commits] [PATCH] D100504: [libc++abi][AIX] add personality and helper functions for the state table EH

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm requested changes to this revision.
cebowleratibm added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: libcxxabi/src/cxa_personality.cpp:1309
+  variable. The state variable represents the current state of the function
+  for EH and is found in the traceback table of the function during unwinding.
+  The FSM is an array of state entries. Each state entry has the following
----------------
Clarification suggestions:
suggest "...also has an autolocal state variable" 
suggest "...and is found through the traceback table..."


================
Comment at: libcxxabi/src/cxa_personality.cpp:1389
+  deleteObject = -3,
+  inlineDestructor = -4,
+  terminate = -5
----------------
"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.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1439
+    // The element count (when the value is positive or zero).
+    size_t elementCount;
+  };
----------------
There's an implicit expectation that size_t and intptr_t are the same size.  Is it better to use 
```
uintptr_t elementCount
```?


================
Comment at: libcxxabi/src/cxa_personality.cpp:1719
+        }
+        // xlC or xlclang++ compiled catch handlers are always appended with
+        // a catch(...) handler so that any catch will match the exception
----------------
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.




================
Comment at: libcxxabi/src/cxa_personality.cpp:1788
+      if (currEntry->flags & FSMEntryFlag::conditionalStateChange) {
+        void* addr = compute_obj_addr(currEntry, state, context);
+        state->state = *(reinterpret_cast<int*>(addr));
----------------
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.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1791
+        _LIBCXXABI_TRACE_STATETAB("Flag: FSMEntryFlag::"
+                                  "onditionalStateChange, dereference "
+                                  "addr(%p), set state=%d\n",
----------------
typo "onditionalStateChange"


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100504