[libcxx-commits] [PATCH] D100504: [libc++abi][AIX] add personality and helper functions for the state table EH
Fangrui Song via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Apr 9 21:48:09 PDT 2022
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
More information about the libcxx-commits
mailing list