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

Non-32-bit integer overflow should be well-defined in Naga-enhanced WGSL, but Naga generates HLSL with UB #7109

Open
jimblandy opened this issue Feb 11, 2025 · 6 comments
Labels
area: correctness We're behaving incorrectly area: naga back-end Outputs of naga shader conversion lang: HLSL D3D Shading Language naga Shader Translator

Comments

@jimblandy
Copy link
Member

#7012 ensures that 32-bit signed integer overflow cannot cause UB in HLSL, but it doesn't address other integer bit widths. Official WGSL only has 32-bit integers anyway, but Naga has a 64-bit integer extension, and you could imagine a 16-bit integer extension as well. These should all work the same way as official WGSL integers do.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Feb 11, 2025

I will note that it's IB not UB. https://microsoft.github.io/hlsl-specs/specs/hlsl.html#Conv.iconv

@cwfitzgerald cwfitzgerald changed the title Non-32-bit integer overflow should be well-defined in Naga-enhanced WGSL, but Naga generates HLSL with UB Non-32-bit integer overflow should be well-defined in Naga-enhanced WGSL, but Naga generates HLSL with IB Feb 11, 2025
@jamienicol
Copy link
Contributor

jamienicol commented Feb 12, 2025

You both beat me to the punch, for filing the issue in the first place and then explaining that it's implementation-defined 😄

To clarify for future readers: as things stand we don't do any conversion, we just operate on signed types, for which we believe overflow is undefined behaviour.

For 32-bit signed integers we avoid this by using asint()/asuint() to perform the arithmetic on unsigned types and cast back to signed. But those functions only exist for 32 bit types. For other bit widths we could explicitly convert using constructors or C-style casts, eg int64_t2(uint64t_2(x) * uint64_t2(y)). The conversion from unsigned to signed is implementation defined. Maybe that's fine.

Or if we want to be fully above board we could do something like this:

int64_t to_signed(uint64_t u) {
    return u <= INT64_MAX ? int64_t(u) : -int64_t(-u);
}

I checked on godbolt and DXC seems to be smart enough to optimize that away

@jamienicol
Copy link
Contributor

Actually I think that's still UB if u == INT64_MAX + 1

@jamienicol
Copy link
Contributor

jamienicol commented Feb 12, 2025

Maybe

int64_t to_signed(uint64_t u) {
    return u <= INT64_MAX ? int64_t(u) : int64_t(u - uint64_t(INT64_MIN)) + INT64_MIN;
}

From https://stackoverflow.com/questions/13150449/efficient-unsigned-to-signed-cast-avoiding-implementation-defined-behavior

@jamienicol jamienicol changed the title Non-32-bit integer overflow should be well-defined in Naga-enhanced WGSL, but Naga generates HLSL with IB Non-32-bit integer overflow should be well-defined in Naga-enhanced WGSL, but Naga generates HLSL with UB Feb 12, 2025
@jamienicol
Copy link
Contributor

I reverted the title, as the current situation (signed overflow) is indeed UB 🙂

@cwfitzgerald
Copy link
Member

I hate all of this.

jamienicol added a commit to jamienicol/wgpu that referenced this issue Feb 13, 2025
…ivision, modulo, abs(), and unary negation

Explain we need the wrapper functions not just to avoid undefined
behaviour, but also to ensure we adhere to the WGSL spec. Additionally
link to issue gfx-rs#7109 in cases where our workaround needs follow-up work
for non-32-bit integer types.
jimblandy pushed a commit to jamienicol/wgpu that referenced this issue Feb 14, 2025
…ivision, modulo, abs(), and unary negation

Explain we need the wrapper functions not just to avoid undefined
behaviour, but also to ensure we adhere to the WGSL spec. Additionally
link to issue gfx-rs#7109 in cases where our workaround needs follow-up work
for non-32-bit integer types.
jimblandy pushed a commit that referenced this issue Feb 14, 2025
…ivision, modulo, abs(), and unary negation

Explain we need the wrapper functions not just to avoid undefined
behaviour, but also to ensure we adhere to the WGSL spec. Additionally
link to issue #7109 in cases where our workaround needs follow-up work
for non-32-bit integer types.
marcpabst pushed a commit to marcpabst/wgpu that referenced this issue Feb 19, 2025
…ivision, modulo, abs(), and unary negation

Explain we need the wrapper functions not just to avoid undefined
behaviour, but also to ensure we adhere to the WGSL spec. Additionally
link to issue gfx-rs#7109 in cases where our workaround needs follow-up work
for non-32-bit integer types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: correctness We're behaving incorrectly area: naga back-end Outputs of naga shader conversion lang: HLSL D3D Shading Language naga Shader Translator
Projects
Status: Todo
Development

No branches or pull requests

3 participants