[libcxx-commits] [PATCH] D129968: [libc++] Add a bunch of missing _LIBCPP_HIDE_FROM_ABI

Sat Aug 13 03:21:02 PDT 2022

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM modulo one nit.

================
Comment at: libcxx/docs/TestingLibcxx.rst:101
+These tools are:
+- clang-query
+
----------------
please add clang-tidy too.

================
Comment at: libcxx/utils/libcxx/test/features.py:29
+  try:
+    return int(re.search('[0-9]+', commandOutput(cfg, ['clang-query --version'])).group()) >= 13
+  except ConfigurationRuntimeError:
----------------
ldionne wrote:
> philnik wrote:
> > Mordante wrote:
> > > ldionne wrote:
> > > > Mordante wrote:
> > > > > Mordante wrote:
> > > > > > philnik wrote:
> > > > > > > philnik wrote:
> > > > > > > > Mordante wrote:
> > > > > > > > > Mordante wrote:
> > > > > > > > > > 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.
> > > > > > > > > Can you update the developer documentation with this tool (I think clang-tidy) also isn't listed.
> > > > > > > > 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.
> > > > > > > What exactly do you mean? Where do we have any developer documentation w.r.t. tooling?
> > > > > > https://libcxx.llvm.org/TestingLibcxx.html should list the dependencies to be able to execute the tests.
> > > > > > 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 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.
> > > > > 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.
> > 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?
> 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.
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.

================
Comment at: libcxx/utils/libcxx/test/features.py:29
+  try:
+    return int(re.search('[0-9]+', commandOutput(cfg, ['clang-query --version'])).group()) >= 13
+  except ConfigurationRuntimeError:
----------------
Mordante wrote:
> ldionne wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > ldionne wrote:
> > > > > Mordante wrote:
> > > > > > Mordante wrote:
> > > > > > > philnik wrote:
> > > > > > > > philnik wrote:
> > > > > > > > > Mordante wrote:
> > > > > > > > > > Mordante wrote:
> > > > > > > > > > > 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.
> > > > > > > > > > Can you update the developer documentation with this tool (I think clang-tidy) also isn't listed.
> > > > > > > > > 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.
> > > > > > > > What exactly do you mean? Where do we have any developer documentation w.r.t. tooling?
> > > > > > > https://libcxx.llvm.org/TestingLibcxx.html should list the dependencies to be able to execute the tests.
> > > > > > > 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 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.
> > > > > > 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.
> > > 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?
> > 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.
> 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.
> 
> 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129968/new/

https://reviews.llvm.org/D129968