This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][AIX] Enable ABI list checking for XCOFF
ClosedPublic

Authored by Jake-Egan on Apr 21 2022, 6:29 AM.

Details

Reviewers
daltenty
ldionne
Group Reviewers
Restricted Project
Commits
rG3af7aa520271: [libcxx][AIX] Enable ABI list checking for XCOFF
Summary

The existing nm extractors can't dump the loader symbol table information we need to do the ABI checks for XCOFF, so provide an implementation using the system dump utility. We match the symbol name, whether it's defined, it's import/export status, and its storage mapping class.

Diff Detail

Event Timeline

Jake-Egan created this revision.Apr 21 2022, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 6:29 AM
Jake-Egan requested review of this revision.Apr 21 2022, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 6:29 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Jake-Egan edited the summary of this revision. (Show Details)Apr 21 2022, 6:30 AM
Jake-Egan added a reviewer: daltenty.
daltenty added a comment.EditedApr 21 2022, 7:38 AM

It's something unique to AIX that we add shared objects to archives for our usual shared library implementation. I'm guessing with this implementation of the patch, we'll end up dumping all symbols together regardless of what archive member they belong to?

(Perhaps that's an acceptable limitation for the moment, since in the libc++ build we will only typically have the one shared object)

Jake-Egan updated this revision to Diff 424485.Apr 22 2022, 8:23 AM

Added 32 bit abilist and fixed a typo.

Jake-Egan updated this revision to Diff 424498.Apr 22 2022, 9:10 AM

Removed -X32_64 flag

It's something unique to AIX that we add shared objects to archives for our usual shared library implementation. I'm guessing with this implementation of the patch, we'll end up dumping all symbols together regardless of what archive member they belong to?

(Perhaps that's an acceptable limitation for the moment, since in the libc++ build we will only typically have the one shared object)

Yes, it dumps all symbols regardless of the archive member it's from. I could update the patch to address this if it's necessary.

ldionne accepted this revision.Apr 25 2022, 2:06 PM
ldionne added a subscriber: ldionne.

LGTM with a few nitpicks. I checked a CI run on AIX and it seems like we are indeed running the check-cxx-abilist target on it now.

libcxx/utils/libcxx/sym_check/extract.py
284

Isn't that equivalent but simpler?

289
This revision is now accepted and ready to land.Apr 25 2022, 2:06 PM

-addressed comments
-added call to AIXDumpExtractor that I forgot to add earlier..

Jake-Egan marked an inline comment as done.Apr 26 2022, 10:20 AM
Jake-Egan added inline comments.
libcxx/utils/libcxx/sym_check/extract.py
284

Thanks for the review. I believe 'a' in ext is the way to simplify because os.path.splitext() returns a tuple of size 2.

Jake-Egan added inline comments.Apr 26 2022, 10:43 AM
libcxx/utils/libcxx/sym_check/extract.py
284

My mistake, you're right. ext is one element.

Simplify '.a' extension check

Jake-Egan marked an inline comment as done.Apr 26 2022, 10:46 AM

@ldionne It's worth noting what's in this patch right now won't be our final ABI lists unfortunately. Our build compiler currently doesn't implement visibility support, so _LIBCPP_HIDE_FROM_ABI and friends don't have the intended effect. We're actively working on visibility for LLVM 15, but until that is all in we won't have a build compiler that can generate a stable ABI.

What's your preference for how we should handle this? If you're ok with a bit of churn in the lists till we stabilize I think this can land as is. If not we could potentially use a development clang for the CI with ABI checks on (once that's available).

@ldionne It's worth noting what's in this patch right now won't be our final ABI lists unfortunately. Our build compiler currently doesn't implement visibility support, so _LIBCPP_HIDE_FROM_ABI and friends don't have the intended effect. We're actively working on visibility for LLVM 15, but until that is all in we won't have a build compiler that can generate a stable ABI.

What's your preference for how we should handle this? If you're ok with a bit of churn in the lists till we stabilize I think this can land as is. If not we could potentially use a development clang for the CI with ABI checks on (once that's available).

Sorry, I missed that. But yes, I think it's OK to adjust the ABI lists afterwards. Let's land this.

@ldionne It's worth noting what's in this patch right now won't be our final ABI lists unfortunately. Our build compiler currently doesn't implement visibility support, so _LIBCPP_HIDE_FROM_ABI and friends don't have the intended effect. We're actively working on visibility for LLVM 15, but until that is all in we won't have a build compiler that can generate a stable ABI.

What's your preference for how we should handle this? If you're ok with a bit of churn in the lists till we stabilize I think this can land as is. If not we could potentially use a development clang for the CI with ABI checks on (once that's available).

Sorry, I missed that. But yes, I think it's OK to adjust the ABI lists afterwards. Let's land this.

If we could get D127470 landed first, then we won't have to adjust the ABI list after.

Jake-Egan updated this revision to Diff 436776.EditedJun 14 2022, 7:37 AM

Update ABI lists now that D127470 is landed and adapt to changes made in D126303.

This revision was automatically updated to reflect the committed changes.