Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always treat CCCL as system headers #531

Merged
merged 6 commits into from
Oct 21, 2023
Merged

Conversation

miscco
Copy link
Collaborator

@miscco miscco commented Oct 10, 2023

There are users that build with stronger warning levels that we test against or use newer compilers that added warnings

In that case, their build might fail due to warnings emitted by a cccl header.

To work around this user issue we allow users to define CCCL_AS_SYSTEM_HEADER to silence all warnings originating from our headers

Fixes #527

@miscco miscco requested review from a team as code owners October 10, 2023 13:31
@miscco miscco requested review from griwes, elstehle, jrhemstad, alliepiper, dkolsen-pgi, gevtushenko and wmaxey and removed request for a team and jrhemstad October 10, 2023 13:31
@jrhemstad
Copy link
Collaborator

To work around this user issue we allow users to define CCCL_AS_SYSTEM_HEADER to silence all warnings originating from our headers

This should be opt out, not opt in.

#pragma GCC system_header
#if defined(CCCL_AS_SYSTEM_HEADER)
# define _LIBCUDACXX_IMPLICIT_SYSTEM_HEADER
#elif defined(_LIBCUDACXX_COMPILER_MSVC) \
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This construction here is needed to keep preexisting behavior.

I decided to overwrite the old _LIBCUDACXX_DISABLE_PRAGMA_MSVC_WARNING and _LIBCUDACXX_DISABLE_PRAGMA_GCC_SYSTEM_HEADER behavior in case of CCCL_AS_SYSTEM_HEADER

Shout if you disagree

Copy link
Collaborator

@gevtushenko gevtushenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments:

suggestion: I prefer self-sufficient headers. Most of the changes add macro to the header containing only CUB includes. It's difficult to prove that the include chain will lead us to libcu++ config. It happens to do so due to namespace macro, but it's implicit. Also, in Thrust, you've included libcu++ config into Thrust config, but this is not done for CUB.

important: this PR has to add _CCCL_NO_SYSTEM_HEADER definition to Thrust/CUB tests and examples before it's merged.

suggestion: macro was not added to the following headers:

  • cub/cub/detail/exec_check_disable.cuh
  • cub/cub/detail/detect_cuda_runtime.cuh
  • thrust/thrust/detail/memory_wrapper.h
  • thrust/thrust/detail/numeric_wrapper.h
  • thrust/thrust/detail/config/simple_defines.h
  • thrust/thrust/detail/algorithm_wrapper.h
  • thrust/thrust/detail/preprocessor.h
  • thrust/thrust/version.h

@miscco
Copy link
Collaborator Author

miscco commented Oct 10, 2023

A few comments:

suggestion: I prefer self-sufficient headers. Most of the changes add macro to the header containing only CUB includes. It's difficult to prove that the include chain will lead us to libcu++ config. It happens to do so due to namespace macro, but it's implicit. Also, in Thrust, you've included libcu++ config into Thrust config, but this is not done for CUB.

I intentionally tried to keep the changes minimal. Thats why in CUB I only added the include to cub/version.cuh because that is transitively included everywhere. I can move it to config.cuh though

important: this PR has to add _CCCL_NO_SYSTEM_HEADER definition to Thrust/CUB tests and examples before it's merged.

That will not work. As it stands, thrust and cub currently require libcu++ as system header because of deprecated symbols. I am absolutely unclear why but I believe that is for another day

suggestion: macro was not added to the following headers:

  • cub/cub/detail/exec_check_disable.cuh
  • cub/cub/detail/detect_cuda_runtime.cuh
  • thrust/thrust/detail/memory_wrapper.h
  • thrust/thrust/detail/numeric_wrapper.h
  • thrust/thrust/detail/config/simple_defines.h
  • thrust/thrust/detail/algorithm_wrapper.h
  • thrust/thrust/detail/preprocessor.h
  • thrust/thrust/version.h

I was unsure about the wrapper headers and headers that are purely defining macros, but I can happily add the Macro and the config include to those headers as well

Copy link
Collaborator

@gevtushenko gevtushenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will not work. As it stands, thrust and cub currently require libcu++ as system header because of deprecated symbols. I am absolutely unclear why but I believe that is for another day

If it's problematic, I'm fine merging this PR once remaining comments are addressed. But before doing that, please, file an issue, link it here, and prioritize disabling the macro in our tests.

@miscco miscco requested a review from a team as a code owner October 11, 2023 13:14
@jrhemstad
Copy link
Collaborator

jrhemstad commented Oct 11, 2023

@miscco in order to add a sanity test that these changes work as expected, I think we should update the example_project to add some aggressive warning flags. This will verify that anyone linking against the cmake target won't get warnings from CCCL headers. Perhaps @cliffburdick can share the warning flags he was using that led to the warnings he saw.

@miscco miscco force-pushed the silence_warnings branch 2 times, most recently from 0a65b5c to 1dd8286 Compare October 11, 2023 16:01
Copy link
Collaborator

@griwes griwes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked mainly the implementation of the macros, it seems correct.

@miscco miscco force-pushed the silence_warnings branch 3 times, most recently from 8280bfb to c167db1 Compare October 16, 2023 09:16
@miscco miscco force-pushed the silence_warnings branch 2 times, most recently from d229d91 to 66a34a0 Compare October 17, 2023 07:03
@miscco miscco force-pushed the silence_warnings branch 4 times, most recently from f37a430 to 41b46f6 Compare October 20, 2023 07:01
We just reused the libcu++ config, but going forward we want to move more common parts into a unified config.
…header`

nvc++ breaks if we put `#pragma GCC system_header` into a macro. As we really want that macro for compilers that are different than gcc we work around this by special casing nvc++ in the individual header files
@miscco miscco merged commit e6375bc into NVIDIA:main Oct 21, 2023
509 checks passed
@miscco miscco deleted the silence_warnings branch October 21, 2023 06:22
@jrhemstad jrhemstad changed the title Enable users to treat CCCL as system headers Always treat CCCL as system headers Oct 23, 2023
jrhemstad pushed a commit to jrhemstad/cccl that referenced this pull request Nov 6, 2023
* Expand the use of `system_header` in libcu++

* Add `_CCCL_IMPLICIT_SYSTEM_HEADER` to thrust headers

* Add `_CCCL_IMPLICIT_SYSTEM_HEADER` to cub headers
@jrhemstad jrhemstad mentioned this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEA]: CCCL headers should ensure users do not see warnings caused by CCCL code
6 participants