Page MenuHomePhabricator

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

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

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
philnik requested review of this revision.Jan 23 2023, 1:27 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 491355.Jan 23 2023, 6:51 AM

Try to fix CI

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

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

philnik updated this revision to Diff 506374.Sun, Mar 19, 3:56 AM

Try to fix CI