Workaround std::thread begin not copy-constructible

[serge-sans-paille]

Summary

It's not possible to push_back in a vector of std::thread because std::thread is not copy-constructible. And moving to emplace_back triggers a method lookup error related to the custom allocator, as showcased in https://godbolt.org/z/jsbEKC.

[george.karpenkov]

LGTM, but one of the code owners would need to sign of.
The benefit is not creating the temporary object, right?

[serge-sans-paille]

Yeah, especially as he copy constructor is deleted, push_back implies a copy, so relying on the compiler to optimize to a move seems fragile.

[dblaikie]

There should be an rvalue overload of std::vector<T>::push_back - is there not? ( https://en.cppreference.com/w/cpp/container/vector/push_back )

[serge-sans-paille]

In D58117#1401457, @dblaikie wrote:

There should be an rvalue overload of std::vector<T>::push_back - is there not? ( https://en.cppreference.com/w/cpp/container/vector/push_back )

Right! https://godbolt.org/z/c3iMsT proves you right. The part that actually matters seems to be the `construct` member forwarding (why is inheritance not enough in that case?).

[dblaikie]

In D58117#1401491, @serge-sans-paille wrote:

In D58117#1401457, @dblaikie wrote:

There should be an rvalue overload of std::vector<T>::push_back - is there not? ( https://en.cppreference.com/w/cpp/container/vector/push_back )

Right! https://godbolt.org/z/c3iMsT proves you right. The part that actually matters seems to be the `construct` member forwarding (why is inheritance not enough in that case?).

That failure appears only on trunk GCC/libstdc++, by the looks of it? Does it matter that Clang doesn't compile on an unreleased compiler? (8.2 seems to compile the example OK) I can't seem to find a convenient web view of the current libstdc++ sources, so I'm only guessing, but that might well be a bug in trunk libstdc++.

[serge-sans-paille]

Fedora is currently using gcc-9.0.1, svn revision 268719 (see https://src.fedoraproject.org/rpms/gcc/blob/master/f/gcc.spec) which triggers this issue. I'll forward the test case to the gcc team and report here.

[dblaikie]

In D58117#1401919, @serge-sans-paille wrote:

Fedora is currently using gcc-9.0.1, svn revision 268719 (see https://src.fedoraproject.org/rpms/gcc/blob/master/f/gcc.spec) which triggers this issue. I'll forward the test case to the gcc team and report here.

GCC 9 isn't released yet, though - is this a release of Redhat or more "in-development Redhat is using in-development GCC"?

But yeah, I'd check with libstdc++ folks to see if this is a bug there. Vaguely looks like it.

[vitalybuka]

lib/fuzzer/FuzzerDefs.h

187

please clang-format

189

Could this be just:

template<class... Args >
void construct(Args&&... args ) {
  std::allocator<T>::construct(std::forward<Args>(args)...);
}

[serge-sans-paille]

I've reported this strange behavior to gcc https://bugzilla.redhat.com/show_bug.cgi?id=1678613

[kcc]

1. I am not sure what problem does it solve. Everything seems to work.
2. We can now safely remove fuzzer_allocator and rename Vector to just plain std::vector. (we don't need this monstrosity any more since we are using private STL now)

[dblaikie]

In D58117#1414613, @kcc wrote:

1. I am not sure what problem does it solve. Everything seems to work.
2. We can now safely remove fuzzer_allocator and rename Vector to just plain std::vector. (we don't need this monstrosity any more since we are using private STL now)

It was a workaround for a bug in an unreleased version of libstdc++ - nothing to fix on our end.

I think this change should be abandoned.

[serge-sans-paille]

I confirm the bug has been fixed in trunk gcc and in fedora rawhide's gcc, closing the issue.

lib/fuzzer/FuzzerDefs.h

class fuzzer_allocator: public std::allocator<T> {
     public:
       fuzzer_allocator() = default;
 
       template<class U>
       fuzzer_allocator(const fuzzer_allocator<U>&) {}
 
       template<class Other>
       struct rebind { typedef fuzzer_allocator<Other> other;  };
+
+      template< class U, class... Args >
+      void construct( U* p, Args&&... args ) {
+        std::allocator<T>::construct(p, std::forward<Args>(args)...);
+      }
+
   };
 
 template<typename T>
 using Vector = std::vector<T, fuzzer_allocator<T>>;
 
 template<typename T>
 using Set = std::set<T, std::less<T>, fuzzer_allocator<T>>;
 

lib/fuzzer/FuzzerDriver.cpp

static int RunInMultipleProcesses(const Vector<std::string> &Args,
   std::atomic<bool> HasErrors(false);
   Command Cmd(Args);
   Cmd.removeFlag("jobs");
   Cmd.removeFlag("workers");
   Vector<std::thread> V;
   std::thread Pulse(PulseThread);
   Pulse.detach();
   for (unsigned i = 0; i < NumWorkers; i++)
-    V.push_back(std::thread(WorkerThread, std::ref(Cmd), &Counter, NumJobs, &HasErrors));
+    V.emplace_back(WorkerThread, std::ref(Cmd), &Counter, NumJobs, &HasErrors);
   for (auto &T : V)
     T.join();
   return HasErrors ? 1 : 0;
 }
 
 static void RssThread(Fuzzer *F, size_t RssLimitMb) {
   while (true) {
     SleepSeconds(1);