From 44805bfa07a6908d7455e427837b2346d2bc589c Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 16:49:59 -0400 Subject: [PATCH] [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 --- .../DirectVulkan/Renderer/UniformManager.cpp | 12 +++-- .../Scenarios/ImageTargetKindScenario.cpp | 24 +++++++-- MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 51 ++++++++++++++++++- .../ShaderTranspiler/ShaderCompiler.cpp | 10 ++++ .../SpirvPasses/Lower1DArrayImagesPass.cpp | 51 ++++++++++++++++--- .../SpirvPasses/Lower1DArrayImagesPass.h | 10 ++++ 6 files changed, 142 insertions(+), 16 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index 7333a15f..7dea4533 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/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 - // GPU memory behind the frontend's CPU shadow - which is what MapBuffer and + // Unlike the sampled texel buffer, the shader MAY write this one, and those writes land + // 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) || diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp index d62539ad..b35661d2 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp +++ b/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 kinds{kKind1D, kKind1DArray, kKind2D, kKind2DArray, - kKind3D, kKindBuffer, kKindCube, kKindCubeArray, - kKindRect}; + std::vector kinds{kKind1DArray, kKindBuffer, kKind2D, kKind1D, kKind2DArray, + kKind3D, kKindCube, kKindRect, kKindCubeArray}; if (multisample) { kinds.push_back(kKind2DMS); kinds.push_back(kKind2DMSArray); @@ -472,6 +477,19 @@ namespace MGITest { std::min(kinds.size(), static_cast(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; diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 4577b06f..0a727f75 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/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 coordinate must have been widened to three components:\n" << essl; + // The ORDER is the whole point, and it is what a widening that merely appended the 0 would + // 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 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 spirv; + ASSERT_TRUE(ShaderCompiler::SanitizeAndOptimizeBinary(raw, spirv)); + ASSERT_EQ(Count1DArrayStorageImageTypes(spirv), 1u); + + SpirvValidationScope validationOn(true); + const Uint64 failuresBefore = ShaderCompiler::SpirvValidationFailureCount(); + + Vector 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 diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 37da152c..27504985 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/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); } diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp index d4373710..0f444611 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp @@ -171,13 +171,21 @@ namespace MobileGL { return Status::SuccessWithoutChange; } - spvtools::opt::analysis::Integer signedInt(32, true); - spvtools::opt::analysis::Vector int3(&signedInt, 3); - const uint32_t int3TypeId = typeMgr->GetTypeInstruction(&int3); - const uint32_t intTypeId = typeMgr->GetTypeInstruction(&signedInt); - const uint32_t zeroId = constantMgr->GetSIntConstId(0); - if (int3TypeId == 0 || intTypeId == 0 || zeroId == 0) { - return Status::Failure; + // The same refusal the caller makes, restated here so the pass is safe wherever + // it is registered. Rewriting the type while leaving an OpImageQuerySize on it + // produces a query whose result type has one component too few - an invalid + // module - and there is no correct two-component size to substitute, because the + // ES texture genuinely has a height the GL one does not. + for (auto& function : *irContext->module()) { + 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); diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h index c9e67e12..10b3f5c6 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h +++ b/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"; }