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

Thu Aug 5 06:39:59 PDT 2021

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

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

https://reviews.llvm.org/D107124