[SYCL] Add C++ linkage specification in SYCL assert headers #15570

lbushi25 · 2024-10-01T17:03:45Z

Some SYCL applications are seeing compilation failures related to the SYCL assert headers. Specifically, they are wrapping the SYCL assert headers in C linkage specifications in their applications. These headers in turn include files that contain templates which emit errors under C linkage specifications so this PR explicitly adds C++ linkage specifications in the header contents to override any extra specification that the clients may add. Here is an example of faulty client code:

#ifdef __cplusplus
extern "C" {
#endif

#include <assert.h>

#ifdef __cplusplus
}
#endif

int main() {}

Error fragments:

/nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/CL/__spirv/spirv_types.hpp:140:1: error: templates must have C++ linkage
  140 | template <typename T, std::size_t R, std::size_t C, MatrixLayout L,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  141 |           Scope::Flag S = Scope::Flag::Subgroup,
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  142 |           MatrixUse U = MatrixUse::MatrixA>
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../test.cpp:2:1: note: extern "C" language linkage specification begins here
    2 | extern "C" {
      | ^
In file included from ../test.cpp:5:
In file included from /nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/sycl/stl_wrappers/assert.h:21:
In file included from /nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/CL/__spirv/spirv_vars.hpp:13:
/nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/CL/__spirv/spirv_types.hpp:156:1: error: templates must have C++ linkage
  156 | template <typename dataT>
      | ^~~~~~~~~~~~~~~~~~~~~~~~~
../test.cpp:2:1: note: extern "C" language linkage specification begins here
    2 | extern "C" {
      | ^
In file included from ../test.cpp:5:
In file included from /nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/sycl/stl_wrappers/assert.h:21:
In file included from /nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/CL/__spirv/spirv_vars.hpp:13:
/nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/CL/__spirv/spirv_types.hpp:158:1: error: templates must have C++ linkage
  158 | template <typename dataT>
      | ^~~~~~~~~~~~~~~~~~~~~~~~~
../test.cpp:2:1: note: extern "C" language linkage specification begins here
    2 | extern "C" {
      | ^
In file included from ../test.cpp:5:
In file included from /nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/sycl/stl_wrappers/assert.h:21:
In file included from /nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/CL/__spirv/spirv_vars.hpp:13:
/nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/CL/__spirv/spirv_types.hpp:162:1: error: templates must have C++ linkage
  162 | template <typename dataT, int dims>
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../test.cpp:2:1: note: extern "C" language linkage specification begins here
    2 | extern "C" {
      | ^
In file included from ../test.cpp:5:
In file included from /nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/sycl/stl_wrappers/assert.h:21:
In file included from /nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/CL/__spirv/spirv_vars.hpp:13:
/nfs/site/home/lbushi/sycl_workspace/llvm/build/bin/../include/CL/__spirv/spirv_types.hpp:177:1: error: templates must have C++ linkage
  177 | template <int Bits> using ap_int = _BitInt(Bits);
      | ^~~~~~~~~~~~~~~~~~~
../test.cpp:2:1: note: extern "C" language linkage specification begins here
    2 | extern "C" {
      | ^

AlexeySachkov

The example from the commit message should be moved into sycl/test

sycl/include/sycl/stl_wrappers/assert.h

sycl/test/basic_tests/assert_header_with_c_linkage.cpp

aelovikov-intel · 2024-10-04T17:35:21Z

sycl/include/sycl/stl_wrappers/cassert

@@ -16,6 +16,7 @@
 #include <../include/cassert>
 #endif

+extern "C++" {


Do we really need this? <cassert> is a C++ header, not C.

I think something like

#ifdef __cplusplus extern "C" { #endif #include <cassert> #ifdef __cplusplus } #endif int main() {}

is well-defined code and without the changes, it fails to compile.
Also note that the external linkage specification is after the actual C++ STL header <cassert> is included so we're not actually changing anything in the STL header contents.

@tahonermann , @AaronBallman , is the above code really well-defined?

I don't think including a C++ standard library header in a linkage specification block is well-formed.

What problem is the extern "C++" declaration intended to solve? If the problem is that code somewhere else is including the <cassert> header inside of an extern "C" declaration then that is the bug that should be fixed.

Note that, per [support.c.headers.general]p1, there is no need to include C library headers within a extern "C" block when compiling as C++.

I might be wrong, but I think libstdc++ itself might be doing something like that.

Since libstdc++ is an implementation of the C++ standard library, it is allowed to do things that user code is not. If it "knows" that the header it is including is a C header that doesn't itself place its contents in an extern "C" block, then it makes since for it to do so as part of wrapping that header (e.g., inside the <cassert> header).

Note that, per [support.c.headers.general]p1, there is no need to include C library headers within a extern "C" block when compiling as C++.

"there is no need" doesn't mean "it's prohibited", right?

It also isn't right to include C headers in an extern "C" block when compiling as C++.
Can the clients that are including C headers be fixed to not do so within a extern "C" block?

Do you have a link to the C++ draft section which says this? I initially wanted to push back on the bug report to make customer rewrite their code, but I haven't been able to find a C++ spec section which would pronounce this ill-formed

I'm not aware of wording in the C++ standard that explicitly makes inclusion of a C header within an extern "C" block ill-formed. My interpretation of such inclusion being ill-formed comes from the absence of constraints that would prohibit use of C++ features in the C++ standard library implementation of the C headers.

As an example where C++ features are used in the implementation of C headers provided by a C++ standard library implementation, see the source code for libstdc++. It provides a set of C compatibility headers that are wrappers around the C++ equivalents. Specifically, see its implementation of the stdlib.h header; it unconditionally uses C++ using declarations. Whether these compatibility headers are used is a decision made when gcc is configured and built and depends on the availability of a C library implementation.

This [PR](#15570) added C++ linkage specifications for the SYCL wrappers around the STL assert headers `assert.h` and `cassert`. For `cassert`, this is actually superfluous since it is purely a C++ header. This PR removes it.

Add C++ linkage specification to SYCL assert headers

e6388c6

lbushi25 temporarily deployed to WindowsCILock October 1, 2024 17:04 — with GitHub Actions Inactive

lbushi25 marked this pull request as ready for review October 1, 2024 17:06

lbushi25 requested a review from a team as a code owner October 1, 2024 17:06

lbushi25 requested review from againull and AlexeySachkov October 1, 2024 17:06

lbushi25 temporarily deployed to WindowsCILock October 1, 2024 17:31 — with GitHub Actions Inactive

AlexeySachkov requested changes Oct 1, 2024

View reviewed changes

Add tests for assert headers linkage specifications

3e80979

lbushi25 had a problem deploying to WindowsCILock October 1, 2024 19:12 — with GitHub Actions Error

Add tests for assert headers linkage specifications

7b77170

lbushi25 had a problem deploying to WindowsCILock October 1, 2024 19:14 — with GitHub Actions Error

Merge the two tests into one

73ce7a8

lbushi25 temporarily deployed to WindowsCILock October 1, 2024 19:24 — with GitHub Actions Inactive

lbushi25 requested a review from AlexeySachkov October 1, 2024 19:48

lbushi25 temporarily deployed to WindowsCILock October 1, 2024 19:53 — with GitHub Actions Inactive

AlexeySachkov reviewed Oct 2, 2024

View reviewed changes

sycl/include/sycl/stl_wrappers/assert.h Outdated Show resolved Hide resolved

AlexeySachkov reviewed Oct 2, 2024

View reviewed changes

sycl/test/basic_tests/assert_header_with_c_linkage.cpp Outdated Show resolved Hide resolved

lbushi25 added 3 commits October 2, 2024 08:41

Update assert.h

a9c030c

Update cassert

ae60749

Update assert_header_with_c_linkage.cpp

ad2bacc

lbushi25 had a problem deploying to WindowsCILock October 2, 2024 12:42 — with GitHub Actions Error

lbushi25 had a problem deploying to WindowsCILock October 2, 2024 12:43 — with GitHub Actions Error

Update assert.h

4868065

lbushi25 temporarily deployed to WindowsCILock October 2, 2024 12:43 — with GitHub Actions Inactive

lbushi25 requested a review from AlexeySachkov October 2, 2024 12:44

lbushi25 temporarily deployed to WindowsCILock October 2, 2024 13:07 — with GitHub Actions Inactive

AlexeySachkov approved these changes Oct 2, 2024

View reviewed changes

againull approved these changes Oct 3, 2024

View reviewed changes

againull merged commit 208ec48 into intel:sycl Oct 3, 2024
12 checks passed

aelovikov-intel reviewed Oct 4, 2024

View reviewed changes

This was referenced Oct 4, 2024

[SYCL] Remove superfluous C++ linkage specification from cassert header #15613

Closed

[SYCL] Remove superfluous C++ linkage spec from assert headers #15614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Add C++ linkage specification in SYCL assert headers #15570

[SYCL] Add C++ linkage specification in SYCL assert headers #15570

Uh oh!

lbushi25 commented Oct 1, 2024 •

edited

Loading

Uh oh!

AlexeySachkov left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aelovikov-intel Oct 4, 2024

Uh oh!

lbushi25 Oct 4, 2024 •

edited

Loading

Uh oh!

aelovikov-intel Oct 4, 2024

Uh oh!

tahonermann Oct 4, 2024

Uh oh!

tahonermann Oct 4, 2024

Uh oh!

aelovikov-intel Oct 4, 2024

Uh oh!

tahonermann Oct 4, 2024

Uh oh!

AlexeySachkov Oct 5, 2024

Uh oh!

tahonermann Oct 8, 2024

Uh oh!

tahonermann Oct 8, 2024

Uh oh!

Uh oh!

[SYCL] Add C++ linkage specification in SYCL assert headers #15570

[SYCL] Add C++ linkage specification in SYCL assert headers #15570

Uh oh!

Conversation

lbushi25 commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbushi25 Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lbushi25 commented Oct 1, 2024 •

edited

Loading

lbushi25 Oct 4, 2024 •

edited

Loading