[libcxx] [Windows] Pick a unique bit for __regex_word

[mstorsjo]

Summary

The old __regex_word aliased the mask for xdigit, causing stray
test failures.

The diff may look surprising, as if the previous faulty value had
been set specifically for Windows - but this is due to a restructuring
in 411c630bae0e0d50697651797709987e2cfea92d. Prior to that, there
were OS specific settings for some OSes, and one fallback used for
the rest (which turns out to not work for Windows).

[• Quuxplusone]

libcxx/include/__locale

548

Could we realistically put a future-proofing static_assert down here, something like

static_assert(__regex_word & ~(space | print | cntrl | upper | lower | alpha | digit | punct | xdigit | blank) == __regex_word);

and I would expect

static_assert(upper | lower == alpha);

also? And/or, could we make a test for this?

[mstorsjo]

libcxx/include/__locale

548

I guess such a static_assert for __regex_word would be good to have, yes.

static_assert(upper | lower == alpha); doesn't hold on Windows, _ALPHA is defined as 0x0100 | _UPPER | _LOWER actually.

[mstorsjo]

Added a static assert.

[mstorsjo]

Looks like the static assert is failing on AIX, indicating that the same issue is present there. CC @DanielMcIntosh-IBM @muiez

[mstorsjo]

Would it be ok to land this without the static assert, fixing the issue for one platform at least? Then when the AIX case gets fixed, we can add the static assert. Also CC @daltenty for more AIX people.

[muiez]

libcxx/include/__locale

548

On z/OS, the following static_assert passes:

static_assert((__regex_word & ~(space | print | cntrl | upper | lower | alpha | digit | punct | xdigit | blank)) == __regex_word);

However, this one fails:

static_assert((upper | lower) == alpha);

Note: we should place parentheses around the | expression to evaluate it first since == has a higher precedence.

[mstorsjo]

libcxx/include/__locale

548

Yep, I added parentheses in that form in the updated patch. But that assert fails on AIX in the precommit testing.

The second assert fails on windows too, so I didn’t include it in the patch.

[• Quuxplusone]

In D118188#3278624, @mstorsjo wrote:

Would it be ok to land this without the static assert, fixing the issue for one platform at least? Then when the AIX case gets fixed, we can add the static assert.

FWIW, I'd prefer to land a static_assert now so we don't forget about it; but guard it under #ifndef _AIX or XFAIL: LIBCXX-AIX-FIXME or something like that (depending on where you put the assert), so that it's greppable for people working on AIX specifically.

[mstorsjo]

In D118188#3279825, @Quuxplusone wrote:

In D118188#3278624, @mstorsjo wrote:

Would it be ok to land this without the static assert, fixing the issue for one platform at least? Then when the AIX case gets fixed, we can add the static assert.

FWIW, I'd prefer to land a static_assert now so we don't forget about it; but guard it under #ifndef _AIX or XFAIL: LIBCXX-AIX-FIXME or something like that (depending on where you put the assert), so that it's greppable for people working on AIX specifically.

Oh, right, I could move the assert to a test - I placed it in the header so far, where it broke everything.

[• Quuxplusone]

In D118188#3279833, @mstorsjo wrote:

In D118188#3279825, @Quuxplusone wrote:

In D118188#3278624, @mstorsjo wrote:

Would it be ok to land this without the static assert, fixing the issue for one platform at least? Then when the AIX case gets fixed, we can add the static assert.

FWIW, I'd prefer to land a static_assert now so we don't forget about it; but guard it under #ifndef _AIX or XFAIL: LIBCXX-AIX-FIXME or something like that (depending on where you put the assert), so that it's greppable for people working on AIX specifically.

Oh, right, I could move the assert to a test - I placed it in the header so far, where it broke everything.

Even in the header, it could be guarded by #ifndef _AIX. (That said, a test sounds good too. It'd just have to be in libcxx/test/libcxx/, right? because __regex_word is an implementation detail of libc++. Unless there's a way to regression-test the observable bug that was originally caused by this issue; that would be excellent too.)

[mstorsjo]

Moved the assert into a test, temporarily.

[mstorsjo]

Retry/rerun CI (which got failed on unrelated things last time)

[mstorsjo]

Add a message to the static assert, hopefully fixing building in pre-C++17 modes.

[• Quuxplusone]

libcxx/test/std/re/re.traits/lookup_classname.pass.cpp

33

(1) Couldn't this go in include/__locale today, just guarded with #if !defined(_AIX) and a TODO comment?
(2) I don't think this can go in libcxx/test/std/ because __regex_word isn't a standard name. It should go in libcxx/test/libcxx/ (unless you just put it in include/__locale).

[mstorsjo]

libcxx/test/std/re/re.traits/lookup_classname.pass.cpp

33

Yeah, that should work too, and good point about this being a standard test. (I went with placing it in a test instead of in the header, to break less if there’s other platforms where it doesn’t pass, that aren’t covered by CI. But if it isn’t covered by CI, we can’t give any promises I guess.)

[mstorsjo]

Moved the static assert back to the header, as suggested.

[• Quuxplusone]

LGTM now!

@daltenty: would it be useful for someone to file a GitHub issue to remember to fix this on AIX? ...well, I guess the XFAIL: LIBCXX-AIX-FIXME on the relevant tests may be good enough. Anyway, I'll leave it to the AIX people to figure out what they want to do.

[vpalatin]

This is failing for Newlib / Picolibc too (defining _NEWLIB_VERSION).

As far as I can tell, the mask there is a char and all 8 bits are currently defined in newlib:

#define _U      01
#define _L      02
#define _N      04
#define _S      010
#define _P      020
#define _C      040
#define _X      0100
#define _B      0200

What would be the proper fix ?

[mstorsjo]

In D118188#3321928, @vpalatin wrote:

This is failing for Newlib / Picolibc too (defining _NEWLIB_VERSION).

As far as I can tell, the mask there is a char and all 8 bits are currently defined in newlib:

#define _U      01
#define _L      02
#define _N      04
#define _S      010
#define _P      020
#define _C      040
#define _X      0100
#define _B      0200

What would be the proper fix ?

Hmm, that looks problematic. I think you'd have to enlargen the mask type, which I would guess breaks the libc++ ABI for that platform - and I don't know if that one needs to match some size used by the libc too.

libcxx/include/__locale

#elif defined(_LIBCPP_MSVCRT_LIKE)
     static const mask cntrl  = _CONTROL;
     static const mask upper  = _UPPER;
     static const mask lower  = _LOWER;
     static const mask alpha  = _ALPHA;
     static const mask digit  = _DIGIT;
     static const mask punct  = _PUNCT;
     static const mask xdigit = _HEX;
     static const mask blank  = _BLANK;
-    static const mask __regex_word = 0x80;
+    static const mask __regex_word = 0x4000; // 0x8000 and 0x0100 and 0x00ff are used
 # define _LIBCPP_CTYPE_MASK_IS_COMPOSITE_PRINT
 #elif defined(__APPLE__) || defined(__FreeBSD__) || defined(__EMSCRIPTEN__) || defined(__NetBSD__)
 # ifdef __APPLE__
     typedef __uint32_t mask;
 # elif defined(__FreeBSD__)
     typedef unsigned long mask;
 # elif defined(__EMSCRIPTEN__) || defined(__NetBSD__)
     typedef unsigned short mask;
# endif
     static const mask __regex_word = 0x8000;
 #else
 # error unknown rune table for this platform -- do you mean to define _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE?
 #endif
     static const mask alnum  = alpha | digit;
     static const mask graph  = alnum | punct;
 
     _LIBCPP_INLINE_VISIBILITY ctype_base() {}
 };
 
 template <class _CharT> class _LIBCPP_TEMPLATE_VIS ctype;
 
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 template <>
 class _LIBCPP_TYPE_VIS ctype<wchar_t>
     : public locale::facet,
       public ctype_base

libcxx/test/std/re/re.traits/isctype.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-// XFAIL: LIBCXX-WINDOWS-FIXME
 // XFAIL: LIBCXX-AIX-FIXME
 
 // <regex>
 
 // template <class charT> struct regex_traits;
 
 // bool isctype(charT c, char_class_type f) const;
 

libcxx/test/std/re/re.traits/lookup_classname.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-// XFAIL: LIBCXX-WINDOWS-FIXME
 // XFAIL: LIBCXX-AIX-FIXME
 
 // <regex>
 
 // template <class charT> struct regex_traits;
 
 // template <class ForwardIterator>
 //   char_class_type
 //   lookup_classname(ForwardIterator first, ForwardIterator last,
 //                    bool icase = false) const;
 
 #include <regex>
 #include <cassert>
 #include "test_macros.h"
 #include "test_iterators.h"
 
+// TODO: Move the assert to include/__locale when it passes on all platforms.
+static_assert((std::ctype_base::__regex_word &
+               ~(std::ctype_base::space | std::ctype_base::print |
+                 std::ctype_base::cntrl | std::ctype_base::upper |
+                 std::ctype_base::lower | std::ctype_base::alpha |
+                 std::ctype_base::digit | std::ctype_base::punct |
+                 std::ctype_base::xdigit | std::ctype_base::blank)) ==
+              std::ctype_base::__regex_word,
+              "__regex_word can't overlap other bits");
+
 template <class char_type>
 void
 test(const char_type* A,
      typename std::regex_traits<char_type>::char_class_type expected,
      bool icase = false)
 {
     typedef typename std::regex_traits<char_type>::char_class_type char_class_type;
     std::regex_traits<char_type> t;