[libcxx] [test] Disable allocation checks in class.path tests on windows

[mstorsjo]

Summary

On windows, the path internal representation is wchar_t, and
input/output often goes through utf8 inbetween, which causes extra
allocations.

MS STL does the same, so this shouldn't be a standards compliance
issue.

This is a more targeted and precise solution, as an alternative to
D97497.

[mstorsjo]

Use the right form of string_view (matching the path char type) in the
concat test, to avoid allocations in one case there.

[mstorsjo]

Also if preferred, I can split out some of the changes that aren't straightforward disablings (two changes in the concat test, that allow keeping the current checks).

[curdeius]

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

190

Aren't there cases where it won't allocate? Even if CharT is path::value_type? Looking at the code the answer is negative, but I'd like your confirmation.

On a different note.
I've been looking at operator/= to which append delegates, and we seem to miss an optimization when calling substr.

At https://github.com/llvm/llvm-project/blob/main/libcxx/include/filesystem#L1029 and https://github.com/llvm/llvm-project/blob/main/libcxx/include/filesystem#L1034, __p.__pn_ is a string (string_type), so .substr() returns a string.
This string gets appended to __pn_ and immediately discarded.
I think we should do string_view_type{__p.__pn_}.substr() and so avoid this temporary string. WDYT?

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

347

Here (see below).

373

That's a drive-by fix, right? If this is correct, shouldn't line 347 be similar?
BTW, preferably I'd remove this change from this revision.

[mstorsjo]

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

190

Thanks for the suggestions about optimizations - yeah that could help (and would avoid one temporary allocation I presume?).

Regarding this particular case, both parameters to operator/= and append() are of type InputIter, which are pass through an intermediate path object, see https://github.com/llvm/llvm-project/blob/main/libcxx/include/filesystem#L1039-L1051.

However, the append that we do on line 367 (with RHS being a path), doesn't do allocations, so we could add such a guard there. That seems to run successfully even with the current form of the header.

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

373

It's a separate fix to make it not require allocations, yeah, otherwise we'd need to disable the allocation guard below. Sure, I can split it out for clarity. I guess we could change line 347 too - there's no allocation guard there so it works either way, but having it use the path native type probably is closer to the original intent.

[mstorsjo]

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

190

Actually, scratch that, the appends on line 367 don't use long enough things to hit the allocator.

We could add another case here above (say around line 182) that appends using a plain path as RHS. With your suggestion for operator/=, that one succeeds with a DisableAllocationGuard, so we could add such a thing here too. Should that go before or after the rest here, though? It feels a bit silly to be adding a new testcase to show off that the header file fix has the intended effect, when the rest of the file (with its existing DisableAllocationGuards) actually is failing.

[mstorsjo]

Updated after splitting out D98406. Updated path.native.obs/string_alloc.pass.cpp to disable the checks much more precisely, keeping the checks for the native type.

[• Quuxplusone]

libcxx/test/support/test_macros.h

378

I think this macro's name needs to start with TEST_.
Orthogonally, I think WIN should be WIN32, for greppability.
So perhaps #define TEST_NOT_WIN32(...)? I wanted to say TEST_EXCEPT_ON_WIN32, except that I think someone might misread "EXCEPT" as having something to do with C++ exceptions.

Alternatively, introduce a specific type DisableAllocationGuardExceptOnWin32 and use that instead of DisableAllocationGuard in certain places.

(Really DisableAllocation was a bad name in the first place — it doesn't disable the allocation guard, it is the allocation guard! — and what's meant is more like ExpectNoAllocations, ExpectNoAllocationsExceptOnWin32. Blech.)

[mstorsjo]

libcxx/test/support/test_macros.h

378

I kinda intentionally left out the "test" prefix of the macro, designing it similar to the LIBCPP_ONLY() macro, but inverted. And it's not so much a "test this"-macro but more generically just a more convenient ifdef. (In some cases, a macro like IF_WIN32_ELSE(win, else) macro also would be convenient, but probably not worth it, and it'd be less clear.)

WIN32 for greppability sounds good.

Agree about DisableAllocation being a non-ideal name, ExpectNoAllocations sounds good. I'm not entirely sold on making an ExceptOnWin32 variant though, as the exact condition for where/when to disable things (ie require that no allocations are done) varies, see e.g. the current modifications in string_alloc.pass.cpp.

[• Quuxplusone]

libcxx/test/support/test_macros.h

378

I kinda intentionally left out the "test" prefix of the macro, designing it similar to the LIBCPP_ONLY() macro

I do like analogies to existing code... but... look at literally every other macro in "test_macros.h"! They either begin with TEST_ (TEST_HAS_NO_EXCEPTIONS, TEST_STD_VER, TEST_CONSTEXPR_CXX14, TEST_SAFE_STATIC, TEST_IGNORE_NODISCARD) or their names are arguably specific to testing (ASSERT_NOEXCEPT, LIBCPP_ASSERT) or it's the LIBCPP_ONLY macro, which feels like it just snuck in next to the various flavors of LIBCPP_ASSERT and nobody noticed. 😛
So I think starting with TEST_ needs to be part of any macro-based solution.

[mstorsjo]

libcxx/test/support/test_macros.h

378

Ok, fair enough, TEST_NOT_WIN32() it is then.

[mstorsjo]

Renamed the macro from NOT_WIN() to TEST_NOT_WIN32() as requested.

[mstorsjo]

@Quuxplusone Does this look ok to you now? It's much more targeted and precise than before, and allows retaining some checks against allocations in the files.

Regarding renaming the classes to make their use more obvious, that's probably doable, but I'd like to do such changes after the changes I have here (and maybe a few more in my queue) have landed.

[• Quuxplusone]

Sure, LGTM. (The naming/logic is still a bit uglier than I wish, but I have no concrete suggestions and don't wish to block this.) @curdeius, LGTY?

[curdeius]

Yes, LGTM.

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

 //      path& append(InputIterator first, InputIterator last);
 
 
 #include "filesystem_include.h"
 #include <type_traits>
 #include <string_view>
 #include <cassert>
 
+// On Windows, the append function converts all inputs (pointers, iterators)
+// to an intermediate path object, causing allocations in cases where no
+// allocations are done on other platforms.
+
 #include "test_macros.h"
 #include "test_iterators.h"
 #include "count_new.h"
 #include "filesystem_test_helper.h"
 
 
 struct AppendOperatorTestcase {
   MultiStringType lhs;
void doAppendSourceAllocTest(AppendOperatorTestcase const& TC)
   const Ptr R = RShort.data();
   const Str& E = EShort;
   std::size_t ReserveSize = E.size() + 3;
   // basic_string
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     Str  RHS(R);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       LHS /= RHS;
     }
     assert(PathEq(LHS, E));
   }
   // basic_string_view
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     StrView  RHS(R);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       LHS /= RHS;
     }
     assert(PathEq(LHS, E));
   }
   // CharT*
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     Ptr RHS(R);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       LHS /= RHS;
     }
     assert(PathEq(LHS, E));
   }
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     Ptr RHS(R);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       LHS.append(RHS, StrEnd(RHS));
     }
     assert(PathEq(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 "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.
+#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);
       LHS /= RHS;
     }

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

 //      path& assign(InputIterator first, InputIterator last);
 
 
 #include "filesystem_include.h"
 #include <type_traits>
 #include <string_view>
 #include <cassert>
 
+// On Windows, charset conversions cause allocations in the path class in
+// cases where no allocations are done on other platforms.
+
 #include "test_macros.h"
 #include "test_iterators.h"
 #include "count_new.h"
 #include "filesystem_test_helper.h"
 
 
 template <class CharT>
 void RunTestCase(MultiStringType const& MS) {
   using namespace fs;
   const fs::path::value_type* Expect = MS;
   const CharT* TestPath = MS;
   const CharT* TestPathEnd = StrEnd(TestPath);
   const std::size_t Size = TestPathEnd - TestPath;
   const std::size_t SSize = StrEnd(Expect) - Expect;
   assert(Size == SSize);
   //////////////////////////////////////////////////////////////////////////////
   // basic_string<Char, Traits, Alloc>
   {
     const std::basic_string<CharT> S(TestPath);
     path p; PathReserve(p, S.length() + 1);
     {
       // string provides a contiguous iterator. No allocation needed.
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       path& pref = (p = S);
       assert(&pref == &p);
     }
     assert(p.native() == Expect);
     assert(p.string<CharT>() == TestPath);
     assert(p.string<CharT>() == S);
   }
   {
     const std::basic_string<CharT> S(TestPath);
     path p; PathReserve(p, S.length() + 1);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       path& pref = p.assign(S);
       assert(&pref == &p);
     }
     assert(p.native() == Expect);
     assert(p.string<CharT>() == TestPath);
     assert(p.string<CharT>() == S);
   }
   // basic_string<Char, Traits, Alloc>
   {
     const std::basic_string_view<CharT> S(TestPath);
     path p; PathReserve(p, S.length() + 1);
     {
       // string provides a contiguous iterator. No allocation needed.
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       path& pref = (p = S);
       assert(&pref == &p);
     }
     assert(p.native() == Expect);
     assert(p.string<CharT>() == TestPath);
     assert(p.string<CharT>() == S);
   }
   {
     const std::basic_string_view<CharT> S(TestPath);
     path p; PathReserve(p, S.length() + 1);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       path& pref = p.assign(S);
       assert(&pref == &p);
     }
     assert(p.native() == Expect);
     assert(p.string<CharT>() == TestPath);
     assert(p.string<CharT>() == S);
   }
   //////////////////////////////////////////////////////////////////////////////
   // Char* pointers
   {
     path p; PathReserve(p, Size + 1);
     {
       // char* pointers are contiguous and can be used with code_cvt directly.
       // no allocations needed.
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       path& pref = (p = TestPath);
       assert(&pref == &p);
     }
     assert(p.native() == Expect);
     assert(p.string<CharT>() == TestPath);
   }
   {
     path p; PathReserve(p, Size + 1);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       path& pref = p.assign(TestPath);
       assert(&pref == &p);
     }
     assert(p.native() == Expect);
     assert(p.string<CharT>() == TestPath);
   }
   {
     path p; PathReserve(p, Size + 1);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       path& pref = p.assign(TestPath, TestPathEnd);
       assert(&pref == &p);
     }
     assert(p.native() == Expect);
     assert(p.string<CharT>() == TestPath);
   }
   //////////////////////////////////////////////////////////////////////////////
   // Iterators

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

 
 
 #include "filesystem_include.h"
 #include <type_traits>
 #include <string>
 #include <string_view>
 #include <cassert>
 
+// On Windows, charset conversions cause allocations in the path class in
+// cases where no allocations are done on other platforms.
+
 #include "test_macros.h"
 #include "test_iterators.h"
 #include "count_new.h"
 #include "filesystem_test_helper.h"
 
 
 struct ConcatOperatorTestcase {
   MultiStringType lhs;
void doConcatSourceAllocTest(ConcatOperatorTestcase const& TC)
   const Ptr R = TC.rhs;
   const Ptr E =  TC.expect;
   std::size_t ReserveSize = StrLen(E) + 1;
   // basic_string
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     Str  RHS(R);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       LHS += RHS;
     }
     assert(LHS == E);
   }
   // basic_string_view
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     StrView  RHS(R);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       LHS += RHS;
     }
     assert(LHS == E);
   }
   // CharT*
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     Ptr RHS(R);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       LHS += RHS;
     }
     assert(LHS == E);
   }
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     Ptr RHS(R);
     {
-      DisableAllocationGuard g;
+      TEST_NOT_WIN32(DisableAllocationGuard g);
       LHS.concat(RHS, StrEnd(RHS));
     }
     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 "char" no allocations will be performed because no conversion is
-  // required.
-  bool DisableAllocations = std::is_same<CharT, char>::value;
+  // For the path native type, no allocations will be performed because no
+  // conversion is required.
+  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);
       LHS += RHS;
     }
for (auto const & TC : Cases) {
       path LHS((const char*)TC.lhs);
       path RHS((const char*)TC.rhs);
       path& Ref = (LHS += RHS);
       assert(LHS == (const char*)TC.expect);
       assert(&Ref == &LHS);
     }
     {
       path LHS((const char*)TC.lhs);
       std::basic_string_view<path::value_type> RHS((const path::value_type*)TC.rhs);
       path& Ref = (LHS += RHS);
       assert(LHS == (const char*)TC.expect);
       assert(&Ref == &LHS);
     }
     doConcatSourceTest<char>    (TC);
     doConcatSourceTest<wchar_t> (TC);
     doConcatSourceTest<char16_t>(TC);
     doConcatSourceTest<char32_t>(TC);
// Do path test
         LIBCPP_ONLY(DisableAllocationGuard g);
         path& Ref = (LHS += RHS);
         assert(&Ref == &LHS);
       }
       assert(LHS == E);
     }
     {
       path LHS((const char*)TC.lhs);
       std::basic_string_view<path::value_type> RHS((const path::value_type*)TC.rhs);
       const char* E = TC.expect;
       PathReserve(LHS, StrLen(E) + 5);
       {
         LIBCPP_ONLY(DisableAllocationGuard g);
         path& Ref = (LHS += RHS);
         assert(&Ref == &LHS);
       }
       assert(LHS == E);

libcxx/test/std/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp

 template <class CharT>
 void doShortStringTest(MultiStringType const& MS) {
   using namespace fs;
   using Ptr = CharT const*;
   using Str = std::basic_string<CharT>;
   using Alloc = std::allocator<CharT>;
   Ptr value = MS;
   const path p((const char*)MS);
+#ifdef _WIN32
+  // On Windows, charset conversions cause allocations outside of the
+  // provided allocator, but accessing the native type should work without
+  // extra allocations.
+  bool DisableAllocations = std::is_same<CharT, path::value_type>::value;
+#else
+  // On other platforms, these methods only use the provided allocator, and
+  // no extra allocations should be done.
+  bool DisableAllocations = true;
+#endif
   {
-      DisableAllocationGuard g;
+      DisableAllocationGuard g(DisableAllocations);
       Str s = p.string<CharT>();
       assert(s == value);
       Str s2 = p.string<CharT>(Alloc{});
       assert(s2 == value);
   }
   using MAlloc = malloc_allocator<CharT>;
   MAlloc::reset();
   {
       using Traits = std::char_traits<CharT>;
       using AStr = std::basic_string<CharT, Traits, MAlloc>;
-      DisableAllocationGuard g;
+      DisableAllocationGuard g(DisableAllocations);
       AStr s = p.string<CharT, Traits, MAlloc>();
       assert(s == value);
       assert(MAlloc::alloc_count == 0);
       assert(MAlloc::outstanding_alloc() == 0);
   }
   MAlloc::reset();
   { // Other allocator - provided copy
       using Traits = std::char_traits<CharT>;
       using AStr = std::basic_string<CharT, Traits, MAlloc>;
-      DisableAllocationGuard g;
+      DisableAllocationGuard g(DisableAllocations);
       MAlloc a;
       // don't allow another allocator to be default constructed.
       MAlloc::disable_default_constructor = true;
       AStr s = p.string<CharT, Traits, MAlloc>(a);
       assert(s == value);
       assert(MAlloc::alloc_count == 0);
       assert(MAlloc::outstanding_alloc() == 0);
   }
const path p((const char*)MS);
       using Alloc = std::allocator<CharT>;
       Str s = p.string<CharT>();
       assert(s == value);
       Str s2 = p.string<CharT>(Alloc{});
       assert(s2 == value);
   }
   using MAlloc = malloc_allocator<CharT>;
   MAlloc::reset();
+#ifdef _WIN32
+  // On Windows, charset conversions cause allocations outside of the
+  // provided allocator, but accessing the native type should work without
+  // extra allocations.
+  bool DisableAllocations = std::is_same<CharT, path::value_type>::value;
+#else
+  // On other platforms, these methods only use the provided allocator, and
+  // no extra allocations should be done.
+  bool DisableAllocations = true;
+#endif
+
   { // Other allocator - default construct
       using Traits = std::char_traits<CharT>;
       using AStr = std::basic_string<CharT, Traits, MAlloc>;
-      DisableAllocationGuard g;
+      DisableAllocationGuard g(DisableAllocations);
       AStr s = p.string<CharT, Traits, MAlloc>();
       assert(s == value);
       assert(MAlloc::alloc_count > 0);
       assert(MAlloc::outstanding_alloc() == 1);
   }
   MAlloc::reset();
   { // Other allocator - provided copy
       using Traits = std::char_traits<CharT>;
       using AStr = std::basic_string<CharT, Traits, MAlloc>;
-      DisableAllocationGuard g;
+      DisableAllocationGuard g(DisableAllocations);
       MAlloc a;
       // don't allow another allocator to be default constructed.
       MAlloc::disable_default_constructor = true;
       AStr s = p.string<CharT, Traits, MAlloc>(a);
       assert(s == value);
       assert(MAlloc::alloc_count > 0);
       assert(MAlloc::outstanding_alloc() == 1);
   }

libcxx/test/support/test_macros.h

 #elif defined(_MSC_VER)
 #define TEST_ALWAYS_INLINE __forceinline
 #define TEST_NOINLINE __declspec(noinline)
 #else
 #define TEST_ALWAYS_INLINE
 #define TEST_NOINLINE
 #endif
 
+#ifdef _WIN32
+#define TEST_NOT_WIN32(...) ((void)0)
+#else
+#define TEST_NOT_WIN32(...) __VA_ARGS__
+#endif
+
 #if defined(__GNUC__)
 #pragma GCC diagnostic pop
 #endif
 
 #endif // SUPPORT_TEST_MACROS_HPP