Page MenuHomePhabricator
Differential D142332

[libc++] Add hide_from_abi check for classes
Needs ReviewPublic
Actions

Authored by philnik on Jan 23 2023, 1:27 AM.

Details

Reviewers
ldionne
Mordante
var-const
jdoerfert
Group Reviewers
Restricted Project
Summary

We already have a clang-tidy check for making sure that _LIBCPP_HIDE_FROM_ABI is on free functions. This patch extends this to class members. The places where we don't check for _LIBCPP_HIDE_FROM_ABI are classes for which we have an instantiation in the library.

Diff Detail

Unit TestsFailed
View All

TimeTest
3,090 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp
Script: -- : 'RUN: at line 15'; clang-tidy-16 /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* --checks='-*,libcpp-*' --load=/home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-cxx11/libcxx/test/tools/clang_tidy_checks/libcxx-tidy.plugin -- -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -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 -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -fno-modules
4,830 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.libcxx::double_include.sh.cpp
Script: -- : 'RUN: at line 11'; /usr/bin/g++-12 -c /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/double_include.sh.cpp -o /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/test/libcxx/Output/double_include.sh.cpp.dir/t.tmp.first.o -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/support -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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess
4,800 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.libcxx::min_max_macros.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/min_max_macros.compile.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/support -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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -fsyntax-only
4,780 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.libcxx::nasty_macros.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/nasty_macros.compile.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/support -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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -fsyntax-only
4,810 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.libcxx::no_assert_include.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/no_assert_include.compile.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/eeef97b651a0-1/llvm-project/libcxx-ci/libcxx/test/support -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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -fsyntax-only
View Full Test Results (410 Failed)

Event Timeline

philnik created this revision.Jan 23 2023, 1:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 1:27 AM
Herald added subscribers: smeenai, miyuki, krytarowski. · View Herald Transcript
philnik requested review of this revision.Jan 23 2023, 1:27 AM
Herald added a reviewer: jdoerfert. · View Herald TranscriptJan 23 2023, 1:27 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: libcxx-commits, sstefan1. · View Herald Transcript
Harbormaster completed remote builds in B209292: Diff 491253.Jan 23 2023, 1:52 AM
philnik updated this revision to Diff 491355.Jan 23 2023, 6:51 AM

Try to fix CI

Harbormaster completed remote builds in B209364: Diff 491355.Jan 23 2023, 7:16 AM
Mordante added a comment.Jan 30 2023, 9:52 AM

In general I like it, for reviewing it would have been nicer to split the patch in multiple parts:

  • removal from trivial functions
  • adding on non-trivial functions

That would also make it easier to see which part of the clang-tidy enables which test.

Also please don't make unrelated changes in the library code.

libcxx/include/__chrono/day.h
32

Is it an issue to have this unneeded _LIBCPP_HIDE_FROM_ABI? I ask since when the class becomes non-trivial we need to add it again.
(Obviously we can't just change this class, but it would apply to internal classes that are not user "visible".)

libcxx/include/__ranges/iota_view.h
51

Please don't hide these kind of changes in this patch.

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
108

why only testing for trivially copyable?

philnik added inline comments.Jan 30 2023, 10:56 AM
libcxx/include/__chrono/day.h
32

Not really. I wouldn't expect internal classes to be a problem either. It's not like we regularly change a class to be trivial which wasn't trivial before. The main reason I added this check is for tag classes and the like where it's just noise, e.g. struct some_tag { explicit some_tag() = default; } shouldn't have a _LIBCPP_HIDE_FROM_ABI IMO.

libcxx/include/__ranges/iota_view.h
51

What do you mean with "these kinds of changes"? I didn't think this would be a problem, given that it will never be executed anyways.

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
108

I'm not sure what you mean. The name is wrong, but I check for isTrivial(), not isTriviallyCopyable(). Was it just about the name being wrong?

ldionne accepted this revision as: ldionne.Thu, Mar 2, 9:32 AM

I am personally find with this patch after a few minor changes outlined in my comments. I think this is a large mechanical patch and splitting it might or might not help reviewing, I am not sure. I went through 90% of the mechanical changes to validate them and everything looked fine to me. Will let Mark LGTM the patch since he had comments.

libcxx/include/__algorithm/comp_ref_type.h
18

Can your commit message please explain a bit more what the patch is about?

libcxx/include/__ranges/iota_view.h
51

So there were two choices here:

  1. Add _LIBCPP_HIDE_FROM_ABI, or
  2. add consteval to shut up the clang-tidy check

I think this patch should add _LIBCPP_HIDE_FROM_ABI because the reviewer's assumption is that all these changes are 100% mechanical, and this violates that assumption.

We should then (trivially) make it consteval in a separate one-liner patch, cause I agree that's probably what it should have been in the first place. Or, alternatively, add consteval as a NFC prior to this patch.

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
85
94
108
philnik edited the summary of this revision. (Show Details)Sun, Mar 5, 5:07 AM
philnik updated this revision to Diff 502430.Sun, Mar 5, 5:15 AM
philnik marked 7 inline comments as done.

Address comments

Harbormaster completed remote builds in B217416: Diff 502430.Sun, Mar 5, 5:40 AM
philnik updated this revision to Diff 503444.Wed, Mar 8, 10:53 AM

Try to fix CI

philnik updated this revision to Diff 503449.Wed, Mar 8, 11:03 AM

More fixes

Harbormaster completed remote builds in B218166: Diff 503449.Wed, Mar 8, 1:03 PM
philnik updated this revision to Diff 506374.Sun, Mar 19, 3:56 AM

Try to fix CI

Herald added subscribers: mikhail.ramalho, jplehr. · View Herald TranscriptSun, Mar 19, 3:56 AM
Harbormaster completed remote builds in B220283: Diff 506374.Sun, Mar 19, 4:24 AM

Large Diff

This large diff affects 110 files. Files without inline comments have been collapsed. Expand All Files

Revision Contents
Changeset List

PathSize
libcxx/
include/
__algorithm/
7 lines
7 lines
6 lines
__atomic/
3 lines
__chrono/
2 lines
2 lines
5 lines
2 lines
2 lines
4 lines
2 lines
2 lines
2 lines
2 lines
__concepts/
6 lines
__coroutine/
6 lines
__expected/
4 lines
2 lines
__filesystem/
16 lines
8 lines
2 lines
6 lines
10 lines
__format/
2 lines
__functional/
16 lines
67 lines
12 lines
4 lines
4 lines
10 lines
12 lines
74 lines
__iterator/
24 lines
2 lines
2 lines
__memory/
6 lines
12 lines
30 lines
32 lines
8 lines
__memory_resource/
4 lines
14 lines
2 lines
16 lines
__random/
5 lines
2 lines
9 lines
3 lines
3 lines
3 lines
3 lines
2 lines
4 lines
8 lines
3 lines
3 lines
17 lines
17 lines
5 lines
2 lines
4 lines
3 lines
10 lines
40 lines
2 lines
__ranges/
2 lines
3 lines
9 lines
6 lines
2 lines
2 lines
__string/
136 lines
101 lines
__tuple_dir/
10 lines
__utility/
6 lines
22 lines
2 lines
4 lines
4 lines
2 lines
experimental/
12 lines
18 lines
62 lines
82 lines
ext/
30 lines
24 lines
93 lines
4 lines
134 lines
2 lines
4 lines
97 lines
36 lines
16 lines
4 lines
48 lines
10 lines
4 lines
10 lines
16 lines
2 lines
2 lines
2 lines
9 lines
2 lines
62 lines
48 lines
26 lines
4 lines
test/
tools/
clang_tidy_checks/
57 lines

Diff 506374

View Options

libcxx/include/__algorithm/comp_ref_type.h

Loading...
View Options

libcxx/include/__algorithm/make_projected.h

View Options

libcxx/include/__algorithm/shuffle.h

View Options

libcxx/include/__atomic/atomic_flag.h

View Options

libcxx/include/__chrono/calendar.h

View Options

libcxx/include/__chrono/day.h

Loading...
View Options

libcxx/include/__chrono/duration.h

View Options

libcxx/include/__chrono/month.h

View Options

libcxx/include/__chrono/monthday.h

View Options

libcxx/include/__chrono/weekday.h

View Options

libcxx/include/__chrono/year.h

View Options

libcxx/include/__chrono/year_month.h

View Options

libcxx/include/__chrono/year_month_day.h

View Options

libcxx/include/__chrono/year_month_weekday.h

View Options

libcxx/include/__concepts/swappable.h

View Options

libcxx/include/__coroutine/coroutine_handle.h

View Options

libcxx/include/__expected/bad_expected_access.h

View Options

libcxx/include/__expected/unexpect.h

View Options

libcxx/include/__filesystem/directory_entry.h

View Options

libcxx/include/__filesystem/file_status.h

View Options

libcxx/include/__filesystem/filesystem_error.h

View Options

libcxx/include/__filesystem/path_iterator.h

View Options

libcxx/include/__filesystem/recursive_directory_iterator.h

View Options

libcxx/include/__format/format_args.h

View Options

libcxx/include/__functional/boyer_moore_searcher.h

View Options

libcxx/include/__functional/function.h

View Options

libcxx/include/__functional/hash.h

View Options

libcxx/include/__functional/identity.h

View Options

libcxx/include/__functional/invoke.h

View Options

libcxx/include/__functional/perfect_forward.h

View Options

libcxx/include/__functional/ranges_operations.h

View Options

libcxx/include/__hash_table

View Options

libcxx/include/__iterator/common_iterator.h

View Options

libcxx/include/__iterator/move_sentinel.h

View Options

libcxx/include/__locale

View Options

libcxx/include/__memory/allocator.h

View Options

libcxx/include/__memory/builtin_new_allocator.h

View Options

libcxx/include/__memory/ranges_uninitialized_algorithms.h

View Options

libcxx/include/__memory/shared_ptr.h

View Options

libcxx/include/__memory/temp_value.h

View Options

libcxx/include/__memory_resource/monotonic_buffer_resource.h

View Options

libcxx/include/__memory_resource/polymorphic_allocator.h

View Options

libcxx/include/__memory_resource/synchronized_pool_resource.h

View Options

libcxx/include/__mutex_base

View Options

libcxx/include/__random/binomial_distribution.h

View Options

libcxx/include/__random/discard_block_engine.h

View Options

libcxx/include/__random/discrete_distribution.h

View Options

libcxx/include/__random/exponential_distribution.h

View Options

libcxx/include/__random/extreme_value_distribution.h

View Options

libcxx/include/__random/fisher_f_distribution.h

View Options

libcxx/include/__random/gamma_distribution.h

View Options

libcxx/include/__random/independent_bits_engine.h

View Options

libcxx/include/__random/linear_congruential_engine.h

View Options

libcxx/include/__random/mersenne_twister_engine.h

View Options

libcxx/include/__random/negative_binomial_distribution.h

View Options

libcxx/include/__random/normal_distribution.h

View Options

libcxx/include/__random/piecewise_constant_distribution.h

View Options

libcxx/include/__random/piecewise_linear_distribution.h

View Options

libcxx/include/__random/poisson_distribution.h

View Options

libcxx/include/__random/random_device.h

View Options

libcxx/include/__random/seed_seq.h

View Options

libcxx/include/__random/student_t_distribution.h

View Options

libcxx/include/__random/subtract_with_carry_engine.h

View Options

libcxx/include/__random/uniform_int_distribution.h

View Options

libcxx/include/__random/uniform_real_distribution.h

View Options

libcxx/include/__ranges/drop_view.h

View Options

libcxx/include/__ranges/iota_view.h

Loading...
View Options

libcxx/include/__ranges/non_propagating_cache.h

View Options

libcxx/include/__ranges/owning_view.h

View Options

libcxx/include/__ranges/range_adaptor.h

View Options

libcxx/include/__ranges/subrange.h

View Options

libcxx/include/__string/char_traits.h

View Options

libcxx/include/__tree

View Options

libcxx/include/__tuple_dir/sfinae_helpers.h

View Options

libcxx/include/__utility/in_place.h

View Options

libcxx/include/__utility/pair.h

View Options

libcxx/include/any

View Options

libcxx/include/array

View Options

libcxx/include/barrier

View Options

libcxx/include/condition_variable

View Options

libcxx/include/experimental/iterator

View Options

libcxx/include/experimental/memory_resource

View Options

libcxx/include/experimental/propagate_const

View Options

libcxx/include/experimental/simd

View Options

libcxx/include/ext/hash_map

View Options

libcxx/include/ext/hash_set

View Options

libcxx/include/forward_list

View Options

libcxx/include/fstream

View Options

libcxx/include/future

View Options

libcxx/include/ios

View Options

libcxx/include/latch

View Options

libcxx/include/list

View Options

libcxx/include/locale

View Options

libcxx/include/map

View Options

libcxx/include/optional

View Options

libcxx/include/regex

View Options

libcxx/include/scoped_allocator

View Options

libcxx/include/set

View Options

libcxx/include/shared_mutex

View Options

libcxx/include/stdexcept

View Options

libcxx/include/strstream

View Options

libcxx/include/system_error

View Options

libcxx/include/thread

View Options

libcxx/include/tuple

View Options

libcxx/include/typeinfo

View Options

libcxx/include/unordered_map

View Options

libcxx/include/unordered_set

View Options

libcxx/include/variant

View Options

libcxx/include/vector

View Options

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp

Loading...