Page MenuHomePhabricator

Switch from llvm::is_trivially_copyable to std::is_trivially_copyable
ClosedPublic

Authored by MaskRay on Dec 2 2020, 3:20 PM.

Details

Summary

GCC<5 did not support std::is_trivially_copyable. Now LLVM builds require 5.1
we can migrate to std::is_trivially_copyable.

The Optional.h change made MSVC choke
(https://buildkite.com/llvm-project/premerge-checks/builds/18587#cd1bb616-ffdc-4581-9795-b42c284196de)
so I leave it out for now.

Diff Detail

Event Timeline

MaskRay created this revision.Dec 2 2020, 3:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 3:20 PM
MaskRay requested review of this revision.Dec 2 2020, 3:20 PM
MaskRay added a subscriber: aheejin.EditedDec 2 2020, 4:47 PM

MSVC is unhappy (https://buildkite.com/llvm-project/premerge-checks/builds/18597#12d9421a-c989-4774-b9fb-e0823600baa4) with https://reviews.llvm.org/D92514?id=309075#change-P7ceLWWfmLDd

FAILED: lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/WasmEHPrepare.cpp.obj
sccache C:\BuildTools\VC\Tools\MSVC\14.27.29110\bin\Hostx64\x64\cl.exe  /nologo /TP -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\CodeGen -IC:\ws\w32-1\llvm-project\premerge-checks\llvm\lib\CodeGen -Iinclude -IC:\ws\w32-1\llvm-project\premerge-checks\llvm\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2    /EHs-c- /GR- -UNDEBUG -std:c++14 /showIncludes /Folib\CodeGen\CMakeFiles\LLVMCodeGen.dir\WasmEHPrepare.cpp.obj /Fdlib\CodeGen\CMakeFiles\LLVMCodeGen.dir\LLVMCodeGen.pdb /FS -c C:\ws\w32-1\llvm-project\premerge-checks\llvm\lib\CodeGen\WasmEHPrepare.cpp
C:\ws\w32-1\llvm-project\premerge-checks\llvm\include\llvm/ADT/BreadthFirstIterator.h(96): error C2280: 'llvm::Optional<std::pair<llvm::DomTreeNodeBase<llvm::BasicBlock> *,llvm::Optional<llvm::DomTreeNodeBase<llvm::BasicBlock> *const *>>> &llvm::Optional<std::pair<llvm::DomTreeNodeBase<llvm::BasicBlock> *,llvm::Optional<llvm::DomTreeNodeBase<llvm::BasicBlock> *const *>>>::operator =(const llvm::Optional<std::pair<llvm::DomTreeNodeBase<llvm::BasicBlock> *,llvm::Optional<llvm::DomTreeNodeBase<llvm::BasicBlock> *const *>>> &)': attempting to reference a deleted function
C:\ws\w32-1\llvm-project\premerge-checks\llvm\include\llvm/ADT/Optional.h(250): note: see declaration of 'llvm::Optional<std::pair<llvm::DomTreeNodeBase<llvm::BasicBlock> *,llvm::Optional<llvm::DomTreeNodeBase<llvm::BasicBlock> *const *>>>::operator ='

@aheejin

MaskRay updated this revision to Diff 309123.Dec 2 2020, 7:00 PM
MaskRay retitled this revision from Use std::is_trivially_copyable to Switch from llvm::is_trivially_copyable to std::is_trivially_copyable.
MaskRay edited the summary of this revision. (Show Details)

Remove Optional.h

This revision was not accepted when it landed; it landed in state Needs Review.Dec 2 2020, 10:03 PM
This revision was automatically updated to reflect the committed changes.

Generally - once code is sent for review, it should not be committed without approval: https://llvm.org/docs/CodeReview.html#code-review-workflow

Generally - once code is sent for review, it should not be committed without approval: https://llvm.org/docs/CodeReview.html#code-review-workflow

For this one, it was just to have a check on premerge-checks (MSVC appears to have some edge-case issues). I think it meets likely-community-consensus requirements (previously jlebar and serge-sans-paille and probably others had similar cleanups this year, too)

Hmm. I think there was a similar discussion with @mehdi_amini :) ?

Generally - once code is sent for review, it should not be committed without approval: https://llvm.org/docs/CodeReview.html#code-review-workflow

For this one, it was just to have a check on premerge-checks (MSVC appears to have some edge-case issues).

Given this produces email to llvm-commits, I don't think it's appropriate. If phab can be configured in such a way as to not send such email and specifically tag "reviews" as not being for review - I'm OK with that.

I think it meets likely-community-consensus requirements (previously jlebar and serge-sans-paille and probably others had similar cleanups this year, too)

Agreed - I don't have an issue with the patch itself (I'd have been happy to approve it today), but with the review being sent and then committed without approval.

Given this produces email to llvm-commits, I don't think it's appropriate. If phab can be configured in such a way as to not send such email and specifically tag "reviews" as not being for review - I'm OK with that.

One can open revisions without sending them for review I believe: arc diff --draft
I just fixed the Herald rules to not email on draft revisions, e.g.: https://reviews.llvm.org/D92595

Given this produces email to llvm-commits, I don't think it's appropriate. If phab can be configured in such a way as to not send such email and specifically tag "reviews" as not being for review - I'm OK with that.

One can open revisions without sending them for review I believe: arc diff --draft
I just fixed the Herald rules to not email on draft revisions, e.g.: https://reviews.llvm.org/D92595

Awesome!

Given this produces email to llvm-commits, I don't think it's appropriate. If phab can be configured in such a way as to not send such email and specifically tag "reviews" as not being for review - I'm OK with that.

One can open revisions without sending them for review I believe: arc diff --draft
I just fixed the Herald rules to not email on draft revisions, e.g.: https://reviews.llvm.org/D92595

Ah, excellent - thanks a bunch!

I spent some time root causing an llvm::Optional build error on MSVC 16.8.3 today related to using std::is_trivially_copyable to set llvm::is_trivially_copyable<T>::value. It looks like the same failure mentioned in this review.

62>C:\src\ocg_llvm\llvm-project\llvm\include\llvm/ADT/BreadthFirstIterator.h(96,12): error C2280: 'llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>> &llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>>::operator =(const llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>> &)': attempting to reference a deleted function (compiling source file C:\src\ocg_llvm\llvm-project\llvm\unittests\ADT\BreadthFirstIteratorTest.cpp)
...

The "trivial" specialization of optional_detail::OptionalStorage assumes that the value type is trivially copy constructible and trivially copy assignable. The specialization is invoked based on a check of is_trivially_copyable, which does not imply both is_trivially_copy_assignable and is_trivially_copy_constructible are true.

According to the spec, a deleted assignment operator does not make is_trivially_copyable false. So I think all these properties need to be checked explicitly in order to specialize OptionalStorage to the "trivial" version:

/// Storage for any type.
template <typename T, bool = std::is_trivially_copy_constructible<T>::value
                          && std::is_trivially_copy_assignable<T>::value>
class OptionalStorage {

Above fixed my build break in MSVC, but I think we might need to explicitly check is_trivially_copy_constructible too since it might be possible the copy constructor is deleted.

I spent some time root causing an llvm::Optional build error on MSVC 16.8.3 today related to using std::is_trivially_copyable to set llvm::is_trivially_copyable<T>::value. It looks like the same failure mentioned in this review.

62>C:\src\ocg_llvm\llvm-project\llvm\include\llvm/ADT/BreadthFirstIterator.h(96,12): error C2280: 'llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>> &llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>>::operator =(const llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>> &)': attempting to reference a deleted function (compiling source file C:\src\ocg_llvm\llvm-project\llvm\unittests\ADT\BreadthFirstIteratorTest.cpp)
...

The "trivial" specialization of optional_detail::OptionalStorage assumes that the value type is trivially copy constructible and trivially copy assignable. The specialization is invoked based on a check of is_trivially_copyable, which does not imply both is_trivially_copy_assignable and is_trivially_copy_constructible are true.

According to the spec, a deleted assignment operator does not make is_trivially_copyable false. So I think all these properties need to be checked explicitly in order to specialize OptionalStorage to the "trivial" version:

/// Storage for any type.
template <typename T, bool = std::is_trivially_copy_constructible<T>::value
                          && std::is_trivially_copy_assignable<T>::value>
class OptionalStorage {

Above fixed my build break in MSVC, but I think we might need to explicitly check is_trivially_copy_constructible too since it might be possible the copy constructor is deleted.

All sounds pretty plausible to me - if you want to send a patch to add those checks?

I spent some time root causing an llvm::Optional build error on MSVC 16.8.3 today related to using std::is_trivially_copyable to set llvm::is_trivially_copyable<T>::value. It looks like the same failure mentioned in this review.

62>C:\src\ocg_llvm\llvm-project\llvm\include\llvm/ADT/BreadthFirstIterator.h(96,12): error C2280: 'llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>> &llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>>::operator =(const llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>> &)': attempting to reference a deleted function (compiling source file C:\src\ocg_llvm\llvm-project\llvm\unittests\ADT\BreadthFirstIteratorTest.cpp)
...

The "trivial" specialization of optional_detail::OptionalStorage assumes that the value type is trivially copy constructible and trivially copy assignable. The specialization is invoked based on a check of is_trivially_copyable, which does not imply both is_trivially_copy_assignable and is_trivially_copy_constructible are true.

According to the spec, a deleted assignment operator does not make is_trivially_copyable false. So I think all these properties need to be checked explicitly in order to specialize OptionalStorage to the "trivial" version:

/// Storage for any type.
template <typename T, bool = std::is_trivially_copy_constructible<T>::value
                          && std::is_trivially_copy_assignable<T>::value>
class OptionalStorage {

Above fixed my build break in MSVC, but I think we might need to explicitly check is_trivially_copy_constructible too since it might be possible the copy constructor is deleted.

All sounds pretty plausible to me - if you want to send a patch to add those checks?

Sure... just getting my feet wet in LLVM, so may take some time to generate a patch / figure out how to submit it.