Repository: gfx-rs/wgpu
Pull request #6438 (2024-10-22 13:30:13)

fix(msl-out): use `namer` for `<fun>{Input,Output}` structs
**Connections**

- This appears to be a long-standing issue in the Metal backend.
- Discovered with [bug 1870208, comment 3](https://bugzilla.mozilla.org/show_bug.cgi?id=1870208#c3), which ~~had the audacity to use~~ uses the names `vs` for an entrypoint with a local variable called `vsOutput`.

**Description**

Names generated for `<fun>{Input,Output}` structures in the Metal backend are not escaped using the `namer` API. This means the naming does not avoid conflicts with shader-provided identifiers that may use the same names.

Fix this by using the `namer` API.

**Testing**

- One can reproduce this issue by using the following shader in a render pipeline:

	```wgsl
	  struct OurVertexShaderOutput {
	    @builtin(position) position: vec4f,
	    @location(0) texcoord: vec2f,
	  };
	
	  @vertex fn vs(
	    @location(0) xy: vec2f
	  ) -> OurVertexShaderOutput {
	    var vsOutput: OurVertexShaderOutput;
	    vsOutput.position = vec4f(xy, 0.0, 1.0);
	    return vsOutput;
	  }
	
	  @fragment fn fs() -> @location(0) vec4f {
	    return vec4f(1.0, 0.0, 0.0, 1.0);
	  }
	```
	
	When the pipeline is created, this (rather confusing) Metal compiler error is emitted:
	
	```
	Uncaptured WebGPU error: Internal error in ShaderStages(VERTEX) shader: Metal: program_source:44:12: error: no viable conversion from returned value of type 'OurVertexShaderOutput' to function return type 'vsOutput'
	    return vsOutput { _tmp.position, _tmp.texcoord };
	           ^~~~~~~~
	program_source:25:8: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'OurVertexShaderOutput' to 'const vsOutput &' for 1st argument
	struct vsOutput {
	       ^
	program_source:25:8: note: candidate c
	```

- A regression test has been added using the above case, whose snapshot output is adjusted with this fix.

- I've consumed this change in Firefox, and validated that the issue noted at <https://webgpufundamentals.org/webgpu/lessons/webgpu-fundamentals.html#drawing-triangles-to-textures> is fixed.

**Checklist**

- [x] Run `cargo fmt`.
- [x] Run `taplo format`.
- [x] Run `cargo clippy`. If applicable, add:
  - [x] `--target wasm32-unknown-unknown`
  - [x] `--target wasm32-unknown-emscripten`
- [x] Run `cargo xtask test` to run tests.
- [x] Add change to `CHANGELOG.md`. See simple instructions inside file.


ErichDonGubler: I didn't delve into `naga`-repo-era history, but it seems the most recent author to touch the code in question is @jimblandy. Tagging for his awareness; let's be more suspicious of identifiers that don't go through the `namer`.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 16d160d5b..fc24c146b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -118,6 +118,7 @@ By @bradwerth [#6216](https://github.com/gfx-rs/wgpu/pull/6216).
 - Added `gl_DrawID` to glsl and `DrawIndex` to spv. By @ChosenName in [#6325](https://github.com/gfx-rs/wgpu/pull/6325).
 - Matrices can now be indexed by value (#4337), and indexing arrays by value no longer causes excessive spilling (#6358). By @jimblandy in [#6390](https://github.com/gfx-rs/wgpu/pull/6390).
 - Add support for `textureQueryLevels` to the GLSL parser. By @magcius in [#6325](https://github.com/gfx-rs/wgpu/pull/6415).
+- Fix unescaped identifiers in the Metal backend shader I/O structures causing shader miscompilation. By @ErichDonGubler in [#6438](https://github.com/gfx-rs/wgpu/pull/6438).
 
 #### General
 
diff --git a/naga/src/back/msl/writer.rs b/naga/src/back/msl/writer.rs
index 06d23d014..458214b23 100644
--- a/naga/src/back/msl/writer.rs
+++ b/naga/src/back/msl/writer.rs
@@ -5063,7 +5063,7 @@ template <typename A>
             // a struct type named `<fun>Input` to hold them. If we are doing
             // vertex pulling, we instead update our attribute mapping to
             // note the types, names, and zero values of the attributes.
-            let stage_in_name = format!("{fun_name}Input");
+            let stage_in_name = self.namer.call(&format!("{fun_name}Input"));
             let varyings_member_name = self.namer.call("varyings");
             let mut has_varyings = false;
             if !flattened_arguments.is_empty() {
@@ -5115,7 +5115,7 @@ template <typename A>
 
             // Define a struct type named for the return value, if any, named
             // `<fun>Output`.
-            let stage_out_name = format!("{fun_name}Output");
+            let stage_out_name = self.namer.call(&format!("{fun_name}Output"));
             let result_member_name = self.namer.call("member");
             let result_type_name = match fun.result {
                 Some(ref result) => {


Test patch:
diff --git a/naga/tests/in/6438-conflicting-idents.wgsl b/naga/tests/in/6438-conflicting-idents.wgsl
new file mode 100644
index 000000000..2e4598be2
--- /dev/null
+++ b/naga/tests/in/6438-conflicting-idents.wgsl
@@ -0,0 +1,16 @@
+struct OurVertexShaderOutput {
+    @builtin(position) position: vec4f,
+    @location(0) texcoord: vec2f,
+};
+
+@vertex fn vs(
+    @location(0) xy: vec2f
+) -> OurVertexShaderOutput {
+    var vsOutput: OurVertexShaderOutput;
+    vsOutput.position = vec4f(xy, 0.0, 1.0);
+    return vsOutput;
+}
+
+@fragment fn fs() -> @location(0) vec4f {
+    return vec4f(1.0, 0.0, 0.0, 1.0);
+}
diff --git a/naga/tests/out/glsl/6438-conflicting-idents.fs.Fragment.glsl b/naga/tests/out/glsl/6438-conflicting-idents.fs.Fragment.glsl
new file mode 100644
index 000000000..d5d8fc7b5
--- /dev/null
+++ b/naga/tests/out/glsl/6438-conflicting-idents.fs.Fragment.glsl
@@ -0,0 +1,16 @@
+#version 310 es
+
+precision highp float;
+precision highp int;
+
+struct OurVertexShaderOutput {
+    vec4 position;
+    vec2 texcoord;
+};
+layout(location = 0) out vec4 _fs2p_location0;
+
+void main() {
+    _fs2p_location0 = vec4(1.0, 0.0, 0.0, 1.0);
+    return;
+}
+
diff --git a/naga/tests/out/glsl/6438-conflicting-idents.vs.Vertex.glsl b/naga/tests/out/glsl/6438-conflicting-idents.vs.Vertex.glsl
new file mode 100644
index 000000000..7937cba42
--- /dev/null
+++ b/naga/tests/out/glsl/6438-conflicting-idents.vs.Vertex.glsl
@@ -0,0 +1,23 @@
+#version 310 es
+
+precision highp float;
+precision highp int;
+
+struct OurVertexShaderOutput {
+    vec4 position;
+    vec2 texcoord;
+};
+layout(location = 0) in vec2 _p2vs_location0;
+layout(location = 0) smooth out vec2 _vs2fs_location0;
+
+void main() {
+    vec2 xy = _p2vs_location0;
+    OurVertexShaderOutput vsOutput = OurVertexShaderOutput(vec4(0.0), vec2(0.0));
+    vsOutput.position = vec4(xy, 0.0, 1.0);
+    OurVertexShaderOutput _e6 = vsOutput;
+    gl_Position = _e6.position;
+    _vs2fs_location0 = _e6.texcoord;
+    gl_Position.yz = vec2(-gl_Position.y, gl_Position.z * 2.0 - gl_Position.w);
+    return;
+}
+
diff --git a/naga/tests/out/hlsl/6438-conflicting-idents.hlsl b/naga/tests/out/hlsl/6438-conflicting-idents.hlsl
new file mode 100644
index 000000000..179ac8ec8
--- /dev/null
+++ b/naga/tests/out/hlsl/6438-conflicting-idents.hlsl
@@ -0,0 +1,25 @@
+struct OurVertexShaderOutput {
+    float4 position : SV_Position;
+    float2 texcoord : LOC0;
+};
+
+struct VertexOutput_vs {
+    float2 texcoord : LOC0;
+    float4 position : SV_Position;
+};
+
+VertexOutput_vs vs(float2 xy : LOC0)
+{
+    OurVertexShaderOutput vsOutput = (OurVertexShaderOutput)0;
+
+    vsOutput.position = float4(xy, 0.0, 1.0);
+    OurVertexShaderOutput _e6 = vsOutput;
+    const OurVertexShaderOutput ourvertexshaderoutput = _e6;
+    const VertexOutput_vs ourvertexshaderoutput_1 = { ourvertexshaderoutput.texcoord, ourvertexshaderoutput.position };
+    return ourvertexshaderoutput_1;
+}
+
+float4 fs() : SV_Target0
+{
+    return float4(1.0, 0.0, 0.0, 1.0);
+}
diff --git a/naga/tests/out/hlsl/6438-conflicting-idents.ron b/naga/tests/out/hlsl/6438-conflicting-idents.ron
new file mode 100644
index 000000000..393d3d674
--- /dev/null
+++ b/naga/tests/out/hlsl/6438-conflicting-idents.ron
@@ -0,0 +1,16 @@
+(
+    vertex:[
+        (
+            entry_point:"vs",
+            target_profile:"vs_5_1",
+        ),
+    ],
+    fragment:[
+        (
+            entry_point:"fs",
+            target_profile:"ps_5_1",
+        ),
+    ],
+    compute:[
+    ],
+)
diff --git a/naga/tests/out/msl/6438-conflicting-idents.msl b/naga/tests/out/msl/6438-conflicting-idents.msl
new file mode 100644
index 000000000..596ebad40
--- /dev/null
+++ b/naga/tests/out/msl/6438-conflicting-idents.msl
@@ -0,0 +1,37 @@
+// language: metal1.0
+#include <metal_stdlib>
+#include <simd/simd.h>
+
+using metal::uint;
+
+struct OurVertexShaderOutput {
+    metal::float4 position;
+    metal::float2 texcoord;
+};
+
+struct vsInput {
+    metal::float2 xy [[attribute(0)]];
+};
+struct vsOutput_1 {
+    metal::float4 position [[position]];
+    metal::float2 texcoord [[user(loc0), center_perspective]];
+};
+vertex vsOutput_1 vs(
+  vsInput varyings [[stage_in]]
+) {
+    const auto xy = varyings.xy;
+    OurVertexShaderOutput vsOutput = {};
+    vsOutput.position = metal::float4(xy, 0.0, 1.0);
+    OurVertexShaderOutput _e6 = vsOutput;
+    const auto _tmp = _e6;
+    return vsOutput_1 { _tmp.position, _tmp.texcoord };
+}
+
+
+struct fsOutput {
+    metal::float4 member_1 [[color(0)]];
+};
+fragment fsOutput fs(
+) {
+    return fsOutput { metal::float4(1.0, 0.0, 0.0, 1.0) };
+}
diff --git a/naga/tests/out/spv/6438-conflicting-idents.spvasm b/naga/tests/out/spv/6438-conflicting-idents.spvasm
new file mode 100644
index 000000000..589eeb346
--- /dev/null
+++ b/naga/tests/out/spv/6438-conflicting-idents.spvasm
@@ -0,0 +1,60 @@
+; SPIR-V
+; Version: 1.1
+; Generator: rspirv
+; Bound: 36
+OpCapability Shader
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Vertex %15 "vs" %8 %11 %13
+OpEntryPoint Fragment %33 "fs" %32
+OpExecutionMode %33 OriginUpperLeft
+OpMemberDecorate %6 0 Offset 0
+OpMemberDecorate %6 1 Offset 16
+OpDecorate %8 Location 0
+OpDecorate %11 BuiltIn Position
+OpDecorate %13 Location 0
+OpDecorate %32 Location 0
+%2 = OpTypeVoid
+%4 = OpTypeFloat 32
+%3 = OpTypeVector %4 4
+%5 = OpTypeVector %4 2
+%6 = OpTypeStruct %3 %5
+%9 = OpTypePointer Input %5
+%8 = OpVariable  %9  Input
+%12 = OpTypePointer Output %3
+%11 = OpVariable  %12  Output
+%14 = OpTypePointer Output %5
+%13 = OpVariable  %14  Output
+%16 = OpTypeFunction %2
+%17 = OpConstant  %4  0.0
+%18 = OpConstant  %4  1.0
+%20 = OpTypePointer Function %6
+%21 = OpConstantNull  %6
+%23 = OpTypePointer Function %3
+%26 = OpTypeInt 32 0
+%25 = OpConstant  %26  0
+%32 = OpVariable  %12  Output
+%34 = OpConstantComposite  %3  %18 %17 %17 %18
+%15 = OpFunction  %2  None %16
+%7 = OpLabel
+%19 = OpVariable  %20  Function %21
+%10 = OpLoad  %5  %8
+OpBranch %22
+%22 = OpLabel
+%24 = OpCompositeConstruct  %3  %10 %17 %18
+%27 = OpAccessChain  %23  %19 %25
+OpStore %27 %24
+%28 = OpLoad  %6  %19
+%29 = OpCompositeExtract  %3  %28 0
+OpStore %11 %29
+%30 = OpCompositeExtract  %5  %28 1
+OpStore %13 %30
+OpReturn
+OpFunctionEnd
+%33 = OpFunction  %2  None %16
+%31 = OpLabel
+OpBranch %35
+%35 = OpLabel
+OpStore %32 %34
+OpReturn
+OpFunctionEnd
\ No newline at end of file
diff --git a/naga/tests/out/wgsl/6438-conflicting-idents.wgsl b/naga/tests/out/wgsl/6438-conflicting-idents.wgsl
new file mode 100644
index 000000000..d13216bf9
--- /dev/null
+++ b/naga/tests/out/wgsl/6438-conflicting-idents.wgsl
@@ -0,0 +1,18 @@
+struct OurVertexShaderOutput {
+    @builtin(position) position: vec4<f32>,
+    @location(0) texcoord: vec2<f32>,
+}
+
+@vertex 
+fn vs(@location(0) xy: vec2<f32>) -> OurVertexShaderOutput {
+    var vsOutput: OurVertexShaderOutput;
+
+    vsOutput.position = vec4<f32>(xy, 0f, 1f);
+    let _e6 = vsOutput;
+    return _e6;
+}
+
+@fragment 
+fn fs() -> @location(0) vec4<f32> {
+    return vec4<f32>(1f, 0f, 0f, 1f);
+}
diff --git a/naga/tests/snapshots.rs b/naga/tests/snapshots.rs
index 244c9f506..e132bc3d4 100644
--- a/naga/tests/snapshots.rs
+++ b/naga/tests/snapshots.rs
@@ -937,6 +937,10 @@ fn convert_wgsl() {
         ),
         ("6220-break-from-loop", Targets::SPIRV),
         ("index-by-value", Targets::SPIRV | Targets::IR),
+        (
+            "6438-conflicting-idents",
+            Targets::SPIRV | Targets::METAL | Targets::GLSL | Targets::HLSL | Targets::WGSL,
+        ),
     ];
 
     for &(name, targets) in inputs.iter() {