[Fix, Test] (MG_Util, MG_Backend/DirectVulkan, MG_IntegrationTest): adversarial review - the rewritten 1D-array image collided with a module's own 2D-array one and left an invalid duplicate type; pin the component order, keep the subject kinds in a truncated matrix

MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp

@@ -805,12 +805,16 @@ namespace MobileGL::MG_Backend::DirectVulkan {
             return false;
         }
 
-        // Unlike the sampled texel buffer, the shader may WRITE this one, and those writes land in
+        // Unlike the sampled texel buffer, the shader MAY write this one, and those writes land
-        // GPU memory behind the frontend's CPU shadow - which is what MapBuffer and
+        // in GPU memory behind the frontend's CPU shadow - which is what MapBuffer and
         // GetBufferSubData read. Same two calls, and for the same reason, as the storage-block
-        // path above.
+        // path above - but only the residency is unconditional. Marking a GL_READ_ONLY binding
+        // GPU-written would make the next map or readback wait for a dispatch that could not have
+        // changed a byte of it.
         bufferObject->EnsureGpuResidentStorage();
-        bufferObject->MarkGpuWritten();
+        if (imageBinding.Access != GL_READ_ONLY) {
+            bufferObject->MarkGpuWritten();
+        }
 
         BufferSlice slice{};
         if (!m_bufferManager->AcquireResidentSlice(BufferKind::TextureBuffer, bufferObject, slice) ||

MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp

@@ -453,10 +453,15 @@ namespace MGITest {
         if (!Ready()) return;
         if (!ImagesAreUsable()) GTEST_SKIP() << "no compute image uniforms";
 
+        // The two kinds this whole scenario file exists for come FIRST, and that ordering is
+        // load-bearing rather than cosmetic. The list has to be truncated to the device's image
+        // unit count, and the guaranteed minimum is small - ES 3.1 promises only four compute
+        // image uniforms - so a list in the conformance case's own order would put imageBuffer
+        // at index five and drop it on exactly the devices most likely to get it wrong. A test
+        // that quietly stops covering its own subject is worse than one that fails.
         const bool multisample = MultisampleImagesAreUsable();
-        std::vector<TargetKind> kinds{kKind1D,   kKind1DArray,   kKind2D,        kKind2DArray,
+        std::vector<TargetKind> kinds{kKind1DArray, kKindBuffer, kKind2D,   kKind1D,      kKind2DArray,
-                                      kKind3D,   kKindBuffer,    kKindCube,      kKindCubeArray,
+                                      kKind3D,      kKindCube,   kKindRect, kKindCubeArray};
-                                      kKindRect};
         if (multisample) {
             kinds.push_back(kKind2DMS);
             kinds.push_back(kKind2DMSArray);
@@ -472,6 +477,19 @@ namespace MGITest {
             std::min<std::size_t>(kinds.size(), static_cast<std::size_t>(std::max(0, std::min(maxComputeImageUniforms,
                                                                                               maxImageUnits))));
         if (count == 0) GTEST_SKIP() << "no image units";
+        // Named, not silently dropped: `expected` is computed over whatever survives, so a
+        // truncated run is self-consistently green and would otherwise never say what it stopped
+        // covering.
+        if (count < kinds.size()) {
+            std::string dropped;
+            for (std::size_t i = count; i < kinds.size(); ++i) {
+                if (!dropped.empty()) dropped += ", ";
+                dropped += kinds[i].name;
+            }
+            RecordProperty("dropped_image_target_kinds", dropped);
+            GTEST_LOG_(INFO) << "only " << count << " image units, so these kinds are not covered by the "
+                             << "combined case: " << dropped;
+        }
         kinds.resize(count);
 
         std::string declarations;

MobileGL/MG_Test/Program/ProgramUtilTest.cpp

@@ -3569,8 +3569,55 @@ TEST_F(ProgramUtilTest, Lower1DArrayImagesRewritesTheTypeAndWidensTheCoordinate)
         << "the image must be declared as the 2D array the texture is stored as:\n" << essl;
     EXPECT_EQ(essl.find("ivec2(ivec2("), String::npos)
         << "the malformed constructor must be gone:\n" << essl;
-    EXPECT_NE(essl.find("ivec3("), String::npos)
+    // The ORDER is the whole point, and it is what a widening that merely appended the 0 would
-        << "the coordinate must have been widened to three components:\n" << essl;
+    // get wrong while still producing a three-component constructor that compiles. The fixture
+    // reads (u=2, layer=3), and the ES 2D array holds height 1 with the layers in depth
+    // (TextureImpl::GetBackendUploadSize), so the only correct spelling is (2, 0, 3).
+    EXPECT_NE(essl.find("ivec3(2, 0, 3)"), String::npos)
+        << "the layer must land in the third component and Y must be 0; ivec3(2, 3, 0) would read "
+           "row 3 of a one-row texture and layer 0 of every access:\n"
+        << essl;
+}
+
+// The shape that made the first cut of this pass emit INVALID SPIR-V, and the shape the
+// conformance case actually has: a 1D-array image and a real 2D-array image of the same sampled
+// type and format in one module. Rewriting the first one's Dim in place makes the two
+// OpTypeImage declarations structurally identical, and SPIR-V forbids duplicate non-aggregate
+// types - so the module the ESSL path hands on failed validation and quietly bumped the latch.
+// A single-image fixture cannot see any of that.
+TEST_F(ProgramUtilTest, Lower1DArrayImagesDeduplicatesAgainstAnExisting2DArrayImage) {
+    using namespace MG_Util::ShaderTranspiler;
+
+    const Vector<Uint32> raw = BuildSpirvForStage(R"(#version 440 core
+layout (local_size_x = 1) in;
+layout (location = 0, r32ui) readonly uniform uimage1DArray i0;
+layout (location = 1, r32ui) readonly uniform uimage2DArray i1;
+layout (std430, binding = 0) buffer SSB { uint sum; } ssb;
+void main() { ssb.sum = imageLoad(i0, ivec2(2, 3)).r + imageLoad(i1, ivec3(1, 1, 1)).r; }
+)",
+                                                  GL_COMPUTE_SHADER);
+    ASSERT_FALSE(raw.empty());
+
+    Vector<Uint32> spirv;
+    ASSERT_TRUE(ShaderCompiler::SanitizeAndOptimizeBinary(raw, spirv));
+    ASSERT_EQ(Count1DArrayStorageImageTypes(spirv), 1u);
+
+    SpirvValidationScope validationOn(true);
+    const Uint64 failuresBefore = ShaderCompiler::SpirvValidationFailureCount();
+
+    Vector<Uint32> lowered;
+    ASSERT_TRUE(ShaderCompiler::Lower1DArrayImagesForEssl(spirv, lowered));
+    ASSERT_FALSE(lowered.empty());
+
+    EXPECT_EQ(Count1DArrayStorageImageTypes(lowered), 0u) << DisassembleSpirv(lowered);
+    EXPECT_EQ(ShaderCompiler::SpirvValidationFailureCount(), failuresBefore)
+        << "the rewritten 1D-array image collided with the module's own 2D-array image and left a "
+           "duplicate type declaration behind:\n"
+        << DisassembleSpirv(lowered);
+
+    const String essl = DecompileToEssl(lowered);
+    ASSERT_FALSE(essl.empty());
+    EXPECT_NE(essl.find("ivec3(2, 0, 3)"), String::npos) << essl;
 }
 
 // Scope, half one: a NON-arrayed 1D storage image is emitted correctly by the very same

MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp

@@ -858,6 +858,16 @@ namespace MobileGL {
 
                 Optimizer optimizer(SPV_ENV_VULKAN_1_1);
                 optimizer.RegisterPass(Lower1DArrayImagesPass::CreateLower1DArrayImagesPass());
+                // Mandatory, not tidying. Rewriting a 1D-array image type to the 2D-array one
+                // makes it structurally IDENTICAL to any real 2D-array image of the same sampled
+                // type and format that the module already declared - and SPIR-V forbids duplicate
+                // non-aggregate type declarations, so the result fails validation. That collision
+                // is not exotic: it is the shape of this whole change's headline case, where one
+                // compute shader declares uimage1DArray and uimage2DArray side by side, both
+                // r32ui. The same applies one level up, to the OpTypePointer instructions that
+                // named the two types, and to the Image1D capability the rewrite turns into a
+                // second Shader. Deduplicating afterwards collapses all three at once.
+                optimizer.RegisterPass(CreateRemoveDuplicatesPass());
 
                 return RunOptimizerChecked("Lower1DArrayImagesForEssl", optimizer, inputBinary, outputBinary);
             }

MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp

@@ -171,13 +171,21 @@ namespace MobileGL {
                     return Status::SuccessWithoutChange;
                 }
 
-                spvtools::opt::analysis::Integer signedInt(32, true);
+                // The same refusal the caller makes, restated here so the pass is safe wherever
-                spvtools::opt::analysis::Vector int3(&signedInt, 3);
+                // it is registered. Rewriting the type while leaving an OpImageQuerySize on it
-                const uint32_t int3TypeId = typeMgr->GetTypeInstruction(&int3);
+                // produces a query whose result type has one component too few - an invalid
-                const uint32_t intTypeId = typeMgr->GetTypeInstruction(&signedInt);
+                // module - and there is no correct two-component size to substitute, because the
-                const uint32_t zeroId = constantMgr->GetSIntConstId(0);
+                // ES texture genuinely has a height the GL one does not.
-                if (int3TypeId == 0 || intTypeId == 0 || zeroId == 0) {
+                for (auto& function : *irContext->module()) {
-                    return Status::Failure;
+                    for (auto& block : function) {
+                        for (auto& instruction : block) {
+                            if (QueriesImageSize(instruction.opcode()) && instruction.NumInOperands() >= 1 &&
+                                Is1DArrayStorageImageType(
+                                    ResolveImageType(irContext, instruction.GetSingleWordInOperand(0)))) {
+                                return Status::SuccessWithoutChange;
+                            }
+                        }
+                    }
                 }
 
                 // (u, layer) -> (u, 0, layer). The height the ES 2D array carries is 1, so Y is
@@ -197,6 +205,35 @@ namespace MobileGL {
                             }
 
                             const uint32_t coordinateId = instruction.GetSingleWordInOperand(coordinateOperand);
+
+                            // Built from the COORDINATE's own component type rather than a
+                            // hardcoded signed int. GLSL only ever spells these ivec2, but SPIR-V
+                            // permits an unsigned coordinate, and extracting a uint component
+                            // into an int result is an invalid module rather than a wrong answer -
+                            // the kind of defect that reaches a driver as "compiles here, not
+                            // there".
+                            Instruction* coordinateDef = irContext->get_def_use_mgr()->GetDef(coordinateId);
+                            if (coordinateDef == nullptr) return Status::Failure;
+                            const auto* coordinateType = typeMgr->GetType(coordinateDef->type_id());
+                            const auto* coordinateVector = coordinateType != nullptr ? coordinateType->AsVector()
+                                                                                     : nullptr;
+                            if (coordinateVector == nullptr || coordinateVector->element_count() != 2) {
+                                return Status::Failure;
+                            }
+                            const auto* component = coordinateVector->element_type();
+                            const auto* componentInteger = component != nullptr ? component->AsInteger() : nullptr;
+                            if (componentInteger == nullptr) return Status::Failure;
+
+                            spvtools::opt::analysis::Vector widenedVector(component, 3);
+                            const uint32_t int3TypeId = typeMgr->GetTypeInstruction(&widenedVector);
+                            const uint32_t intTypeId = typeMgr->GetTypeInstruction(component);
+                            const uint32_t zeroId = componentInteger->IsSigned()
+                                                        ? constantMgr->GetSIntConstId(0)
+                                                        : constantMgr->GetUIntConstId(0);
+                            if (int3TypeId == 0 || intTypeId == 0 || zeroId == 0) {
+                                return Status::Failure;
+                            }
+
                             InstructionBuilder builder(
                                 irContext, &instruction,
                                 IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping);

MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h

@@ -60,6 +60,16 @@ namespace MobileGL {
             // the rewrite OpImageQuerySize yields three components where the shader consumes two,
             // and silently handing back a differently-shaped size is worse than refusing. The
             // caller logs it and leaves the module alone.
+            //
+            // KNOWN LIMITATION - the decline is per MODULE, and a program is several of them. A
+            // program whose vertex and fragment stages share a uimage1DArray uniform, where only
+            // one stage calls imageSize() on it, gets that stage declined and the other rewritten:
+            // the two then declare the same uniform with different types and the ES LINK fails on
+            // a type mismatch, rather than the single compile error a reader of the comment above
+            // would expect. Correlating the decision across a program's stages needs the decision
+            // to be made where the program is known, which is above this pass; it is left undone
+            // deliberately rather than papered over, because both outcomes are a refusal and the
+            // shape has never been observed outside a deliberately constructed shader.
             class Lower1DArrayImagesPass final : public spvtools::opt::Pass {
             public:
                 const char* name() const override { return "mobilegl-lower-1d-array-images"; }