[libc++][test] Remove non-portable assumption that thread's constructor allocates with ::new

[CaseyCarter]

Summary

Drive-by:

• Fix potential race between check and update of throw_one in operator new
• Fix latent bug in operator delete, which shouldn't decrement outstanding_new when passed a null pointer
• Specifically catch the expected bad_alloc in main instead of ...

[EricWF]

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

121

Why?

174

I'm not sure I understand this change either.

[EricWF]

Requesting changes to move this out of my TODO review queue.

[CaseyCarter]

I'll put this explanation in the comments and push a change.

EDIT: Sorry for the slow response: I didn't get a notification from Phabricator that you'd commented or requested changes. I need to check my settings, I suppose.

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

121

main non-portably assumes that thread creation results in at least one call to ::operator new. This change fixes that assumption by counting the number of calls to ::operator new here for creation of a do-nothing thread, and communicating that count to main via numAllocs.

174

If thread creation in test_throwing_new_during_thread_creation resulted in 0 calls to ::operator new, the expectation is that the same will occur here when we create a thread. If ::operator new isn't called, it can't throw the exception this test is expecting to catch.

[CaseyCarter]

Clarify use of numAllocs.

[CaseyCarter]

Ping?

[EricWF]

LGTM minus inline comments.

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

174

Since libc++ seems to allocate, could we sanity test that?

// The test below is non-portable because it expects `std::thread` to call `new`, which may not be the case for all implementations.
LIBCPP_ASSERT(numAllocs > 0); // libc++ should call new. Sanity check `numAllocs`.
if (numAllocs > 0) { ... }

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

 #include "test_macros.h"
 
 std::atomic<unsigned> throw_one(0xFFFF);
 std::atomic<unsigned> outstanding_new(0);
 
 
 void* operator new(std::size_t s) TEST_THROW_SPEC(std::bad_alloc)
 {
-    if (throw_one == 0)
-        TEST_THROW(std::bad_alloc());
-    --throw_one;
+    unsigned expected = throw_one;
+    do {
+        if (expected == 0) TEST_THROW(std::bad_alloc());
+    } while (!throw_one.compare_exchange_weak(expected, expected - 1));
     ++outstanding_new;
     void* ret = std::malloc(s);
     if (!ret) std::abort(); // placate MSVC's unchecked malloc warning
     return ret;
 }
 
 void  operator delete(void* p) TEST_NOEXCEPT
 {
+    if (!p) return;
     --outstanding_new;
     std::free(p);
 }
 
 bool f_run = false;
 
 void f()
 {

 //-----------------------------
 // Concerns:
 //  A Each allocation performed during thread construction should be performed
 //    in the parent thread so that std::terminate is not called if
 //    std::bad_alloc is thrown by new.
 //  B std::thread's constructor should properly handle exceptions and not leak
 //    memory.
 // Plan:
-//  1 Create a thread and count the number of allocations, 'N', it performs.
+//  1 Create a thread and count the number of allocations, 'numAllocs', it performs.
 //  2 For each allocation performed run a test where that allocation throws.
 //    2.1 check that the exception can be caught in the parent thread.
 //    2.2 Check that the functor has not been called.
 //    2.3 Check that no memory allocated by the creation of the thread is leaked.
-//  3 Finally check that a thread runs successfully if we throw after 'N+1'
+//  3 Finally check that a thread runs successfully if we throw after 'numAllocs+1'
 //    allocations.
+int numAllocs;
+
 void test_throwing_new_during_thread_creation() {
 #ifndef TEST_HAS_NO_EXCEPTIONS
     throw_one = 0xFFF;
     {
         std::thread t(f);
         t.join();
     }
-    const int numAllocs = 0xFFF - throw_one;
+    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;
         unsigned old_outstanding = outstanding_new;
         try {
             std::thread t(f);
             assert(i == numAllocs); // Only final iteration will not throw.
test_throwing_new_during_thread_creation();
             std::thread t(g);
             t.join();
         }
         assert(G::n_alive == 0);
         assert(G::op_run);
     }
     G::op_run = false;
 #ifndef TEST_HAS_NO_EXCEPTIONS
-    {
+    if (numAllocs > 0) { // Creating a thread might not call new at all.
         try
         {
             throw_one = 0;
             assert(G::n_alive == 0);
             assert(!G::op_run);
             std::thread t((G()));
             assert(false);
         }
-        catch (...)
+        catch (std::bad_alloc const&)
         {
             throw_one = 0xFFFF;
             assert(G::n_alive == 0);
             assert(!G::op_run);
         }
     }
 #endif
 #if TEST_STD_VER >= 11