This is an archive of the discontinued LLVM Phabricator instance.

[AIX][libc++] Always opt in to _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
ClosedPublic

Authored by francii on Dec 26 2022, 4:41 PM.

Details

Summary

For XCOFF (AIX) linkers, we have a fatal error if we see a weak hidden ref of a symbol in user code with a local weak def and then a strong def
in the library, as the default weak resolution semantics don't favour weak local defs for XCOFF leading to a conflict.

std::bad_function_call has just such a problem in ABI version 1, as it only leaves out the user code definition if _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION is defined in ABI version 2 and above. To fix this, we opt-in to _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION on all ABI versions for AIX in this patch.

Diff Detail

Event Timeline

francii created this revision.Dec 26 2022, 4:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 26 2022, 4:41 PM
Herald added a subscriber: nemanjai. · View Herald Transcript
francii requested review of this revision.Dec 26 2022, 4:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 26 2022, 4:41 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I suggest adding a reproducer for the specific error we're seeing on AIX. I expect there is an issue when the affected bad_function_call symbols are generated in user objects with hidden visibility, which conflict with the definition provided in the library. A test would document this understanding.

@francii, from offline discussion, a change is pending to use a different approach (the current one here breaks load ABI). If the change is going to take time, please mark this as having changes planned (i.e., not ready for review).

Mordante requested changes to this revision.Jan 7 2023, 4:26 AM

@francii, from offline discussion, a change is pending to use a different approach (the current one here breaks load ABI). If the change is going to take time, please mark this as having changes planned (i.e., not ready for review).

Based on this comment I marked it as request changes.

This revision now requires changes to proceed.Jan 7 2023, 4:26 AM
francii updated this revision to Diff 488430.Jan 11 2023, 4:59 PM

Do not remove from abi

w2yehia added inline comments.
libcxx/include/__config
175

extra newline

francii updated this revision to Diff 490177.Jan 18 2023, 8:27 AM

Updated fix

Looking better, still needs a test though.

libcxx/include/__config
178

Looking at the structure of this, I'm starting to think we should opt-in to the entire block above (starting with # if defined(_LIBCPP_BUILDING_LIBRARY) || _LIBCPP_ABI_VERSION >= 2), since the rationale described inside this block means we likely have the same problem with both these macros.

francii updated this revision to Diff 491844.Jan 24 2023, 9:52 AM

Add missing test case

I will update the test case to include the missing RUN lines.

libcxx/test/libcxx/vendor/ibm/bad_function_call.cpp
1 ↗(On Diff #491844)

Will update to add the missing RUN lines.

daltenty added inline comments.Jan 24 2023, 10:37 AM
libcxx/test/libcxx/vendor/ibm/bad_function_call.cpp
1 ↗(On Diff #491844)

Not sure that this needs a run line, but what this probably does need is:

  • A requires line limiting the target to AIX
  • The boilerplate LLVM license file header
  • Let's follow the file name format of the tests here. Seems like test which are designed to compile and execute successfully end in .pass.cpp
francii updated this revision to Diff 494159.Feb 1 2023, 9:13 PM

Update test case

w2yehia added inline comments.Feb 2 2023, 8:15 AM
libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp
1 ↗(On Diff #494159)

how is this test run?

daltenty accepted this revision.Feb 2 2023, 8:17 AM
daltenty retitled this revision from [AIX] Fix libc++ Symbol Visibility on AIX to [AIX][libc++] Always opt in to _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION.
daltenty edited the summary of this revision. (Show Details)

LGTM, with some minor nits to address before commit. Updated the description to describe the details of the issue.

libcxx/include/__config
177

nit: for clarity, move this inside the elif _LIBCPP_ABI_VERSION == 1 block above, since this is already on for ABI version 2.

I recommend a comment too:

francii updated this revision to Diff 494346.EditedFeb 2 2023, 9:51 AM

Update test case and moved logic

francii updated this revision to Diff 494355.Feb 2 2023, 10:19 AM

Update visibility line in test case

francii updated this revision to Diff 494356.Feb 2 2023, 10:21 AM

Update comment in __config

Mordante added inline comments.Feb 2 2023, 10:22 AM
libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp
1 ↗(On Diff #494346)

Please add the comment at line 10, this is the style we use in libc++

16 ↗(On Diff #494346)

Some of our platforms require this main signature. These platforms also require an explicit return 0; for main.

18 ↗(On Diff #494346)

This test seems to do nothing, if that is intended please add comment.

francii updated this revision to Diff 494360.Feb 2 2023, 10:29 AM

Update test case based on review

francii updated this revision to Diff 494362.Feb 2 2023, 10:34 AM

Update test case comment

daltenty added inline comments.Feb 2 2023, 1:37 PM
libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp
1 ↗(On Diff #494159)

how is this test run?

@w2yehia the lit config for libcxx has some magic for the file extensions. See https://github.com/llvm/llvm-project/commit/80a2ddf65ccd9837a7cdd0dfb96bb57863dd57d5 for details

francii updated this revision to Diff 494460.Feb 2 2023, 3:59 PM

clang-format

daltenty accepted this revision.Feb 2 2023, 5:21 PM

Latest format changes LGTM. Small nit about the test comment.

libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp
12–15 ↗(On Diff #494460)

nit: Tweak this to:

  1. identify the problematic function, since it's not obvious from the code
  2. elaborate on the failing case we avoid
francii updated this revision to Diff 494659.Feb 3 2023, 8:23 AM

Update test case main()

francii updated this revision to Diff 494662.Feb 3 2023, 8:43 AM

More information in test case description

Mordante added inline comments.Feb 5 2023, 3:46 AM
libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp
24 ↗(On Diff #494662)

Based on the comments of the test, wouldn't it be better also add test that actually throws bad_function_call?

Maybe a function like

void test_throw()
{
#ifndef TEST_HAS_NO_EXCEPTIONS
  std::function<int()> f;
  try {
   f();
   assert(false);
  } catch(const std::bad_function_call&) {
    return;
  }
  assert(false);
#endif // TEST_HAS_NO_EXCEPTIONS
}

And call this function from main?

Note this change requires the following include #include "test_macros.h"

francii added inline comments.Feb 7 2023, 10:07 AM
libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp
24 ↗(On Diff #494662)

Thanks for the suggestion!

We're actually looking that it doesn't throw bad_function_call, as that is the original behaviour that this patch corrects. This test case is expected to compile without errors.

francii added inline comments.Feb 7 2023, 11:38 PM
libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp
24 ↗(On Diff #494662)

Sorry, I misunderstood your message. I see what you were saying now. I will update the test case shortly.

Thank you :)

francii updated this revision to Diff 496833.Feb 12 2023, 8:47 PM

Update test case

@Mordante Please re-review when you are available. Thanks!

LGTM modulo one request.

For future reviews, when you address a review comment, can you mark it as done? That makes reviewing patches a second time a lot easier.

libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp
21 ↗(On Diff #497397)

I think it would be good to also have the non-throwing call to f you originally had.

francii marked 9 inline comments as done.Feb 15 2023, 2:49 PM
francii updated this revision to Diff 497815.Feb 15 2023, 2:49 PM

Update test case

francii marked 3 inline comments as done.Feb 15 2023, 2:54 PM
francii updated this revision to Diff 498017.Feb 16 2023, 7:34 AM

Update test case

francii updated this revision to Diff 498090.Feb 16 2023, 11:10 AM

Update main in test case

Mordante accepted this revision.Feb 16 2023, 12:48 PM

Thanks, LGTM!

This revision is now accepted and ready to land.Feb 16 2023, 12:48 PM
philnik requested changes to this revision.Feb 16 2023, 12:55 PM
philnik added a subscriber: philnik.

Sorry for chiming in so late, but I've got D142842 to remove _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION. IIUC that should also fix your problem, right?

This revision now requires changes to proceed.Feb 16 2023, 12:55 PM

Sorry for chiming in so late, but I've got D142842 to remove _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION. IIUC that should also fix your problem, right?

Unfortunately, as I've described in https://reviews.llvm.org/D142842, we already rely on the symbols in the dylib in the AIX linkage model, so removing it won't really work for us.

Since https://reviews.llvm.org/D142842 still needs some revision to be ABI-safe, and this was already good to go, we'll proceed with this patch and D142842 can rebase against it if there are no objections.

philnik accepted this revision.Mar 8 2023, 7:02 AM
This revision is now accepted and ready to land.Mar 8 2023, 7:02 AM
This revision was automatically updated to reflect the committed changes.