LGTM

These look good to me. I do see the the AIX CI build is broken. This is due to -X32_64 being passed to llvm-ar. That appears independent of this change. @daltenty will need to take a look at that.

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?

@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.

We'll have to create another patch to apply the side deck renaming to the libc++exception added in https://reviews.llvm.org/D118620/new/.

LGTM

In D114473#3174515, @ldionne wrote:

In D114473#3174261, @SeanP wrote:

Hi Louis, you missed us on z/OS and AIX (@daltenty) . We provide 32-bit builds. We'll need to recover from this and may need to add some things back.

Ah, yeah, sorry for missing that. Any reason why you can't use target triple approach mentioned above?

Hi Louis, you missed us on z/OS and AIX (@daltenty) . We provide 32-bit builds. We'll need to recover from this and may need to add some things back.

In D99913#3173537, @ldionne wrote:

We can chat on Discord if you want more info on what I'm thinking about.

Can you update the summary to remove the part about including the unwind header.

LGTM

Louis, we don't want to make the entire threading model for z/OS to be dynamic. We would like to say the threading model uses pthread, but we need to enable these guard functions to work in programs that use threading and not. As Daniel pointed out a number of the libc++ threading functions used by these guard functions are not trivial to implement for a non-threading application. I would also worry about other parts of the library that should be using threading behaving correctly. We are open to other options. We could change the conditional compilations so both forms of the guard functions are available and call the right one depending on if threading is enabled or not. Would that be acceptable?

@Quuxplusone what parts of this do you feel are unmaintainable? We are open to ideas on how to address any concerns you have.

In D102798#2825509, @ldionne wrote:

LGTM, but just out of curiosity, will this stay unsupported forever, or will this be implemented eventually?

Normally, we try to support things unless we have a fundamental reason why it can't be done (for example supporting exceptions on an embedded system). But it seems a bit strange to me to mark aligned allocation function tests as unsupported based on the *architecture* - instead I would have expected it to be marked as unsupported based on specific OS versions that do not implement the feature yet.

Why did you have to add a specialization based on size? Could you provide a test case or two to help explain. My first thought is the _LIBCPP_SHORT_WCHAR macro should be good enough since aix & z/OS only support one wchar_t size per 32/64 bit mode.

LGTM

Changed the code checking the env vars so it doesn't assign a null_ptr to the string.