[SystemZ]:[z/OS]:[libcxx]: fix the mask in stage2_float_loop function

[NancyWang2222]

Summary

This patch is to fix issue related to __stage2_float_loop function, float point value comparison is not working on EBCDIC mode because the mask is hard-coded and assumes character is ASCII, fix is to use toupper function when do the comparison.

[NancyWang2222]

remove an extra curly bracket

[abhina.sreeskantharajan]

LGTM, but I think you need a libc++ group reviewer approval

[NancyWang2222]

ping -:)

[ldionne]

libcxx/include/locale

549

This is going to be really naive, but can you explain why exp & 0x7F this is even equivalent to toupper(exp)?

I understand the equivalence for c & 0x5f based on the table below, but I don't see the link for 0x7F:

0x41  01000001    A
0x61  01100001    a
0x42  01000010    B
0x62  01100010    b
0x43  01000011    C
0x63  01100011    c
0x44  01000100    D
0x64  01100100    d
0x45  01000101    E
0x65  01100101    e
0x46  01000110    F
0x66  01100110    f
0x47  01000111    G
0x67  01100111    g
0x48  01001000    H
0x68  01101000    h
0x49  01001001    I
0x69  01101001    i
[...]
0x58  01011000    X
0x78  01111000    x
0x59  01011001    Y
0x79  01111001    y
0x5A  01011010    Z
0x7A  01111010    z

0x5F  01011111
0x7F  01111111
0x80  10000000

560

I also don't understand this one.

[SeanP]

libcxx/include/locale

549

The 0x7f is tied to the "|0x80" below. It is an ugly hack that takes advantage of the fact all ascii characters are in the range of 0-127. The hack enables or disables the check on line 558 ((__x&0x5f)) == __exp) from matching the letter 'E' as a valid character in the mantissa or the start of the exponent. When the high order bit is set in __exp, the test on line 558 will match 'a' and 'A', but never 'e' or 'E', but the test here will match 'e' and 'E'.

This hack doesn't work for ebcdic since the characters are in the range of 0-255. To make the code work with both we changed the code to make __exp 'e' instead of 'E' plus the high order bit (line 560) and then on this line we upper case both __exp and the character to get the case insensitive compare and just upper case the character on line 558 to get the context sensitive compare.

I recall the logic is:

• line 549 - match any 'e'/'E' in the string
• line 558 - only match the first 'e'/'E'

Making __exp lower case instead of setting the high order bit achieves the same results.

[NancyWang2222]

if no more comments on this PR. plan to land it . Thanks for the feedback.

[NancyWang2222]

kindly ping for review or approval from libcxx group :)

[SeanP]

@ldionne, did the explanation help? I was thinking we could clean this logic up even better by adding a reference parameter to __stage2_float_loop() to track whether __exp has been seen or not rather than trying to store state in the character.

The logic around line 558 would become:

else if (!__seen_exp && _VSTD::toupper(__x) == __exp)
{
    __seen_exp = true;

And the compare on line 549 would simplify to:

if (__a_end == __a || (_VSTD::toupper(__a_end[-1]) == __exp))

[NancyWang2222]

Can I have 2nd review from libcxx. Thanks.

[philnik]

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

libcxx/include/locale

549

Same below.

[NancyWang2222]

In D118930#3332708, @philnik wrote:

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

[philnik]

In D118930#3332884, @NancyWang2222 wrote:

In D118930#3332708, @philnik wrote:

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

I would have expected test failures in locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, since this patch changes behavior of std::num_get on AIX IIUC. So either I'm not understanding something here or there are some test cases missing. Wait there are also no // XFAIL: AIX in std/numerics/rand.dis/rand.dis.bern/. Are these tests currently disabled on AIX somehow?

[NancyWang2222]

In D118930#3332943, @philnik wrote:

In D118930#3332884, @NancyWang2222 wrote:

In D118930#3332708, @philnik wrote:

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

I would have expected test failures in locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, since this patch changes behavior of std::num_get on AIX IIUC. So either I'm not understanding something here or there are some test cases missing. Wait there are also no // XFAIL: AIX in std/numerics/rand.dis/rand.dis.bern/. Are these tests currently disabled on AIX somehow?

I will take a look at locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, I didn't see this test case failing on z/OS. the patch will fix the following test cases on z/OS because z/OS has EBCDIC mode, I wasn't sure about AIX. @daltenty @hubert.reinterpretcast may know.

libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bernoulli/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.geo/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.negbin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.cauchy/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.chisq/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.f/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.lognormal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.normal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.t/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.exp/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.extreme/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.gamma/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.poisson/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.weibull/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/io.pass.cpp

[philnik]

In D118930#3333013, @NancyWang2222 wrote:

In D118930#3332943, @philnik wrote:

In D118930#3332884, @NancyWang2222 wrote:

In D118930#3332708, @philnik wrote:

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

I would have expected test failures in locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, since this patch changes behavior of std::num_get on AIX IIUC. So either I'm not understanding something here or there are some test cases missing. Wait there are also no // XFAIL: AIX in std/numerics/rand.dis/rand.dis.bern/. Are these tests currently disabled on AIX somehow?

I will take a look at locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, I didn't see this test case failing on z/OS. the patch will fix the following test cases on z/OS because z/OS has EBCDIC mode, I wasn't sure about AIX. @daltenty @hubert.reinterpretcast may know.

libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bernoulli/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.geo/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.negbin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.cauchy/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.chisq/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.f/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.lognormal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.normal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.t/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.exp/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.extreme/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.gamma/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.poisson/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.weibull/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/io.pass.cpp

Oh, sorry. For some reason I thought z/OS and AIX were the same thing. Are you working on libc++ officially supporting z/OS? If that is the case, maybe it would be a good idea to add a CI node for that?

[NancyWang2222]

Oh, sorry. For some reason I thought z/OS and AIX were the same thing. Are you working on libc++ officially supporting z/OS? If that is the case, maybe it would be a good idea to add a CI node for that?

no problem. yeah. we are working on making libc++ supporting z/OS as well. good idea. we will consider to add CI node for z/OS in future.

[SeanP]

In D118930#3333038, @philnik wrote:

In D118930#3333013, @NancyWang2222 wrote:

In D118930#3332943, @philnik wrote:

In D118930#3332884, @NancyWang2222 wrote:

In D118930#3332708, @philnik wrote:

There should also be a test changed somewhere in libcxx/test/std/localization, or is this only part of the fix?

Thanks for providing information, can you provide more detail about what change need for libcxx/test/localization, the patch will fix bunch of Assert d1==d2 issues in std/numerics/rand/rand.dis/rand.dist.bern/ on z/OS related to float point value.

I would have expected test failures in locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, since this patch changes behavior of std::num_get on AIX IIUC. So either I'm not understanding something here or there are some test cases missing. Wait there are also no // XFAIL: AIX in std/numerics/rand.dis/rand.dis.bern/. Are these tests currently disabled on AIX somehow?

I will take a look at locale.categories/category.numeric/facet.num.get.members/get_float.pass.cpp, I didn't see this test case failing on z/OS. the patch will fix the following test cases on z/OS because z/OS has EBCDIC mode, I wasn't sure about AIX. @daltenty @hubert.reinterpretcast may know.

libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bernoulli/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.geo/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.negbin/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.cauchy/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.chisq/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.f/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.lognormal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.normal/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.norm/rand.dist.norm.t/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.exp/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.extreme/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.gamma/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.poisson/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.weibull/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/io.pass.cpp
libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/io.pass.cpp

Oh, sorry. For some reason I thought z/OS and AIX were the same thing. Are you working on libc++ officially supporting z/OS? If that is the case, maybe it would be a good idea to add a CI node for that?

The exp parameter that is being updated is internal to the floating point. The changes we are making are consistent with the way the parameter was being updated. That is why it doesn't impact anything else.

I agree having a CI node for z/OS would be good. Hopefully we'll have one pretty soon.

[ldionne]

Thanks for the explanation.

[ldionne]

Please use std:: instead of _VSTD::, though (as suggested by @philnik)

[NancyWang2222]

In D118930#3354564, @ldionne wrote:

Please use std:: instead of _VSTD::, though (as suggested by @philnik)

hi Louis, sorry for late reply, haven't been active working on this for a while. I noticed there are many other places using '_VSTD::' in 'libcxx/include/locale' as well. I think should replace all of them to "std::" in separate patch. Thanks

libcxx/include/locale

549

This needs to use macro _VSTD which is defined as std, same with other line.

[philnik]

In D118930#3834907, @NancyWang2222 wrote:

In D118930#3354564, @ldionne wrote:

Please use std:: instead of _VSTD::, though (as suggested by @philnik)

hi Louis, sorry for late reply, haven't been active working on this for a while. I noticed there are many other places using '_VSTD::' in 'libcxx/include/locale' as well. I think should replace all of them to "std::" in separate patch. Thanks

We don't want to replace all the _VSTDs in libc++ at once, since there we still have it in a lot of places (> 3300 uses), but we don't want to introduce new uses. The idea is to remove _VSTD when we touch the code anyways.

[NancyWang2222]

use std:: to replace _VSTD::

libcxx/include/locale

if (__ct == __thousands_sep && __grouping.size() != 0)
         return 0;
     }
     ptrdiff_t __f = find(__atoms, __atoms + 32, __ct) - __atoms;
     if (__f >= 32)
         return -1;
     char __x = __src[__f];
     if (__x == '-' || __x == '+')
     {
-        if (__a_end == __a || (__a_end[-1] & 0x5F) == (__exp & 0x7F))
+        if (__a_end == __a || (_VSTD::toupper(__a_end[-1]) == _VSTD::toupper(__exp)))
         {
             *__a_end++ = __x;
             return 0;
         }
         return -1;
     }
     if (__x == 'x' || __x == 'X')
         __exp = 'P';
-    else if ((__x & 0x5F) == __exp)
+    else if (_VSTD::toupper(__x) == __exp)
     {
-        __exp |= (char) 0x80;
+        __exp = _VSTD::tolower(__exp);
         if (__in_units)
         {
             __in_units = false;
             if (__grouping.size() != 0 && __g_end-__g < __num_get_buf_sz)
                 *__g_end++ = __dc;
         }
     }
     *__a_end++ = __x;