[libcxx] [test] Use ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS in more places

[mstorsjo]

ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS allows waiving asserts, for cases when we can't count allocations that happen within the libc++ shared library.

When compiling with optimization, it is possible that some calls end up generated inline, where the overridden operator new/delete do get called (counting those calls), whereas the compiler may decide to leave some calls to the external definition (inside the shared library, where we can't count the calls).

In particular, in one case, a non-optimized build calls _ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEED1Ev from the DLL, while it gets inlined (including direct calls to operator delete) when built with optimization.

Therefore; for the cases where we can't count allocations internally within the library, waive these asserts.

This fixes all testcases in mingw mode, when built with optimization enabled.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Martin Storsjö (mstorsjo)

Changes

ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS allows waiving asserts, for cases when we can't count allocations that happen within the libc++ shared library.

When compiling with optimization, it is possible that some calls end up generated inline, where the overridden operator new/delete do get called (counting those calls), whereas the compiler may decide to leave some calls to the external definition (inside the shared library, where we can't count the calls).

In particular, in one case, a non-optimized build calls _ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEED1Ev from the DLL, while it gets inlined (including direct calls to operator delete) when built with optimization.

Therefore; for the cases where we can't count allocations internally within the library, waive these asserts.

This fixes all testcases in mingw mode, when built with optimization enabled.

---

Full diff: https://github.com/llvm/llvm-project/pull/144339.diff

2 Files Affected:

• (modified) libcxx/test/std/containers/sequences/vector/common.h (+4-4)
• (modified) libcxx/test/support/count_new.h (+1-1)

diff --git a/libcxx/test/std/containers/sequences/vector/common.h b/libcxx/test/std/containers/sequences/vector/common.h
index 4af6559a06e73..34453f8889b73 100644
--- a/libcxx/test/std/containers/sequences/vector/common.h
+++ b/libcxx/test/std/containers/sequences/vector/common.h
@@ -214,10 +214,10 @@ struct throwing_iterator {
 };
 
 inline void check_new_delete_called() {
-  assert(globalMemCounter.new_called == globalMemCounter.delete_called);
-  assert(globalMemCounter.new_array_called == globalMemCounter.delete_array_called);
-  assert(globalMemCounter.aligned_new_called == globalMemCounter.aligned_delete_called);
-  assert(globalMemCounter.aligned_new_array_called == globalMemCounter.aligned_delete_array_called);
+  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.new_called == globalMemCounter.delete_called);
+  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.new_array_called == globalMemCounter.delete_array_called);
+  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.aligned_new_called == globalMemCounter.aligned_delete_called);
+  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.aligned_new_array_called == globalMemCounter.aligned_delete_array_called);
 }
 
 template <class T, typename Alloc>
diff --git a/libcxx/test/support/count_new.h b/libcxx/test/support/count_new.h
index c8169d3acceab..beb11ee4deae4 100644
--- a/libcxx/test/support/count_new.h
+++ b/libcxx/test/support/count_new.h
@@ -626,7 +626,7 @@ struct RequireAllocationGuard {
     void requireExactly(std::size_t N) { m_req_alloc = N; m_exactly = true; }
 
     ~RequireAllocationGuard() {
-        assert(globalMemCounter.checkOutstandingNewEq(static_cast<int>(m_outstanding_new_on_init)));
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkOutstandingNewEq(static_cast<int>(m_outstanding_new_on_init)));
         std::size_t Expect = m_new_count_on_init + m_req_alloc;
         assert(globalMemCounter.checkNewCalledEq(static_cast<int>(Expect)) ||
                (!m_exactly && globalMemCounter.checkNewCalledGreaterThan(static_cast<int>(Expect))));

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- libcxx/test/std/containers/sequences/vector/common.h libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp libcxx/test/support/count_new.h

View the diff from clang-format here.

diff --git a/libcxx/test/std/containers/sequences/vector/common.h b/libcxx/test/std/containers/sequences/vector/common.h
index 34453f888..ccc07e760 100644
--- a/libcxx/test/std/containers/sequences/vector/common.h
+++ b/libcxx/test/std/containers/sequences/vector/common.h
@@ -216,8 +216,10 @@ struct throwing_iterator {
 inline void check_new_delete_called() {
   ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.new_called == globalMemCounter.delete_called);
   ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.new_array_called == globalMemCounter.delete_array_called);
-  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.aligned_new_called == globalMemCounter.aligned_delete_called);
-  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.aligned_new_array_called == globalMemCounter.aligned_delete_array_called);
+  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(
+      globalMemCounter.aligned_new_called == globalMemCounter.aligned_delete_called);
+  ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(
+      globalMemCounter.aligned_new_array_called == globalMemCounter.aligned_delete_array_called);
 }
 
 template <class T, typename Alloc>
diff --git a/libcxx/test/support/count_new.h b/libcxx/test/support/count_new.h
index f175bc2ff..eb4b9f61c 100644
--- a/libcxx/test/support/count_new.h
+++ b/libcxx/test/support/count_new.h
@@ -627,13 +627,14 @@ struct RequireAllocationGuard {
 
     ~RequireAllocationGuard() {
 #ifdef ALLOW_MISMATCHING_LIBRRARY_INTERNAL_ALLOCATIONS
-        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkOutstandingNewEq(static_cast<int>(m_outstanding_new_on_init)));
+      ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(
+          globalMemCounter.checkOutstandingNewEq(static_cast<int>(m_outstanding_new_on_init)));
 #else
-        assert(globalMemCounter.checkOutstandingNewEq(static_cast<int>(m_outstanding_new_on_init)));
+      assert(globalMemCounter.checkOutstandingNewEq(static_cast<int>(m_outstanding_new_on_init)));
 #endif
-        std::size_t Expect = m_new_count_on_init + m_req_alloc;
-        assert(globalMemCounter.checkNewCalledEq(static_cast<int>(Expect)) ||
-               (!m_exactly && globalMemCounter.checkNewCalledGreaterThan(static_cast<int>(Expect))));
+      std::size_t Expect = m_new_count_on_init + m_req_alloc;
+      assert(globalMemCounter.checkNewCalledEq(static_cast<int>(Expect)) ||
+             (!m_exactly && globalMemCounter.checkNewCalledGreaterThan(static_cast<int>(Expect))));
     }
 
 private:

[mstorsjo]

clang-format wants to entirely reindent the function in libcxx/test/support/count_new.h here - do you want me to update the PR to do that, or ignore that formatting suggestion?

[mstorsjo]

Ping

[EricWF]

Shouldn't the call to new be replaced no matter where it occurs? I'm worried this is papering over bugs.

Like what happens if we allocate on one side of the DLL, but de-allocate on the with a replacement deallocation function which cannot handle memory from our DLL new call?

And I'm not sure the best fix for one or two allocations is to disable all tests using the mechanic entirely. For example, they may be able to work around the unannotated allocation. So, I think this seems like too heavy of a hammer.

How many failing tests are remaining?

[mstorsjo]

Shouldn't the call to new be replaced no matter where it occurs?

Ideally yes, but when dealing with separate EXE/DLLs, it's impossible afaik. When linking a DLL, in this case libc++.dll, the references to operator new and delete are fixed at that link time - any overriding operators in another DLL or in the loading EXE doesn't affect those references any more. (PE/COFF doesn't have that mechanism that ELF/MachO has.)

According to the comments in https://github.com/llvm/llvm-project/blob/llvmorg-21-init/libcxx/test/support/test_macros.h#L357-L373, IBM zOS and AIX also have the same operator overloading limitation.

Like what happens if we allocate on one side of the DLL, but de-allocate on the with a replacement deallocation function which cannot handle memory from our DLL new call?

That would indeed cause mismatched operators to be called, which could indeed be fatal.

And I'm not sure the best fix for one or two allocations is to disable all tests using the mechanic entirely. For example, they may be able to work around the unannotated allocation. So, I think this seems like too heavy of a hammer.

How many failing tests are remaining?

There's currently 6 tests failing in this configuration:

    llvm-libc++-mingw.cfg.in :: std/containers/sequences/vector/vector.capacity/reserve_exceptions.pass.cpp
    llvm-libc++-mingw.cfg.in :: std/containers/sequences/vector/vector.capacity/resize_size_exceptions.pass.cpp
    llvm-libc++-mingw.cfg.in :: std/containers/sequences/vector/vector.capacity/resize_size_value_exceptions.pass.cpp
    llvm-libc++-mingw.cfg.in :: std/containers/sequences/vector/vector.capacity/shrink_to_fit_exceptions.pass.cpp
    llvm-libc++-mingw.cfg.in :: std/input.output/filesystems/class.path/path.member/path.append.pass.cpp
    llvm-libc++-mingw.cfg.in :: std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp

While looking into it (with e.g. printf debugging), I noticed that the compiler is very erratic when it comes to deciding on generating the inline functions from the headers or just calling the preexisting symbol (from the DLL). For some of these cases, in non-optimized builds, we've seen 1 new and 1 free; with optimization on, I'd get 2 news and 1 free, or 1 new and 2 frees, or 2 news and 2 frees. So I'm not sure if these are the only potential cases where this could crop up, with different compiler optimization decisions though.

[EricWF]

I'm cool disabling these for the filesystem tests at least, because there are likely std::string's being constructed within the dylib in some hard-to-avoid manner.

The vector tests seem much more suspect. Do we have instantiations of vector in the dylib (on purpose)?

[EricWF]

ording to the comments in https://github.com/llvm/llvm-project/blob/llvmorg-21-init/libcxx/test/support/test_macros.h#L357-L373, IBM zOS and AIX also have the same operator overloading limitation.

I acknowledge that the limitations exist. My concern is whether disabling all allocation tests is the appropriate hammer, since not all "allocations under test" occur in the dylib, and in cases where they don't, they can likely work around the inconsistencies.

[mstorsjo]

I'm cool disabling these for the filesystem tests at least, because there are likely std::string's being constructed within the dylib in some hard-to-avoid manner.

It's not so much about strings constructed in the dylib; it's about the test code constructing code, but the compiler deciding on a case by case basis to either instantiate the ctors/dtors or call e.g. _ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEED1Ev from the DLL (which won't end up calling the overridden operator delete).

The vector tests seem much more suspect. Do we have instantiations of vector in the dylib (on purpose)?

These tests do std::vector<std::string>.

I acknowledge that the limitations exist. My concern is whether disabling all allocation tests is the appropriate hammer, since not all "allocations under test" occur in the dylib, and in cases where they don't, they can likely work around the inconsistencies.

It's indeed a rather big hammer. However we already have the vast majority of those allocation tests already disabled through ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS (try grepping around the testsuite for this macro); this seems to be just a small number of more cases that gets weren't ignored so far.

And while disabling tests like that does lose some coverage, this macro only skips things for windows-dll cases. We still have clang-cl-static and mingw-static test configurations, where this macro doesn't skip anything and we still have full test coverage of the allocations on Windows.

[EricWF]

It's indeed a rather big hammer. However we already have the vast majority of those allocation tests already disabled through ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS (try grepping around the testsuite for this macro); this seems to be just a small number of more cases that gets weren't ignored so far.

All of the other instances of ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS are disabling singular asserts inside tests. This change proposes disabling entire test utilities. I'm concerned that this will disable passing tests (both new and existing) and obsure what tests are actually running.

I would be OK if we only disabled the failing assertions inside the failing tests. I'm not OK disabling all tests which inspect the allocations.

We could also make the failing tests work, assuming they're not actually trying to test allocation across the dylib.
One way to get the vector tests passing would be to use a custom std::basic_string with a new allocator, preventing the instantiations from playing any role.

---

As an aside, we should make an attempt to address the underlying bug here (which appears to be a serious one).

It would make sense to me to disable the external instantiations for std::string (or at least some of them) on platforms where new cannot be correctly replaced.

This won't fix all occurrences of new or delete in the dylib, but it should fix a large majority of them.

[mstorsjo]

All of the other instances of ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS are disabling singular asserts inside tests. This change proposes disabling entire test utilities. I'm concerned that this will disable passing tests (both new and existing) and obsure what tests are actually running.

I would be OK if we only disabled the failing assertions inside the failing tests. I'm not OK disabling all tests which inspect the allocations.

Fair enough, I can try to rework the patch to do that - that shouldn't be hard.

We could also make the failing tests work, assuming they're not actually trying to test allocation across the dylib. One way to get the vector tests passing would be to use a custom std::basic_string with a new allocator, preventing the instantiations from playing any role.

Right - that's a bit above my C++-fu though, so I'd need some concrete examples to follow for that. (Not sure how relevant this thread is though, as I find the suggestion below even more interesting.)

As an aside, we should make an attempt to address the underlying bug here (which appears to be a serious one).

Indeed, it's quite a fatal issue actually, if people are overriding operator new/delete. (Not sure how common this is in the wild across real world C++ codebases though, but it still seems fatal enough if that's ever done.)

It would make sense to me to disable the external instantiations for std::string (or at least some of them) on platforms where new cannot be correctly replaced.

This won't fix all occurrences of new or delete in the dylib, but it should fix a large majority of them.

That sounds like a very reasonable strategy to avoid the problem for actual users of libc++ as well, yeah.

[mstorsjo]

Also, it’s curious how this issue only did appear in the mingw builds, not in the clang-cl builds, even if both have the same issue about not being able to override operator new inside the DLL. I haven’t had time to try to track down why Clang makes different decisions regarding calling the external instantiations or generating the code (or probably inlineing it), between mingw and msvc mode - whether this just is due to a butterfly effect regarding inlineing heuristics, or whether there’s some systematic difference (e.g. msvc mode always picking the external instantiation if one exists?). I guess it would be good to have clarity in that too, but that’ll be a quite large amount of digging…

[EricWF]

Marking as "Changes Requested".

I'll take a look again once you've re-jiggered the tests.

[mstorsjo]

Updated the test now to only do ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS in count_new.h for two specific tests, enabled with a -D flag in those two tests.

The change to test/std/containers/sequences/vector/common.h is retained as is.