[libc++] Fix __regex_word value when using newlib/picolibc

[arichardson]

Summary

The ctype mask for newlib/picolibc is fully saturated, so __regex_word
has to overlap with one of the values. This commit uses the same workaround
as bionic did (uint16_t for char_class_type inside regex_traits). It
should be possible to have libc++ provide the default rune table instead,
but that will require a new mechanism to detect newlib inside __config
since the header defining the newlib/picolibc macros has not been included
yet inside __config. This also avoids duplicating the ctype table for
newlib, reducing the global data size.

[michaelplatings]

I think this could be considered an ABI break. I don't see an alternative and given that the static assert has been failing for some time maybe no-one cares. But just in case, would it be worth noting in the release notes?

[philnik]

In D138195#3937400, @michaelplatings wrote:

I think this could be considered an ABI break. I don't see an alternative and given that the static assert has been failing for some time maybe no-one cares. But just in case, would it be worth noting in the release notes?

This is definitely an ABI break, but I'm not sure we care for newlib about that. We don't officially support it and most people that are using newlib + libc++ are probably in an environment where ABI stability isn't much of a concern. It's mostly used in embedded environments, right? It's probably a good idea to add a note to the ABI Affecting Changes section though.

[arichardson]

In D138195#3937471, @philnik wrote:

In D138195#3937400, @michaelplatings wrote:

I think this could be considered an ABI break. I don't see an alternative and given that the static assert has been failing for some time maybe no-one cares. But just in case, would it be worth noting in the release notes?

This is definitely an ABI break, but I'm not sure we care for newlib about that. We don't officially support it and most people that are using newlib + libc++ are probably in an environment where ABI stability isn't much of a concern. It's mostly used in embedded environments, right? It's probably a good idea to add a note to the ABI Affecting Changes section though.

I agree this is technically an ABI break, but I believe there can't be any affected code since the static_assert would have failed previously when including <regex> for a libc++ build with locale support? And even if it is, I would assume this should not matter for newlib where you generally don't have a system libc++.so that is being linked against.
Happy to add a release note though.

[philnik]

In D138195#3937494, @arichardson wrote:

In D138195#3937471, @philnik wrote:

In D138195#3937400, @michaelplatings wrote:

I think this could be considered an ABI break. I don't see an alternative and given that the static assert has been failing for some time maybe no-one cares. But just in case, would it be worth noting in the release notes?

This is definitely an ABI break, but I'm not sure we care for newlib about that. We don't officially support it and most people that are using newlib + libc++ are probably in an environment where ABI stability isn't much of a concern. It's mostly used in embedded environments, right? It's probably a good idea to add a note to the ABI Affecting Changes section though.

I agree this is technically an ABI break, but I believe there can't be any affected code since the static_assert would have failed previously when including <regex> for a libc++ build with locale support? And even if it is, I would assume this should not matter for newlib where you generally don't have a system libc++.so that is being linked against.
Happy to add a release note though.

The static_assert was only added in LLVM 14, so there could be users that simply haven't upgraded yet. Anyways, I think this patch LGTM with a release note. @jfb @jroelofs it looks like you worked a bit on newlib + libc++ support. Do you have any opinion here? #libc_vendors is anybody using newlib with libc++? If yes, what's your opinion?

[jroelofs]

The newlib + libc++ work I did for a previous employer (CodeSourcery/MentorGraphics/Siemens) was aimed at building a clang toolchain that could support building for baremetal arm. An ABI break for that environment would not be the end of the world, especially considering those environments cannot afford the code size from locale support. AFAIK, @abidh is still maintaining that work, and would be able to give a definitive answer.

AFAIR, the newlib work @jfb did was for (P)NaCl, which has been deprecated in favor of Wasm [1].

1: https://developer.chrome.com/docs/native-client/nacl-and-pnacl/

[abidh]

In D138195#3937618, @jroelofs wrote:

The newlib + libc++ work I did for a previous employer (CodeSourcery/MentorGraphics/Siemens) was aimed at building a clang toolchain that could support building for baremetal arm. An ABI break for that environment would not be the end of the world, especially considering those environments cannot afford the code size from locale support. AFAIK, @abidh is still maintaining that work, and would be able to give a definitive answer.

My last work on this stuff was before that assert was added so never faced this problem. But I agree with above comment. We could live with such ABI breakage.

[arichardson]

Add release notes

[philnik]

LGTM, since everybody seems to be on the same page here. @ldionne do you agree with this?

libcxx/docs/ReleaseNotes.rst

135–136

It's still an ABI break, we just don't expect people to care.

[ldionne]

Thanks for checking. I guess this is fine from an ABI standpoint, however I do have a problem with us making Newlib-related changes at all since it's not supported. @arichardson How did you come across this issue?

[michaelplatings]

In D138195#3941657, @ldionne wrote:

I do have a problem with us making Newlib-related changes at all since it's not supported.

I've got an RFC up to make a step towards testing newlib: https://discourse.llvm.org/t/rfc-testing-of-newlib-picolibc/66509

[arichardson]

In D138195#3941657, @ldionne wrote:

Thanks for checking. I guess this is fine from an ABI standpoint, however I do have a problem with us making Newlib-related changes at all since it's not supported. @arichardson How did you come across this issue?

I was trying to build some C++ code for an embedded target and as part of that tried to build libc++ because I saw there was existing newlib code inside libc++. It had bitrotted a bit due to missing CI, but wasn't too difficult to get working. Looking forward to the CI proposed by @michaelplatings :)

[ldionne]

Oh, I had missed this. Thanks.

This LGTM but as I explained on the RFC, we need to add a BuildKite runner, not a BuildBot runner. Libc++ doesn't deal with BuildBot at all anymore. If you do the work, you might as well do it such that it fits nicely with libc++'s CI from the start. Reach out to me on Discord for details.

libcxx/docs/ReleaseNotes.rst

- In freestanding mode, ``atomic<small enum class>`` does not contain a lock byte anymore if the platform
   can implement lockfree atomics for that size. More specifically, in LLVM <= 11.0.1, an ``atomic<small enum class>``
   would not contain a lock byte. This was broken in LLVM >= 12.0.0, where it started including a lock byte despite
   the platform supporting lockfree atomics for that size. Starting in LLVM 15.0.1, the ABI for these types has been
   restored to what it used to be (no lock byte), which is the most efficient implementation.
 
   This ABI break only affects users that compile with ``-ffreestanding``, and only for ``atomic<T>`` where ``T``
   is a non-builtin type that could be lockfree on the platform. See https://llvm.org/D133377 for more details.
 
+- When building libc++ against newlib/picolibc, the type of ``regex_type_traits::char_class_type`` was changed to
+  ``uint16_t`` since all values of ``ctype_base::mask`` are taken. This is technically an ABI break, but including
+  ``<regex> `` has triggered a ``static_assert`` failure since libc++ 14, so it is unlikely that this causes
+   problems for existing users.
+
 Build System Changes
 --------------------
 - Support for ``libcxx``, ``libcxxabi`` and ``libunwind`` in ``LLVM_ENABLE_PROJECTS`` has officially
   been removed. Instead, please build according to :ref:`these instructions <build instructions>`.

libcxx/include/__locale

#elif defined(_NEWLIB_VERSION)
     static const mask cntrl  = _C;
     static const mask upper  = _U;
     static const mask lower  = _L;
     static const mask alpha  = _U | _L;
     static const mask digit  = _N;
     static const mask punct  = _P;
     static const mask xdigit = _X | _N;
     static const mask blank  = _B;
-    static const mask __regex_word = 0x80;
+    // mask is already fully saturated, use a different type in regex_type_traits.
+    static const unsigned short __regex_word = 0x100;
 # define _LIBCPP_CTYPE_MASK_IS_COMPOSITE_PRINT
 # define _LIBCPP_CTYPE_MASK_IS_COMPOSITE_ALPHA
 # define _LIBCPP_CTYPE_MASK_IS_COMPOSITE_XDIGIT
 #elif defined(__MVS__)
 # if defined(__NATIVE_ASCII_F)
     typedef unsigned int mask;
     static const mask space  = _ISSPACE_A;
     static const mask print  = _ISPRINT_A;

 #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() {}
 
-    static_assert((__regex_word & ~(space | print | cntrl | upper | lower | alpha | digit | punct | xdigit | blank)) == __regex_word,
+    static_assert((__regex_word & ~(std::make_unsigned<mask>::type)(space | print | cntrl | upper | lower | alpha |
+                                                                    digit | punct | xdigit | blank)) == __regex_word,
                   "__regex_word can't overlap other bits");
 };
 
 template <class _CharT> class _LIBCPP_TEMPLATE_VIS ctype;
 
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 template <>
 class _LIBCPP_TYPE_VIS ctype<wchar_t>

libcxx/include/regex

 
 template <class _CharT>
 struct _LIBCPP_TEMPLATE_VIS regex_traits
 {
 public:
     typedef _CharT                  char_type;
     typedef basic_string<char_type> string_type;
     typedef locale                  locale_type;
-#ifdef __BIONIC__
+#if defined(__BIONIC__) || defined(_NEWLIB_VERSION)
     // Originally bionic's ctype_base used its own ctype masks because the
     // builtin ctype implementation wasn't in libc++ yet. Bionic's ctype mask
     // was only 8 bits wide and already saturated, so it used a wider type here
     // to make room for __regex_word (then a part of this class rather than
     // ctype_base). Bionic has since moved to the builtin ctype_base
     // implementation, but this was not updated to match. Since then Android has
     // needed to maintain a stable libc++ ABI, and this can't be changed without
     // an ABI break.
+    // We also need this workaround for newlib since _NEWLIB_VERSION is not
+    // defined yet inside __config, so we can't set the
+    // _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE macro. Additionally, newlib is
+    // often used for space constrained environments, so it makes sense not to
+    // duplicate the ctype table.
     typedef uint16_t char_class_type;
 #else
     typedef ctype_base::mask        char_class_type;
 #endif
 
     static const char_class_type __regex_word = ctype_base::__regex_word;
 private:
     locale __loc_;