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

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

Closed
4 tasks done
jrhemstad opened this issue Oct 6, 2023 · 4 comments · Fixed by #531
Closed
4 tasks done

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

jrhemstad opened this issue Oct 6, 2023 · 4 comments · Fixed by #531
Assignees
Labels
cub For all items related to CUB feature request New feature or request. libcu++ For all items related to libcu++ thrust For all items related to Thrust.

Comments

@jrhemstad
Copy link
Collaborator

jrhemstad commented Oct 6, 2023

Is this a duplicate?

Area

General CCCL

Is your feature request related to a problem? Please describe.

As a user of CCCL, when I include a CCCL header like #include <cub/cub.cuh> and compile my program, I do not want to see warnings emitted from the cub.cuh header. In addition to being annoying, I may compile with -werror that turns all warnings into errors and such warnings would break my build.

The normal solution to this problem is to use -isystem for the include search path:

...GCC gives code found in system headers special treatment. All warnings, other than those generated by ‘#warning’, are suppressed while GCC is processing a system header.

Header files found in directories added to the search path with the -isystem and -idirafter command-line options are treated as system headers for the purposes of diagnostics.

So including headers with -isystem will silence any warnings.

However, -isystem does not work when compiling with nvcc with any headers that are shipped in the CTK.

This is because nvcc implicitly includes the CTK include dir as -I. This can be seen by inspecting the verbose output from nvcc -v.

nvcc -v ./test.cu -isystem=.

yields

gcc "-I/usr/local/cuda/bin/../targets/x86_64-linux/include"   -isystem "." 

-I paths are searched before isystem paths, so the headers from the CTK will always be found first.

Luckily there is another solution:

There is also a directive, #pragma GCC system_header, which tells GCC to consider the rest of the current include file a system header, no matter where it was found. Code that comes before the ‘#pragma’ in the file is not affected. #pragma GCC system_header has no effect in the primary source file.

gcc, clang, and msvc all support some form of this same directive:

Note, using this #pragma in a header will not leak to the file that included it. It applies only to the file in which the pragma resides.

Therefore, if we add these pragmas to the beginning of every CCCL header, then it will automatically silence warnings from our headers.

Note
This pragma will not resolve the fact that using nvcc -isystem=path/to/local/cccl will still cause CTK headers to override local headers. However, there will be far less reason for anyone to try and include CCCL headers as isystem if we add this pragma.

Describe the solution you'd like

We should define a new macro like (naming is just for illustration and can be changed):

// Check if the disable flag isn't set
#ifndef CCCL_DISABLE_IMPLICIT_SYSTEM_HEADER

// Check for GCC (probably needs version checks)
#if defined(__GNUC__) && !defined(__clang__)
    #define CCCL_IMPLICIT_SYSTEM_HEADER #pragma GCC system_header

// Check for Clang (probably needs version checks)
#elif defined(__clang__) 
    #define CCCL_IMPLICIT_SYSTEM_HEADER #pragma clang system_header

// Check for MSVC (probably needs version checks)
#elif defined(_MSC_VER) 
    #define CCCL_IMPLICIT_SYSTEM_HEADER #pragma system_header

// If none of the known compilers are detected, define it as an empty statement
#else
    #define CCCL_IMPLICIT_SYSTEM_HEADER
#endif

// If the disable flag is set, define it as an empty statement
#else
    #define CCCL_IMPLICIT_SYSTEM_HEADER
#endif

And use this at the top of every CCCL header file. It must be used directly in each file since it does not impact any files other than the one in which it is used.

It should be opt-out with CCCL_DISABLE_IMPLICIT_SYSTEM_HEADER so we can still have warnings enabled when we compile our own tests.

libcudacxx headers already use #pragma GCC system_header directly:

So we should update this, and add it to Thrust and CUB headers as well.

Tasks

Describe alternatives you've considered

No response

Additional context

It's likely we'll have other scenarios where we want to insert stuff at the top of every CCCL header file. Therefore, we could also consider adding a higher-level macro like:

#define CCCL_DETAIL FILE_BEGIN \
    CCCL_IMPLICIT_SYSTEM_HEADER

Additionally, I thought an easy way we could sneak in the CCCL_IMPLICIT_SYSTEM_HEADER macro without having to manually touch every file would be to just prepend it to our namespace macros like so:

#define _LIBCUDACXX_BEGIN_NAMESPACE_STD \
    CCCL_IMPLICIT_SYSTEM_HEADER \
    _LIBCUDACXX_BEGIN_NAMESPACE_STD_NOVERSION inline namespace _LIBCUDACXX_ABI_NAMESPACE {

but that expands out to:

 #pragma GCC system_header inline namespace 

which fails to compile. I don't think there's a way to generate a new line as part of a macro expansion.

@jrhemstad jrhemstad added the feature request New feature or request. label Oct 6, 2023
@github-project-automation github-project-automation bot moved this to Todo in CCCL Oct 6, 2023
@jrhemstad
Copy link
Collaborator Author

After sleeping on it, CCCL_SILENCE_WARNINGS is probably a more descriptive name for what the macro is actually doing.

@miscco
Copy link
Collaborator

miscco commented Oct 9, 2023

We currently only guard against gcc for that in libcu++:

#if defined(_LIBCUDACXX_USE_PRAGMA_GCC_SYSTEM_HEADER)
#  pragma GCC system_header
#endif

We can happily expand this for other compilers

@miscco miscco self-assigned this Oct 9, 2023
@miscco miscco added thrust For all items related to Thrust. cub For all items related to CUB libcu++ For all items related to libcu++ labels Oct 9, 2023
@jrhemstad
Copy link
Collaborator Author

We can happily expand this for other compilers

Exactly, and for all other CCCL headers.

@miscco
Copy link
Collaborator

miscco commented Oct 10, 2023

I went with CCCL_AS_SYSTEM_HEADER which I believe is a bit more descriptive

@cccl-authenticator-app cccl-authenticator-app bot moved this from Todo to In Review in CCCL Oct 10, 2023
@github-project-automation github-project-automation bot moved this from In Review to Done in CCCL Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cub For all items related to CUB feature request New feature or request. libcu++ For all items related to libcu++ thrust For all items related to Thrust.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants