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

[francii]

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.

[Mordante]

In D140675#4029021, @hubert.reinterpretcast wrote:

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

[francii]

Do not remove from abi

[w2yehia]

libcxx/include/__config

189

extra newline

[francii]

Updated fix

[daltenty]

Looking better, still needs a test though.

libcxx/include/__config

190

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]

Add missing test case

[francii]

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]

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]

Update test case

[w2yehia]

libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp

2

how is this test run?

[daltenty]

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

libcxx/include/__config

189

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]

Update test case and moved logic

[francii]

Update visibility line in test case

[francii]

Update comment in __config

[Mordante]

libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp

2

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

17

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

19

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

[francii]

Update test case based on review

[francii]

Update test case comment

[daltenty]

libcxx/include/__config

172

Please clang-format this. See:
https://buildkite.com/llvm-project/libcxx-ci/builds/18490#0186136d-9db4-4162-84b3-3ea430f44654

[daltenty]

libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp

2

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]

clang-format

[daltenty]

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

libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp

13–16

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]

Update test case main()

[francii]

More information in test case description

[Mordante]

libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp

25

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]

libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp

25

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]

Rebase

[francii]

libcxx/test/libcxx/vendor/ibm/bad_function_call.pass.cpp

25

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

Thank you :)

[francii]

Update test case

[francii]

Rebase

[francii]

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

[Mordante]

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

22

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

[francii]

Update test case

[francii]

Update test case

[francii]

Update main in test case

[Mordante]

Thanks, LGTM!

[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?

[daltenty]

In D140675#4132958, @philnik wrote:

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.

[daltenty]

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.