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

[Fangrui Song via Phabricator via libcxx-commits]

MaskRay added a comment.

In D100504#3419067 <https://reviews.llvm.org/D100504#3419067>, @ldionne wrote:

> @MaskRay Does this look reasonable to you?

Sorry for the belated response. I was on trips for the past two weeks and spent little time on reading patches.

I know nearly nothing about AIX. `scan_state_tab` and the personality routine have similar structures with the Itanium implementation, and the structure looks good to me.
There are many many extra stuff specific to AIX. I think we need to leave that to the domain expert.
Left some code styles comments inline.
>From the structure of the file cxa_personality.cpp, I think it is cleaner to move the large AIX `#ifdef #endif` to a separate file.
The separate file can be a .cpp file, or if the impl needs to reuse many `static` functions, an `.inc` file (llvm and compiler-rt have a dozen `.inc` files. I can see that libc++/libc++abi haven't adopted this type of files yet).



================
Comment at: libcxxabi/src/cxa_personality.cpp:1352
+#    define _LIBCXXABI_TRACE_STATETAB0(_args...)
+#    define _LIBCXXABI_TRACING_STATETAB (0)
+#  else
----------------



================
Comment at: libcxxabi/src/cxa_personality.cpp:1354
+#  else
+bool StateTabDbg() {
+  static bool checked = false;
----------------
Add static? Function naming generally uses `snake_case` in this file.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1379
+
+typedef void (*destruct_f)(void*);
+
----------------
Prefer `using` to `typedef`


================
Comment at: libcxxabi/src/cxa_personality.cpp:1412
+// The finite state machine to be walked.
+struct FSMEntry {
+  union {
----------------
See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: libcxxabi/src/cxa_personality.cpp:1462
+
+static const size_t dtrArgument = 0x02; // Flag to destructor indicating free
+                                        // virtual bases, don't delete object.
----------------
Just use constexpr. The non-template variable of non-volatile const-qualified type having namespace-scope has internal linkage even in the absence of `static`. Also fix the style of `gRegisterNum`


================
Comment at: libcxxabi/src/cxa_personality.cpp:1465
+
+static void __Invoke__Destructor(FSMEntry* fsmEntry, void* addr) {
+  _LIBCXXABI_TRACE_STATETAB("Destruct object=%p, fsmEntry=%p\n", addr, reinterpret_cast<void*>(fsmEntry));
----------------
Try avoiding `__` (reserved identifiers) for functions not specified by ABI.

Pick a `snake_case` function name.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1466
+static void __Invoke__Destructor(FSMEntry* fsmEntry, void* addr) {
+  _LIBCXXABI_TRACE_STATETAB("Destruct object=%p, fsmEntry=%p\n", addr, reinterpret_cast<void*>(fsmEntry));
+  try {
----------------
`reinterpret_cast<void*>` can be removed. This applies to many places in the file.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1486
+static void __Invoke__Delete(FSMEntry* fsmEntry, void* addr) {
+  char* objectAddress = *(reinterpret_cast<char**>(addr));
+
----------------
`*reinterpret_cast` does not need parentheses. This applies to many places in the file.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1490
+                            reinterpret_cast<void*>(fsmEntry));
+  try {
+    _LIBCXXABI_TRACE_STATETAB("..calling delete()\n");
----------------
I don't think the personality impl is supposed to use `try catch`. You may use other mechanisms to avoid try catch.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1504
+// which is at the end of each function.
+static uintptr_t get_frame_addr(_Unwind_Context* context) {
+  int framePointerReg = 1; // default frame pointer == SP
----------------



================
Comment at: libcxxabi/src/cxa_personality.cpp:1511
+  while (*p)
+    p++;
+  tbtable* TBTable = reinterpret_cast<tbtable*>(p + 1);
----------------
The coding standard prefers pre-increment `++p`.

ditto below


================
Comment at: libcxxabi/src/cxa_personality.cpp:1522
+  if (TBTable->tb.has_tboff) {
+    p++;
+  }
----------------
drop parentheses https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: libcxxabi/src/cxa_personality.cpp:1531
+  if (TBTable->tb.has_ctl) {
+    p += 1 + *p;
+  }
----------------
drop parentheses https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: libcxxabi/src/cxa_personality.cpp:1576
+
+static void scan_state_tab(scan_results& results, _Unwind_Action actions, bool native_exception,
+                           _Unwind_Exception* unwind_exception, _Unwind_Context* context) {
----------------
The function is called only once and is not necessarily in a separate function.

For the Itanium impl, I think some `if` conditions can be reworked to simplify overall complexity.

You can try placing the whole body into `__xlcxx_personality_v0` and check whether the total number of `if` branches can be decreased.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1578
+                           _Unwind_Exception* unwind_exception, _Unwind_Context* context) {
+  // Initialize results to found nothing but an error
+  results.ttypeIndex = 0;
----------------



================
Comment at: libcxxabi/src/cxa_personality.cpp:1880
+  if (!__isOurExceptionClass(exceptionObject)) {
+    // If it is not a c++ exception, return 0 to indicate no match
+    _LIBCXXABI_TRACE_STATETAB("No match, not a C++ exception\n");
----------------
Delete the comment since the `_LIBCXXABI_TRACE_STATETAB` self explains.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1941
+  uintptr_t exceptionObject;
+
+  asm("mr %0, 14" : "=r"(exceptionObject));
----------------
delete blank line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100504