[SystemZ][z/OS][libcxx]: Disable some new operator test cases on z/OS

[ldionne]

Summary

This Patch is to disable some test cases related to new operator, test cases are written based on Linux run time linking model, it selects symbols which are available at run time over the one in shared library, but on z/OS, references are determined at link step, it chose symbols it knows about in shared library not the one user defined. The program has c++ rt new[] or new call another new operator , assume it will call user defined new, which is not the case on z/OS.

[• Quuxplusone]

This appears to be the same as D105910. I advise merging these changes into that one; and then also taking the approach suggested by mstorsjo, ldionne and myself over there, of introducing a higher-level macro like ASSERT_VIA_OVERRIDDEN_NEW on the specific lines that are expected to fail when operator new can't be overridden on the platform.

[NancyWang2222]

Thanks. will look into how ASSERT_VIA_OVERRIDDEN_NEW works

[NancyWang2222]

Hi I only updated 3 test cases , I didnt modify test cases in https://reviews.llvm.org/D105910 , because @DanielMcIntosh-IBM did some investigation already in those test cases, I will let him handle it. another reason is I am unable to test windows if it requires modify macros for windows as well

[NancyWang2222]

@Quuxplusone hi Arthur, Can you review ?

[• Quuxplusone]

LGTM mod some trivial comments. I'd also like to wait up to 48 hours for @mstorsjo's feedback, if any.

libcxx/test/support/test_macros.h

335–341

Yes, I like how clean this looks! @mstorsjo, how would this play on Windows? Could we replace the Windows testing macro stuff with this macro, as a followup commit?

@NancyWang2222, I don't think there'd ever be a platform that allowed overriding new without delete, or delete without new; therefore, ASSERT_VIA_OVERRIDDEN_DELETE is always going to be the same as ASSERT_VIA_OVERRIDDEN_NEW; therefore, you should just get rid of ASSERT_VIA_OVERRIDDEN_DELETE. Replace all its uses with ASSERT_VIA_OVERRIDDEN_NEW.
(One could imagine renaming the macro to ASSERT_VIA_OVERRIDDEN_NEW_{AND,OR}_DELETE to gain technical correctness at the cost of identifier length and readability; but I think ASSERT_VIA_OVERRIDDEN_NEW is perfectly clear enough.)

[NancyWang2222]

@Quuxplusone Thanks Arthur, I can remove delete macro. let me know how windows works if need me to check windows as well.

[DanielMcIntosh-IBM]

I will update D105910 with these test cases, so this can be closed

[mstorsjo]

(Adding libcxx-commits as subscriber, it seems like this review thread has managed to slip past the mailing list?)

@ NancyWang2222 - We have CI for Windows (both for the DLL and statically linked configurations), so if your patch changes them it should be visible in the CI status. (It won't notice if the patch removes asserts that were passing though - so that's the main thing we need to be on the lookut for.)

libcxx/test/support/test_macros.h

335–341

It looks like this is a different thing than D105910...

So on Windows, if libc++ is built as a DLL, then calls to operator new within the libc++ implementation cpp can't be overriden in user code. That's what's handled in D105910, and the checks updated there could be shared with the existing exceptions for the Windows/DLL configuration.

But this patch modifies tests that work just fine on Windows - where the user code (the test) both provides the overridden operator new and calls it, and that configuration works just fine. (These tests have no UNSUPPORTED or XFAIL for windows, so the plain assert() is passing there.)

So if we make generic macros in test_macros.h for it, we need to distinguish between the two cases, as one should be omitted on Windows/DLL but the other one shouldn't. (We don't want to accidentally remove working asserts from some configuration either, as that's reducing test coverage.)

But in this case, the tests we're modifying pretty much only test this one thing (overriding operator new), so by removing these asserts we leave next to nothing in the testcases. Therefore, just adding UNSUPPORTED in these cases would probably be fine. But in D105910 on the other hand, the operator new overriding/counting aspect is only a minor detail of the tests, so it's much better to keep running the whole test and just omit the allocation counting aspect, instead of skipping the whole test.

[• Quuxplusone]

libcxx/test/support/test_macros.h

335–341

Therefore, just adding UNSUPPORTED in these cases would probably be fine

Technically yes; but (1) the state of the art is the awful regex UNSUPPORTED: target={{.+}}-zos{{.*}}, so one goal here is to be less ugly and also more clear and granular about why the test needs disabling; and (2) it'd be nice to have a solution that "scales" to test files that aren't so simple as these.

we need to distinguish between the two cases, as one should be omitted on Windows/DLL but the other one shouldn't

Agreed. Do you have suggestions for how to refer to these two cases? I'll suggest ASSERT_VIA_DIRECTLY_OVERRIDDEN_NEW and ASSERT_VIA_INDIRECTLY_OVERRIDDEN_NEW, but I'm not sure I have understood the issue, and if I have, then my suggestions are a little technically/grammatically incorrect. (It's the call to the overridden new that is direct/indirect, right? not the overriding itself.)

[mstorsjo]

libcxx/test/support/test_macros.h

335–341

Technically yes; but (1) the state of the art is the awful regex UNSUPPORTED: target={{.+}}-zos{{.*}}, so one goal here is to be less ugly and also more clear and granular about why the test needs disabling;

Well I wouldn't let the syntax of that scare away from that solution, if it would be the best one. Also zos could be added as one of the identified OSes in libcxx/utils/libcxx/test/features.py like so it'd just be UNSUPPORTED: zos, and such a line can be accompanied with a comment explaining why it's there.

and (2) it'd be nice to have a solution that "scales" to test files that aren't so simple as these.

Yeah total agree on that. My 2 cents here was that in these particular tests, there's very little of value left after disabling this aspect.

we need to distinguish between the two cases, as one should be omitted on Windows/DLL but the other one shouldn't

Agreed. Do you have suggestions for how to refer to these two cases? I'll suggest ASSERT_VIA_DIRECTLY_OVERRIDDEN_NEW and ASSERT_VIA_INDIRECTLY_OVERRIDDEN_NEW, but I'm not sure I have understood the issue, and if I have, then my suggestions are a little technically/grammatically incorrect. (It's the call to the overridden new that is direct/indirect, right? not the overriding itself.)

Sounds like you've understood it exactly right to me (that's great!). Yep, wording/naming things concisely is always hard.

I'm not sure VIA is the best word, would FOR or WITH be better do you think? As for how to explain the indirect-call-to-overridden-new correctly in less than 5 words, I don't really know...

[• Quuxplusone]

libcxx/test/support/test_macros.h

335–341

Actually, on closer inspection: in e.g. new_array_nothrow_replace.pass.cpp we are not directly calling the new that is overridden. We're overriding single-element operator new(size_t) but calling new A[3], which calls operator new[](size_t). So the test is basically testing that [operator new[](size_t) from new.cpp](https://github.com/llvm/llvm-project/blob/main/libcxx/src/new.cpp#L103-L108) is handled correctly by the runtime.

@mstorsjo, is this related to the existing annotation XFAIL: libcpp-no-vcruntime? If so, then basically (modulo naming) every test case that currently mentions libcpp-no-vcruntime could be refactored to use ASSERT_VIA_DIRECTLY_OVERRIDDEN_NEW (D107124), and every test case that currently mentions TEST_NOT_WIN32_DLL could be refactored to use ASSERT_VIA_INDIRECTLY_OVERRIDDEN_NEW (D105910).

Btw, I see the technical term these days is that operator new has been "replaced" — not "overridden" as I'd been saying — so maybe the naming should reflect that. https://eel.is/c++draft/new.delete

Finally, I still don't understand the mechanism that distinguishes "a call to operator new compiled into src/new.cpp" (D107124) from "a call to operator new compiled into src/filesystem.cpp" (D105910). I see that empirically they must behave differently on both Windows and z/OS, but I don't understand why that should be. Aren't new.o and filesystem.o both archived into the same library, along with everything else in libcxx/src/? ...Do z/OS and/or Windows have a similar problem with the new calls embedded within std::to_string(42), and if so, which of the two problems is it?

[mstorsjo]

libcxx/test/support/test_macros.h

335–341

Oh, great observation actually.

No, libcpp-no-vcruntime is not set in the regular windows configurations. (That's a configuration that I'm not entirely sure how well it works in practice, and I'm not sure if the marking for that configuration are up to date.)

When building for Windows in MSVC configurations, it uses MSVC's vcruntime as the ABI library, so operator new and all the fallback mechanisms between them comes from there, not provided by libc++. The contents of libcxx/src/new.cpp is wrapped in #if !defined(_LIBCPP_ABI_VCRUNTIME). And the set of operator new implementations that fallback between each other from vcruntime, essentially the equivalent of src/new.cpp, is statically linked into the user executable. So a call to operator new[](size_t) in user code ends up picking up the replaced operator new(size_t) in the executable.

However, this only holds for MSVC configurations; for MinGW configurations, we do use the implementations from src/new.cpp, and these tests do fail there (our CI setup don't have the necessary things for running tests in such a configuration, not yet at least). I think the same holds for the libcpp-no-vcruntime configuration too.

I have a local branch for testing in MinGW mode, and there I've added an XFAIL: mingw && windows-dll in these particular tests; in particular for these tests:

.../new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
.../new.delete.array/new_array_nothrow_replace.pass.cpp
.../new.delete.array/new_array_replace.pass.cpp
.../new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
.../new.delete.single/new_nothrow_replace.pass.cpp

That's pretty much the same as this patch modifies, modulo the align_val_t tests.

So then I guess the distinction needs to be whether the indirection is via a different operator new (essentially the ABI library, which can be provided by libc++ or externally) or via calls from within libc++ proper (what's a good name for that?)

Would ASSERT_WITH_OPERATOR_NEW_FALLBACKS vs ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS be descriptive for the distinction?

[• Quuxplusone]

libcxx/test/support/test_macros.h

335–341

Would ASSERT_WITH_OPERATOR_NEW_FALLBACKS vs ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS be descriptive for the distinction?

Yes maybe; although too verbose for my liking. I think we need some input from @ldionne and/or @EricWF here.
I somewhat understand your distinction between "the ABI library" and "libc++ proper"; but IIUC, the operator new fallbacks are linked/archived into libc++.*, not into libc++abi.*, so I still don't really understand the mechanism by which the difference is produced nor the meaning of "the ABI library" in libc++'s context. I wildly guess that these troublesome platforms unset the LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS option in CMake...?

[mstorsjo]

libcxx/test/support/test_macros.h

335–341

I somewhat understand your distinction between "the ABI library" and "libc++ proper"; but IIUC, the operator new fallbacks are linked/archived into libc++.*, not into libc++abi.*

No, there's no such guarantee

so I still don't really understand the mechanism by which the difference is produced nor the meaning of "the ABI library" in libc++'s context. I wildly guess that these troublesome platforms unset the LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS option in CMake...?

That option actually defaults to OFF, so the default configuration would have them in libc++abi.so/dylib.

In general, a toolchain vendor can configure this setup in a multitude of ways; one can build libc++ on top of libsupc++/libstdc++, or vcruntime, or libc++abi, one can statically link in libc++abi into a dynamically linked libc++, etc. In the case of vcruntime, the vcruntime import library is a kind of hybrid library, that does dynamic linking of most things, but statically links and embeds those fallbacks into the linked user executable.

[NancyWang2222]

hi Any suggestion how we should fix those test case, I can add unsupport on z/OS.

[ldionne]

Commandeering to abandon since this has been handled in 128b2136ec62.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_array_nothrow_replace.pass.cpp

 
 int main(int, char**)
 {
     new_called = 0;
     A *ap = new (std::nothrow) A[3];
     DoNotOptimize(ap);
     assert(ap);
     assert(A_constructed == 3);
-    assert(new_called);
+    ASSERT_VIA_OVERRIDDEN_NEW(new_called);
     delete [] ap;
     DoNotOptimize(ap);
     assert(A_constructed == 0);
-    assert(!new_called);
+    ASSERT_VIA_OVERRIDDEN_DELETE(!new_called);
 
   return 0;
 }

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_array_replace.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
 //
 //===----------------------------------------------------------------------===//
 
 // test operator new[] replacement by replacing only operator new
 
 // UNSUPPORTED: sanitizer-new-delete
 // XFAIL: libcpp-no-vcruntime
 
-
 #include <new>
 #include <cstddef>
 #include <cstdlib>
 #include <cassert>
 #include <limits>
 
 #include "test_macros.h"
 

 
 int main(int, char**)
 {
     new_called = 0;
     A *ap = new A[3];
     DoNotOptimize(ap);
     assert(ap);
     assert(A_constructed == 3);
-    assert(new_called == 1);
+    ASSERT_VIA_OVERRIDDEN_NEW(new_called == 1);
     delete [] ap;
     DoNotOptimize(ap);
     assert(A_constructed == 0);
-    assert(new_called == 0);
+    ASSERT_VIA_OVERRIDDEN_DELETE(new_called == 0);
 
   return 0;
 }

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_nothrow_replace.pass.cpp

 
 int main(int, char**)
 {
     new_called = 0;
     A *ap = new (std::nothrow) A;
     DoNotOptimize(ap);
     assert(ap);
     assert(A_constructed);
-    assert(new_called);
+    ASSERT_VIA_OVERRIDDEN_NEW(new_called);
     delete ap;
     DoNotOptimize(ap);
     assert(!A_constructed);
-    assert(!new_called);
+    ASSERT_VIA_OVERRIDDEN_DELETE(!new_called);
 
   return 0;
 }

libcxx/test/support/test_macros.h

 template <class T>
 struct is_same<T, T> { enum {value = 1}; };
 } // namespace test_macros_detail
 
 #define ASSERT_SAME_TYPE(...) \
     static_assert((test_macros_detail::is_same<__VA_ARGS__>::value), \
                  "Types differ unexpectedly")
 
+// z/OS has different runtime linking model, references are determined at
+// link step, C++RT new or new[] operator will call the new operator it knows
+// about in shared library not the one user defined. Same with delete
+// operator.
+#if defined(__MVS__)
+#define ASSERT_VIA_OVERRIDDEN_NEW(x)
+#define ASSERT_VIA_OVERRIDDEN_DELETE(x)
+#else
+#define ASSERT_VIA_OVERRIDDEN_NEW(x) assert(x)
+#define ASSERT_VIA_OVERRIDDEN_DELETE(x) assert(x)
+#endif
+
 #ifndef TEST_HAS_NO_EXCEPTIONS
 #define TEST_THROW(...) throw __VA_ARGS__
 #else
 #if defined(__GNUC__)
 #define TEST_THROW(...) __builtin_abort()
 #else
 #include <stdlib.h>
 #define TEST_THROW(...) ::abort()