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.

The change made me think we should add the negative test with -mlong-double-128, but then I thought we should probably make that an error until we've sorted through the AIX binary compat implications and tested it. So the patch you've proposed makes sense to me and we should follow up with handling for mlong-double-128.

LGTM

Are there any tests planned for this patch? We would have to commit XL binaries, or I think a better approach would be to write C code with a user defined eh fsm table.

LGTM.

Would like you to prune the new tests, then I think this will be ready to land.

I think it makes sense to split the STDC macro changes and TOS_AIX__ into different patches.

Please rework the test update into init-ppc.c

LGTM

Minor suggestion to keep and remap some of the diagnostic cases.

Reworked the prefixes and indentation.

Added ppc64le target.