[libcxx-commits] [PATCH] D101298: [libc++abi][AIX] Enable calculating addresses with DW_EH_PE_datarel

[Sean Fertile via Phabricator via libcxx-commits]

sfertile added inline comments.


================
Comment at: libcxxabi/src/cxa_personality.cpp:654
     uint8_t lpStartEncoding = *lsda++;
     const uint8_t* lpStart = (const uint8_t*)readEncodedPointer(&lsda, lpStartEncoding);
     if (lpStart == 0)
----------------
IIUC this is going to work for AIX because the encoding is `DW_EH_PE_omit`, but if we are adding support for `DW_EH_PE_datarel then we need to pass base in here. If someone comes along and adds a target that uses data relative encoding for the offset to the lsda then it should already work (after this patch).


================
Comment at: libcxxabi/src/cxa_personality.cpp:683
         // The call sites are ordered in increasing value of start
         uintptr_t start = readEncodedPointer(&callSitePtr, callSiteEncoding);
         uintptr_t length = readEncodedPointer(&callSitePtr, callSiteEncoding);
----------------
Does it make sense to add an assert here that `callSiteEncoding` is not data relative for documentation as why we use the default value of `base=0`. (Sorry this should;ld have probably gone in the previous patch)


================
Comment at: libcxxabi/src/cxa_personality.cpp:971
     set_registers(unwind_exception, context, results);
+    // Cache base for calculating the address of ttype in __cxa_call_unexpected.
+    if (results.ttypeIndex < 0) {
----------------
xingxue wrote:
> MaskRay wrote:
> > Normal Itanium doesn't need/shouldn't have this code. `_UA_CLEANUP_PHASE` doesn't need caching.
> > 
> > Can you re-confirm AIX needs this? (I suspect this is not the right place to add the code.)
> For `AIX` `EH`, the `DW_EH_PE_datarel` encoding is used in the range table to calculate relative addresses with a `base`.  So, parameter `base` is added to functions invoked by `scan_eh_tab()` and `readEncodedPointer()` is extended to enable the `DW_EH_PE_datarel` encoding.
> 
> For code such as the segment below, the `landingpad` calls `__cxa_call_unexpected()` which in turn calls the unexpected exception handler.  When `__cxa_call_unexpected()` handles the exception thrown from the unexpected exception handler, it calls `exception_spec_can_catch()` where a `base` is needed for the `DW_EH_PE_datarel` encoding. `__cxa_call_unexpected()` takes one parameter which is the pointer to the exception object.  So, we need a way to communicate the `base` to `__cxa_call_unexpected()`. `catchTemp` is used for caching the `landingpad` but after `get_registers()` sets the `landingpad` into the `IP` register, the `landingpad` cached in `catchTemp` is no longer useful. This is why this location is chosen. The `__cxa_exception` structure has being used in communicating `lsda`, `ttypeIndex`, etc. between the personality routine and `__cxa_call_unexpected()`.  So, I think it makes sense to use `catchTemp` which is also a member of `__cxa_exception` for this purpose as well but I am open to suggestions.
> 
> ```
> struct E {
>         int i;
>         E() { i=0; }
> };
> 
> struct S {
>         S() {}
>         S(int i) {}
>         ~S() { ret = std::uncaught_exception(); }
> };
> 
> void f(void) throw (E) {
>     register S s;
>     throw 1;
> }
> ```
> Normal Itanium doesn't need/shouldn't have this code. `_UA_CLEANUP_PHASE` doesn't need caching.
> 
> Can you re-confirm AIX needs this? (I suspect this is not the right place to add the code.)

FWIW it makes sense that targets that use `DW_EH_PE_pcrel` or `DW_EH_PE_absptr` don't need the caching. The absptr encoding you don't add any base address, and the pcrel encoding you use the data pointer you just read from as the base address. 


================
Comment at: libcxxabi/src/cxa_personality.cpp:1199
             uint8_t lpStartEncoding = *lsda++;
             const uint8_t* lpStart = (const uint8_t*)readEncodedPointer(&lsda, lpStartEncoding);
             (void)lpStart;  // purposefully unused.  Just needed to increment lsda.
----------------
ditto on adding base argument to this call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101298