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

[BUG]: To avoid GCC to fail in preprocessing the __concept header files using "-C" option #1927

Closed
1 task done
zkhatami opened this issue Jul 1, 2024 · 3 comments · Fixed by #1931
Closed
1 task done
Assignees
Labels
bug Something isn't working right. libcu++ For all items related to libcu++

Comments

@zkhatami
Copy link

zkhatami commented Jul 1, 2024

Is this a duplicate?

Type of Bug

Compile-time Error

Component

libcu++

Describe the bug

For this test:

#include "cuda/std/atomic"

GCC started failing during preprocessing with the "-C" option after this commit:
https://github.com/NVIDIA/cccl/commit/680c01207c8611c011ba6eed4ba30cb0c8ab2d20#diff-97033c0ba44556602cf7d7eeda4fa26ead8d8ba9ee63b55be99ae263194e124a

This bug in GCC is triggered by the new changes in the __concept header files when using the "-C" option.
For example, for this definition of the Movable in cuda12.3 we have:

template<class _Tp>
_LIBCUDACXX_CONCEPT_FRAGMENT(
  _Movable_,
  requires()
  (requires(is_object_v<_Tp>),
   requires(move_constructible<_Tp>),
   requires(assignable_from<_Tp&, _Tp>),
   requires(swappable<_Tp>)));

, while with cuda12.4, we have:

_LIBCUDACXX_CONCEPT_FRAGMENT(
  _Movable_,
  requires()(// <---
    requires(is_object_v<_Tp>),
    requires(move_constructible<_Tp>),
    requires(assignable_from<_Tp&, _Tp>),
    requires(swappable<_Tp>)
  ));

Having the comment "//" within the code causes GCC to fail in the preprocessing phase when using "-C" option:

__concepts/movable.h:46:1: error: pasting "_LIBCUDACXX_CONCEPT_FRAGMENT_REQS_SELECT_PROBE_" and "/**/" does not give a valid preprocessing token
   46 | _LIBCUDACXX_CONCEPT_FRAGMENT(
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Here is the smaller reproducer that confirms this bug in GCC preprocessing when using "-C" option:

#define CONCAT(A, B) A ## B
/* comment */
int CONCAT(ma, /**/ in)() { }

$ g++ -E -C ec.cpp > /dev/null
ec.cpp:3:12: error: pasting "ma" and "/**/" does not give a valid preprocessing token
    3 | int CONCAT(ma, /**/ in)() { }
      |            ^~
ec.cpp:1:22: note: in definition of macro ‘CONCAT’
    1 | #define CONCAT(A, B) A ## B
      |                      

In the commit that CCCL has made, there is no explanation why the comment was added to those __concept header files. If the comment gets eliminated, then GCC would compile this test fine.

How to Reproduce

g++ -E -C ec.cpp

Expected behavior

To not failing in the preprocessing phase.

Reproduction link

No response

Operating System

No response

nvidia-smi output

No response

NVCC version

No response

@zkhatami zkhatami added the bug Something isn't working right. label Jul 1, 2024
@bernhardmgruber
Copy link
Contributor

bernhardmgruber commented Jul 2, 2024

Thank you for reporting this! I assume @miscco added the comments for guiding clang-format into a more pleasant formatting. Maybe replacing the // by a simple /**/ would work.

@bernhardmgruber bernhardmgruber added the libcu++ For all items related to libcu++ label Jul 2, 2024
@dkolsen-pgi
Copy link
Collaborator

Replacing // with /**/ won't help. GCC chokes when a comment of any form is in the middle of a token concatenation when comments are preserved by the preprocessor (the -C option).

miscco added a commit to miscco/cccl that referenced this issue Jul 2, 2024
…within macros.

Those comments were ment to force clang-format into doing not so bad things with the concept emulation.

However, with the recent versions of clang-format this is not necessary anymore

Fixes NVIDIA#1927
@miscco
Copy link
Collaborator

miscco commented Jul 2, 2024

Those comments were used to force clang-format into submission as it completely mangled these pieces of code.

With our current version this does not happen anymore, so we can drop them alltogether

@miscco miscco linked a pull request Jul 2, 2024 that will close this issue
miscco added a commit to miscco/cccl that referenced this issue Jul 2, 2024
…within macros.

Those comments were ment to force clang-format into doing not so bad things with the concept emulation.

However, with the recent versions of clang-format this is not necessary anymore

Fixes NVIDIA#1927
miscco added a commit to miscco/cccl that referenced this issue Jul 2, 2024
…within macros.

Those comments were ment to force clang-format into doing not so bad things with the concept emulation.

However, with the recent versions of clang-format this is not necessary anymore

Fixes NVIDIA#1927
miscco added a commit to miscco/cccl that referenced this issue Jul 3, 2024
…within macros.

Those comments were ment to force clang-format into doing not so bad things with the concept emulation.

However, with the recent versions of clang-format this is not necessary anymore

Fixes NVIDIA#1927
miscco added a commit that referenced this issue Jul 3, 2024
* It seems that the gcc proprocessor at times has issues with comments within macros.

Those comments were ment to force clang-format into doing not so bad things with the concept emulation.

However, with the recent versions of clang-format this is not necessary anymore

Fixes #1927
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right. libcu++ For all items related to libcu++
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants