This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Add '-Wdeprecated-copy', '-Wdeprecated-copy-dtor' warnings to the test suite
ClosedPublic

Authored by ldionne on Feb 24 2023, 10:00 PM.

Diff Detail

Event Timeline

fsb4000 created this revision.Feb 24 2023, 10:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2023, 10:00 PM
Herald added a subscriber: arichardson. · View Herald Transcript
fsb4000 requested review of this revision.Feb 24 2023, 10:00 PM
fsb4000 edited the summary of this revision. (Show Details)Feb 24 2023, 10:10 PM
fsb4000 updated this revision to Diff 500368.Feb 24 2023, 11:24 PM

fix tests

fsb4000 updated this revision to Diff 500370.Feb 24 2023, 11:27 PM
fsb4000 updated this revision to Diff 500410.Feb 25 2023, 3:54 AM

fix more tests

fsb4000 updated this revision to Diff 500412.Feb 25 2023, 4:15 AM
fsb4000 updated this revision to Diff 500423.Feb 25 2023, 5:36 AM
fsb4000 updated this revision to Diff 500431.Feb 25 2023, 8:06 AM
fsb4000 updated this revision to Diff 500486.Feb 25 2023, 6:16 PM
fsb4000 updated this revision to Diff 500487.Feb 25 2023, 6:33 PM
fsb4000 updated this revision to Diff 500489.Feb 25 2023, 7:33 PM

Guess I'll stop working on this ;)

libcxx/utils/libcxx/test/params.py
15

I suggest using both -Wdeprecated-copy and -Wdeprecated-copy-dtor here (I'm not sure if GCC supports the longer -Wdeprecated-copy-with-user-provided-dtor, but the manual does include -Wdeprecated-copy-dtor.)

fsb4000 updated this revision to Diff 500493.Feb 25 2023, 9:28 PM
fsb4000 marked an inline comment as done.
fsb4000 retitled this revision from [libc++][test] Add `-Wdeprecated-copy-with-user-provided-dtor` warning to the test suite to [libc++][test] Add '-Wdeprecated-copy', '-Wdeprecated-copy-dtor' warnings to the test suite.
fsb4000 added a reviewer: Mordante.
fsb4000 added inline comments.
libcxx/utils/libcxx/test/params.py
15

Yes, GCC doesn't support -Wdeprecated-copy-with-user-provided-dtor

I changed the warning flag.

fsb4000 updated this revision to Diff 500528.Feb 26 2023, 2:08 AM

Thanks for working on this!
I assume this fixes https://github.com/llvm/llvm-project/issues/60977. If so can you mention it in the commit message?

libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp
30

Can these be deleted instead? I assume the id was intentionally const qualified.

libcxx/test/support/counting_predicates.h
24

Are they used? Copying the count_ seems wrong.

The same for copies of other stateful parts in the tests.

fsb4000 added inline comments.Feb 26 2023, 5:51 AM
libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp
30

Without my changes:

/home/libcxx-builder/.buildkite-agent/builds/99763c758770-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:29:3: error: definition of implicit copy constructor for 'Base' is deprecated because it has a user-provided destructor [-Werror,-Wdeprecated-copy-with-user-provided-dtor]
  ~Base() { shared_buff[(*cur_idx)++] = id; }
  ^
/home/libcxx-builder/.buildkite-agent/builds/99763c758770-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:32:8: note: in implicit copy constructor for 'Base' first required here
struct A : Base {
       ^
/home/libcxx-builder/.buildkite-agent/builds/99763c758770-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:53:8: note: in implicit move constructor for 'A' first required here
  func(A(shared_buf, &cur_idx), std::unique_ptr<B>(new B(shared_buf, &cur_idx)),

If I add =default:

/home/libcxx-builder/.buildkite-agent/builds/c072959aa3b7-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:30:9: error: explicitly defaulted copy assignment operator is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
  Base& operator=(const Base&) = default;
        ^
/home/libcxx-builder/.buildkite-agent/builds/c072959aa3b7-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:25:14: note: copy assignment operator of 'Base' is implicitly deleted because field 'id' is of const-qualified type 'const char'
  const char id;
             ^
/home/libcxx-builder/.buildkite-agent/builds/c072959aa3b7-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:30:34: note: replace 'default' with 'delete'
  Base& operator=(const Base&) = default;
                                 ^~~~~~~
                                 delete
1 error generated.

If I add =delete

/home/libcxx-builder/.buildkite-agent/builds/ab7607b38669-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:34:8: error: call to deleted constructor of 'Base'
struct A : Base {
       ^
/home/libcxx-builder/.buildkite-agent/builds/ab7607b38669-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:55:8: note: in implicit copy constructor for 'A' first required here
  func(A(shared_buf, &cur_idx), std::unique_ptr<B>(new B(shared_buf, &cur_idx)),
       ^
/home/libcxx-builder/.buildkite-agent/builds/ab7607b38669-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:29:3: note: 'Base' has been explicitly marked deleted here
  Base(const Base&) = delete;
  ^
/home/libcxx-builder/.buildkite-agent/builds/ab7607b38669-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:42:8: error: call to deleted constructor of 'Base'
struct C : Base {
       ^
/home/libcxx-builder/.buildkite-agent/builds/ab7607b38669-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:56:8: note: in implicit copy constructor for 'C' first required here
       C(shared_buf, &cur_idx));
       ^
/home/libcxx-builder/.buildkite-agent/builds/ab7607b38669-1/llvm-project/libcxx-ci/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp:29:3: note: 'Base' has been explicitly marked deleted here
  Base(const Base&) = delete;
  ^
2 errors generated.
30

one more thing.
On Windows I can write whatever in that test.
It passes without my changes, it passes with =default, it passes with =delete

I run the test this way:

C:\Dev\STL\llvm-project\build>python bin\llvm-lit.py ..\libcxx\test\libcxx\memory\trivial_abi\unique_ptr_destruction_order.pass.cpp -v
llvm-lit.py: C:\Dev\STL\llvm-project\libcxx\utils\libcxx\test\config.py:19: note: (llvm-libc++-shared-clangcl.cfg.in) Using %{cxx} substitution: ''C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/x64/bin/clang-cl.exe''
llvm-lit.py: C:\Dev\STL\llvm-project\libcxx\utils\libcxx\test\config.py:19: note: (llvm-libc++-shared-clangcl.cfg.in) Using %{flags} substitution: '--driver-mode=g++ --target=x86_64-pc-windows-msvc'
llvm-lit.py: C:\Dev\STL\llvm-project\libcxx\utils\libcxx\test\config.py:19: note: (llvm-libc++-shared-clangcl.cfg.in) Using %{compile_flags} substitution: '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Wdeprecated-copy -Wdeprecated-copy-dtor -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL'
llvm-lit.py: C:\Dev\STL\llvm-project\libcxx\utils\libcxx\test\config.py:19: note: (llvm-libc++-shared-clangcl.cfg.in) Using %{link_flags} substitution: '-llibc++experimental -nostdlib -L %{lib} -lc++ -lmsvcrt -lmsvcprt -loldnames'
llvm-lit.py: C:\Dev\STL\llvm-project\libcxx\utils\libcxx\test\config.py:19: note: (llvm-libc++-shared-clangcl.cfg.in) Using %{exec} substitution: '%{executor} --execdir %T --env PATH=%{lib} -- '
llvm-lit.py: C:\Dev\STL\llvm-project\libcxx\utils\libcxx\test\config.py:19: note: (llvm-libc++-shared-clangcl.cfg.in) All available features: no-filesystem, buildhost=win32, clang-15.0, target=x86_64-pc-windows-msvc, stdlib=libc++, stdlib=llvm-libc++, locale.fr_CA.ISO8859-1, -faligned-allocation, locale.zh_CN.UTF-8, locale.fr_FR.UTF-8, buildhost=windows, clang-15, clang-15.0.1, -fsized-deallocation, locale.en_US.UTF-8, clang, executor-has-no-bash, libcpp-abi-version=1, msvc-19.36, win32-broken-utf8-wchar-ctype, diagnose-if-support, windows, fdelayed-template-parsing, c++2b, thread-safety, objective-c++, c++experimental, windows-dll, long_tests, msvc-19, has-fblocks, locale.ja_JP.UTF-8, verify-support, locale.ru_RU.UTF-8, has-fconstexpr-steps, msvc
-- Testing: 1 tests, 1 workers --
PASS: llvm-libc++-shared-clangcl.cfg.in :: libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp (1 of 1)

Testing Time: 5.96s
  Passed: 1
libcxx/test/support/counting_predicates.h
24

I saw the warnings but I will invistigate that.

fsb4000 added inline comments.Feb 26 2023, 6:05 AM
libcxx/test/support/counting_predicates.h
24

Yes, they called.

C:/Dev/STL/llvm-project/libcxx/test/support\counting_predicates.h:25:5: error: definition of implicit copy constructor for 'unary_counting_predicate<bool (*)(int), int>' is deprecated because it has a user-provided destructor [-Werror,-Wdeprecated-copy-with-user-provided-dtor]
    ~unary_counting_predicate() {}
    ^
C:\Dev\STL\llvm-project\libcxx\test\std\utilities\function.objects\refwrap\refwrap.helpers\ref_2.pass.cpp:60:22: note: in implicit copy constructor for 'unary_counting_predicate<bool (*)(int), int>' first required here
    assert(call_pred(cp));
                     ^

https://github.com/llvm/llvm-project/blob/bbaa79470423e63d075abef869ab4d5a203c51d3/libcxx/test/std/utilities/function.objects/refwrap/refwrap.helpers/ref_2.pass.cpp#L25

https://github.com/llvm/llvm-project/blob/bbaa79470423e63d075abef869ab4d5a203c51d3/libcxx/test/std/utilities/function.objects/refwrap/refwrap.helpers/ref_2.pass.cpp#L60

fsb4000 added inline comments.Feb 26 2023, 6:17 AM
libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp
30

I can add

Base(const Base&) = default;
Base& operator=(const Base&) = delete;

It seems it works: https://gcc.godbolt.org/z/6nEGfffEj

fsb4000 updated this revision to Diff 500562.Feb 26 2023, 6:21 AM

restore const char in test\libcxx\memory\trivial_abi\unique_ptr_destruction_order.pass.cpp

fsb4000 marked an inline comment as done.Feb 26 2023, 6:21 AM
fsb4000 added inline comments.Feb 26 2023, 6:29 AM
libcxx/test/support/counting_predicates.h
24

About CDeleter:

In file included from C:\Dev\STL\llvm-project\libcxx\test\std\utilities\smartptr\unique.ptr\unique.ptr.class\unique.ptr.asgn\move.pass.cpp:28:
C:/Dev/STL/llvm-project/libcxx/test/support/deleter_types.h:169:24: error: definition of implicit copy assignment operator for 'CDeleter<A>' is deprecated because it has a user-provided destructor [-Werror,-Wdeprecated-copy-with-user-provided-dtor]
  TEST_CONSTEXPR_CXX23 ~CDeleter() {
                       ^
C:/Dev/STL/llvm-project/build/include/c++/v1\__memory/unique_ptr.h:232:21: note: in implicit copy assignment operator for 'CDeleter<A>' first required here
    __ptr_.second() = _VSTD::forward<deleter_type>(__u.get_deleter());
                    ^
C:\Dev\STL\llvm-project\libcxx\test\std\utilities\smartptr\unique.ptr\unique.ptr.class\unique.ptr.asgn\move.pass.cpp:76:8: note: in instantiation of member function 'std::unique_ptr<A, CDeleter<A> &>::operator=' requested here
    s2 = std::move(s1);
       ^
C:\Dev\STL\llvm-project\libcxx\test\std\utilities\smartptr\unique.ptr\unique.ptr.class\unique.ptr.asgn\move.pass.cpp:133:5: note: in instantiation of function template specialization 'test_basic<false>' requested here
    test_basic</*IsArray*/ false>();
    ^
In file included from C:\Dev\STL\llvm-project\libcxx\test\std\utilities\smartptr\unique.ptr\unique.ptr.class\unique.ptr.asgn\move.pass.cpp:28:
C:/Dev/STL/llvm-project/libcxx/test/support/deleter_types.h:194:24: error: definition of implicit copy assignment operator for 'CDeleter<A[]>' is deprecated because it has a user-provided destructor [-Werror,-Wdeprecated-copy-with-user-provided-dtor]
  TEST_CONSTEXPR_CXX23 ~CDeleter() {
                       ^
C:/Dev/STL/llvm-project/build/include/c++/v1\__memory/unique_ptr.h:434:21: note: in implicit copy assignment operator for 'CDeleter<A[]>' first required here
    __ptr_.second() = _VSTD::forward<deleter_type>(__u.get_deleter());
                    ^
C:\Dev\STL\llvm-project\libcxx\test\std\utilities\smartptr\unique.ptr\unique.ptr.class\unique.ptr.asgn\move.pass.cpp:76:8: note: in instantiation of member function 'std::unique_ptr<A[], CDeleter<A[]> &>::operator=' requested here
    s2 = std::move(s1);
       ^
C:\Dev\STL\llvm-project\libcxx\test\std\utilities\smartptr\unique.ptr\unique.ptr.class\unique.ptr.asgn\move.pass.cpp:137:5: note: in instantiation of function template specialization 'test_basic<true>' requested here
    test_basic</*IsArray*/ true>();
    ^
2 errors generated.

error: command failed with exit status: 1
philnik added inline comments.Mar 1 2023, 8:57 AM
libcxx/test/support/nasty_containers.h
46–48

Maybe just remove the destructor instead? If it breaks a test ignore the suggestion.

ldionne commandeered this revision.Sep 8 2023, 8:17 AM
ldionne added a reviewer: fsb4000.
ldionne added a subscriber: ldionne.

[Github PR transition cleanup]

Commandeering to pick up.

ldionne updated this revision to Diff 556267.Sep 8 2023, 8:34 AM
ldionne edited the summary of this revision. (Show Details)

Rebase.

Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2023, 8:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Sep 8 2023, 8:36 AM

Based on the discussion, I don't think any of the comments are blocking. I'll ship this if CI is green, everything in this patch seems reasonable to me and it fixes a real issue for MSVC's STL usage of the test suite.

This revision is now accepted and ready to land.Sep 8 2023, 8:36 AM
ldionne updated this revision to Diff 556467.Sep 11 2023, 11:15 AM

Clang-format.

ldionne updated this revision to Diff 556487.Sep 11 2023, 2:39 PM

More missing format.

ldionne updated this revision to Diff 556513.Sep 11 2023, 6:31 PM

Rebase and fix C++11 test.