Details
- Reviewers
ldionne Mordante var-const huixie90 - Group Reviewers
Restricted Project - Commits
- rG80c7e93a2a84: [libc++] Add a bunch of missing _LIBCPP_HIDE_FROM_ABI
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
@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 ;-)
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 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. | |
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. | |
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? |
libcxx/include/__algorithm/equal_range.h | ||
---|---|---|
32 ↗ | (On Diff #445329) | I also added this in https://reviews.llvm.org/D129796 |
libcxx/include/__string/char_traits.h | ||
168 | is this meant to be static? |
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 | ||
---|---|---|
168 | 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? |
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 |
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. |
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. |
libcxx/utils/libcxx/test/features.py | ||
---|---|---|
29 |
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 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. |
- 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? |
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.
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. |
LGTM modulo one nit.
libcxx/docs/TestingLibcxx.rst | ||
---|---|---|
105 | 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. 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 prefer to do it directly, but I won't object to do it in a followup. |
please add clang-tidy too.