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

Thu Aug 5 07:30:15 PDT 2021

mstorsjo 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
----------------
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?

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

https://reviews.llvm.org/D107124