[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:1352
+#    define _LIBCXXABI_TRACE_STATETAB0(_args...)
+#    define _LIBCXXABI_TRACING_STATETAB (0)
+#  else
----------------
MaskRay wrote:
> 
Changed as suggested, thanks!


================
Comment at: libcxxabi/src/cxa_personality.cpp:1354
+#  else
+bool StateTabDbg() {
+  static bool checked = false;
----------------
MaskRay wrote:
> Add static? Function naming generally uses `snake_case` in this file.
Added `static` and changed function name to `state_tab_dbg`. Thanks!


================
Comment at: libcxxabi/src/cxa_personality.cpp:1379
+
+typedef void (*destruct_f)(void*);
+
----------------
MaskRay wrote:
> Prefer `using` to `typedef`
Changed to use `using`, thanks!


================
Comment at: libcxxabi/src/cxa_personality.cpp:1412
+// The finite state machine to be walked.
+struct FSMEntry {
+  union {
----------------
MaskRay wrote:
> See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces
Used anonymous namespace for 3 local structures, thanks!


================
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.
----------------
MaskRay wrote:
> 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`
Changed to use `constexpr` and renamed `gRegisterNum` to `REG_EXCP_OBJ`.


================
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));
----------------
MaskRay wrote:
> Try avoiding `__` (reserved identifiers) for functions not specified by ABI.
> 
> Pick a `snake_case` function name.
Renamed `__Invoke__Destructor` and `__Invoke__Delete` to `invoke_destructor` and `invoke_delete` respectively.


================
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 {
----------------
MaskRay wrote:
> `reinterpret_cast<void*>` can be removed. This applies to many places in the file.
The compiler issues a warning if `reinterpret_cast<void*>` is removed.
```
 warning: format specifies type 'void *' but the argument has type '__cxxabiv1::__state_table_eh::FSMEntry *' [-Wformat-pedantic]
```


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


================
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
----------------
MaskRay wrote:
> 
`get_frame_addr` calls `_Unwind_GetIP` and `_Unwind_GetGR` with `context` as an argument.  Adding `const` will cause mismatches because `_Unwind_GetIP` and `_Unwind_GetGR` do not have the `const` qualifier for argument `context`.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1511
+  while (*p)
+    p++;
+  tbtable* TBTable = reinterpret_cast<tbtable*>(p + 1);
----------------
MaskRay wrote:
> The coding standard prefers pre-increment `++p`.
> 
> ditto below
Fixed this and other occurrences, thanks for pointing out!


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


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


================
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) {
----------------
MaskRay wrote:
> 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.
Function `scan_state_tab` is indeed called only once.  It is separated from `__xlcxx_personality_v0` so that the structure of the state table personality is the same as that of the Itanium personality. This makes the code easier to compare and understand.

I tried to place the whole body into `__xlcxx_personality_v0` and the number of `if` branches is decreased by 2. These `if` branches are ones in the original `__xlcxx_personality_v0` code because it now returns directly from the code in `scan_state_tab` without going through  `__xlcxx_personality_v0`.

With that, I am inclined to keep the structure as is.


================
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;
----------------
MaskRay wrote:
> 
Fixed, thanks!


================
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");
----------------
MaskRay wrote:
> Delete the comment since the `_LIBCXXABI_TRACE_STATETAB` self explains.
Done, thanks!


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


================
Comment at: libcxxabi/src/cxa_personality.cpp:1946
+
+// This function is for xlclang++. It aborts when a deleted virtual
+// function is called.
----------------
MaskRay wrote:
> `xlclang++ may generate calls to __Deleted_Virtual` may be clearer.
Changed as suggested, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100504