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

[Xing Xue via Phabricator via libcxx-commits]

xingxue added inline comments.


================
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
----------------
cebowleratibm wrote:
> Clarification suggestions:
> suggest "...also has an autolocal state variable" 
> suggest "...and is found through the traceback table..."
Changes as suggested, thanks!


================
Comment at: libcxxabi/src/cxa_personality.cpp:1389
+  deleteObject = -3,
+  inlineDestructor = -4,
+  terminate = -5
----------------
cebowleratibm wrote:
> "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.
Changed `inline_destructor` to `cleanupLabel` as discussed off line. Term "inline destructor" has been changed in comments as well.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1439
+    // The element count (when the value is positive or zero).
+    size_t elementCount;
+  };
----------------
cebowleratibm wrote:
> There's an implicit expectation that size_t and intptr_t are the same size.  Is it better to use 
> ```
> uintptr_t elementCount
> ```?
`uintptr_t` is a type that is guaranteed to hold pointer `void*`, whether it is 32-bit or 64-bit. `elementCount` is a count so I think `size_t` is better.


================
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
----------------
cebowleratibm wrote:
> 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.
> 
> 
Modified comments as suggested, thanks!


================
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));
----------------
cebowleratibm wrote:
> 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.
Added the comment as suggested, thanks!


================
Comment at: libcxxabi/src/cxa_personality.cpp:1791
+        _LIBCXXABI_TRACE_STATETAB("Flag: FSMEntryFlag::"
+                                  "onditionalStateChange, dereference "
+                                  "addr(%p), set state=%d\n",
----------------
cebowleratibm wrote:
> typo "onditionalStateChange"
Fixed, thanks for catching it!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100504