LGTM, thanks!

LGTM, thanks!

Resolved by https://reviews.llvm.org/D125141

Differentiate between inlines hidden and regular visibility in test

Any guidance as to how I should proceed?

Make assert for mixed non-export visibility and dllexport a fatal error

LGTM, we haven't shipped any of the C++17 and up interfaces on AIX yet, so this seem fine to me.

@daltenty - what do you want to do about it? Add LIBCXX_ENABLE_WERROR=OFF in the AIX build config (which also would affect the 64 bit build), do silence it differently somehow?

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

This change seems ok in principal, but these functions are extern 'C", so now that they're no longer internal linkage they'll provide the same symbol as the real thing. Adding such functions with linkable POSIX reserved names into user program seems like we are asking for trouble. Can we get these moved into some kind of namespace or at least with some kind of double underscore prefix?

Make MCWasmStreamer ignore MCSA_Exported

Update the patch based on feedback on the RFC (https://discourse.llvm.org/t/rfc-adding-exported-visibility-style-to-the-ir-to-model-xcoff-exported-visibility/61853/8). There seem to be limited visibility bits, and the implementation has kind of reserved space for internal, so adding exported may cause problems. On the other hand dllexport appears to match the semantic and currently only be used as a marker to indicate to some passes that we should preserve the symbol since it will be exported from shared objects (which is exactly what we want too), so I've updated the implementation as suggested to use dllexport instead.

In D122983#3464759, @aaron.ballman wrote:

In D122983#3464724, @Jake-Egan wrote:

Hi, we also have a failure on AIX with test-suite call to undeclared library function '%0' with type %1; ISO C99 and later https://lab.llvm.org/buildbot/#/builders/214/builds/825/steps/9/logs/stdio

Thanks for letting me know, this should now be fixed.

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?

LGTM, thanks!

Thanks for getting this fixed this so quickly and sorry for the messy revert.

In D123345#3460639, @Jake-Egan wrote:

Hi, unfortunately there's a build failure on AIX: https://lab.llvm.org/buildbot/#/builders/214/builds/779/steps/9/logs/stdio. Could you take a look?

Address review comments

LGTM as an interm fix. We'll revert and replace this once we have a fix in Google Benchmark.

Right. But that's not the question i asked, i believe?

In D122316#3402566, @lebedev.ri wrote:

Please can you submit this upstream first?

LGTM, thanks!

LGTM, similarly to the other platforms indicated the bits can be set through the filesystem interface (though our chmod -h is a no-op)

We could pass the linker an export list with the desired visibility for the functions in test on AIX if we need the visibility for the test. This is likely what we'll have to do in other places in libc++ until clang on AIX has real visibility support.

Rename remaining ABI lists

Rebase to trigger CI

I trust that you're right when you say it affects the ABI, but do you have an example (I'm sure there's many, I just can't think of one right now).

Gentle ping

Rebase to retrigger CI

Gentle ping

clang-format

clang-format

LGTM, just copying the longer rational why the message is expected on AIX.

LGTM, thanks!

Add back in CmdArgs.push_back(Args.MakeArgString(Plugin)) that got accidentally removed

• Rebase
• incorporate suggestion from review
• add fuse-ld=ld to debugger tuning test, to make it invariant when running on platforms that might use ldd by default

@ldionne gentle ping. I think we've addressed your previous concerns here. We've moved responsibility for setting the macro to suppress the AIX libc definitions to the compiler side as you suggested in D109078, so this patch should hopefully be non-controversial now.

clang-format

Add missing semicolon

Change seperator option type to bool

Add additional context

In D102172#3304810, @Quuxplusone wrote:

In D102172#3304759, @jsji wrote:

@daltenty Can you please update the patch to include more *Context*, looks like the Context not available. Or use arc to update it. Thanks.

In case it's needed: The command I use to get context in my diffs (without using arc) is git show -U999 HEAD >filetoupload.diff (or replace HEAD with whatever-commit-hash).

Update to a new version that requires the compiler to define the __LIBC_NO_CPP_MATH_OVERLOADS__ macro (which clang does).

Remove whitespace change

Run clang-format

Slightly tweak whitespace formatting

Fix missing context on diff

Confirming LGTM, thanks!

LGTM with minor nit. We'll see how we can better remediate some on on AIX these later, but looking at the existing CMake, we're not the only ones skipping some of these.

LGTM, with minor nit

Thanks for pinging us on this. After taking a look at the AIX test failure, and dumping the error_code we get back from the new implementation, I think this is actually due to some ambiguity in the expected errno when the combination of O_DIRECTORY and O_NOFOLLOW is used and the path is a symlink.