Page MenuHomePhabricator

Workaround std::thread begin not copy-constructible
AbandonedPublic

Authored by serge-sans-paille on Feb 12 2019, 6:40 AM.

Details

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.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 12 2019, 6:40 AM
Herald added subscribers: Restricted Project, llvm-commits, jfb. · View Herald Transcript

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

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.

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 )

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

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

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.

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 added inline comments.Feb 19 2019, 12:34 PM
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)...);
}
vitalybuka requested changes to this revision.Feb 28 2019, 5:31 PM
This revision now requires changes to proceed.Feb 28 2019, 5:31 PM
kcc added a comment.Feb 28 2019, 8:49 PM
  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)
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 abandoned this revision.Mar 6 2019, 5:03 AM

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