[libcxx] [test] Generalize defines for skipping allocation checks

[mstorsjo]

Summary

This allows waiving the right amount of asserts on Windows and zOS.
This should supersede D107124 and D105910 (but I haven't had tested
it on zOS).

Some of the new.delete tests end up with lots of conditional asserts
and very little actual testing left intact, but this at least shows
off what needs to be disabled; for those tests I'm personally ok
with adding UNSUPPORTED lines instead.

[ldionne]

This LGTM, please wait for CI to finish.

[• Quuxplusone]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp

153–155

I would find this easier to reason about if it were like

RequireAllocationGuard g(0);  // require "at least zero" allocations by default
#if TEST_SUPPORTS_LIBRARY_INTERNAL_ALLOCATIONS
if (DisableAllocations) {
    g.requireExactly(0);
}
#endif // TEST_SUPPORTS_LIBRARY_INTERNAL_ALLOCATIONS
LHS += RHS;

I'm not thrilled by the macro name WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS for two reasons: (1) no ASSERT or TEST prefix; (2) the associated assert macro is ASSERT_WITH, not ASSERT_WITHOUT.

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

65–68

You shouldn't need to alter these lines.

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

65–68

You shouldn't need to alter these lines.

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.constr/F.pass.cpp

149–150

Could we change the "no-op" version of the macro to

#define ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(...) (void)(__VA_ARGS__)

so that line 149 won't be necessary?

libcxx/test/support/test_macros.h

417

This PR currently doesn't demonstrate any uses of ASSERT_WITH_OPERATOR_NEW_FALLBACKS.

I think you should go ahead and merge D107124 and D105910's changes into this PR, and then ask @NancyWang2222 and @DanielMcIntosh-IBM to download your patch and test it locally.

[mstorsjo]

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

65–68

One has to; as the overridden operator new above wasn’t called, Buff is still null, and new_called is zero.

libcxx/test/support/test_macros.h

417

Oh, oops. The patch does cover the tests for both of them, but I accidentally used the wrong macro for the new/delete cases. I’ll revise the patch in 4-5 hours.

[NancyWang2222]

Let me know if this is ready, I will need to test it on z/OS.

[mstorsjo]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp

153–155

Thanks, that does indeed make it less obfuscated!

Unfortunately, it requires moving the DisableAllocations variable into an ifdef, but I think the end result still is better.

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.constr/F.pass.cpp

149–150

Nice, yes, that does reduce the clutter.

[mstorsjo]

Applied the suggestions; removed the WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS define, using ASSERT_WITH_OPERATOR_NEW_FALLBACKS in the new/delete tests.

[mstorsjo]

In D107755#2934547, @NancyWang2222 wrote:

Let me know if this is ready, I will need to test it on z/OS.

I think this should be fine to try on z/OS now.

[• Quuxplusone]

LGTM if buildkite is happy. I think the DisableAllocations renaming should be done, but if you object, then I don't care to block this over it.

libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp

205

As long as we're messing with this, is this a good time to change the name DisableAllocations to ExpectNoAllocations? Here we're not disabling anything from happening; we're just setting up what we expect to see happen.

Could even

bool ExpectNoAllocations = std::is_same<CharT, char>::value && std::is_same<path::value_type, char>::value;

to eliminate that #if _WIN32, right?

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

65–68

Oh, I see, we're calling new through the fallback but we're calling this delete[] directly from line 78. OK then.

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

65–68

As above: never mind.

[mstorsjo]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp

205

Oh, good idea about the renaming - yes, that’s much clearer. Regarding merging the windows ifdef that way, I guess that’d work. Semantically there’s a very minor difference, maybe, but it’s close enough to make it worthwhile. I’ll give it a shot.

[mstorsjo]

Renamed DisableAllocations into ExpectNoAllocations, simplified the definition of the variable in path.append.pass.cpp to avoid an ifdef.

[NancyWang2222]

The changes look ok to me although I havent tried it on z/OS as long as build pass. I will fix in separate patch if there is an issue on z/OS. just want to confirm those assert wont be run on z/OS and assert macros are empty. I will need some time to download patch and test it out.

[NancyWang2222]

libcxx/test/support/test_macros.h

418

those VA_ARGS set 0 on WIN32 only but still set on MVS.

[NancyWang2222]

libcxx/test/support/test_macros.h

418

nvm. I guess it is just checking condition and doesn't trigger assert

[mstorsjo]

libcxx/test/support/test_macros.h

418

Yes, it was suggested to do it this way, to avoid warnings about unused variables when the assert isn't used. So now it evaluates the expression, but doesn't assert on it being true.

[NancyWang2222]

hi There is the failing test case libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp , this test case is not in https://reviews.llvm.org/D107124 and https://reviews.llvm.org/D105910 , the failure seems not related to this patch. it fails same without this patch. we will investigate separately. rest of tests in https://reviews.llvm.org/D107124 and https://reviews.llvm.org/D105910 passed. Thanks for the fixes.
Assertion failed: Result == E, file: /plex/wangn/llvm/dev/llvm-project/libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp, line: 250
CEE5207E The signal SIGABRT was received.

[NancyWang2222]

hi do you know Why this patch was closed?

[mstorsjo]

In D107755#2936886, @NancyWang2222 wrote:

hi do you know Why this patch was closed?

I applied it already as it was approved, as I thought you said you can fix any remaining z/OS related details in separate patches later?

[NancyWang2222]

In D107755#2936908, @mstorsjo wrote:

In D107755#2936886, @NancyWang2222 wrote:

hi do you know Why this patch was closed?

I applied it already as it was approved, as I thought you said you can fix any remaining z/OS related details in separate patches later?

Yes. changes are good. I just want to make sure this changes will be landed.

libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp

void doAppendSourceAllocTest(AppendOperatorTestcase const& TC)
   }
   // input iterator - For non-native char types, appends needs to copy the
   // iterator range into a contiguous block of memory before it can perform the
   // code_cvt conversions.
   // For "char" no allocations will be performed because no conversion is
   // required.
   // On Windows, the append method is more complex and uses intermediate
   // path objects, which causes extra allocations.
-  // In DLL builds on Windows, the overridden operator new won't pick up
-  // allocations done within the DLL, so the RequireAllocationGuard below
-  // won't necessarily see allocations in the cases where they're expected.
+  // If the overridden operator new can't pick up allocations done within
+  // the library implementation, the RequireAllocationGuard below won't
+  // necessarily see allocations in the cases where they're expected, so
+  // set requireAtLeast(0) in those cases.
 #ifdef _WIN32
   bool DisableAllocations = false;
 #else
   bool DisableAllocations = std::is_same<CharT, char>::value;
 #endif
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     InputIter RHS(R);
     {
       RequireAllocationGuard  g; // requires 1 or more allocations occur by default
       if (DisableAllocations) g.requireExactly(0);
-      else TEST_ONLY_WIN32_DLL(g.requireAtLeast(0));
+      else WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS(g.requireAtLeast(0));
       LHS /= RHS;
     }
     assert(PathEq(LHS, E));
   }
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     InputIter RHS(R);
     InputIter REnd(StrEnd(R));
     {
       RequireAllocationGuard g;
       if (DisableAllocations) g.requireExactly(0);
-      else TEST_ONLY_WIN32_DLL(g.requireAtLeast(0));
+      else WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS(g.requireAtLeast(0));
       LHS.append(RHS, REnd);
     }
     assert(PathEq(LHS, E));
   }
 }
 
 template <class CharT>
 void doAppendSourceTest(AppendOperatorTestcase const& TC)

libcxx/test/std/input.output/filesystems/class.path/path.member/path.assign/move.pass.cpp

 
 
 int main(int, char**) {
   using namespace fs;
   static_assert(std::is_nothrow_move_assignable<path>::value, "");
   assert(globalMemCounter.checkOutstandingNewEq(0));
   const std::string s("we really really really really really really really "
                       "really really long string so that we allocate");
-  // On windows, the operator new from count_new.h can't override the default
-  // operator for calls within the libc++ DLL.
-  TEST_NOT_WIN32_DLL(assert(globalMemCounter.checkOutstandingNewEq(1)));
+  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkOutstandingNewEq(1));
   const fs::path::string_type ps(s.begin(), s.end());
   path p(s);
   {
     DisableAllocationGuard g;
     path p2;
     path& pref = (p2 = std::move(p));
     assert(p2.native() == ps);
     assert(p.native() != ps); // Testing moved from state
     assert(&pref == &p2);
   }
 
   return 0;
 }

libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp

// CharT*
     assert(LHS == E);
   }
   // input iterator - For non-native char types, appends needs to copy the
   // iterator range into a contiguous block of memory before it can perform the
   // code_cvt conversions.
   // For the path native type, no allocations will be performed because no
   // conversion is required.
 
-  // In DLL builds on Windows, the overridden operator new won't pick up
-  // allocations done within the DLL, so the RequireAllocationGuard below
-  // won't necessarily see allocations in the cases where they're expected.
+  // If the overridden operator new can't pick up allocations done within
+  // the library implementation, the RequireAllocationGuard below won't
+  // necessarily see allocations in the cases where they're expected, so
+  // set requireAtLeast(0) in those cases.
   bool DisableAllocations = std::is_same<CharT, path::value_type>::value;
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     InputIter RHS(R);
     {
       RequireAllocationGuard  g; // requires 1 or more allocations occur by default
       if (DisableAllocations) g.requireExactly(0);
-      else TEST_ONLY_WIN32_DLL(g.requireAtLeast(0));
+      else WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS(g.requireAtLeast(0));
       LHS += RHS;
     }
     assert(LHS == E);
   }
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     InputIter RHS(R);
     InputIter REnd(StrEnd(R));
     {
       RequireAllocationGuard g;
       if (DisableAllocations) g.requireExactly(0);
-      else TEST_ONLY_WIN32_DLL(g.requireAtLeast(0));
+      else WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS(g.requireAtLeast(0));
       LHS.concat(RHS, REnd);
     }
     assert(LHS == E);
   }
 }
 
 template <class CharT>
 void doConcatSourceTest(ConcatOperatorTestcase const& TC)

libcxx/test/std/input.output/filesystems/class.path/path.member/path.construct/move.pass.cpp

 
 
 int main(int, char**) {
   using namespace fs;
   static_assert(std::is_nothrow_move_constructible<path>::value, "");
   assert(globalMemCounter.checkOutstandingNewEq(0));
   const std::string s("we really really really really really really really "
                       "really really long string so that we allocate");
-  // On windows, the operator new from count_new.h can't override the default
-  // operator for calls within the libc++ DLL.
-  TEST_NOT_WIN32_DLL(assert(globalMemCounter.checkOutstandingNewEq(1)));
+  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkOutstandingNewEq(1));
   const fs::path::string_type ps(s.begin(), s.end());
   path p(s);
   {
     DisableAllocationGuard g;
     path p2(std::move(p));
     assert(p2.native() == ps);
     assert(p.native() != ps); // Testing moved from state
   }
 
   return 0;
 }

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

void* operator new[](std::size_t s, std::align_val_t a) TEST_THROW_SPEC(std::bad_alloc)
     assert(s <= sizeof(Buff));
     assert(static_cast<std::size_t>(a) == OverAligned);
     ++new_called;
     return Buff;
 }
 
 void  operator delete[](void* p, std::align_val_t a) TEST_NOEXCEPT
 {
-    assert(p == Buff);
+    (void)p;
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(p == Buff);
     assert(static_cast<std::size_t>(a) == OverAligned);
-    assert(new_called);
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called);
     --new_called;
 }
 
 int main(int, char**)
 {
     {
         A* ap = new (std::nothrow) A[2];
         assert(ap);
         assert(A_constructed == 2);
-        assert(new_called);
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called);
         delete [] ap;
         assert(A_constructed == 0);
-        assert(!new_called);
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(!new_called);
     }
     {
         B* bp = new (std::nothrow) B[2];
         assert(bp);
         assert(B_constructed == 2);
-        assert(!new_called);
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(!new_called);
         delete [] bp;
-        assert(!new_called);
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(!new_called);
         assert(!B_constructed);
     }
 
   return 0;
 }

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_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called);
     delete [] ap;
     DoNotOptimize(ap);
     assert(A_constructed == 0);
-    assert(!new_called);
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(!new_called);
 
   return 0;
 }

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

 
 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_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called == 1);
     delete [] ap;
     DoNotOptimize(ap);
     assert(A_constructed == 0);
-    assert(new_called == 0);
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called == 0);
 
   return 0;
 }

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

void* operator new(std::size_t s, std::align_val_t a) TEST_THROW_SPEC(std::bad_alloc)
     assert(s <= sizeof(Buff));
     assert(static_cast<std::size_t>(a) == OverAligned);
     ++new_called;
     return Buff;
 }
 
 void  operator delete(void* p, std::align_val_t a) TEST_NOEXCEPT
 {
-    assert(p == Buff);
+    (void)p;
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(p == Buff);
     assert(static_cast<std::size_t>(a) == OverAligned);
-    assert(new_called);
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called);
     --new_called;
 }
 
 
 int main(int, char**)
 {
     {
         A* ap = new (std::nothrow) A;
         assert(ap);
         assert(A_constructed);
-        assert(new_called);
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called);
         delete ap;
         assert(!A_constructed);
-        assert(!new_called);
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(!new_called);
     }
     {
         B* bp = new (std::nothrow) B;
         assert(bp);
         assert(B_constructed);
-        assert(!new_called);
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(!new_called);
         delete bp;
-        assert(!new_called);
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(!new_called);
         assert(!B_constructed);
     }
 
   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_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called);
     delete ap;
     DoNotOptimize(ap);
     assert(!A_constructed);
-    assert(!new_called);
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(!new_called);
 
   return 0;
 }

libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.dtor/dtor.pass.cpp

assert(globalMemCounter.checkDeleteArrayCalledEq(0));
         assert(globalMemCounter.checkDeleteArrayCalledEq(0));
     }
     assert(globalMemCounter.checkDeleteArrayCalledEq(0));
     {
         std::locale l(std::locale::classic(),
             new std::ctype<char>(new std::ctype<char>::mask[256], true));
         assert(globalMemCounter.checkDeleteArrayCalledEq(0));
     }
-    // On windows, the operator new from count_new.h can't override the default
-    // operator for calls within the libc++ DLL.
-    TEST_NOT_WIN32_DLL(assert(globalMemCounter.checkDeleteArrayCalledEq(1)));
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkDeleteArrayCalledEq(1));
 
   return 0;
 }

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.constr/F.pass.cpp

throw_one = 0xFFF;
         std::thread t(f);
         t.join();
     }
     numAllocs = 0xFFF - throw_one;
     // i <= numAllocs means the last iteration is expected not to throw.
     for (int i=0; i <= numAllocs; ++i) {
         throw_one = i;
         f_run = false;
-        TEST_NOT_WIN32_DLL(unsigned old_outstanding = outstanding_new);
+        unsigned old_outstanding = outstanding_new;
         try {
             std::thread t(f);
             assert(i == numAllocs); // Only final iteration will not throw.
             t.join();
             assert(f_run);
         } catch (std::bad_alloc const&) {
             assert(i < numAllocs);
             assert(!f_run); // (2.2)
         }
-        // In DLL builds on Windows, the overridden operators new/delete won't
-        // override calls from within the DLL, so this won't match.
-        TEST_NOT_WIN32_DLL(assert(old_outstanding == outstanding_new)); // (2.3)
+        (void)old_outstanding;
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(old_outstanding == outstanding_new); // (2.3)
     }
     f_run = false;
     throw_one = 0xFFF;
 #endif
 }
 
 int main(int, char**)
 {

libcxx/test/support/test_macros.h

 #endif
 
 #ifdef _WIN32
 #define TEST_NOT_WIN32(...) ((void)0)
 #else
 #define TEST_NOT_WIN32(...) __VA_ARGS__
 #endif
 
-#if defined(_WIN32) && !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#define TEST_NOT_WIN32_DLL(...) ((void)0)
-#define TEST_ONLY_WIN32_DLL(...) __VA_ARGS__
+#if (defined(_WIN32) && !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)) || defined(__MVS__)
+// Macros for waiving cases when we can't count allocations done within
+// the library implementation.
+//
+// On Windows, when libc++ is built as a DLL, references to operator new/delete
+// within the DLL are bound at link time to the operator new/delete within
+// the library; replacing them in the user executable doesn't override the
+// calls within the library.
+//
+// The same goes on IBM zOS.
+#define ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(...) ((void)0)
+#define WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS(...) __VA_ARGS__
+#else
+#define ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(...) assert(__VA_ARGS__)
+#define WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS(...) ((void)0)
+#endif
+
+#if (defined(_WIN32) && !defined(_MSC_VER) && !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)) || defined(__MVS__)
+// Normally, a replaced e.g. 'operator new' ends up used if the user code
+// does a call to e.g. 'operator new[]'; it's enough to replace the base
+// versions and have it override all of them.
+//
+// When the fallback operators are located within the libc++ library and we
+// can't override the calls within it (see above), this fallback mechanism
+// doesn't work either.
+//
+// On Windows, when using the MSVC vcruntime, the operator new/delete fallbacks
+// are linked separately from the libc++ library, linked statically into
+// the end user executable, and these fallbacks work even in DLL configurations.
+// In MinGW configurations when built as a DLL, and on zOS, these fallbacks
+// don't work though.
+#define ASSERT_WITH_OPERATOR_NEW_FALLBACKS(...) ((void)0)
 #else
-#define TEST_NOT_WIN32_DLL(...) __VA_ARGS__
-#define TEST_ONLY_WIN32_DLL(...) ((void)0)
+#define ASSERT_WITH_OPERATOR_NEW_FALLBACKS(...) assert(__VA_ARGS__)
 #endif
 
 #ifdef _WIN32
 #define TEST_WIN_NO_FILESYSTEM_PERMS_NONE
 #endif
 
 #if defined(__GNUC__)
 #pragma GCC diagnostic pop
 #endif
 
 #endif // SUPPORT_TEST_MACROS_HPP