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

Strengthen address space pointer checks #1963

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

silverclaw
Copy link
Contributor

Add negative tests for address space casts.

Add negative tests for address space casts.
@svenvh
Copy link
Member

svenvh commented May 10, 2024

Formatting violations should be ignored here.

@lakshmih
Copy link
Contributor

Reviewing

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

I'm not convinced these "negative tests" are possible without a spec clarification.

These to_xxx functions currently say, for example:

Returns a pointer that points to a region in the global address space if to_global can cast ptr to the global address space. Otherwise it returns NULL.

... but they don't make any guarantees when the pointer cannot be converted to the requested address space. You could imagine some CPU devices, for example, where a generic pointer could be converted to any arbitrary address space and, at least, be safely dereferenced.

FWIW, the same is true for the SPIR-V instruction OpGenericCastToPtrExplicit, which says:

[...] If the cast fails, the instruction result is an OpConstantNull pointer in the Storage Storage Class.

... but never says when the cast may / must / should "fail". See internal SPIR-V GitLab issue 741.

To be clear, I think it would be great to strengthen the spec so we can add these tests also, but I think we'll need the spec change first.

@lakshmih
Copy link
Contributor

Reviewing

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Removing "focused review" while we are discussing related spec changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants