[libcxx-commits] [PATCH] D107124: [SystemZ][z/OS][libcxx]: Disable some new operator test cases on z/OS

Thu Aug 5 09:05:35 PDT 2021

Quuxplusone added subscribers: ldionne, EricWF.
Quuxplusone added inline comments.

================
Comment at: libcxx/test/support/test_macros.h:335-341
+#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
----------------
mstorsjo wrote:
> Quuxplusone wrote:
> > mstorsjo wrote:
> > > Quuxplusone wrote:
> > > > mstorsjo wrote:
> > > > > Quuxplusone wrote:
> > > > > > 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.)
> > > > > 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.
> > > > > 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.)
> > > > 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...
> > > 
> > 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?
> 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?
> 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...?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107124/new/

https://reviews.llvm.org/D107124