Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 3, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e6388c6
Add C++ linkage specification to SYCL assert headers
lbushi25 Oct 1, 2024
3e80979
Add tests for assert headers linkage specifications
lbushi25 Oct 1, 2024
7b77170
Add tests for assert headers linkage specifications
lbushi25 Oct 1, 2024
73ce7a8
Merge the two tests into one
lbushi25 Oct 1, 2024
a9c030c
Update assert.h
lbushi25 Oct 2, 2024
ae60749
Update cassert
lbushi25 Oct 2, 2024
ad2bacc
Update assert_header_with_c_linkage.cpp
lbushi25 Oct 2, 2024
4868065
Update assert.h
lbushi25 Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion sycl/include/sycl/stl_wrappers/assert.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
//===----------------------------------------------------------------------===//

// Must not be guarded. C++ standard says the macro assert is redefined
// according to the current state of NDEBUG each time that <cassert> is
// according to the current state of NDEBUG each time that <assert.h> is
// included.

#if defined(__has_include_next)
Expand All @@ -16,6 +16,7 @@
#include <../ucrt/assert.h>
#endif

extern "C++" {
#ifdef __SYCL_DEVICE_ONLY__
#include <CL/__spirv/spirv_vars.hpp>

Expand All @@ -42,3 +43,4 @@ __devicelib_assert_fail(const char *, const char *, int32_t, const char *,
#endif
#endif
#endif
}
2 changes: 2 additions & 0 deletions sycl/include/sycl/stl_wrappers/cassert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <../include/cassert>
#endif

extern "C++" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@lbushi25 lbushi25 Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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++.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

#ifdef __SYCL_DEVICE_ONLY__
#include <CL/__spirv/spirv_vars.hpp>

Expand All @@ -42,3 +43,4 @@ __devicelib_assert_fail(const char *, const char *, int32_t, const char *,
#endif
#endif
#endif
}
19 changes: 19 additions & 0 deletions sycl/test/basic_tests/assert_header_with_c_linkage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// RUN: %clangxx -fsycl -DASSERT -fsyntax-only %s
// RUN: %clangxx -fsycl -DCASSERT -fsyntax-only %s

// Verify that compilation works when assert.h/cassert is wrapped by a C linkage
// specification.

#ifdef __cplusplus
extern "C" {
#endif

#if defined(ASSERT)
#include <assert.h>
#elif defined(CASSERT)
#include <cassert>
#endif

#ifdef __cplusplus
}
#endif
Loading