[SYCL] Allow kernel names in anonymous namespace #3649

srividya-sundaram · 2021-04-28T21:36:50Z

Originally, the Clang FE code rejected kernel name types declared within an anonymous namespace because those types would not be forward declarable at the global namespace scope.

However, based on a discussion between the SYCL spec authors and the Khronos group about what is meant by 'at namespace scope', it was decided that phrase means "at the same namespace as the original declaration", and so anonymous namespaces are acceptable because we can forward declare into an anonymous namespace.

Because of this relaxation in forward declaration of non-inline kernel names, it is now legal to declare a kernel name in the anonymous namespace.

AaronBallman · 2021-04-29T16:33:00Z

Can you add a description to the PR that explains why this change is happening? (This helps when we need to do a git blame for some reason.)

erichkeane · 2021-04-29T20:07:44Z

So the allowing of anonymous namespaces is NOT because of the int-footer solution, it is that we CAN forward declare anonymous namespaces (and probably should be!). There are some solutions we can do now that that will allow this, such as making sure we preserve anonymous namespaces when forward declaring types.

erichkeane

Quick look seems OK, but needs some integration-header type tests to make sure we're forward declaring correctly.

srividya-sundaram · 2021-04-29T20:17:38Z

Quick look seems OK, but needs some integration-header type tests to make sure we're forward declaring correctly.

Included a test case for kernel name in anonymous namespace in
clang/test/CodeGenSYCL/int_header1.cpp
Is that sufficient?
If not, can you elaborate what needs to be specifically checked for anonymous case?

erichkeane · 2021-04-29T20:21:39Z

Quick look seems OK, but needs some integration-header type tests to make sure we're forward declaring correctly.

Included a test case for kernel name in anonymous namespace in
clang/test/CodeGenSYCL/int_header1.cpp
Is that sufficient?
If not, can you elaborate what needs to be specifically checked for anonymous case?

Thats most of the way (and the part I was more worried about), but we also need to check to make sure that the forward declaration in the int-header is also in an anonymous namespace. I don't see any such tests in that test-file however, which is also concerning...

clang/test/CodeGenSYCL/int_header1.cpp

srividya-sundaram · 2021-04-30T05:11:06Z

So the allowing of anonymous namespaces is NOT because of the int-footer solution, it is that we CAN forward declare anonymous namespaces (and probably should be!). There are some solutions we can do now that that will allow this, such as making sure we preserve anonymous namespaces when forward declaring types.

OK. My understanding was different from the email thread.
Updated description with your comment.

AaronBallman · 2021-04-30T13:30:56Z

So the allowing of anonymous namespaces is NOT because of the int-footer solution, it is that we CAN forward declare anonymous namespaces (and probably should be!). There are some solutions we can do now that that will allow this, such as making sure we preserve anonymous namespaces when forward declaring types.

OK. My understanding was different from the email thread.
Updated description with your comment.

I think the description should be self-contained rather than having people's comments replicated in it (that gets harder to understand and makes for a pretty long commit message). How about:

Originally, the Clang FE code rejected kernel name types declared within an anonymous namespace because those types would not be forward declarable at the global namespace scope.

However, based on a discussion between the SYCL spec authors and the Khronos group about what is meant by 'at namespace scope', it was decided that phrase means "at the same namespace as the original declaration", and so anonymous namespaces are acceptable because we can forward declare into an anonymous namespace.

Because of this relaxation in forward declaration of non-inline kernel names, it is now legal to declare a kernel name in the anonymous namespace.

WDYT? (@erichkeane -- please make sure I'm not misrepresenting the decision from the email thread)

clang/test/SemaSYCL/nested-anon-and-std-ns.cpp

erichkeane · 2021-04-30T13:34:06Z

So the allowing of anonymous namespaces is NOT because of the int-footer solution, it is that we CAN forward declare anonymous namespaces (and probably should be!). There are some solutions we can do now that that will allow this, such as making sure we preserve anonymous namespaces when forward declaring types.

OK. My understanding was different from the email thread.
Updated description with your comment.

I think the description should be self-contained rather than having people's comments replicated in it (that gets harder to understand and makes for a pretty long commit message). How about:
Originally, the Clang FE code rejected kernel name types declared within an anonymous namespace because those types would not be forward declarable at the global namespace scope.

However, based on a discussion between the SYCL spec authors and the Khronos group about what is meant by 'at namespace scope', it was decided that phrase means "at the same namespace as the original declaration", and so anonymous namespaces are acceptable because we can forward declare into an anonymous namespace.

Because of this relaxation in forward declaration of non-inline kernel names, it is now legal to declare a kernel name in the anonymous namespace.
WDYT? (@erichkeane -- please make sure I'm not misrepresenting the decision from the email thread)

I like that wording, and it properly captures the discussion.

AaronBallman

Code changes LGTM, but a small testing request.

elizabethandrews

Changes LGTM other than test nit mentioned by other reviewers. Please add the tests requested for namespace hierarchy. Thanks!

srividya-sundaram · 2021-05-03T17:25:51Z

Updated test with anonymous test case at intel/llvm-test-suite: intel/llvm-test-suite#262

AaronBallman

LGTM

erichkeane

@vladimirlaz : I think by now @AaronBallman and @srividya-sundaram are the experts here :) That said, i looked over this quickly and think it seems ok.

AaronBallman · 2021-05-06T14:08:45Z

This is awaiting review from @intel/llvm-reviewers-runtime, I believe.

s-kanaev

SYCL RT test looks good.

Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.5.0 to 2.6.0. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/urllib3/urllib3/releases">urllib3's releases</a>.</em></p> <blockquote> <h2>2.6.0</h2> <h2>🚀 urllib3 is fundraising for HTTP/2 support</h2> <p><a href="https://sethmlarson.dev/urllib3-is-fundraising-for-http2-support">urllib3 is raising ~$40,000 USD</a> to release HTTP/2 support and ensure long-term sustainable maintenance of the project after a sharp decline in financial support. If your company or organization uses Python and would benefit from HTTP/2 support in Requests, pip, cloud SDKs, and thousands of other projects <a href="https://opencollective.com/urllib3">please consider contributing financially</a> to ensure HTTP/2 support is developed sustainably and maintained for the long-haul.</p> <p>Thank you for your support.</p> <h2>Security</h2> <ul> <li>Fixed a security issue where streaming API could improperly handle highly compressed HTTP content ("decompression bombs") leading to excessive resource consumption even when a small amount of data was requested. Reading small chunks of compressed data is safer and much more efficient now. (CVE-2025-66471 reported by <a href="https://github.com/Cycloctane"><code>@Cycloctane</code></a>, 8.9 High, GHSA-2xpw-w6gg-jr37)</li> <li>Fixed a security issue where an attacker could compose an HTTP response with virtually unlimited links in the <code>Content-Encoding</code> header, potentially leading to a denial of service (DoS) attack by exhausting system resources during decoding. The number of allowed chained encodings is now limited to 5. (CVE-2025-66418 reported by <a href="https://github.com/illia-v"><code>@illia-v</code></a>, 8.9 High, GHSA-gm62-xv2j-4w53)</li> </ul> <blockquote> <p>[!IMPORTANT]</p> <ul> <li>If urllib3 is not installed with the optional <code>urllib3[brotli]</code> extra, but your environment contains a Brotli/brotlicffi/brotlipy package anyway, make sure to upgrade it to at least Brotli 1.2.0 or brotlicffi 1.2.0.0 to benefit from the security fixes and avoid warnings. Prefer using <code>urllib3[brotli]</code> to install a compatible Brotli package automatically.</li> <li>If you use custom decompressors, please make sure to update them to respect the changed API of <code>urllib3.response.ContentDecoder</code>.</li> </ul> </blockquote> <h2>Features</h2> <ul> <li>Enabled retrieval, deletion, and membership testing in <code>HTTPHeaderDict</code> using bytes keys. (<a href="https://github.com/urllib3/urllib3/issues/3653">#3653</a>)</li> <li>Added host and port information to string representations of <code>HTTPConnection</code>. (<a href="https://github.com/urllib3/urllib3/issues/3666">#3666</a>)</li> <li>Added support for Python 3.14 free-threading builds explicitly. (<a href="https://github.com/urllib3/urllib3/issues/3696">#3696</a>)</li> </ul> <h2>Removals</h2> <ul> <li>Removed the <code>HTTPResponse.getheaders()</code> method in favor of <code>HTTPResponse.headers</code>. Removed the <code>HTTPResponse.getheader(name, default)</code> method in favor of <code>HTTPResponse.headers.get(name, default)</code>. (<a href="https://github.com/urllib3/urllib3/issues/3622">#3622</a>)</li> </ul> <h2>Bugfixes</h2> <ul> <li>Fixed redirect handling in <code>urllib3.PoolManager</code> when an integer is passed for the retries parameter. (<a href="https://github.com/urllib3/urllib3/issues/3649">#3649</a>)</li> <li>Fixed <code>HTTPConnectionPool</code> when used in Emscripten with no explicit port. (<a href="https://github.com/urllib3/urllib3/issues/3664">#3664</a>)</li> <li>Fixed handling of <code>SSLKEYLOGFILE</code> with expandable variables. (<a href="https://github.com/urllib3/urllib3/issues/3700">#3700</a>)</li> </ul> <h2>Misc</h2> <ul> <li>Changed the <code>zstd</code> extra to install <code>backports.zstd</code> instead of <code>zstandard</code> on Python 3.13 and before. (<a href="https://github.com/urllib3/urllib3/issues/3693">#3693</a>)</li> <li>Improved the performance of content decoding by optimizing <code>BytesQueueBuffer</code> class. (<a href="https://github.com/urllib3/urllib3/issues/3710">#3710</a>)</li> <li>Allowed building the urllib3 package with newer setuptools-scm v9.x. (<a href="https://github.com/urllib3/urllib3/issues/3652">#3652</a>)</li> <li>Ensured successful urllib3 builds by setting Hatchling requirement to ≥ 1.27.0. (<a href="https://github.com/urllib3/urllib3/issues/3638">#3638</a>)</li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/urllib3/urllib3/blob/main/CHANGES.rst">urllib3's changelog</a>.</em></p> <blockquote> <h1>2.6.0 (2025-12-05)</h1> <h2>Security</h2> <ul> <li>Fixed a security issue where streaming API could improperly handle highly compressed HTTP content ("decompression bombs") leading to excessive resource consumption even when a small amount of data was requested. Reading small chunks of compressed data is safer and much more efficient now. (<code>GHSA-2xpw-w6gg-jr37 <https://github.com/urllib3/urllib3/security/advisories/GHSA-2xpw-w6gg-jr37></code>__)</li> <li>Fixed a security issue where an attacker could compose an HTTP response with virtually unlimited links in the <code>Content-Encoding</code> header, potentially leading to a denial of service (DoS) attack by exhausting system resources during decoding. The number of allowed chained encodings is now limited to 5. (<code>GHSA-gm62-xv2j-4w53 <https://github.com/urllib3/urllib3/security/advisories/GHSA-gm62-xv2j-4w53></code>__)</li> </ul> <p>.. caution::</p> <ul> <li> <p>If urllib3 is not installed with the optional <code>urllib3[brotli]</code> extra, but your environment contains a Brotli/brotlicffi/brotlipy package anyway, make sure to upgrade it to at least Brotli 1.2.0 or brotlicffi 1.2.0.0 to benefit from the security fixes and avoid warnings. Prefer using <code>urllib3[brotli]</code> to install a compatible Brotli package automatically.</p> </li> <li> <p>If you use custom decompressors, please make sure to update them to respect the changed API of <code>urllib3.response.ContentDecoder</code>.</p> </li> </ul> <h2>Features</h2> <ul> <li>Enabled retrieval, deletion, and membership testing in <code>HTTPHeaderDict</code> using bytes keys. (<code>[#3653](urllib3/urllib3#3653) <https://github.com/urllib3/urllib3/issues/3653></code>__)</li> <li>Added host and port information to string representations of <code>HTTPConnection</code>. (<code>[#3666](urllib3/urllib3#3666) <https://github.com/urllib3/urllib3/issues/3666></code>__)</li> <li>Added support for Python 3.14 free-threading builds explicitly. (<code>[#3696](urllib3/urllib3#3696) <https://github.com/urllib3/urllib3/issues/3696></code>__)</li> </ul> <h2>Removals</h2> <ul> <li>Removed the <code>HTTPResponse.getheaders()</code> method in favor of <code>HTTPResponse.headers</code>. Removed the <code>HTTPResponse.getheader(name, default)</code> method in favor of <code>HTTPResponse.headers.get(name, default)</code>. (<code>[#3622](urllib3/urllib3#3622) <https://github.com/urllib3/urllib3/issues/3622></code>__)</li> </ul> <h2>Bugfixes</h2> <ul> <li>Fixed redirect handling in <code>urllib3.PoolManager</code> when an integer is passed for the retries parameter. (<code>[#3649](urllib3/urllib3#3649) <https://github.com/urllib3/urllib3/issues/3649></code>__)</li> <li>Fixed <code>HTTPConnectionPool</code> when used in Emscripten with no explicit port. (<code>[#3664](urllib3/urllib3#3664) <https://github.com/urllib3/urllib3/issues/3664></code>__)</li> <li>Fixed handling of <code>SSLKEYLOGFILE</code> with expandable variables. (<code>[#3700](urllib3/urllib3#3700) <https://github.com/urllib3/urllib3/issues/3700></code>__)</li> </ul>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/urllib3/urllib3/commit/720f484b605f18887a48eef448d0084e2b76902d"><code>720f484</code></a> Release 2.6.0</li> <li><a href="https://github.com/urllib3/urllib3/commit/24d7b67eac89f94e11003424bcf0d8f7b72222a8"><code>24d7b67</code></a> Merge commit from fork</li> <li><a href="https://github.com/urllib3/urllib3/commit/c19571de34c47de3a766541b041637ba5f716ed7"><code>c19571d</code></a> Merge commit from fork</li> <li><a href="https://github.com/urllib3/urllib3/commit/816fcf04528bc0f89672e13398eb813dcc892490"><code>816fcf0</code></a> Bump actions/setup-python from 6.0.0 to 6.1.0 (<a href="https://github.com/urllib3/urllib3/issues/3725">#3725</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/18af0a10efc4c99dd028f7ad5a461470b9a8b0fd"><code>18af0a1</code></a> Improve speed of <code>BytesQueueBuffer.get()</code> by using memoryview (<a href="https://github.com/urllib3/urllib3/issues/3711">#3711</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/1f6abac3e6d426c3939b8a17cf4afa099e691ab2"><code>1f6abac</code></a> Bump versions of pre-commit hooks (<a href="https://github.com/urllib3/urllib3/issues/3716">#3716</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/1c8fbf787b8e6ed151842c5d6874c9d5bdbf1d0b"><code>1c8fbf7</code></a> Bump actions/checkout from 5.0.0 to 6.0.0 (<a href="https://github.com/urllib3/urllib3/issues/3722">#3722</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/7784b9eee95b7c90802c02b111e98df70259ae4f"><code>7784b9e</code></a> Add Python 3.15 to CI (<a href="https://github.com/urllib3/urllib3/issues/3717">#3717</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/0241c9e7286d3008e3cce18effc13b40dc633385"><code>0241c9e</code></a> Updated docs to reflect change in optional zstd dependency from <code>zstandard</code> t...</li> <li><a href="https://github.com/urllib3/urllib3/commit/7afcabb6489d9a8ea95a40e5afcb46463af17351"><code>7afcabb</code></a> Expand environment variable of SSLKEYLOGFILE (<a href="https://github.com/urllib3/urllib3/issues/3705">#3705</a>)</li> <li>Additional commits viewable in <a href="https://github.com/urllib3/urllib3/compare/2.5.0...2.6.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=urllib3&package-manager=pip&previous-version=2.5.0&new-version=2.6.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/intel/llvm/network/alerts). </details> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

srividya-sundaram added 2 commits April 28, 2021 13:05

[SYCL] Allow kernel names in anonymous namespace

98a1d7d

update test

d75185f

srividya-sundaram changed the title ~~Allow anon ns~~ Allow kernel names in anonymous namespace Apr 29, 2021

srividya-sundaram marked this pull request as ready for review April 29, 2021 04:42

srividya-sundaram requested review from a team, AaronBallman, elizabethandrews and premanandrao as code owners April 29, 2021 04:42

srividya-sundaram requested a review from sergey-semenov April 29, 2021 04:42

bader changed the title ~~Allow kernel names in anonymous namespace~~ [SYCL] Allow kernel names in anonymous namespace Apr 29, 2021

erichkeane reviewed Apr 29, 2021

View reviewed changes

Update int_header1.cpp test

4b26c05

erichkeane reviewed Apr 29, 2021

View reviewed changes

clang/test/CodeGenSYCL/int_header1.cpp Show resolved Hide resolved

Add check-next to int_header1.cpp

c197614

AaronBallman reviewed Apr 30, 2021

View reviewed changes

clang/test/SemaSYCL/nested-anon-and-std-ns.cpp Show resolved Hide resolved

AaronBallman reviewed Apr 30, 2021

View reviewed changes

elizabethandrews reviewed Apr 30, 2021

View reviewed changes

Update test with nested anon ns cases

705af36

srividya-sundaram mentioned this pull request May 3, 2021

[SYCL][LIT] Update test with anonymous case intel/llvm-test-suite#262

Merged

srividya-sundaram requested review from AaronBallman and elizabethandrews May 3, 2021 22:05

AaronBallman approved these changes May 4, 2021

View reviewed changes

vladimirlaz requested a review from erichkeane May 6, 2021 04:47

erichkeane approved these changes May 6, 2021

View reviewed changes

s-kanaev approved these changes May 6, 2021

View reviewed changes

vladimirlaz merged commit e47dbad into intel:sycl May 6, 2021

[SYCL] Allow kernel names in anonymous namespace #3649

[SYCL] Allow kernel names in anonymous namespace #3649

Uh oh!

Conversation

srividya-sundaram commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Apr 29, 2021

Uh oh!

erichkeane commented Apr 29, 2021

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

srividya-sundaram commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane commented Apr 29, 2021

Uh oh!

Uh oh!

srividya-sundaram commented Apr 30, 2021

Uh oh!

AaronBallman commented Apr 30, 2021

Uh oh!

Uh oh!

erichkeane commented Apr 30, 2021

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

srividya-sundaram commented May 3, 2021

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented May 6, 2021

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

srividya-sundaram commented Apr 28, 2021 •

edited

Loading

srividya-sundaram commented Apr 29, 2021 •

edited

Loading