[libcxx-commits] [libcxx] [libcxx] [test] Use ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS in more places (PR #144339)

[Martin Storsjö via libcxx-commits]

mstorsjo wrote:

> 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.

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