From 74622ea206bd671b40fc9c2c9c4f3c1666274604 Mon Sep 17 00:00:00 2001
From: Shahbaz Youssefi <syoussefi@chromium.org>
Date: Thu, 11 Jun 2026 18:35:19 -0400
Subject: [PATCH] Fix clamping indices to gl_FragData vs dual source blending

Bug: chromium:514482938
Change-Id: If96682b8cf5c514f2587ad887a9a2b7ab1376dc5
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/7926995
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Reviewed-by: Yuxin Hu <yuxinhu@google.com>
---

diff --git a/src/compiler/translator/ParseContext.cpp b/src/compiler/translator/ParseContext.cpp
index c4a8c94..ef293f8 100644
--- a/src/compiler/translator/ParseContext.cpp
+++ b/src/compiler/translator/ParseContext.cpp
@@ -357,10 +357,7 @@
     {
         usesFragColor = true;
     }
-    // Extension variables may not always be initialized (saves some time at symbol table init).
-    bool secondaryFragDataUsed =
-        symbolTable.gl_SecondaryFragDataEXT() != nullptr &&
-        symbolTable.isStaticallyUsed(*symbolTable.gl_SecondaryFragDataEXT());
+    const bool secondaryFragDataUsed = symbolTable.isSecondaryFragDataUsed();
     if (symbolTable.isStaticallyUsed(*symbolTable.gl_FragData()) || secondaryFragDataUsed)
     {
         usesFragData = true;
diff --git a/src/compiler/translator/SymbolTable.cpp b/src/compiler/translator/SymbolTable.cpp
index c280656..c728662 100644
--- a/src/compiler/translator/SymbolTable.cpp
+++ b/src/compiler/translator/SymbolTable.cpp
@@ -247,6 +247,12 @@
     return static_cast<const TVariable *>(m_gl_SecondaryFragDataEXT);
 }
 
+bool TSymbolTable::isSecondaryFragDataUsed() const
+{
+    // Extension variables may not always be initialized (saves some time at symbol table init).
+    return gl_SecondaryFragDataEXT() != nullptr && isStaticallyUsed(*gl_SecondaryFragDataEXT());
+}
+
 TSymbolTable::VariableMetadata *TSymbolTable::getOrCreateVariableMetadata(const TVariable &variable)
 {
     int id    = variable.uniqueId().get();
diff --git a/src/compiler/translator/SymbolTable.h b/src/compiler/translator/SymbolTable.h
index a196c3b..f4a58ee 100644
--- a/src/compiler/translator/SymbolTable.h
+++ b/src/compiler/translator/SymbolTable.h
@@ -240,6 +240,7 @@
 
     const TVariable *gl_FragData() const;
     const TVariable *gl_SecondaryFragDataEXT() const;
+    bool isSecondaryFragDataUsed() const;
 
     void markStaticUse(const TVariable &variable);
 
diff --git a/src/compiler/translator/ir/src/compile.rs b/src/compiler/translator/ir/src/compile.rs
index b23372c..3e6e593 100644
--- a/src/compiler/translator/ir/src/compile.rs
+++ b/src/compiler/translator/ir/src/compile.rs
@@ -569,6 +569,7 @@
     // variables can be detected as inactive.  However, reflection info for inactive variables must
     // also be collected, so the transformation reports the set of active interface variables
     // without removing inactive ones.
+    ir.meta.cache_built_in_static_use_before_dce();
     let active_interface_variables = transform::run!(dead_code_eliminate, ir);
 
     {
@@ -587,24 +588,31 @@
     // varying, but the FS reading from it, which is not allowed.  That's why inactive shader
     // outputs are not removed.  Inactive fragment shader outputs can be removed though.
     //
-    // For now, inactive built-ins are also retained.
-    if options.remove_inactive_interface_variables {
-        let retain_inactive_outputs = options.retain_inactive_fragment_outputs
-            || ir.meta.get_shader_type() != ShaderType::Fragment;
+    let shader_type = ir.meta.get_shader_type();
+    let retain_inactive_outputs = !options.remove_inactive_interface_variables
+        || options.retain_inactive_fragment_outputs
+        || shader_type != ShaderType::Fragment;
 
-        ir.meta.prune_global_variables(|variable_id, variable| {
-            // Keep the variable if:
-            //
-            // * Not an interface variable, or
-            // * Is active, or
-            // * Is output and should be kept, or
-            // * Is built-in
-            !variable.is_interface_variable()
-                || active_interface_variables.contains(&variable_id)
-                || (retain_inactive_outputs && variable.decorations.has(Decoration::Output))
-                || variable.is_built_in()
-        });
-    }
+    ir.meta.prune_global_variables(|variable_id, variable| {
+        // Keep the variable if:
+        //
+        // * Not an interface variable, or
+        // * Is active, or
+        // * Is output and should be kept, or
+        // * Is not built-in and options.remove_inactive_interface_variables is not set.
+        //   * Inactive built-ins are always removed.
+        //   * gl_Position is exceptionally retained so it can be zero-initialized.
+        //   * gl_ClipDistance and gl_CullDistance are retained, as various AST transformations
+        //     don't expect them to be pruned.
+        !variable.is_interface_variable()
+            || active_interface_variables.contains(&variable_id)
+            || (retain_inactive_outputs && variable.decorations.has(Decoration::Output))
+            || (!options.remove_inactive_interface_variables && !variable.is_built_in())
+            || matches!(
+                variable.built_in,
+                Some(BuiltIn::Position) | Some(BuiltIn::ClipDistance) | Some(BuiltIn::CullDistance)
+            )
+    });
 }
 
 fn common_post_variable_collection_transforms(ir: &mut IR, options: &Options) {
@@ -639,6 +647,7 @@
     if options.clamp_indirect_indices {
         let transform_options = transform::localized_workarounds::Options {
             clamp_indirect_indices: options.clamp_indirect_indices,
+            max_dual_source_draw_buffers: options.limits.max_dual_source_draw_buffers,
         };
         transform::run!(localized_workarounds, ir, &transform_options);
     }
diff --git a/src/compiler/translator/ir/src/ir.rs b/src/compiler/translator/ir/src/ir.rs
index 798635b..5e8e8fd 100644
--- a/src/compiler/translator/ir/src/ir.rs
+++ b/src/compiler/translator/ir/src/ir.rs
@@ -2500,6 +2500,7 @@
     variables_pending_zero_initialization: HashSet<VariableId>,
     // Shader reflection info
     reflection_info: reflection::Info,
+    uses_secondary_frag_data: bool,
 }
 
 impl IRMeta {
@@ -2645,6 +2646,7 @@
             per_vertex_out_is_redeclared: false,
             variables_pending_zero_initialization: HashSet::new(),
             reflection_info: reflection::Info::new(),
+            uses_secondary_frag_data: false,
         }
     }
 
@@ -3406,6 +3408,16 @@
         type_info.get_element_type_id().unwrap()
     }
 
+    // For some transformations, it matters if some built-in is statically used, even if it's
+    // dead-code eliminated.  Calculate that before DCE.
+    pub fn cache_built_in_static_use_before_dce(&mut self) {
+        self.uses_secondary_frag_data =
+            self.get_built_in_variable(BuiltIn::SecondaryFragDataEXT).is_some();
+    }
+    pub fn uses_secondary_frag_data(&self) -> bool {
+        self.uses_secondary_frag_data
+    }
+
     pub fn take_reflection_info(&mut self) -> reflection::Info {
         std::mem::replace(&mut self.reflection_info, reflection::Info::new())
     }
diff --git a/src/compiler/translator/ir/src/transform/initialize_uninitialized_variables.rs b/src/compiler/translator/ir/src/transform/initialize_uninitialized_variables.rs
index e015542..9f6ff57 100644
--- a/src/compiler/translator/ir/src/transform/initialize_uninitialized_variables.rs
+++ b/src/compiler/translator/ir/src/transform/initialize_uninitialized_variables.rs
@@ -289,12 +289,23 @@
             let variable = TypedId::from_variable_id(ir_meta, id);
             // For gl_FragData, the array elements are assigned one by one to keep the AST
             // compatible with ESSL 1.00 which doesn't have array assignment.
-            current_block = initialize_with_zeros(
-                ir_meta,
-                current_block,
-                variable,
-                options.loops_allowed_when_initializing_variables && !is_fragment_output_array,
-            );
+            //
+            // If dual source blending is enabled, only initialize element 0.
+            current_block = if is_fragment_output_array && ir_meta.uses_secondary_frag_data() {
+                let element_zero = current_block.add_typed_instruction(instruction::index(
+                    ir_meta,
+                    variable,
+                    TYPED_CONSTANT_ID_INT_ZERO,
+                ));
+                initialize_with_zeros(ir_meta, current_block, element_zero, false)
+            } else {
+                initialize_with_zeros(
+                    ir_meta,
+                    current_block,
+                    variable,
+                    options.loops_allowed_when_initializing_variables && !is_fragment_output_array,
+                )
+            };
             ir_meta.on_variable_zero_initialization_done(id);
             any_code_generated = true;
         }
diff --git a/src/compiler/translator/ir/src/transform/localized_workarounds.rs b/src/compiler/translator/ir/src/transform/localized_workarounds.rs
index 5a360a3..54562f9 100644

diff --git a/src/tests/gl_tests/GLSLTest.cpp b/src/tests/gl_tests/GLSLTest.cpp
index cd8223f..52cdcf2 100644
--- a/src/tests/gl_tests/GLSLTest.cpp
+++ b/src/tests/gl_tests/GLSLTest.cpp
@@ -24215,6 +24215,7 @@
     ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), kFS);
     drawQuad(program, essl3_shaders::PositionAttrib(), 0.5f);
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::red);
+    ASSERT_GL_NO_ERROR();
 }
 
 // Make sure gl_FragColor can be marked invariant in presence of GL_EXT_draw_buffers, even if
@@ -24233,6 +24234,112 @@
     // Verify compilation only, as output is not written to.
     ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), kFS);
 }
+
+// Test that indirect indices to gl_FragData get clamped to the right bounds when
+// gl_SecondaryFragDataEXT is used.
+//
+// The same test for ES3 is not needed, because unlike gl_FragData in ESSL 100, it's not allowed to
+// index a fragment output variable with a non-constant index in ESSL 300+.
+TEST_P(WebGLGLSLTest, FragDataIndexClampWithSecondaryFragData)
+{
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_draw_buffers"));
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_blend_func_extended"));
+
+    constexpr char kFS[] = R"(#extension GL_EXT_draw_buffers : require
+#extension GL_EXT_blend_func_extended : require
+precision mediump float;
+void main() {
+    // gl_SecondaryFragDataEXT follows, which means only one output is valid.  Make sure all the
+    // following writes in the loop end up writing to gl_FragData[0].
+    gl_FragData[0] = vec4(1, 0, 0, 1);
+    for (int i = 0; i < 8; i++) {
+        gl_FragData[i] += vec4(-0.1, 0.05, 0.0, 0.0);
+    }
+
+    gl_SecondaryFragDataEXT[0] = vec4(1.0, 0.0, 0.0, 1.0);
+})";
+
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), kFS);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f, 1.0f, true);
+    EXPECT_PIXEL_COLOR_NEAR(0, 0, GLColor(51, 102, 0, 255), 1);
+    ASSERT_GL_NO_ERROR();
+}
+
+// Same test as FragDataIndexClampWithSecondaryFragData, but gl_SecondaryFragDataEXT is not written
+// to.
+TEST_P(WebGLGLSLTest, FragDataIndexClampWithSecondaryFragDataRead)
+{
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_draw_buffers"));
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_blend_func_extended"));
+
+    constexpr char kFS[] = R"(#extension GL_EXT_draw_buffers : require
+#extension GL_EXT_blend_func_extended : require
+precision mediump float;
+void main() {
+    gl_FragData[0] = vec4(1, 0, 0, 1);
+    for (int i = 0; i < 8; i++) {
+        gl_FragData[i] += vec4(-0.1, 0.05, 0.0, 0.0);
+    }
+
+    gl_FragData[0] += gl_SecondaryFragDataEXT[0];
+})";
+
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), kFS);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f, 1.0f, true);
+    EXPECT_PIXEL_COLOR_NEAR(0, 0, GLColor(51, 102, 0, 255), 1);
+    ASSERT_GL_NO_ERROR();
+}
+
+// Same test as FragDataIndexClampWithSecondaryFragData, but gl_SecondaryFragDataEXT is only used in
+// dead code.
+TEST_P(WebGLGLSLTest, FragDataIndexClampWithSecondaryFragDataDeadCode)
+{
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_draw_buffers"));
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_blend_func_extended"));
+
+    constexpr char kFS[] = R"(#extension GL_EXT_draw_buffers : require
+#extension GL_EXT_blend_func_extended : require
+precision mediump float;
+void main() {
+    gl_FragData[0] = vec4(1, 0, 0, 1);
+    for (int i = 0; i < 8; i++) {
+        gl_FragData[i] += vec4(-0.1, 0.05, 0.0, 0.0);
+    }
+
+    if (false)
+    {
+        gl_SecondaryFragDataEXT[0] = vec4(1.0, 0.0, 0.0, 1.0);
+    }
+})";
+
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), kFS);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f, 1.0f, true);
+    EXPECT_PIXEL_COLOR_NEAR(0, 0, GLColor(51, 102, 0, 255), 1);
+    ASSERT_GL_NO_ERROR();
+}
+
+// Same test as FragDataIndexClampWithSecondaryFragData, but gl_SecondaryFragDataEXT is not actually
+// used (only the extension is enabled.
+TEST_P(WebGLGLSLTest, FragDataIndexClampWithSecondaryFragDataUnused)
+{
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_draw_buffers"));
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_blend_func_extended"));
+
+    constexpr char kFS[] = R"(#extension GL_EXT_draw_buffers : require
+#extension GL_EXT_blend_func_extended : require
+precision mediump float;
+void main() {
+    gl_FragData[0] = vec4(1, 0, 0, 1);
+    for (int i = 0; i < 8; i++) {
+        gl_FragData[i] += vec4(-0.1, 0.05, 0.0, 0.0);
+    }
+})";
+
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), kFS);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f, 1.0f, true);
+    EXPECT_PIXEL_COLOR_NEAR(0, 0, GLColor(229, 13, 0, 255), 1);
+    ASSERT_GL_NO_ERROR();
+}
 }  // anonymous namespace
 
 ANGLE_INSTANTIATE_TEST_ES2_AND_ES3_AND_ES31_AND_ES32(