[libcxx-commits] [PATCH] D100504: [libc++abi][AIX] initial patch for EH on AIX

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm added a comment.

Review in still on-going.



================
Comment at: libcxxabi/src/cxa_personality.cpp:1357
+enum FSMMagic : uint32_t {
+  number0 = 0xbeefdead, // xlC compiler
+  number2 = 0xbeeedead,
----------------
why number0, 2 3? reads as though there a missing number1


================
Comment at: libcxxabi/src/cxa_personality.cpp:1384
+                          // The temporary is an automatic
+  thisFlag = 0x01,
+  vBaseFlag = 0x02,
----------------
Suggest adding a comment.

The address of the object for the cleanup action is based on the StateVariable::thisValue


================
Comment at: libcxxabi/src/cxa_personality.cpp:1385
+  thisFlag = 0x01,
+  vBaseFlag = 0x02,
+  globalObj = 04
----------------
Also suggest adding a comment but I don't have a suggestion at this time.  It looks like it tells the unwinder an extra indirect is required to arrive at the virtual base to evaluate the address of the object for the cleanup action.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1386
+  vBaseFlag = 0x02,
+  globalObj = 04
+};
----------------
Although I know this wasn't documented in the internal IBM source I think we should provide a comment

// The table entry offset is the start address of the object.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1392
+
+#define DTR_ARGUMENT 0x2 // Free virtual bases, don't delete object
+
----------------
this is only used in __Invoke__Destructor.  I recommend a static const local.  The preprocessor is not needed here.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1448
+  } else if (fsmEntry->flags & FSMEntryFlag::indirect) {
+    addr = (void*)*(char**)(fsmEntry->offset);
+    _LIBCXXABI_TRACE_STATETAB("Address calculation (indirect obj) "
----------------
The XL code adds in the value in the frame pointer register, which you're no longer doing here.  Can you explain the difference? (same question for the else path below)


================
Comment at: libcxxabi/src/cxa_personality.cpp:1556
+                                "return _URC_FATAL_PHASE1_ERROR\n");
+      results.reason = _URC_FATAL_PHASE1_ERROR;
+      return;
----------------
Noting a behaviour difference to confirm: I expect the legacy IBM code would have quietly skipped over the frame.  This return would force fail the unwind.  


================
Comment at: libcxxabi/src/cxa_personality.cpp:1561
+                                "return _URC_FATAL_PHASE2_ERROR\n");
+      results.reason = _URC_FATAL_PHASE2_ERROR;
+      return;
----------------
Likewise, the legacy IBM unwinder would quietly skip over the frame.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1579
+      FSMEntry* currEntry = &fsm->table[currState - 1];
+      _LIBCXXABI_TRACE_STATETAB("Processing state=%d, flags=0x%hx\n", currState,
+                                currEntry->flags);
----------------
Minor suggestion: "Processing" -> "Searching" or similar so that the log more clearly denotes the search phase.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1594
+        }
+        _LIBCXXABI_TRACE_STATETAB("Found a catch handler, "
+                                  "return _URC_HANDLER_FOUND\n");
----------------
The missing catch matching logic is ominous.  Suggest adding a comment that we know that any XL compiled catch handler always appended a catch(...) handler so we know that any catch will match the exception.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1601
+                                  "return _URC_HANDLER_FOUND\n");
+        results.reason = _URC_HANDLER_FOUND;
+        return;
----------------
Are we better to delay the call to terminate and begin the cleanup phase or is it better to call terminate before any cleanup actions are performed?


================
Comment at: libcxxabi/src/cxa_personality.cpp:1604
+      } else if (currEntry->flags & FSMEntryFlag::stateChangeOld) {
+        // Deplicated conditional expression.
+        currState = *((int*)(currEntry->offset));
----------------

I also suggest adding a union type for the conditional state table entry so that the code reads cleaner rather than chasing seemingly random fields in the code. (we happen to know the offset field holds a pointer to the state before the conditional state?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100504