[libcxx-commits] [PATCH] D124555: [libcxx][AIX][z/OS] basic_ios<wchar_t> cannot store fill character WCHAR_MAX

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm added a comment.

In D124555#3571553 <https://reviews.llvm.org/D124555#3571553>, @hubert.reinterpretcast wrote:

> In D124555#3566746 <https://reviews.llvm.org/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.

For background, the (customized) libc++ that is provided on AIX used the form where the bool member is injected directly in `basic_ios`.

I drafted an update with the `_OptionalFill` / `_SentinelFill` suggestion and have confirmed that there are layout differences on AIX in derived classes when the first derived member is 1 or 2 byte aligned.  Alternatively, even if all inheritance of `basic_ios` is virtual, there's still potential layout differences where a user derived class has multiple virtual bases and the second virtual base wants to start inside the padding.  I did try `[[no_unique_address]]`, however, the syntax is not accepted with `-std=c++03`.  Unless we want to version the 03 code from the C++11 forward code, I don't think there's a way to get a layout compatible type using this approach.

One idea Hubert and I discussed offline is to use a member declaration `bool __is_set[Boolean expression]` and have it deliberately zero-size when the `__is_set` member isn't needed.  We can still use `_OptionalFill` / `_SentinelFill` to control access to the member.

Another obstacle is that the proper check to enable to use of `_OptionalFill` looks like:

  using _FillType = _If<traits_type::eq_int_type(
                            std::numeric_limits<char_type>::max(),
                            traits_type::eof()),
                        _OptionalFill, _SentinelValueFill>;

but note that `eq_int_type` and `eof` can only participate in constant expressions where they are constexpr in C++11, so I again have a challenge in delivering a solution that works in C++03.  I'll need to use a wchar specialization and use something like `WEOF < (wint_t)WCHAR_MIN || (win_t)WCHAR_MAX < WEOF`.

Given my limited experience with LLVM libc++ some guidance along which fundamental approach we should take would be appreciated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124555/new/

https://reviews.llvm.org/D124555