[libcxx-commits] [PATCH] D100504: [libc++abi][AIX] add personality and helper functions for the state table EH
Xing Xue via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Feb 7 13:12:58 PST 2022
xingxue added inline comments.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1295
+
+// Code for the personality of the state table.
+
----------------
cebowleratibm wrote:
> Because the "state table" we're referring to is an IBM proprietary implementation there ought to be an abstract description of what it is and which compilers generate it.
Added a high level description of the state table.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1326
+
+typedef void (*destruct_f)(void*);
+
----------------
cebowleratibm wrote:
> suggest namespacing all of the types that pertain to the "state table" implementation.
Added namespace `__state_table_eh`.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1335
+ };
+ intptr_t count;
+ size_t elemSize;
----------------
cebowleratibm wrote:
> I suggest making this:
>
> ```
> union {
> FSMEntryCount flag;
> intptr_t count;
> }
>
> ```
> which makes overlapping use of the memory more clear.
Changed to:
```
union {
// The flag for actions (when the value is negative).
FSMEntryCount actionFlag;
// The element count (when the value is positive or zero).
intptr_t elementCount;
};
```
Also changed to use a union for the `offset` field.
union {
// Offset of the object within its stack frame or containing object.
intptr_t offset;
// Address of a global object.
intptr_t address;
// Address of the next state if it is an old conditional state change entry.
intptr_t nextStatePtr;
};
```
================
Comment at: libcxxabi/src/cxa_personality.cpp:1337
+ size_t elemSize;
+ uint16_t flags;
+ uint16_t nextState;
----------------
cebowleratibm wrote:
> should the flags be of type FSMEntryFlag? Noting FSMEntryFlag has type int16_t
Good point, changed to use type `FSMEntryFlag`. Thanks!
================
Comment at: libcxxabi/src/cxa_personality.cpp:1357
+enum FSMMagic : uint32_t {
+ number0 = 0xbeefdead, // xlC compiler
+ number2 = 0xbeeedead,
----------------
cebowleratibm wrote:
> why number0, 2 3? reads as though there a missing number1
These are mapped from the legacy runtime (`FSM_MAGIC`, `FSM_MAGIC2`, and `FSM_MAGIC3`) except that 'FSM_MAGIC' should have been mirrored to `number` rather than `number0`. Changed from `number0` to `number`.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1380
+ // the address is direct. (static local)
+ stateChange = 0x800, // State table entry is an indirect state
+ // change, dereference the address in
----------------
cebowleratibm wrote:
> IMO "stateChange" is poorly named. These flags exist for handling conditional state changes so that we know which path was taken at runtime. Suggest "conditionalStateChange" and "oldConditionalStateChange"
>
> Ex: "(b ? (T1(), foo()) : (T2(), foo())), throw 42;" This causes us to encounter a conditional state change so that we know if T1 or T2 need to be destroyed. I think the comments should be more helpful.
Changed the naming and added comments as suggested, thanks!
================
Comment at: libcxxabi/src/cxa_personality.cpp:1384
+ // The temporary is an automatic
+ thisFlag = 0x01,
+ vBaseFlag = 0x02,
----------------
cebowleratibm wrote:
> Suggest adding a comment.
>
> The address of the object for the cleanup action is based on the StateVariable::thisValue
Added the comment as suggested, thanks!
================
Comment at: libcxxabi/src/cxa_personality.cpp:1385
+ thisFlag = 0x01,
+ vBaseFlag = 0x02,
+ globalObj = 04
----------------
cebowleratibm wrote:
> 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.
Added the comment "The object is of a virtual base class.".
================
Comment at: libcxxabi/src/cxa_personality.cpp:1386
+ vBaseFlag = 0x02,
+ globalObj = 04
+};
----------------
cebowleratibm wrote:
> 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.
Changed as suggested. Also added description of the state table fields.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1392
+
+#define DTR_ARGUMENT 0x2 // Free virtual bases, don't delete object
+
----------------
cebowleratibm wrote:
> this is only used in __Invoke__Destructor. I recommend a static const local. The preprocessor is not needed here.
Changed as suggested, thanks.
================
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) "
----------------
cebowleratibm wrote:
> 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)
Good catch, fixed. Thanks and sorry for my oversight.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1556
+ "return _URC_FATAL_PHASE1_ERROR\n");
+ results.reason = _URC_FATAL_PHASE1_ERROR;
+ return;
----------------
cebowleratibm wrote:
> 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.
The state table is corrupted when this branch is entered so it makes sense to hard stop rather than quietly ignoring the frame IMHO.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1561
+ "return _URC_FATAL_PHASE2_ERROR\n");
+ results.reason = _URC_FATAL_PHASE2_ERROR;
+ return;
----------------
cebowleratibm wrote:
> Likewise, the legacy IBM unwinder would quietly skip over the frame.
Same as the comment above.
================
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);
----------------
cebowleratibm wrote:
> Minor suggestion: "Processing" -> "Searching" or similar so that the log more clearly denotes the search phase.
The debugging statement at the beginning of this function (line 1499) prints out the current action, search or cleanup. Then "Processing state=" is printed in a loop that processes the states.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1582
+
+ void* addr = compute_obj_addr(currEntry, state);
+
----------------
cebowleratibm wrote:
> `addr` is only used on the FSMEntryFlag::stateChange code path. I think it's better to push this down into that path so that we're not evaluating garbage addresses on the other paths, even if they're not used it's just something to go wrong in the future.
>
> It's also unnecessary to call the general compute_obj_addr on this path as we know exactly how to find the next state from the conditional state table entry. Suggest a new function compute_next_state or similar and use this function on both the search and cleanup phases.
Changed to call `compute_obj_addr` only in paths where `addr` is used.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1594
+ }
+ _LIBCXXABI_TRACE_STATETAB("Found a catch handler, "
+ "return _URC_HANDLER_FOUND\n");
----------------
cebowleratibm wrote:
> 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.
Added the comment as suggested, thanks!
================
Comment at: libcxxabi/src/cxa_personality.cpp:1601
+ "return _URC_HANDLER_FOUND\n");
+ results.reason = _URC_HANDLER_FOUND;
+ return;
----------------
cebowleratibm wrote:
> 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?
The legacy runtime EH has one phase and performs cleanups while unwinding the stack. I am inclined to be consistent with that behavior because this code is dealing with the frame generated by the legacy compilers. xlC or xlclang++. We can do it the other way if you think that is better.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1604
+ } else if (currEntry->flags & FSMEntryFlag::stateChangeOld) {
+ // Deplicated conditional expression.
+ currState = *((int*)(currEntry->offset));
----------------
cebowleratibm wrote:
>
> 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?)
Fixed. Also added a union member `nextStatePtr` in structure `FSMEntry`.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1629
+ return;
+ } else if (_UA_CLEANUP_PHASE & actions) {
+ // Start walking the table.
----------------
cebowleratibm wrote:
> suggest "else if" -> "if"
Changed as suggested.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1640
+
+ void* addr = compute_obj_addr(currEntry, state);
+
----------------
cebowleratibm wrote:
> similar comment to what I had for the search phase: some of the paths don't have meaningful address value computations. I think it's better to split those paths above the call to compute_obj_addr so that we only evaluate it on the paths where it's meaningful.
Changed as suggested, thanks!
================
Comment at: libcxxabi/src/cxa_personality.cpp:1654
+ // we changed a state.
+ } else if (currEntry->flags & FSMEntryFlag::stateChange) {
+ state->state = *((int*)(addr));
----------------
cebowleratibm wrote:
> I suggest changing the "else if" to an "if" because the preceding block always exits the if-stmt. Applies to multiple "else if"s here.
Changes as suggested, thanks.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1668
+ _LIBCXXABI_TRACE_STATETAB("Invoke dtor for object=%p\n", addr);
+ __Invoke__Destructor(currEntry, addr);
+ }
----------------
cebowleratibm wrote:
> The IBM XL proprietary unwinder adds an extra indirect when vBaseFlag is set. I don't see that logic in compute_obj_addr.
Good catch! The IBM XL proprietary compiler xlC uses the IBM object model which needs an extra indirect and the CXA model used by xlclang++ does not. Fortunately, the xlclang++ compiler generates inline destructor and therefore, we can safely assume it is an xlC generated frame when it gets here. Changing to have an extra indirect.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1674
+ } else if (currEntry->count == FSMEntryCount::beginCatch) {
+ // Branch to the landing pad to see if the exception can be
+ // caught.
----------------
cebowleratibm wrote:
> The comment is a bit misleading. We know that the catch will handle the exception and rethrow it if there's no catch match (I don't think this is valid behaviour, however, we're simply replicating what the XL behaviour was)
Added a table above to describe the actions.
================
Comment at: libcxxabi/src/cxa_personality.cpp:1687
+ // End current catch block and branch to the landing pad.
+ _LIBCXXABI_TRACE_STATETAB("FSMEntryCount::endCatch: handler "
+ "%p/%p, return _URC_HANDLER_FOUND\n",
----------------
cebowleratibm wrote:
> Aside from the trace statements I believe the beginCatch, endCatch and inlineDestructor paths do exactly the same thing: retrieve the landing pad address from the entry catchBlock and install the handler. I think it's best to share the code path for these 3 cases.
Changed as suggested, thanks!
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