In D136554#4056862, @cor3ntin wrote:

Yes please! However the warning looks correct to me in that case. A
constructs x which constructs A etc.

I've reduced a regression on:

I had a typo in the commit so this didn't auto close.

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.

LGTM.

Looks good but I just want to confirm that the profiled -L paths are appended in the correct order with respect to the non-profiled paths.

I believe the fix for a52d151f9dde7 inadvertently exposed a code path where by the linkage of a static local of a static function, which would otherwise return LinkageInfo::none() may now return VisibleNoLinkage depending on the incoming computation argument.

Test only patch is safe to commit.

Test only patch is safe to commit.

Suggest: [Test][AIX][p] Add 64-bit driver expected output

Use the [Test] tag in the abstract.

I have no further concerns with this patch and can confirm the solution works for EH support of the XL C++ V16 xlclang++ compiler. No further concerns.

The new test passes/fails based on wchar_t/wint_t types, which is tedious to manage across targets with XFAIL so I've decided to enable it only for AIX/zOS targets where it is expected to pass.

New test fails on macos targets (as it should) but I needed to correct a typo on the XFAIL line.

Formatting and typo.

Thanks for catching the formatting problems Xing. I'll need to look at my workflow to see why I didn't catch those.

Fix formatting and a typo in the __config

• Only add fill set as needed for _LIBCPP_ABI > 1 for all targets
• zOS 64-bit always needs fill set for compatibility
• tweaked the test to use the char value for eof as it exposes the problem for both signed and unsigned char types

Plan to revise the config to pick up the "fill set member as needed" mode in the config for all targets when _LIBCPP_ABI_VERSION >= 2.

Quick testcase fix.

Any basic_ios where sizeof(char_type) >= sizeof(int_type) has at least some char value that overlaps with the eof character. I've modified the proposed test to demonstrate signed char types are affected at -1.

Writing revision to use SentinelFill/OptionalFill classes.

1. The selection of the fill set member is moved to the __config.
2. This patch retains ABI compatibility for any of the libc++ CI that reported failures on this issue.
3. Always inject the fill set member for AIX to retain compatibility for AIX.
4. This only fixes basic_ios<wchar_t>. It is not compliant to instantiate basic_ios with a char type where char_traits::eof is not a sentinel value. #wchar_t is an exception because C++ shouldn't be adding constraints beyond what C guarantees.

The new approach injects the new __fill_set_ only for wchar_t and only if WEOF is in the range [WCHAR_MIN, WCHAR_MAX]. I didn't use numeric_limits because the solution needs to work in C++03.

In D124555#3571553, @hubert.reinterpretcast wrote:

In D124555#3566746, @ldionne wrote:

template <class _Traits>
struct _OptionalFill {
  _OptionalFill() : __set_(false) { }
  _OptionalFill& operator=(typename _Traits::int_type __x) { __set_ = true; __fill_ = __x; return *this; }
  bool __is_set() const { return __set_; }
  typename _Traits::int_type __fill() const { return __fill_; }

private:
  typename _Traits::int_type __fill_;
  bool __set_;
};

Thoughts?

From the AIX and z/OS perspective, the replacement of the two members with a member having two fields is not 100% binary compatible for all usage. At least if the basic_ios specialization is used as a non-virtual base class, any derived class members that could be allocated in the padding following the bool would now move. This could be preemptively avoided if the new class type is given a user-provided copy constructor or destructor and the new member made [[no_unique_address]]. I don't know if [[no_unique_address]] is usable is some form under C++98/03 though.

Addressed early comments. Some offline discussion occurred to decide to only enable the new member on zOS in 64-bit. I've XFAILed other affected targets. They can opt into the new member in the __config at their discretion as the change affects compatibility, though I've mitigated the compatibility break as much as possible.

• Only enabled on zOS in 64-bit.
• Added XFAILs for other targets from the CI that also fail the new test.
• Only check the new __fill_set_ member when necessary to mitigate compat problems if a target already shipped the version without the member.

Revised after D123519.

The change looks good to me but I think others should comment on whether or not the option name change is going to cause anyone problems.

I've had a thorough review and access to the proprietary XL "state table EH" code and I believe this patch is ready to land, nonetheless, I do not have the authority to grant permission to commit this patch. This patch should not affect targets other than AIX and Xing has already run thorough proprietary test coverage against this implementation. Essentially this is a tidied-up version of the EH runtime support we already provide with the libc++ we distribute for AIX.

I updated the patch to use the previous union pattern to bypass the singleton destructors but added constinit, constexpr and attribute((no_destroy)) as possible to avoid the runtime overhead of using a static local.

I was writing a revision of this patch but hit a problem:

The AIX default of ignoring source-level visibility is problematic. I agree the test should be xfailed for AIX.

Makes sense to me.

I'd like to take over this patch and rebase a revision if there are no objections.

This patch has sat idle for 2+ years but I think the current patch is an improvement. Currently, the error_category singletons are implemented as static locals for which there is no guarantee that the destructors won't run prematurely. The proposed patch fixes that problem.

Looks good. I didn't find any semantic issues but I did have a number of suggestions on further refinements for clarity of the code. A vast improvement over the proprietary XL code btw! Thanks Xing.

More comments.

My remaining suggestions are very minor. I've had a thorough look through this patch though I'll look to others to approve the patch to be committed.

Review in still on-going.

Early comments. Continuing to work through the review.

I haven't completed my review. In particular I need to look at the longtbtable, has_vec mix up with more scrutiny but I wanted to submit my comments thus far.

As we lift the backend fatal error we expose the risk of generating silently incompatible code with the XLC compiler on AIX. I think a clang warning diagnostic is warranted for 16 byte aligned byval args where the struct is 16 byte aligned and does not contain a vector member. I don't think this case is common so the diagnostic shouldn't be too verbose.

LGTM.

The XL compiler only defined this weird form of the macros on AIX, but not on the XL Linux on Power compiler. I think it's preferable that we define these macros only on AIX and in the source/commit messages indicate that we're only doing so for AIX XL C/C++ compatibility. Users should prefer the PPC and powerpc macros.

I'd like to see the rationale for adding these forms of the macros in the review and also in the extended commit message. These forms are being added primarily because the AIX XL compiler documented and defined them. The patch itself looks fine.

Given that we have a legacy XL macro with no legacy cross compiler I think it's fine if we set this according to the target only. It's fully redundant to _AIX but we'll define it for any working use-case with the current xlC compiler. If IBM has a compelling use case for a host-specific macro we can always revive this discussion. For now, you're probably making us from making a mistake ;)

In D107242#2934611, @joerg wrote:

clang is fundamentally a cross-compiler only. I don't see any point for having host-specific branches in this case at all. Either the macro should be specified for the target all the time or not at all, but it should most definitely not depend on the host. That's actually breaking a number of existing use case for clang in subtle ways, e.g. partially preprocessed files (-frewrite-includes) should behave the same on any platform given the same command line.

In D107242#2932289, @joerg wrote:

I'm puzzled by this change. I don't think we have any case so far where the compiler behavior changes with the host OS and I don't think it should. What's the point / use case of this macro?

The new version is better than the previous. LGTM.

LGTM

LGTM.

It would be nice if the diagnostic could be deferred to layout, in case the struct is defined but not used in a header, but I understand that #pragma pack(1) and #pragma align(packed) become ambiguous at the point of layout. I think this is a reasonable diagnostic given the impact of silently incompatible codegen.

LGTM.

LGTM. THW_PPC is a macro historically defined by xlc on AIX and defining it may help users port to clang.

The divergence with GCC is regrettable. Unfortunately defining _ARCH_PPC64 in 32-bit has been a long-standing discrepancy between xlc and gcc and one I don't expect gcc to change. This patch is preferable in order to retain preprocessing behaviour on AIX for customers porting 32-bit AIX xlc applications to clang.

The decision was made to make -mlong-double-128 an error on AIX: https://reviews.llvm.org/D106074 [AIX] Clang's library integration support for 128-bit long double is incomplete on AIX.

LGTM.