This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add a bunch of missing _LIBCPP_HIDE_FROM_ABI
ClosedPublic

Authored by philnik on Jul 17 2022, 10:28 AM.

Diff Detail

Event Timeline

philnik created this revision.Jul 17 2022, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2022, 10:28 AM
philnik requested review of this revision.Jul 17 2022, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2022, 10:28 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

@Mordante @var-const @huixie90 @ldionne Please tell me which parts of this PR conflict with any of your PRs. I think it's mostly be the algorithms, especially sort.

Notable exceptions to adding _LIBCPP_HIDE_FROM_ABI are __sort, __insertion_sort_incomplete and operator+(const _CharT*, const basic_string&). These have specializations in the dylib and I have them marked _LIBCPP_HIDDEN instead because of that.

libcxx/include/__format/formatter_floating_point.h
106

@Mordante I'm guessing this was accidentally marked static?

I love this patch! I can't remember how often I forgot to add these macros, in fact I forgot it today. So when a tool can help us it's great. I think we can find more use-cases for AST-matchers! Actually I have some use-cases on my todo list ;-)

@Mordante @var-const @huixie90 @ldionne Please tell me which parts of this PR conflict with any of your PRs. I think it's mostly be the algorithms, especially sort.

Thanks, I'm working on regex too, but I see no big conflicts. Nor in other code I'm working on.

Notable exceptions to adding _LIBCPP_HIDE_FROM_ABI are __sort, __insertion_sort_incomplete and operator+(const _CharT*, const basic_string&). These have specializations in the dylib and I have them marked _LIBCPP_HIDDEN instead because of that.

Thanks for pointing that out.

In general I'm happy with the changes, but several comments. Mainly regarding the infrastructure. As mentioned in the comments I would encourage you to split the fixes from the infrastructure.

libcxx/include/__format/formatter_floating_point.h
106

Yes, nice catch!

libcxx/test/libcxx/clang_query.sh.cpp
10

https://buildkite.com/llvm-project/libcxx-ci/builds/12164#01820d36-8444-4b5e-b2f5-a3a866703ed6
lists the test as unsupported.

Our Docker doesn't install clang-tools-$LLVM_LATEST_VERSION.

Can you update our Docker file in a separate review and rebase this one after it passes.
Maybe it's even better to split this patch in the missing _LIBCPP_HIDE_FROM_ABI and the test improvements. Then you can land the first part quickly without having the risk of merge conflicts in that part.

12

I really like to use variable for clang-query like we do with our compiler. Now the name is hard-coded and may not work on systems where the tool is named differently. (See my comment in features.py too.)

libcxx/test/libcxx/clang_query/hide_from_abi_or_visible.query
2

Please add a copyright header.

I'm familiar with clang-query and the AST, however I fear that people not familiar with this are lost at this code.
Can you add a short description what the query does and a link to the documentation, for example https://clang.llvm.org/docs/LibASTMatchersReference.html

libcxx/utils/libcxx/test/features.py
29

Does this also find clang-query-15? This is the default name used in the apt.llvm.org so I would like that to work out of the box.

29

Can you update the developer documentation with this tool (I think clang-tidy) also isn't listed.

@philnik Would it make sense to add this patch to .git-blame-ignore-revs at the root of the repo once it's committed?

The __algorithm part LGTM. I'm ok with merge conflicts for my patches, they look simple to resolve.

libcxx/test/libcxx/clang_query.sh.cpp
10

Can you add a comment to explain the purpose of this file?

var-const accepted this revision as: var-const.Jul 17 2022, 11:12 PM
huixie90 accepted this revision.Jul 18 2022, 1:35 AM
huixie90 added inline comments.
libcxx/include/__algorithm/equal_range.h
32 ↗(On Diff #445329)

I also added this in https://reviews.llvm.org/D129796
depending on which patches landed first, I guess we need to resolve the conflict

libcxx/include/__string/char_traits.h
169

is this meant to be static?

philnik marked 6 inline comments as done.Aug 2 2022, 4:07 AM

I can't remember how often I forgot to add these macros, in fact I forgot it today. So when a tool can help us it's great.

Exactly my thoughts. :-) The only thing that would be even better is to have these as clang-tidy checks so people get screamed at by their IDE. But that's a lot harder to accomplish properly and way less flexible, so it may not be feasible for all matchers.

I think we can find more use-cases for AST-matchers! Actually I have some use-cases on my todo list ;-)

I'm sure there are. I've got a few thoughts there too. What exactly would you like to have matchers for? I'm happy to implement a few more.

libcxx/include/__string/char_traits.h
169

I don't think so. I'll make another PR to search for this pattern later.

libcxx/test/libcxx/clang_query.sh.cpp
10

What exactly are you asking for? This file is just supposed to run clang-query on the headers.

12

See my comment in features.py. (Marking as done to keep the discussion in one place)

libcxx/test/libcxx/clang_query/hide_from_abi_or_visible.query
2

This is actually only the second time I'm working with matchers and I find it crazy how intuitive it is to write C++ AST matchers. Anyways, a comment explaining what the intention of this matcher is is probably a good idea. I've added a more general description of what this is in a README, since I suspect that part would be copy-pasted into every matcher otherwise.

libcxx/utils/libcxx/test/features.py
29

No, it doesn't find clang-query-15. I also don't know how this could be implemented in a way that finds different versions. We don't know which version is the correct one if we find multiple. IMO we shouldn't try to find some executable but rather ask the user to put a clang-query into the path. That's a lot safer. With the compiler it's a different story, since we get that through CMake.

29

What exactly do you mean? Where do we have any developer documentation w.r.t. tooling?

philnik updated this revision to Diff 449258.Aug 2 2022, 4:07 AM
philnik marked 3 inline comments as done.
  • Address comments

I think we can find more use-cases for AST-matchers! Actually I have some use-cases on my todo list ;-)

I'm sure there are. I've got a few thoughts there too. What exactly would you like to have matchers for? I'm happy to implement a few more.

Validate all main functions in the tests have the proper signature and have a return 0;.

libcxx/test/libcxx/clang_query.sh.cpp
10

Can you add that as comment and add a link to the README.md you added?

libcxx/utils/libcxx/test/features.py
29

https://libcxx.llvm.org/TestingLibcxx.html should list the dependencies to be able to execute the tests.

29

No, it doesn't find clang-query-15. I also don't know how this could be implemented in a way that finds different versions. We don't know which version is the correct one if we find multiple. IMO we shouldn't try to find some executable but rather ask the user to put a clang-query into the path. That's a lot safer. With the compiler it's a different story, since we get that through CMake.

It's already possible to select the wanted compiler and several other features using CMake options. I really would like to make this a configuration option too.

ldionne accepted this revision.Aug 4 2022, 8:57 AM

Validate all main functions in the tests have the proper signature and have a return 0;.

If we go there, we might as well check that main is declared as int main(int, char**) (and not void main(...) or int main(int, char const** or other variations).

LGTM, but please ping me in a week to check whether it's fine to commit this (to avoid conflicts when we cherry-pick to LLVM 15).

libcxx/test/libcxx/clang_query/hide_from_abi_or_visible.query
31

Can you explain why you skip those in a 1 line comment?

libcxx/utils/libcxx/test/features.py
29

I agree we should list tools that *can* be used in the test suite, but we should of course be careful about not requiring them (which is not the case here so it's all good).

@philnik Can you add a section in TestingLibcxx.rst that mentions clang-query as being an optional dependency of the test suite. We can then add more stuff to it (there's gdb and certainly others). I think it will be useful to list them all in a single place.

This revision is now accepted and ready to land.Aug 4 2022, 8:57 AM

Validate all main functions in the tests have the proper signature and have a return 0;.

If we go there, we might as well check that main is declared as int main(int, char**) (and not void main(...) or int main(int, char const** or other variations).

Agreed, that's what I meant with the proper signature ;-)

Mordante requested changes to this revision.Aug 6 2022, 9:12 AM
Mordante added inline comments.
libcxx/utils/libcxx/test/features.py
29

No, it doesn't find clang-query-15. I also don't know how this could be implemented in a way that finds different versions. We don't know which version is the correct one if we find multiple. IMO we shouldn't try to find some executable but rather ask the user to put a clang-query into the path. That's a lot safer. With the compiler it's a different story, since we get that through CMake.

It's already possible to select the wanted compiler and several other features using CMake options. I really would like to make this a configuration option too.

I see this was approved before this was resolved, so I will mark the ticket as needs work, at least it needs additional discussion.

Picking the one in the path doesn't work well on all supported platforms.
On Windows it's uncommon to have every application in the %PATH% so this will make it harder for Windows users.

On Linux by requiring it in $PATH means it's not possible to use different versions of clang-query when needed. This is an issue since our CI uses one Docker image for both the main and release branch. By locking it to one version means it's tricky to update the image; switching the main branch to a newer version might break the release branch. This is maybe not a real issue for clang-query but it probably will be for clang-tidy; an improved detection algorithm can suddenly flag code in the release branch and fail the CI. I really like to keep the release CI green too.

So I really like to see a CMake option to select the proper clang-query and propagate that to the tests.

See D131324 for some more details, note that there I also propose to remove the clang-query symlink from the Dockerfile.

This revision now requires changes to proceed.Aug 6 2022, 9:12 AM
philnik updated this revision to Diff 451115.Aug 9 2022, 5:01 AM
philnik marked 6 inline comments as done.
  • Address comments
libcxx/utils/libcxx/test/features.py
29

I would prefer to change that in a follow-up assuming we want to change it, since this also affects other tools and would be a larger change in itself. Would that be OK for you?

jloser added a subscriber: jloser.Aug 9 2022, 7:25 AM

This is an interesting change - I like the systematic way of finding all the functions missing the hidden visibility markup. Nice job!

In the future, I'd love if we didn't have to write the markup at all for hidden visibility and instead have that be the default visibility at namespace scope in libc++. This inverts the current behavior and would mean we would mark up the functions we want to be exported in the dynamic libs which are far fewer than the ones we want to hide. That's a bigger change and can be addressed after this if desired. It's something @ldionne and I spoke about briefly a while back.

This is an interesting change - I like the systematic way of finding all the functions missing the hidden visibility markup. Nice job!

In the future, I'd love if we didn't have to write the markup at all for hidden visibility and instead have that be the default visibility at namespace scope in libc++. This inverts the current behavior and would mean we would mark up the functions we want to be exported in the dynamic libs which are far fewer than the ones we want to hide. That's a bigger change and can be addressed after this if desired. It's something @ldionne and I spoke about briefly a while back.

I think that's planned, but it's a bit more complicated, since _LIBCPP_HIDE_FROM_ABI also adds __attribute__((__abi_tag__("<libc++ version>"))). We'd need an inline namespace <libc++ version> _LIBCPP_HIDDEN {} or something like that. That would also remove the always_inline when using GCC.

ldionne accepted this revision.Aug 11 2022, 10:25 AM

LGTM with green CI.

libcxx/utils/libcxx/test/features.py
29

Re clang-query-15, I think this should be solved in a different patch, and I'm really not sure that we want to start hardcoding version numbers everywhere in our code -- that seems like something we should constrain to the Docker image. But we can talk about it in detail in D131324.

philnik updated this revision to Diff 451936.Aug 11 2022, 11:40 AM
  • Rebased
  • Try to fix CI
Mordante accepted this revision.Aug 13 2022, 3:21 AM

LGTM modulo one nit.

libcxx/docs/TestingLibcxx.rst
101

please add clang-tidy too.

libcxx/utils/libcxx/test/features.py
29

I agree on not hardcoding version numbers everywhere, but I strongly disagree to do it in the Docker image.
The way now used in the Docker image doesn't work properly. Since the image is used for both the main and release branch our current solution forces us to use the same version for both. So at the moment we're basically stuck with Clang-14 to test the main branch.

IMO the CI runner files are the place to configure the proper toolchain this file differs for the main and release branch allowing us to use different Clang versions to for both branches. D131174 has a proposed fix for Clang.

We can discuss the details further in the other patches.

29

I would prefer to change that in a follow-up assuming we want to change it, since this also affects other tools and would be a larger change in itself. Would that be OK for you?

I prefer to do it directly, but I won't object to do it in a followup.

This revision is now accepted and ready to land.Aug 13 2022, 3:21 AM
philnik updated this revision to Diff 452407.Aug 13 2022, 4:24 AM
philnik marked an inline comment as done.
  • Address comments and fix CI
This revision was automatically updated to reflect the committed changes.