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

[Xing Xue via Phabricator via libcxx-commits]

xingxue marked 4 inline comments as done.
xingxue 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)
----------------
sfertile wrote:
> 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).
Added parameter `base` as suggested, thanks!


================
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);
----------------
sfertile wrote:
> 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)
There is an assert inside `readEncodedPointer()` in line 300 for using the `DW_EH_PE_datarel` encoding with `base`=0.  That should serve the same purpose.


================
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.
----------------
sfertile wrote:
> ditto on adding base argument to this call.
Added parameter `base` as suggested, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101298