[SystemZ]:[z/OS]:[libcxx]: fix isascii function to work for z/OS

[NancyWang2222]

Summary

current logic in isascii function assumes character is ASCII. it is true when character is between 0 - 255. we need to change logic so it will work on both ASCII and EBCDIC mode.

[muiez]

LGTM

[SeanP]

libcxx/include/__locale

571

Rather than making this a static member function, I suggest creating an anonymous namespace outside the class and putting the function in there and as an inline function. You can then get rid of the duplicate definition in the other class.

I suspect to that for MVS we need to have one check for EBCDIC ([0,255]) and another for ASCII ([0,127]).

[SeanP]

libcxx/include/__locale

571

Nancy reminded me off-line that the char traits tables for MVS work on the range 0-255 for both ASCII and EBCDIC mode on MVS. The code, as is, is good.

[NancyWang2222]

ping -:)

[Mordante]

libcxx/include/__locale

571

Names in the library should be uglified, so for example __isbasic. I also like a better name, isascii is quite clear, isbasic not really. How about __is_valid_in_encoding? Maybe a bit long, but it's clear what you test.

573

Should this be #if defined(__MVS__) && !defined(__NATIVE_ASCII_F) like you did in D118849?

686

Is this intended to always return true, if you please just return true. You can use a (void) __c; to silence compiler diagnostics.

[SeanP]

libcxx/include/__locale

573

Thanks for thinking about ebcdic. The purpose of this function is to make sure the value in the range supported by the __tab indexing. On z/OS, the __tab is 256 chars regardless of ascii or ebcdic so the 0-255 range check works for both ascii & ebcdic.

I like the proposed new name.

[NancyWang2222]

libcxx/include/__locale

571

I will use new function name.

686

clang will emit error if argument c is not used in function. that is why I checked c.

[NancyWang2222]

libcxx/include/__locale

686

I will add void(__c);

[NancyWang2222]

change isbasic function name and fix return __c ? true: true statement

[NancyWang2222]

fix function name for locale.cpp as well.

[NancyWang2222]

run git-clang-format for the changes

[Mordante]

In general it looks good, some minor nits. I'd like to see it pass the CI before approving. I expect the ASAN build to fail since a recent compiler update in the build-nodes causes failures.

libcxx/include/__locale

571

@NancyWang2222 when you addressed a comment can you mark it as done? That makes it easier to see which comments haven't been addressed yet.

libcxx/src/locale.cpp

1026

I don't mind the formatting in the other #if blocks, but then please update the indention here also.
Please also look at the other indention changes you made.

[NancyWang2222]

libcxx/src/locale.cpp

1026

sure . I will update it.

[NancyWang2222]

fix indentation for the function modified.

[Mordante]

LGTM, please wait for a libc++ approval before committing.

[ldionne]

libcxx/include/__locale

571

Here and below.

686

Why is this intended to always return true?

[NancyWang2222]

libcxx/include/__locale

571

i will add.

686

isascii function is not right test for z/OS, char type is between [0-255] which is valid character indexing in __tab function on z/OS for both ASCII and EBCDIC.

[NancyWang2222]

add _LIBCPP_HIDE_FROM_ABI for function __is_valid_in_encoding

[NancyWang2222]

change function name and return type

[NancyWang2222]

please ignore comment.

[NancyWang2222]

@ldionne hi Louis, Can you help review again? Thanks everyone for providing feedback.

[NancyWang2222]

kindly ping for review or approval from libcxx group

[NancyWang2222]

can I have 2nd review from libcxx?

[• Quuxplusone]

libcxx/src/locale.cpp

1029

Changes like this one make me nervous. The original purpose of isascii(x) seems to have been a bounds check: it says "yes, x is in the range 0 to 127 inclusive." That doesn't really have anything to do with the ASCII versus EBCDIC question — hence the mass renaming to __is_valid_in_encoding, on which I'm neutral. However, it is very important to all of these table lookups: If x isn't in the range 0 to 127, e.g. if x is -1 or 10000, then these lookups will have UB.

Can you explain why you want to make this change? Specifically, what behaves wrongly on your platform if we don't do any of this?

[SeanP]

libcxx/src/locale.cpp

1029

The tables on z/OS have the range 0-255. This is true for both ASCII and EBCDIC. The tables need to be 0-255 since EBCDIC characters are spread over this entire range (eg. '9' is 0xf9). The system uses the same range in for the ASCII tables which is why the code uses 0-255 for z/OS.

[• Quuxplusone]

libcxx/include/__locale

686

IIUC now (based on @SeanP's comment below), __is_valid_in_encoding(x) means no more or less than "0 <= x && x < N, where N is the number of elements in the array that was passed to the constructor on line 681." For most platforms this N is 128; for z/OS in particular, this N is 256.

Is this N always the same as ctype::table_size from lines 777–779? What goes wrong if we make this function simply

static bool __is_in_range(size_t __c) { return __c < table_size; }

? See
https://en.cppreference.com/w/cpp/locale/ctype_char/classic_table
https://en.cppreference.com/w/cpp/locale/ctype_char/table

Personally (although @ldionne is welcome to overrule me) I'd like to move away from the existing code's use of isascii(x) as a shorthand for x < 128, because this bounds check doesn't really have anything to do with ASCII, and it's silly to have to read a man page just to find out it means x < 128. If we mean x < table_size, we should just say that.

libcxx/src/locale.cpp

1029

Ah, thanks. I've resumed the comment thread on __is_valid_in_encoding above.

[SeanP]

libcxx/include/__locale

686

Good observation. However, I noticed that table_size==256. I don't think that we can assume that ctype<char>::classic_table and __tab_ have the same range. The code as it is will return false for anything out of range of __tab_ for ctype<char>::classic_table. If they should have the same size then we would have to update table_size for non-z/OS platforms. It is currently set to 256 for all platforms. I worry this will break people.

If you think the __is_in_range() fucntion should return table_size, I'd rather do that in a separate patch to avoid mixing it in with making things work for z/OS.

[NancyWang2222]

ping for the review. Thanks

[• Quuxplusone]

libcxx/include/__locale

686

My current thought is that there are at least two, maybe three, changes going on in this PR:
(1) Rename isascii(x) to __is_valid_in_encoding(x). I agree that isascii is the wrong name, but I still hypothesize that the right name is nothing specifically to do with "encoding," but more like __is_valid_table_index. Sometimes the table in question is __tab_; sometimes it's classic_table(); do we want two different functions depending on which table we're talking about? Are all these tables guaranteed to have the same size? IMHO this renaming should be its own PR.
(2) Specifically for z/OS, change the behavior of __is_valid_in_encoding (or whatever its name(s) ends up as). This seems uncontroversial to me, but sadly is blocked on (1).
(3?) Other ad-hoc EBCDICifications, like replacing uses of x - 'a'? I see some non-functional diffs involving L'A' - L'a' and so on, but I'm not sure if there might be some functional diffs hiding among them. Anyway, I would imagine these would be uncontroversial, if they exist.

libcxx/include/__locale

 {
 public:
     typedef wchar_t char_type;
 
     _LIBCPP_INLINE_VISIBILITY
     explicit ctype(size_t __refs = 0)
         : locale::facet(__refs) {}
 
+    _LIBCPP_HIDE_FROM_ABI
+    static bool __is_valid_in_encoding(char_type __c)
+    {
+#if defined(__MVS__)
+        return (__c >=0 && __c < 256);
+#else
+        return isascii(__c);
+#endif
+    }
+
     _LIBCPP_INLINE_VISIBILITY
     bool is(mask __m, char_type __c) const
     {
         return do_is(__m, __c);
     }
 
     _LIBCPP_INLINE_VISIBILITY
     const char_type* is(const char_type* __low, const char_type* __high, mask* __vec) const

 {
     const mask* __tab_;
     bool        __del_;
 public:
     typedef char char_type;
 
     explicit ctype(const mask* __tab = nullptr, bool __del = false, size_t __refs = 0);
 
+    _LIBCPP_HIDE_FROM_ABI
+    static bool __is_valid_in_encoding(char_type __c)
+    {
+#if defined(__MVS__)
+        (void)__c;
+        return true;
+#else
+        return isascii(__c);
+#endif
+    }
+
     _LIBCPP_INLINE_VISIBILITY
     bool is(mask __m, char_type __c) const
     {
-        return isascii(__c) ? (__tab_[static_cast<int>(__c)] & __m) !=0 : false;
+        return __is_valid_in_encoding(__c) ? (__tab_[static_cast<int>(__c)] & __m) !=0 : false;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     const char_type* is(const char_type* __low, const char_type* __high, mask* __vec) const
     {
         for (; __low != __high; ++__low, ++__vec)
-            *__vec = isascii(*__low) ? __tab_[static_cast<int>(*__low)] : 0;
+            *__vec = __is_valid_in_encoding(*__low) ? __tab_[static_cast<int>(*__low)] : 0;
         return __low;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     const char_type* scan_is (mask __m, const char_type* __low, const char_type* __high) const
     {
         for (; __low != __high; ++__low)
-            if (isascii(*__low) && (__tab_[static_cast<int>(*__low)] & __m))
+            if (__is_valid_in_encoding(*__low) && (__tab_[static_cast<int>(*__low)] & __m))
                 break;
         return __low;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     const char_type* scan_not(mask __m, const char_type* __low, const char_type* __high) const
     {
         for (; __low != __high; ++__low)
-            if (!(isascii(*__low) && (__tab_[static_cast<int>(*__low)] & __m)))
+            if (!(__is_valid_in_encoding(*__low) && (__tab_[static_cast<int>(*__low)] & __m)))
                 break;
         return __low;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     char_type toupper(char_type __c) const
     {
         return do_toupper(__c);

libcxx/src/locale.cpp

 
 ctype<wchar_t>::~ctype()
 {
 }
 
 bool
 ctype<wchar_t>::do_is(mask m, char_type c) const
 {
-    return isascii(c) ? (ctype<char>::classic_table()[c] & m) != 0 : false;
+  return __is_valid_in_encoding(c) ? (ctype<char>::classic_table()[c] & m) != 0 : false;
 }
 
 const wchar_t*
 ctype<wchar_t>::do_is(const char_type* low, const char_type* high, mask* vec) const
 {
     for (; low != high; ++low, ++vec)
-        *vec = static_cast<mask>(isascii(*low) ?
-                                   ctype<char>::classic_table()[*low] : 0);
+      *vec = static_cast<mask>(__is_valid_in_encoding(*low) ? ctype<char>::classic_table()[*low] : 0);
     return low;
 }
 
 const wchar_t*
 ctype<wchar_t>::do_scan_is(mask m, const char_type* low, const char_type* high) const
 {
     for (; low != high; ++low)
-        if (isascii(*low) && (ctype<char>::classic_table()[*low] & m))
-            break;
+      if (__is_valid_in_encoding(*low) && (ctype<char>::classic_table()[*low] & m))
     return low;
 }
 
 const wchar_t*
 ctype<wchar_t>::do_scan_not(mask m, const char_type* low, const char_type* high) const
 {
     for (; low != high; ++low)
-        if (!(isascii(*low) && (ctype<char>::classic_table()[*low] & m)))
-            break;
+      if (!(__is_valid_in_encoding(*low) && (ctype<char>::classic_table()[*low] & m)))
     return low;
 }
 
 wchar_t
 ctype<wchar_t>::do_toupper(char_type c) const
 {
 #ifdef _LIBCPP_HAS_DEFAULTRUNELOCALE
-    return isascii(c) ? _DefaultRuneLocale.__mapupper[c] : c;
-#elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || \
-      defined(__NetBSD__) || defined(__MVS__)
-    return isascii(c) ? ctype<char>::__classic_upper_table()[c] : c;
-#else
-    return (isascii(c) && iswlower_l(c, _LIBCPP_GET_C_LOCALE)) ? c-L'a'+L'A' : c;
-#endif
+  return __is_valid_in_encoding(c) ? _DefaultRuneLocale.__mapupper[c] : c;
+#  elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || defined(__NetBSD__) || defined(__MVS__)
+  return __is_valid_in_encoding(c) ? ctype<char>::__classic_upper_table()[c] : c;
+#  else
+  return (__is_valid_in_encoding(c) && iswlower_l(c, _LIBCPP_GET_C_LOCALE)) ? c - L'a' + L'A' : c;
+#  endif
 }
 
 const wchar_t*
 ctype<wchar_t>::do_toupper(char_type* low, const char_type* high) const
 {
     for (; low != high; ++low)
 #ifdef _LIBCPP_HAS_DEFAULTRUNELOCALE
-        *low = isascii(*low) ? _DefaultRuneLocale.__mapupper[*low] : *low;
+      *low = __is_valid_in_encoding(*low) ? _DefaultRuneLocale.__mapupper[*low] : *low;
 #elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || \
       defined(__NetBSD__) || defined(__MVS__)
-        *low = isascii(*low) ? ctype<char>::__classic_upper_table()[*low]
-                             : *low;
+      *low = __is_valid_in_encoding(*low) ? ctype<char>::__classic_upper_table()[*low] : *low;
 #else
-        *low = (isascii(*low) && islower_l(*low, _LIBCPP_GET_C_LOCALE)) ? (*low-L'a'+L'A') : *low;
+      *low = (__is_valid_in_encoding(*low) && islower_l(*low, _LIBCPP_GET_C_LOCALE)) ? (*low - L'a' + L'A') : *low;
 #endif
     return low;
 }
 
 wchar_t
 ctype<wchar_t>::do_tolower(char_type c) const
 {
 #ifdef _LIBCPP_HAS_DEFAULTRUNELOCALE
-    return isascii(c) ? _DefaultRuneLocale.__maplower[c] : c;
-#elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || \
-      defined(__NetBSD__) || defined(__MVS__)
-    return isascii(c) ? ctype<char>::__classic_lower_table()[c] : c;
-#else
-    return (isascii(c) && isupper_l(c, _LIBCPP_GET_C_LOCALE)) ? c-L'A'+'a' : c;
-#endif
+  return __is_valid_in_encoding(c) ? _DefaultRuneLocale.__maplower[c] : c;
+#  elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || defined(__NetBSD__) || defined(__MVS__)
+  return __is_valid_in_encoding(c) ? ctype<char>::__classic_lower_table()[c] : c;
+#  else
+  return (__is_valid_in_encoding(c) && isupper_l(c, _LIBCPP_GET_C_LOCALE)) ? c - L'A' + 'a' : c;
+#  endif
 }
 
 const wchar_t*
 ctype<wchar_t>::do_tolower(char_type* low, const char_type* high) const
 {
     for (; low != high; ++low)
 #ifdef _LIBCPP_HAS_DEFAULTRUNELOCALE
-        *low = isascii(*low) ? _DefaultRuneLocale.__maplower[*low] : *low;
+      *low = __is_valid_in_encoding(*low) ? _DefaultRuneLocale.__maplower[*low] : *low;
 #elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || \
       defined(__NetBSD__) || defined(__MVS__)
-        *low = isascii(*low) ? ctype<char>::__classic_lower_table()[*low]
-                             : *low;
+      *low = __is_valid_in_encoding(*low) ? ctype<char>::__classic_lower_table()[*low] : *low;
 #else
-        *low = (isascii(*low) && isupper_l(*low, _LIBCPP_GET_C_LOCALE)) ? *low-L'A'+L'a' : *low;
+      *low = (__is_valid_in_encoding(*low) && isupper_l(*low, _LIBCPP_GET_C_LOCALE)) ? *low - L'A' + L'a' : *low;
 #endif
     return low;
 }
 
 wchar_t
 ctype<wchar_t>::do_widen(char c) const
 {
     return c;
 }
 
 const char*
 ctype<wchar_t>::do_widen(const char* low, const char* high, char_type* dest) const
 {
     for (; low != high; ++low, ++dest)
         *dest = *low;
     return low;
 }
 
 char
 ctype<wchar_t>::do_narrow(char_type c, char dfault) const
 {
-    if (isascii(c))
-        return static_cast<char>(c);
-    return dfault;
+  if (__is_valid_in_encoding(c))
 }
 
 const wchar_t*
 ctype<wchar_t>::do_narrow(const char_type* low, const char_type* high, char dfault, char* dest) const
 {
     for (; low != high; ++low, ++dest)
-        if (isascii(*low))
-            *dest = static_cast<char>(*low);
-        else
-            *dest = dfault;
+      if (__is_valid_in_encoding(*low))
     return low;
 }
 #endif // _LIBCPP_HAS_NO_WIDE_CHARACTERS
 
 // template <> class ctype<char>;
 
 locale::id ctype<char>::id;
 
ctype<char>::~ctype()
     if (__tab_ && __del_)
         delete [] __tab_;
 }
 
 char
 ctype<char>::do_toupper(char_type c) const
 {
 #ifdef _LIBCPP_HAS_DEFAULTRUNELOCALE
-    return isascii(c) ?
-      static_cast<char>(_DefaultRuneLocale.__mapupper[static_cast<ptrdiff_t>(c)]) : c;
+  return __is_valid_in_encoding(c) ? static_cast<char>(_DefaultRuneLocale.__mapupper[static_cast<ptrdiff_t>(c)]) : c;
 #elif defined(__NetBSD__)
-    return static_cast<char>(__classic_upper_table()[static_cast<unsigned char>(c)]);
 #elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || defined(__MVS__)
-    return isascii(c) ?
-      static_cast<char>(__classic_upper_table()[static_cast<unsigned char>(c)]) : c;
+  return __is_valid_in_encoding(c) ? static_cast<char>(__classic_upper_table()[static_cast<unsigned char>(c)]) : c;
 #else
-    return (isascii(c) && islower_l(c, _LIBCPP_GET_C_LOCALE)) ? c-'a'+'A' : c;
+  return (__is_valid_in_encoding(c) && islower_l(c, _LIBCPP_GET_C_LOCALE)) ? c - 'a' + 'A' : c;
 #endif
 }
 
 const char*
 ctype<char>::do_toupper(char_type* low, const char_type* high) const
 {
     for (; low != high; ++low)
 #ifdef _LIBCPP_HAS_DEFAULTRUNELOCALE
-        *low = isascii(*low) ?
-          static_cast<char>(_DefaultRuneLocale.__mapupper[static_cast<ptrdiff_t>(*low)]) : *low;
+      *low = __is_valid_in_encoding(*low)
+                 ? static_cast<char>(_DefaultRuneLocale.__mapupper[static_cast<ptrdiff_t>(*low)])
+                 : *low;
 #elif defined(__NetBSD__)
-        *low = static_cast<char>(__classic_upper_table()[static_cast<unsigned char>(*low)]);
 #elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || defined(__MVS__)
-        *low = isascii(*low) ?
-          static_cast<char>(__classic_upper_table()[static_cast<size_t>(*low)]) : *low;
+      *low =
+          __is_valid_in_encoding(*low) ? static_cast<char>(__classic_upper_table()[static_cast<size_t>(*low)]) : *low;
 #else
-        *low = (isascii(*low) && islower_l(*low, _LIBCPP_GET_C_LOCALE)) ? *low-'a'+'A' : *low;
+      *low = (__is_valid_in_encoding(*low) && islower_l(*low, _LIBCPP_GET_C_LOCALE)) ? *low - 'a' + 'A' : *low;
 #endif
     return low;
 }
 
 char
 ctype<char>::do_tolower(char_type c) const
 {
 #ifdef _LIBCPP_HAS_DEFAULTRUNELOCALE
-    return isascii(c) ?
-      static_cast<char>(_DefaultRuneLocale.__maplower[static_cast<ptrdiff_t>(c)]) : c;
+  return __is_valid_in_encoding(c) ? static_cast<char>(_DefaultRuneLocale.__maplower[static_cast<ptrdiff_t>(c)]) : c;
 #elif defined(__NetBSD__)
-    return static_cast<char>(__classic_lower_table()[static_cast<unsigned char>(c)]);
 #elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || defined(__MVS__)
-    return isascii(c) ?
-      static_cast<char>(__classic_lower_table()[static_cast<size_t>(c)]) : c;
+  return __is_valid_in_encoding(c) ? static_cast<char>(__classic_lower_table()[static_cast<size_t>(c)]) : c;
 #else
-    return (isascii(c) && isupper_l(c, _LIBCPP_GET_C_LOCALE)) ? c-'A'+'a' : c;
+  return (__is_valid_in_encoding(c) && isupper_l(c, _LIBCPP_GET_C_LOCALE)) ? c - 'A' + 'a' : c;
 #endif
 }
 
 const char*
 ctype<char>::do_tolower(char_type* low, const char_type* high) const
 {
     for (; low != high; ++low)
 #ifdef _LIBCPP_HAS_DEFAULTRUNELOCALE
-        *low = isascii(*low) ? static_cast<char>(_DefaultRuneLocale.__maplower[static_cast<ptrdiff_t>(*low)]) : *low;
+      *low = __is_valid_in_encoding(*low)
+                 ? static_cast<char>(_DefaultRuneLocale.__maplower[static_cast<ptrdiff_t>(*low)])
+                 : *low;
 #elif defined(__NetBSD__)
-        *low = static_cast<char>(__classic_lower_table()[static_cast<unsigned char>(*low)]);
 #elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || defined(__MVS__)
-        *low = isascii(*low) ? static_cast<char>(__classic_lower_table()[static_cast<size_t>(*low)]) : *low;
+      *low =
+          __is_valid_in_encoding(*low) ? static_cast<char>(__classic_lower_table()[static_cast<size_t>(*low)]) : *low;
 #else
-        *low = (isascii(*low) && isupper_l(*low, _LIBCPP_GET_C_LOCALE)) ? *low-'A'+'a' : *low;
+      *low = (__is_valid_in_encoding(*low) && isupper_l(*low, _LIBCPP_GET_C_LOCALE)) ? *low - 'A' + 'a' : *low;
 #endif
     return low;
 }
 
 char
 ctype<char>::do_widen(char c) const
 {
     return c;
 }
 
 const char*
 ctype<char>::do_widen(const char* low, const char* high, char_type* dest) const
 {
     for (; low != high; ++low, ++dest)
         *dest = *low;
     return low;
 }
 
 char
 ctype<char>::do_narrow(char_type c, char dfault) const
 {
-    if (isascii(c))
-        return static_cast<char>(c);
-    return dfault;
+  if (__is_valid_in_encoding(c))
 }
 
 const char*
 ctype<char>::do_narrow(const char_type* low, const char_type* high, char dfault, char* dest) const
 {
     for (; low != high; ++low, ++dest)
-        if (isascii(*low))
-            *dest = *low;
-        else
-            *dest = dfault;
+      if (__is_valid_in_encoding(*low))
     return low;
 }
 
 #if defined(__EMSCRIPTEN__)
 extern "C" const unsigned short ** __ctype_b_loc();
 extern "C" const int ** __ctype_tolower_loc();
 extern "C" const int ** __ctype_toupper_loc();
 #endif

 #endif
 }
 
 const wchar_t*
 ctype_byname<wchar_t>::do_is(const char_type* low, const char_type* high, mask* vec) const
 {
     for (; low != high; ++low, ++vec)
     {
-        if (isascii(*low))
-            *vec = static_cast<mask>(ctype<char>::classic_table()[*low]);
-        else
-        {
-            *vec = 0;
-            wint_t ch = static_cast<wint_t>(*low);
-            if (iswspace_l(ch, __l))
-                *vec |= space;
+      if (__is_valid_in_encoding(*low))
+      else {
 #ifndef _LIBCPP_CTYPE_MASK_IS_COMPOSITE_PRINT
             if (iswprint_l(ch, __l))
                 *vec |= print;
 #endif
             if (iswcntrl_l(ch, __l))
                 *vec |= cntrl;
             if (iswupper_l(ch, __l))
                 *vec |= upper;