Page MenuHomePhabricator

[clang] Fix trivially copyable for copy constructor and copy assignment operator
ClosedPublic

Authored by Javier-varez on Jun 12 2022, 3:22 AM.

Details

Summary

From [class.copy.ctor]:

A non-template constructor for class X is a copy constructor if its first
parameter is of type X&, const X&, volatile X& or const volatile X&, and
either there are no other parameters or else all other parameters have
default arguments (9.3.4.7).

A copy/move constructor for class X is trivial if it is not user-provided and if:
- class X has no virtual functions (11.7.3) and no virtual base classes (11.7.2), and
- the constructor selected to copy/move each direct base class subobject is trivial, and
- or each non-static data member of X that is of class type (or array thereof),
  the constructor selected to copy/move that member is trivial;

otherwise the copy/move constructor is non-trivial.

So T(T&) = default; should be trivial assuming that the previous
provisions are met.

This works in GCC, but not in Clang at the moment:
https://godbolt.org/z/fTGe71b6P

Diff Detail

Event Timeline

Javier-varez created this revision.Jun 12 2022, 3:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2022, 3:22 AM
Javier-varez requested review of this revision.Jun 12 2022, 3:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2022, 3:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Fix clang-tidy tests and run clang-format

Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2022, 4:24 AM
royjacobson requested changes to this revision.Jun 12 2022, 12:05 PM
royjacobson added a reviewer: Restricted Project.
royjacobson added a subscriber: royjacobson.

Hi Javier, thank you for submitting this patch!

As far as I could tell, this patch implements the CWG2171 defect report from 2016: https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2171. That means that you'll have to add a test inside clang/test/CXX/drs/dr21xx.cpp to make sure it shows on the DRs status page.

This change is also a large ABI break, which means it has to be considered carefully. I think the best approach here would be to revert to the previous behavior when the -fclang-abi-compat flag is used for clang <= 14, but I'm not sure about this. I would like the approval of someone with more ABI experience here.

The code itself and the tests look good! If you'll need help with the changes I mentioned please let me know.

clang/lib/Sema/SemaDeclCXX.cpp
9799–9802

I think this should work instead? No need to check for RT since we already returned if !RT.

This revision now requires changes to proceed.Jun 12 2022, 12:05 PM

Hi Javier, thank you for submitting this patch!

As far as I could tell, this patch implements the CWG2171 defect report from 2016: https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2171. That means that you'll have to add a test inside clang/test/CXX/drs/dr21xx.cpp to make sure it shows on the DRs status page.

This change is also a large ABI break, which means it has to be considered carefully. I think the best approach here would be to revert to the previous behavior when the -fclang-abi-compat flag is used for clang <= 14, but I'm not sure about this. I would like the approval of someone with more ABI experience here.

The code itself and the tests look good! If you'll need help with the changes I mentioned please let me know.

Yep, this needs to add a new ABI value to the -fclang-abi-compat stuff, so starting with LangOptions.h's ClangABI enum, probably a Ver15 value.

  • Preseve ABI when clang is invoked with -fclang-abi-compat <= 14.
  • Add test for defect report in clang/test/CXX/drs/dr21xx.cpp.
  • Refactor ConstArg to remove if condition.

Thanks for the review! Indeed this looks like it implements the defect report dr2171. I didn't realize there was a defect report about this, thanks for bringing it up. I am not sure If this behavior should also change for move constructors, but clang currently cannot declare defaulted move constructors with arguments other than T&& (so basically they are not trivial anyway because they are simply not allowed to be defaulted) and it seems GCC also does not allow it. In my opinion that should be fine as is, given const T&& is not quite sensible.

I updated the change with your suggestions. I used ClangABICompat 14 because 15 is unreleased, so I keep the behavior to preserve the ABI if -fclang-abi-compat is called with argument 14 or less.

I also didn't manage to find out why the openmp tests are failing, I'll need to have another look at that.

clang/lib/Sema/SemaDeclCXX.cpp
9799–9802

Ah, good point, lazy refactoring on my side!

Javier-varez marked an inline comment as done.Jun 13 2022, 12:41 PM

This is great, thanks for the changes! I added two follow-up inline comments.

Another two other small details: Please add documentation about this to LangOptions.h (We try to document there the changes for each ABI version), and please add a release note about this change (in clang/docs/ReleaseNotes.rst). The "ABI Changes in Clang" section seems appropriate.

Do you have LLVM commit access? If not, I can commit it for you - let me know the name+email you want the commit to be under in that case.

clang/lib/Sema/SemaDeclCXX.cpp
9778

Please add a comment here about the defect report and what the ClangABICompat14 is about.

clang/test/CXX/special/class.copy/p12-0x.cpp
31

Could you add a test that we use the previous behavior (for this and for trivially assignable) when clang-abi-compat=14 is set? Either a new test specifically for this behavior, or you can use the CLANG_ABI_COMPAT macro and run the same test with the flag.

I don't think the OpenMP tests are failing because of this PR, they are a bit flaky sometimes.

@erichkeane - Since we haven't branched clang15 yet, I think it's Ok not to add the Ver15 value to the ABI options?

I don't think the OpenMP tests are failing because of this PR, they are a bit flaky sometimes.

@erichkeane - Since we haven't branched clang15 yet, I think it's Ok not to add the Ver15 value to the ABI options?

Ah, I think the fact that he's doing <= Ver14 here makes it fine to treat this as a 'latest' feature. I think the 'Latest' addition like to allow this is actually reasonably new (or my memory of how these happen is defective), so i think you're right we don't need to change it.

Javier-varez marked 2 inline comments as done.

Address review feedback

Thanks again for having another look at the changes! I do not have commit access since this is my first change to LLVM. I would appreciate if you could merge it. here's my username and email:
Javier Alvarez <javier.alvarez@allthingsembedded.net>

And let me know if there's anything else you'd like to modify :)

royjacobson accepted this revision.Jun 14 2022, 1:54 PM

Thank you! LGTM. I see the CI still has those OpenMP failures - could you try to rebase on main to get it green?

@erichkeane is this good to go? Should I ping some ABI people about this?

This revision is now accepted and ready to land.Jun 14 2022, 1:54 PM

Rebase on main

Thank you! LGTM. I see the CI still has those OpenMP failures - could you try to rebase on main to get it green?

@erichkeane is this good to go? Should I ping some ABI people about this?

I think this is probably good to go here. The OMP failures are still there though, which is a touch concerning.

The OMP failures are still there though, which is a touch concerning.

Yeah, I rerun the CI and it still fails. I couldn't reproduce the failures locally, though, and the CI logs don't seem to contain any actual problem. I'm not sure what to do from here.

I did try this locally and the built test binaries seem to crash with a segmentation fault. I run one of them in lldb to check the error and I got this backtrace:

llvm-project/build on  fix-trivially-copyable-check [?⇕] via △ v3.22.3 took 3s ❯ lldb /Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp
(lldb) target create "/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp"
Current executable set to '/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp' (arm64).
(lldb) r
Process 61339 launched: '/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp' (arm64)
Process 61339 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
    frame #0: 0x0000000100362f54 libomp.dylib`::__kmp_get_global_thread_id() at kmp_runtime.cpp:201:8
   198
   199 	  /* dynamically updated stack window for uber threads to avoid get_specific
   200 	     call */
-> 201 	  if (!TCR_4(other_threads[i]->th.th_info.ds.ds_stackgrow)) {
   202 	    KMP_FATAL(StackOverflow, i);
   203 	  }
   204
Target 0: (omp_single_copyprivate.c.tmp) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
  * frame #0: 0x0000000100362f54 libomp.dylib`::__kmp_get_global_thread_id() at kmp_runtime.cpp:201:8
    frame #1: 0x00000001003b8c60 libomp.dylib`void __kmp_release_template<kmp_flag_64<false, true> >(flag=0x000000016fdff0b8) at kmp_wait_release.h:790:39
    frame #2: 0x00000001003b8c20 libomp.dylib`::__kmp_release_64(flag=0x000000016fdff0b8) at kmp_wait_release.cpp:25:46
    frame #3: 0x00000001003798d0 libomp.dylib`__kmp_reap_thread(thread=0x0000000101010a40, is_root=0) at kmp_runtime.cpp:6146:9
    frame #4: 0x000000010037497c libomp.dylib`__kmp_internal_end() at kmp_runtime.cpp:6334:7
    frame #5: 0x00000001003745ec libomp.dylib`::__kmp_internal_end_library(gtid_req=-1) at kmp_runtime.cpp:6498:3
    frame #6: 0x0000000100374258 libomp.dylib`::__kmp_internal_end_atexit() at kmp_runtime.cpp:6115:3
    frame #7: 0x0000000100374218 libomp.dylib`::__kmp_internal_end_dtor() at kmp_runtime.cpp:6083:3
    frame #8: 0x0000000189359dc4 libsystem_c.dylib`__cxa_finalize_ranges + 464
    frame #9: 0x0000000189359b68 libsystem_c.dylib`exit + 44
    frame #10: 0x000000018947aec4 libdyld.dylib`dyld4::LibSystemHelpers::exit(int) const + 20
    frame #11: 0x00000001000150d4 dyld`start + 592
(lldb)

Not sure how to proceed from here though... I'm really not familiar with OpenMP

I did try this locally and the built test binaries seem to crash with a segmentation fault. I run one of them in lldb to check the error and I got this backtrace:

llvm-project/build on  fix-trivially-copyable-check [?⇕] via △ v3.22.3 took 3s ❯ lldb /Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp
(lldb) target create "/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp"
Current executable set to '/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp' (arm64).
(lldb) r
Process 61339 launched: '/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp' (arm64)
Process 61339 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
    frame #0: 0x0000000100362f54 libomp.dylib`::__kmp_get_global_thread_id() at kmp_runtime.cpp:201:8
   198
   199 	  /* dynamically updated stack window for uber threads to avoid get_specific
   200 	     call */
-> 201 	  if (!TCR_4(other_threads[i]->th.th_info.ds.ds_stackgrow)) {
   202 	    KMP_FATAL(StackOverflow, i);
   203 	  }
   204
Target 0: (omp_single_copyprivate.c.tmp) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
  * frame #0: 0x0000000100362f54 libomp.dylib`::__kmp_get_global_thread_id() at kmp_runtime.cpp:201:8
    frame #1: 0x00000001003b8c60 libomp.dylib`void __kmp_release_template<kmp_flag_64<false, true> >(flag=0x000000016fdff0b8) at kmp_wait_release.h:790:39
    frame #2: 0x00000001003b8c20 libomp.dylib`::__kmp_release_64(flag=0x000000016fdff0b8) at kmp_wait_release.cpp:25:46
    frame #3: 0x00000001003798d0 libomp.dylib`__kmp_reap_thread(thread=0x0000000101010a40, is_root=0) at kmp_runtime.cpp:6146:9
    frame #4: 0x000000010037497c libomp.dylib`__kmp_internal_end() at kmp_runtime.cpp:6334:7
    frame #5: 0x00000001003745ec libomp.dylib`::__kmp_internal_end_library(gtid_req=-1) at kmp_runtime.cpp:6498:3
    frame #6: 0x0000000100374258 libomp.dylib`::__kmp_internal_end_atexit() at kmp_runtime.cpp:6115:3
    frame #7: 0x0000000100374218 libomp.dylib`::__kmp_internal_end_dtor() at kmp_runtime.cpp:6083:3
    frame #8: 0x0000000189359dc4 libsystem_c.dylib`__cxa_finalize_ranges + 464
    frame #9: 0x0000000189359b68 libsystem_c.dylib`exit + 44
    frame #10: 0x000000018947aec4 libdyld.dylib`dyld4::LibSystemHelpers::exit(int) const + 20
    frame #11: 0x00000001000150d4 dyld`start + 592
(lldb)

Not sure how to proceed from here though... I'm really not familiar with OpenMP

If you remove your patch, does the OMP failure still happen? Or does this patch cause it? If so, you might find that you need to see the difference between the two generated executables and see if something obvious has changed.

I started to get those errors on D126194 as well, definitely annoying.

They both do spooky things with type triviality so maybe this is somehow related? I'm not sure.

I had the same failures with and without this change. It looks like these tests are already broken upstream.

Without this patch:

Unsupported:  15
Passed     :  50
Failed     : 232

With this patch:

Unsupported:  15
Passed     :  50
Failed     : 232
Javier-varez added a comment.EditedJun 16 2022, 1:48 PM

For reference, I run the tests without this change on commit:

commit 94d1692aa155adf3b69609d142762b8c696e0710 (upstream/main, upstream/HEAD)
Author: Fangrui Song <i@maskray.me>
Date:   Tue Jun 14 21:25:56 2022 -0700

    [MC] Remove unused MCStreamer::SwitchSection

    switchSection should be used instead.

And the tests with this patch just has this patch on top of the previous commit

The OpenMP tests passed on D127998 (I think just randomly?). I'm going to commit this tomorrow morning so I can revert if needed.

Javier, thanks again for the patch and for the patience!

This revision was landed with ongoing or failed builds.Jun 17 2022, 12:36 AM
This revision was automatically updated to reflect the committed changes.

Thanks for taking care of this Roy and helping me out through the process!

sberg added a subscriber: sberg.Jun 20 2022, 7:35 AM

Is it intended that a deleted copy assignment op as in struct S { void operator =(S &) = delete; }; (though, somewhat oddly, not in struct S { void operator =(S) = delete; };) is now marked as trivial?

I think that's the root cause why a build of PDFium with clang-cl against the MSVC standard library started to fail for me now, effectively complaining that __is_trivially_assignable(std::pair<int,int> &, std::pair<int,int> const &) is false (as expected) while __has_trivial_assign(std::pair<int,int>) is (unexpectedly) true.

Is it intended that a deleted copy assignment op as in struct S { void operator =(S &) = delete; }; (though, somewhat oddly, not in struct S { void operator =(S) = delete; };) is now marked as trivial?

I think that's the root cause why a build of PDFium with clang-cl against the MSVC standard library started to fail for me now, effectively complaining that __is_trivially_assignable(std::pair<int,int> &, std::pair<int,int> const &) is false (as expected) while __has_trivial_assign(std::pair<int,int>) is (unexpectedly) true.

I don't see it on godbolt trunk: https://godbolt.org/z/KPfxWqnhd.
Did you mean other traits maybe?

sberg added a comment.Jun 20 2022, 2:17 PM

Is it intended that a deleted copy assignment op as in struct S { void operator =(S &) = delete; }; (though, somewhat oddly, not in struct S { void operator =(S) = delete; };) is now marked as trivial?

I think that's the root cause why a build of PDFium with clang-cl against the MSVC standard library started to fail for me now, effectively complaining that __is_trivially_assignable(std::pair<int,int> &, std::pair<int,int> const &) is false (as expected) while __has_trivial_assign(std::pair<int,int>) is (unexpectedly) true.

I don't see it on godbolt trunk: https://godbolt.org/z/KPfxWqnhd.

You'd only see it when compiling with Clang against MSVC's <utility>, where std::pair has a deleted copy assignment op (something I approximated with the struct S above). https://godbolt.org/z/bbEqvosvP shows how the behavior changed in Clang trunk. (Interestingly, the behavior is different between MSVC and GCC, and Clang now changed from matching the MSVC behavior to matching the GCC one.)

You'd only see it when compiling with Clang against MSVC's <utility>, where std::pair has a deleted copy assignment op (something I approximated with the struct S above). https://godbolt.org/z/bbEqvosvP shows how the behavior changed in Clang trunk. (Interestingly, the behavior is different between MSVC and GCC, and Clang now changed from matching the MSVC behavior to matching the GCC one.)

I've done some more lookup, and I believe the current behavior is correct.
The reasoning is that the behavior in regards to deleted functions in __has_trivial_assign is pretty old and is documented here: https://github.com/llvm/llvm-project/issues/33063. As Richard writes there (and as documented in https://clang.llvm.org/docs/LanguageExtensions.html#type-trait-primitives) those type traits are deprecated exactly because they exhibit this weird behavior.

Is it possible to check how often PDFium uses __has_trivial_assign? Depending on how large this breakage is we might need to revert and proceed more carefully..

sberg added a comment.Jun 21 2022, 1:40 AM

Is it possible to check how often PDFium uses __has_trivial_assign? Depending on how large this breakage is we might need to revert and proceed more carefully..

The setup is a bit convoluted: I ran into this when building LibreOffice with clang-cl on Windows. LibreOffice builds a bundled PDFium (where it obtained the PDFium source tarball, including PDFium's bundled third-party/abseil-cpp, with a recipe documented at https://git.libreoffice.org/core/+/71b952340726190d1f178ef0dadfa89677f2c1dd/external/pdfium/README#6). That PDFium in turn contains a bundled third-party/abseil-cpp, whose absl/meta/type_traits.h defines an absl::is_trivially_copy_assignable struct that is implemented via __has_trivial_assign, but then in its implementation also does a static_assert to verify that its behavior (based on __has_trivial_assign) matches the behavior of std::is_trivially_copy_assignable` (which is based on __is_trivially_assignable). And that static_assert started to fail now with clang-cl on Windows against the MSVC standard library:

In file included from C:/lo-clang/core/workdir/UnpackedTarball/pdfium/core/fxge/win32/cgdi_plus_ext.cpp:19:
In file included from C:/lo-clang/core/workdir/UnpackedTarball/pdfium\core/fxcrt/fx_string.h:14:
In file included from C:/lo-clang/core/workdir/UnpackedTarball/pdfium\core/fxcrt/bytestring.h:23:
In file included from C:/lo-clang/core/workdir/UnpackedTarball/pdfium\core/fxcrt/string_view_template.h:18:
In file included from C:/lo-clang/core/workdir/UnpackedTarball/pdfium\third_party/abseil-cpp/absl/types/optional.h:39:
In file included from C:/lo-clang/core/workdir/UnpackedTarball/pdfium/third_party/abseil-cpp\absl/utility/utility.h:51:
C:/lo-clang/core/workdir/UnpackedTarball/pdfium/third_party/abseil-cpp\absl/meta/type_traits.h(501,3): error: static_assert failed due to requirement 'compliant || std::is_trivially_copy_assignable<std::pair<unsigned long long, unsigned long long>>::value' "Not compliant with std::is_trivially_copy_assignable; Standard: false, Implementation: true"
  static_assert(compliant || std::is_trivially_copy_assignable<T>::value,
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/lo-clang/core/workdir/UnpackedTarball/pdfium/third_party/abseil-cpp\absl/types/internal/optional.h(175,21): note: in instantiation of template class 'absl::is_trivially_copy_assignable<std::pair<unsigned long long, unsigned long long>>' requested here
              absl::is_trivially_copy_assignable<typename std::remove_cv<
                    ^
C:/lo-clang/core/workdir/UnpackedTarball/pdfium\third_party/abseil-cpp/absl/types/optional.h(119,45): note: in instantiation of default argument for 'optional_data<std::pair<unsigned long long, unsigned long long>>' required here
class optional : private optional_internal::optional_data<T>,
                                            ^~~~~~~~~~~~~~~~
C:/lo-clang/core/workdir/UnpackedTarball/pdfium/core/fxge/win32/cgdi_plus_ext.cpp(384,43): note: in instantiation of template class 'absl::optional<std::pair<unsigned long long, unsigned long long>>' requested here
absl::optional<std::pair<size_t, size_t>> IsSmallTriangle(
                                          ^

Both the PDFium tarball bundled by LibreOffice, and the third-party/abseil in turn bundled in that PDFium tarball, may be a bit dated, but I checked that PDFium's recent main branch https://pdfium.googlesource.com/pdfium/+/2c495300230ed67f87302716c8b262a146ae26af/core/fxge/win32/cgdi_plus_ext.cpp#375 still uses absl::optional<std::pair<size_t, size_t>>, and Abseil's recent master branch https://github.com/abseil/abseil-cpp/blob/42f22a28401c952f1fc5942231c7fdac80811bf5/absl/meta/type_traits.h#L264 definition of absl::is_trivially_copy_assignable still works as described above.

In general, there's still a handful of __has_trivial_* calls across Abseil's recent master branch absl/meta/type_traits.h.

In general, there's still a handful of __has_trivial_* calls across Abseil's recent master branch absl/meta/type_traits.h.

Breaking abseil seems pretty bad indeed :/ @rsmith do you think we should revert this?
Pinging @thakis as well for chrome.

tycho added a subscriber: tycho.Jun 25 2022, 7:48 PM