This is an archive of the discontinued LLVM Phabricator instance.

[libc++abi][AIX] Enable calculating addresses with DW_EH_PE_datarel
ClosedPublic

Authored by xingxue on Apr 26 2021, 7:57 AM.

Details

Summary

This patch enables calculating relative addresses with the DW_EH_PE_datarel encoding using a base for AIX. After setting registers for jumping to the user code in gxx_personality_v0(), base is cached in exception_header member catchTemp for use in __cxa_call_unexpected if ttypeIndex is less than 0 (exception spec).

Diff Detail

Event Timeline

xingxue requested review of this revision.Apr 26 2021, 7:57 AM
xingxue created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2021, 7:57 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
xingxue updated this revision to Diff 340621.Apr 26 2021, 1:19 PM

Removed the incorrect setting of parent revision. Reload the patch to trigger the pre-merge check. Sorry for the noise.

sfertile added inline comments.Apr 26 2021, 6:10 PM
libcxxabi/src/cxa_personality.cpp
91

errant whitespace change.

250

I'm guessing these indentation changes, and brace placement changes are from clang format, but it no longer matches the formatting of the rest of the file. We shouldn't be changing formatting on otherwise untouched lines, and the changes we do make should match the formatting of the rest of the file.

xingxue updated this revision to Diff 341224.Apr 28 2021, 8:48 AM
xingxue edited the summary of this revision. (Show Details)

This version is clean of format changes made by clang-format.

xingxue updated this revision to Diff 341579.Apr 29 2021, 10:52 AM
xingxue edited the summary of this revision. (Show Details)

Refreshed to base on the split out NFC patch https://reviews.llvm.org/D101545.

This version is clean of format changes made by clang-format.

Thanks Xing, this is much better.

xingxue updated this revision to Diff 342443.May 3 2021, 10:20 AM

Refreshed after changes in the base NFC patch https://reviews.llvm.org/D101545.

MaskRay added inline comments.May 12 2021, 3:09 PM
libcxxabi/src/cxa_personality.cpp
972

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.)

xingxue added inline comments.May 13 2021, 1:36 PM
libcxxabi/src/cxa_personality.cpp
972

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;
}

Hi @MaskRay, Do you have any further comments/concerns? Thanks!

sfertile added inline comments.Jun 9 2021, 8:00 AM
libcxxabi/src/cxa_personality.cpp
654–655

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).

684

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)

972

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.

1200–1201

ditto on adding base argument to this call.

xingxue updated this revision to Diff 351208.Jun 10 2021, 10:23 AM
xingxue marked an inline comment as done.
xingxue added a reviewer: compnerd.

Addressed comments:

  • Added parameter base to readEncodedPointer() calls.
xingxue marked 4 inline comments as done.Jun 10 2021, 10:59 AM
xingxue added inline comments.
libcxxabi/src/cxa_personality.cpp
654–655

Added parameter base as suggested, thanks!

684

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.

1200–1201

Added parameter base as suggested, thanks!

xingxue updated this revision to Diff 351224.Jun 10 2021, 11:17 AM
xingxue marked 3 inline comments as done.

Removed the dependent patch which has landed. Try to trigger the pre-merge check.

sfertile accepted this revision.Jun 10 2021, 11:24 AM

Thanks for the updates Xing, LGTM.

xingxue updated this revision to Diff 352730.Jun 17 2021, 8:17 AM

Minor changes related to the -Wunused-parameter warning.

  • Reverted the change in D104235 that was to avoid the warning because the parameter is used in this patch.
  • Changed from uintptr_t base = 0 to uintptr_t /*base*/ = 0 in 2 places to avoid the warning.
compnerd accepted this revision.Jun 23 2021, 1:05 PM
compnerd added inline comments.
libcxxabi/src/cxa_personality.cpp
1281

It would be nice to use the reserved spelling (__attribute__((__alias__("__gxx_personality_v0"))))

This revision is now accepted and ready to land.Jun 23 2021, 1:05 PM
xingxue marked an inline comment as done.Jun 23 2021, 2:44 PM
xingxue added inline comments.
libcxxabi/src/cxa_personality.cpp
1281

Changed as suggested, thanks!

This revision was automatically updated to reflect the committed changes.
xingxue marked an inline comment as done.