diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 77c231b2..a7236bb1 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -6090,17 +6090,38 @@ namespace MobileGL::MG_Backend::DirectGLES { } } - // Attach current shaders - auto& attachedShaders = stateProgramObject->GetAttachedShaders(); - MGLOG_D("Attaching %zu shaders to program %u", attachedShaders.size(), m_backendProgramId); - for (auto& shader : attachedShaders) { - const auto& src = shader->GetShaderSource(); + // Attach current shaders. + // + // The EXECUTABLE's stage list, not GetAttachedShaders(): this loop indexes + // shaderSpirvs by the same running index, and the generated SPIR-V is a link + // artifact while the attach list is live. GL 4.6 core 7.3 makes glAttachShader take + // effect only at the next link, so a program that is attached to after it linked has + // MORE entries in the attach list than there are modules - and pairing the two read + // straight off the end of shaderSpirvs (a std::vector copy from garbage, which is + // how this crashed). GetLinkedShaderStages() is the list the modules were generated + // from, one entry per module, in module order. + const Vector linkedStages = stateProgramObject->GetLinkedShaderStages(); + auto& shaderSpirvs = stateProgramObject->GetGeneratedSpirv(); + // Both come from the same Link(), so they agree by construction. If they ever did + // not there would be no index this function could safely use for EITHER array, so + // this refuses the build instead of picking one and hoping. + if (linkedStages.size() != shaderSpirvs.size()) { + MGLOG_E_ONCE("Program %u: %zu linked stage(s) but %zu generated SPIR-V module(s); refusing to " + "build a backend program from mismatched link artifacts.", + stateProgramObject->GetExternalIndex(), linkedStages.size(), shaderSpirvs.size()); + m_backendProgramUsable = false; + return; + } + MGLOG_D("Attaching %zu shaders to program %u", linkedStages.size(), m_backendProgramId); + for (const auto& ref : stateProgramObject->GetLinkedShaderSnapshot()) { + if (!ref.shader) continue; const auto& stage = - MG_Util::ConvertGLEnumToString(MG_Util::ConvertShaderStageToGLEnum(shader->GetShaderStage())); + MG_Util::ConvertGLEnumToString(MG_Util::ConvertShaderStageToGLEnum(ref.shader->GetShaderStage())); + // The source THIS link consumed, which a later glShaderSource does not disturb. + const String& src = ref.source ? *ref.source : ref.shader->GetShaderSource(); MGLOG_D("Original src @ %s: \n", stage.c_str()); MGLOG_D("%s:", src.empty() ? "" : src.c_str()); } - auto& shaderSpirvs = stateProgramObject->GetGeneratedSpirv(); const Bool enableSpirvValidation = stateProgramObject->GetSpirvValidationEnabled(); // Blocks a transform-feedback capture request names a member of ("StageData" of @@ -6151,15 +6172,15 @@ namespace MobileGL::MG_Backend::DirectGLES { // stage is rewritten. std::set collidingIoBlockNames; std::set declaredIoBlockNames; - Vector stagePipelineIndices(attachedShaders.size(), -1); + Vector stagePipelineIndices(linkedStages.size(), -1); Bool anyStageCanDeclareBlocksInBothDirections = false; - for (SizeT index = 0; index < attachedShaders.size(); ++index) { - const ShaderStage stage = attachedShaders[index]->GetShaderStage(); + for (SizeT index = 0; index < linkedStages.size(); ++index) { + const ShaderStage stage = linkedStages[index]; stagePipelineIndices[index] = InterStagePipelineIndex(stage); if (CanDeclareBlocksInBothDirections(stage)) anyStageCanDeclareBlocksInBothDirections = true; } if (anyStageCanDeclareBlocksInBothDirections) { - for (SizeT index = 0; index < attachedShaders.size() && index < shaderSpirvs.size(); ++index) { + for (SizeT index = 0; index < shaderSpirvs.size(); ++index) { MG_Util::ShaderTranspiler::ShaderCompiler::ProbeIoBlockNamesForEssl( shaderSpirvs[index], collidingIoBlockNames, declaredIoBlockNames); } @@ -6196,19 +6217,24 @@ namespace MobileGL::MG_Backend::DirectGLES { Int tessEvalShaderIndex = -1; String vertexStageEssl; String tessEvalStageEssl; - for (int index = 0; index < attachedShaders.size(); ++index) { - const auto stage = attachedShaders[index]->GetShaderStage(); + // + // Asked of the executable for the same reason the loop below indexes it: a + // tessellation evaluation shader merely ATTACHED to a linked vertex+fragment program + // is not part of what this program runs, and synthesizing a control stage for it + // would build a tessellating driver program for an executable that does not + // tessellate (and would take tessEvalShaderIndex past the end of shaderSpirvs). + for (SizeT index = 0; index < linkedStages.size(); ++index) { + const ShaderStage stage = linkedStages[index]; if (stage == ShaderStage::TessControl) hasTessControlStage = true; if (stage == ShaderStage::TessEval) { hasTessEvalStage = true; - tessEvalShaderIndex = index; + tessEvalShaderIndex = static_cast(index); } } const Bool needsPassthroughTessControl = hasTessEvalStage && !hasTessControlStage; - for (int index = 0; index < attachedShaders.size(); ++index) { - auto& shader = attachedShaders[index]; - GLenum glShaderType = MG_Util::ConvertShaderStageToGLEnum(shader->GetShaderStage()); + for (SizeT index = 0; index < linkedStages.size(); ++index) { + GLenum glShaderType = MG_Util::ConvertShaderStageToGLEnum(linkedStages[index]); GLuint backendShaderId = g_GLESFuncs.glCreateShader(glShaderType); if (backendShaderId == 0) { @@ -6454,7 +6480,7 @@ namespace MobileGL::MG_Backend::DirectGLES { Uint splitImageUniformCount = 0; source = SplitReadWriteImageUniforms(source, &splitImageUniformCount); if (splitImageUniformCount != 0) { - splitImageUniformStages.push_back({shader->GetShaderStage(), splitImageUniformCount}); + splitImageUniformStages.push_back({linkedStages[index], splitImageUniformCount}); } source = RemoveLayoutBinding(source); source = ProcessOutColorLocations(source); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index 3269165c..ee8590c9 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -83,7 +83,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { Bool isMember = false; }; - ShaderStage PickClipFixupStage(const Vector>& shaders); + ShaderStage PickClipFixupStage(const Vector& stages); Bool IsVec4Float32(spvtools::opt::IRContext* context, Uint32 typeId, Uint32* outFloatTypeId) { auto* vecInst = context->get_def_use_mgr()->GetDef(typeId); @@ -614,15 +614,15 @@ namespace MobileGL::MG_Backend::DirectVulkan { void ReflectStageInterface(ShaderStage targetStage, Bool reflectInputs, - const Vector>& shaders, + const Vector& stages, const Vector>& spirv, StageInterfaceSummary& outSummary, Uint programExternalIndex, const char* stageLabel) { outSummary.slotSignatures.fill(0); - for (SizeT moduleIndex = 0; moduleIndex < shaders.size() && moduleIndex < spirv.size(); ++moduleIndex) { - if (!shaders[moduleIndex] || shaders[moduleIndex]->GetShaderStage() != targetStage) { + for (SizeT moduleIndex = 0; moduleIndex < stages.size() && moduleIndex < spirv.size(); ++moduleIndex) { + if (stages[moduleIndex] != targetStage) { continue; } @@ -690,11 +690,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } - void ValidateRasterizationStageInterface(const Vector>& shaders, + void ValidateRasterizationStageInterface(const Vector& stages, const Vector>& spirv, ProgramFactory::VkProgramObject& entry, Uint programExternalIndex) { - const ShaderStage producerStage = PickClipFixupStage(shaders); + const ShaderStage producerStage = PickClipFixupStage(stages); entry.rasterizationProducerStage = producerStage; entry.producerOutputComponentCount = 0; entry.fragmentInputComponentCount = 0; @@ -703,8 +703,8 @@ namespace MobileGL::MG_Backend::DirectVulkan { } Bool hasFragmentStage = false; - for (const auto& shader : shaders) { - if (shader && shader->GetShaderStage() == ShaderStage::Fragment) { + for (const ShaderStage stage : stages) { + if (stage == ShaderStage::Fragment) { hasFragmentStage = true; break; } @@ -715,9 +715,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { StageInterfaceSummary producerOutputs{}; StageInterfaceSummary fragmentInputs{}; - ReflectStageInterface(producerStage, false, shaders, spirv, producerOutputs, programExternalIndex, + ReflectStageInterface(producerStage, false, stages, spirv, producerOutputs, programExternalIndex, "producer"); - ReflectStageInterface(ShaderStage::Fragment, true, shaders, spirv, fragmentInputs, programExternalIndex, + ReflectStageInterface(ShaderStage::Fragment, true, stages, spirv, fragmentInputs, programExternalIndex, "fragment"); entry.producerOutputComponentCount = CountOccupiedStageInterfaceSlots(producerOutputs); entry.fragmentInputComponentCount = CountOccupiedStageInterfaceSlots(fragmentInputs); @@ -1719,14 +1719,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { return success; } - ShaderStage PickClipFixupStage(const Vector>& shaders) { + ShaderStage PickClipFixupStage(const Vector& stages) { Bool hasGeometry = false; Bool hasTessEval = false; Bool hasVertex = false; - for (const auto& shader : shaders) { - if (!shader) continue; - const auto stage = shader->GetShaderStage(); + for (const ShaderStage stage : stages) { hasGeometry |= (stage == ShaderStage::Geometry); hasTessEval |= (stage == ShaderStage::TessEval); hasVertex |= (stage == ShaderStage::Vertex); @@ -2316,15 +2314,15 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } - void ProgramFactory::ReflectVertexInputs(const Vector>& shaders, + void ProgramFactory::ReflectVertexInputs(const Vector& stages, const Vector>& spirv, VkProgramObject& entry) const { entry.activeVertexInputLocationMask = 0; entry.vertexInputTypes.fill(0); entry.readsBaseVertexBuiltin = false; - for (SizeT moduleIndex = 0; moduleIndex < shaders.size() && moduleIndex < spirv.size(); ++moduleIndex) { - if (!shaders[moduleIndex] || shaders[moduleIndex]->GetShaderStage() != ShaderStage::Vertex) { + for (SizeT moduleIndex = 0; moduleIndex < stages.size() && moduleIndex < spirv.size(); ++moduleIndex) { + if (stages[moduleIndex] != ShaderStage::Vertex) { continue; } @@ -2401,14 +2399,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { // evaluation stages. Rather than guess which one is last, every non-fragment, non-compute // module is asked - one writer anywhere means this program's draws need a multi-viewport // pipeline, and a false positive costs only a wider viewportCount. - void ProgramFactory::ReflectViewportIndexUsage(const Vector>& shaders, + void ProgramFactory::ReflectViewportIndexUsage(const Vector& stages, const Vector>& spirv, VkProgramObject& entry) const { entry.writesViewportIndexBuiltin = false; - for (SizeT moduleIndex = 0; moduleIndex < shaders.size() && moduleIndex < spirv.size(); ++moduleIndex) { - if (!shaders[moduleIndex]) continue; - const ShaderStage stage = shaders[moduleIndex]->GetShaderStage(); + for (SizeT moduleIndex = 0; moduleIndex < stages.size() && moduleIndex < spirv.size(); ++moduleIndex) { + const ShaderStage stage = stages[moduleIndex]; if (stage == ShaderStage::Fragment || stage == ShaderStage::Compute) continue; const auto& module = spirv[moduleIndex]; @@ -2436,15 +2433,15 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } - void ProgramFactory::ReflectFragmentOutputs(const Vector>& shaders, + void ProgramFactory::ReflectFragmentOutputs(const Vector& stages, const Vector>& spirv, VkProgramObject& entry) const { entry.activeFragmentOutputLocationMask = 0; entry.fragmentOutputTypes.fill(0); entry.fragmentReplacesDepth = false; - for (SizeT moduleIndex = 0; moduleIndex < shaders.size() && moduleIndex < spirv.size(); ++moduleIndex) { - if (!shaders[moduleIndex] || shaders[moduleIndex]->GetShaderStage() != ShaderStage::Fragment) { + for (SizeT moduleIndex = 0; moduleIndex < stages.size() && moduleIndex < spirv.size(); ++moduleIndex) { + if (stages[moduleIndex] != ShaderStage::Fragment) { continue; } @@ -3150,7 +3147,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { auto& entry = m_cache[hash]; entry.hash = hash; entry.lastUsedFrame = m_frameCounter; - auto& shaders = program.GetAttachedShaders(); + // The EXECUTABLE's stage list, not GetAttachedShaders(): `spirv` is a link artifact with + // one module per linked stage, while the attach list is live and grows on + // glAttachShader, which GL 4.6 core 7.3 says does not reach the executable until the + // next link. Sizing this loop by the attach list therefore ran it past the end of both + // `spirv` and `moduleSpirvs` for any program attached to after it linked. + const Vector stages = program.GetLinkedShaderStages(); auto& spirv = program.GetGeneratedSpirv(); Vector> moduleSpirvs(spirv.size()); const Bool enableSpirvValidation = program.GetSpirvValidationEnabled(); @@ -3158,14 +3160,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { MG_Util::ShaderTranspiler::ShaderCompiler::PrepareSpirvValidation(); } - const ShaderStage fixupStage = PickClipFixupStage(shaders); + const ShaderStage fixupStage = PickClipFixupStage(stages); - for (SizeT i = 0; i < shaders.size(); ++i) { + // Both lists come from the same Link(), so they agree by construction; the min() is what + // makes that an assumption this loop does not have to bet the process on. + const SizeT moduleCount = std::min(stages.size(), spirv.size()); + for (SizeT i = 0; i < moduleCount; ++i) { auto& spv = spirv[i]; if (spv.empty()) continue; // Apply position fixup if needed - if (fixupStage != ShaderStage::Unknown && shaders[i] && shaders[i]->GetShaderStage() == fixupStage) { + if (fixupStage != ShaderStage::Unknown && stages[i] == fixupStage) { const Vector* fixupInput = &spv; Vector xfbSpirv; if ((flags & ProgramFactory::CompileOptionBit::XfbCapture) && @@ -3181,16 +3186,14 @@ namespace MobileGL::MG_Backend::DirectVulkan { moduleSpirvs[i] = spv; } - if ((flags & ProgramFactory::CompileOptionBit::ExplicitLod0Sampling) && shaders[i] && - shaders[i]->GetShaderStage() == ShaderStage::Fragment) { + if ((flags & ProgramFactory::CompileOptionBit::ExplicitLod0Sampling) && stages[i] == ShaderStage::Fragment) { Vector explicitLodSpirv; if (TransformSpirvForExplicitLod0Sampling(moduleSpirvs[i], explicitLodSpirv)) { moduleSpirvs[i] = Move(explicitLodSpirv); } } - if ((flags & ProgramFactory::CompileOptionBit::FragCoordYFlip) && shaders[i] && - shaders[i]->GetShaderStage() == ShaderStage::Fragment) { + if ((flags & ProgramFactory::CompileOptionBit::FragCoordYFlip) && stages[i] == ShaderStage::Fragment) { Vector fragCoordSpirv; if (TransformSpirvForFragCoordYFlip(moduleSpirvs[i], fragCoordSpirv, m_defaultFramebufferHeight)) { moduleSpirvs[i] = Move(fragCoordSpirv); @@ -3201,7 +3204,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // operations execute natively; module repairs keep the GL contract intact // around them. The opt-in emulation path replaces them only on devices with no // subgroup support at all (MOBILEGL_MAGMA_EMULATE_SUBGROUP). - if (shaders[i] && shaders[i]->GetShaderStage() == ShaderStage::Compute) { + if (stages[i] == ShaderStage::Compute) { // Program 203 broadcasts the first reduction through // prefixSumCache[0], then lets the second reduction overwrite that // scratch without first rendezvousing all readers. Patch that exact @@ -3304,8 +3307,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // The unsupported-device counterpart of this rebase (warning when a shader reads // the builtin but shaderDrawParameters is missing) rides along with // ReflectVertexInputs, which already reflects this stage. - if (shaders[i] && shaders[i]->GetShaderStage() == ShaderStage::Vertex && - m_shaderDrawParametersEnabled) { + if (stages[i] == ShaderStage::Vertex && m_shaderDrawParametersEnabled) { Vector rebasedSpirv; if (MG_Util::ShaderTranspiler::ShaderCompiler::RebaseInstanceIndexForVulkan(moduleSpirvs[i], rebasedSpirv, enableSpirvValidation)) { @@ -3322,8 +3324,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // through CompileOptionBit::ZeroBaseVertex, so the indexed variant of the same // program keeps the native builtin and stays correct for glDrawElementsBaseVertex // and for the baseVertex word of an indexed indirect command. - if (shaders[i] && shaders[i]->GetShaderStage() == ShaderStage::Vertex && - (flags & CompileOptionBit::ZeroBaseVertex)) { + if (stages[i] == ShaderStage::Vertex && (flags & CompileOptionBit::ZeroBaseVertex)) { Vector zeroedSpirv; if (MG_Util::ShaderTranspiler::ShaderCompiler::ZeroBaseVertexForVulkan(moduleSpirvs[i], zeroedSpirv, enableSpirvValidation)) { @@ -3346,7 +3347,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // committed to R32G32{,B32A32}_UINT for the attribute, so a module still declaring // `in double` would reconcile to Unknown and build a pipeline with a UINT format under a // double input - garbage with no diagnostic anywhere. - if (shaders[i] && shaders[i]->GetShaderStage() == ShaderStage::Vertex) { + if (stages[i] == ShaderStage::Vertex) { Vector packedSpirv; const Bool packOk = MG_Util::ShaderTranspiler::ShaderCompiler::PackDoubleVertexInputsForVulkan( moduleSpirvs[i], packedSpirv, enableSpirvValidation); @@ -3385,17 +3386,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { const Bool remapOk = RemapDescriptorBindingsForVulkan(moduleSpirvs, m_maxBindings, moduleSpirvs); MOBILEGL_ASSERT(remapOk, "ProgramFactory::GetOrCreateProgram: descriptor binding remap failed"); - for (SizeT i = 0; i < shaders.size(); ++i) { + for (SizeT i = 0; i < moduleCount; ++i) { auto& moduleSpv = moduleSpirvs[i]; if (moduleSpv.empty()) continue; #if MOBILEGL_LOG_ACTIVE_LEVEL <= MOBILEGL_LOG_LEVEL_DEBUG - ValidateTransformedSpirv(moduleSpv, shaders[i]->GetShaderStage(), program.GetExternalIndex()); + ValidateTransformedSpirv(moduleSpv, stages[i], program.GetExternalIndex()); #else // Final module the driver receives; also checked in the INFO-level CI/test // lanes, where the DEBUG gate above is compiled out. if (enableSpirvValidation) { - ValidateTransformedSpirv(moduleSpv, shaders[i]->GetShaderStage(), program.GetExternalIndex()); + ValidateTransformedSpirv(moduleSpv, stages[i], program.GetExternalIndex()); } #endif @@ -3407,7 +3408,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { VK_VERIFY(vkCreateShaderModule(m_device, &smci, nullptr, &module), "vkCreateShaderModule"); VkPipelineShaderStageCreateInfo stage{VK_STRUCTURE_TYPE_PIPELINE_SHADER_STAGE_CREATE_INFO}; - ShaderStage shaderStage = shaders[i]->GetShaderStage(); + ShaderStage shaderStage = stages[i]; stage.stage = ToVkStage(shaderStage); stage.module = module; stage.pName = "main"; @@ -3442,12 +3443,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { // Reflect and create layout as part of the program object #if MOBILEGL_LOG_ACTIVE_LEVEL <= MOBILEGL_LOG_LEVEL_DEBUG - ValidateRasterizationStageInterface(shaders, moduleSpirvs, entry, program.GetExternalIndex()); + ValidateRasterizationStageInterface(stages, moduleSpirvs, entry, program.GetExternalIndex()); #endif - ReflectVertexInputs(shaders, moduleSpirvs, entry); - ReflectViewportIndexUsage(shaders, moduleSpirvs, entry); - ReflectFragmentOutputs(shaders, moduleSpirvs, entry); - ReflectPassthroughTessControlNeed(shaders, moduleSpirvs, entry); + ReflectVertexInputs(stages, moduleSpirvs, entry); + ReflectViewportIndexUsage(stages, moduleSpirvs, entry); + ReflectFragmentOutputs(stages, moduleSpirvs, entry); + ReflectPassthroughTessControlNeed(stages, moduleSpirvs, entry); ReflectLayout(program, moduleSpirvs, entry); // A failed remap means the modules kept glslang's per-stage auto-mapped binding numbers - // no cross-stage unification, no set->0 normalisation - so the bindings this layout @@ -3659,7 +3660,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { } void ProgramFactory::ReflectPassthroughTessControlNeed( - const Vector>& shaders, + const Vector& stages, const Vector>& spirv, VkProgramObject& entry) const { entry.needsPassthroughTessControl = false; @@ -3668,9 +3669,8 @@ namespace MobileGL::MG_Backend::DirectVulkan { Bool hasTessEval = false; Bool hasTessControl = false; SizeT tessEvalModuleIndex = 0; - for (SizeT i = 0; i < shaders.size(); ++i) { - if (!shaders[i]) continue; - const auto stage = shaders[i]->GetShaderStage(); + for (SizeT i = 0; i < stages.size(); ++i) { + const ShaderStage stage = stages[i]; if (stage == ShaderStage::TessControl) hasTessControl = true; if (stage == ShaderStage::TessEval) { hasTessEval = true; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index 2fe6c62f..bb1eb53a 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -499,13 +499,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { }; static TextureTarget UniformTypeToTextureTarget(GLenum glType); - void ReflectVertexInputs(const Vector>& shaders, + // `stages` is ALWAYS ProgramObject::GetLinkedShaderStages() - one entry per module of + // `spirv`, at the same index. Taking the stages rather than the shader objects is what + // keeps the program's live attach list, which is a longer and differently-indexed list + // the moment a glAttachShader lands after the link, from being passed here by mistake. + void ReflectVertexInputs(const Vector& stages, const Vector>& spirv, VkProgramObject& entry) const; - void ReflectViewportIndexUsage(const Vector>& shaders, + void ReflectViewportIndexUsage(const Vector& stages, const Vector>& spirv, VkProgramObject& entry) const; - void ReflectFragmentOutputs(const Vector>& shaders, + void ReflectFragmentOutputs(const Vector& stages, const Vector>& spirv, VkProgramObject& entry) const; void ReflectLayout(const MG_State::GLState::ProgramObject& program, const Vector>& spirv, @@ -513,7 +517,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // Fills needsPassthroughTessControl / passthroughTessControlEmulatable off the linked // modules. Const and reflection-only: it decides nothing about the pipeline, it only // records what the evaluation stage's input interface is made of. - void ReflectPassthroughTessControlNeed(const Vector>& shaders, + void ReflectPassthroughTessControlNeed(const Vector& stages, const Vector>& spirv, VkProgramObject& entry) const; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 92faee42..d4baee7e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -4765,10 +4765,11 @@ void main() { // link-time properties, so this is safe to fold into a pipeline keyed on the program hash. static Bool ProgramCapturesXfbFromGeometryStage(const MG_State::GLState::ProgramObject& program) { if (program.GetTransformFeedbackVaryingCount() == 0) return false; - for (const auto& shader : program.GetAttachedShaders()) { - if (shader && shader->GetShaderStage() == ShaderStage::Geometry) return true; - } - return false; + // Both halves are link-time properties, so both are asked of the LAST LINK. Reading the + // live attach list would let a glAttachShader that has not been linked in yet - which GL + // 4.6 core 7.3 says changes nothing about what the program runs - flip a property this + // pipeline is cached under, for an executable with no geometry stage in it. + return program.HasLinkedShaderStage(ShaderStage::Geometry); } VkPipeline VulkanRenderer::GetOrCreatePipeline( diff --git a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp index e73c1dd6..16626a9f 100644 --- a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp +++ b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp @@ -45,7 +45,11 @@ namespace MobileGL::MG_Impl::GLImpl { const auto& currentProgram = MG_State::pGLContext->GetProgramForDispatch(); if (!ValidateProgramForExecution(currentProgram, functionName)) return false; - if (currentProgram->GetShaderIndexByStage(ShaderStage::Compute) < 0) { + // Of the EXECUTABLE, not the live attach list: attaching a compute shader to an + // already-linked graphics program does not give that program a compute stage to + // dispatch (GL 4.6 core 7.3), and letting the dispatch through on the strength of the + // attach hands the backend a program whose SPIR-V has no compute module in it. + if (!currentProgram->HasLinkedShaderStage(ShaderStage::Compute)) { MG_State::pGLContext->RecordError( ErrorCode::InvalidOperation, MakeUnique("MG_Impl/GLImpl", functionName, @@ -111,7 +115,7 @@ namespace MobileGL::MG_Impl::GLImpl { // A geometry stage writes what it emits, not what the draw assembled, and the // amplification factor lives in the shader. Record that this span contained such // a draw so the transform feedback queries keep their backend result for it. - if (program->GetShaderIndexByStage(ShaderStage::Geometry) >= 0) { + if (program->HasLinkedShaderStage(ShaderStage::Geometry)) { MG_State::pGLContext->AddTransformFeedbackGeometryCaptureDraw(); } // Capacity in captured vertices = the tightest bound buffer. @@ -208,8 +212,12 @@ namespace MobileGL::MG_Impl::GLImpl { // The EVALUATION stage is what decides: a control stage cannot run without one, and a // program carrying only an evaluation stage still tessellates, through GL's // fixed-function pass-through control stage (11.2.2). - const Bool tessellationActive = - currentProgram && currentProgram->GetShaderIndexByStage(ShaderStage::TessEval) >= 0; + // Asked of the LAST LINK, not the live attach list (GL 4.6 core 7.3): attaching a + // tessellation evaluation shader to an already-linked program does not put it in the + // executable, so reading the live list here would reject every non-GL_PATCHES draw + // against a program that does not tessellate - and keep rejecting them, since a detach + // is likewise deferred to the next link. + const Bool tessellationActive = currentProgram && currentProgram->HasLinkedShaderStage(ShaderStage::TessEval); if (tessellationActive && mode != GL_PATCHES) { MG_State::pGLContext->RecordError( ErrorCode::InvalidOperation, @@ -238,8 +246,13 @@ namespace MobileGL::MG_Impl::GLImpl { // shaders whose input is the most restrictive one - every mode but GL_POINTS was // accepted (KHR-GL43.transform_feedback.api_errors_test draws a points-in geometry // program with GL_LINES and requires INVALID_OPERATION). - const Bool geometryActive = - currentProgram && currentProgram->GetShaderIndexByStage(ShaderStage::Geometry) >= 0; + // + // And it has to be asked of the LAST LINK: gsInputPrimitive is a link artifact, so + // pairing it with the live attach list would re-point the very same 0-aliasing rather + // than remove it. In the window after glAttachShader(GS) on a linked program the live + // list says "geometry present" while the artifact still reads GL_NONE == GL_POINTS, and + // the switch below would silently reject every mode but GL_POINTS. + const Bool geometryActive = currentProgram && currentProgram->HasLinkedShaderStage(ShaderStage::Geometry); const GLenum gsInput = geometryActive ? currentProgram->GetGeometryInputType() : GL_NONE; if (geometryActive && mode != GL_PATCHES) { Bool compatible = false; @@ -281,9 +294,12 @@ namespace MobileGL::MG_Impl::GLImpl { // can only ever be GL_PATCHES. A paused span is exempt: it captures nothing, // so there is nothing for the mode to be incompatible with (GL 4.6 core 13.2.3). const auto& feedbackProgram = MG_State::pGLContext->GetTransformFeedbackProgram(); + // Both stage tests are asked of the last link, for the same reason as the two guards + // above: what relocates the constraint is a stage the program actually RUNS, and an + // attach that has not been linked in yet gives it none. const Bool feedbackModeIsProgramDriven = - feedbackProgram && (feedbackProgram->GetShaderIndexByStage(ShaderStage::Geometry) >= 0 || - feedbackProgram->GetShaderIndexByStage(ShaderStage::TessEval) >= 0); + feedbackProgram && (feedbackProgram->HasLinkedShaderStage(ShaderStage::Geometry) || + feedbackProgram->HasLinkedShaderStage(ShaderStage::TessEval)); if (MG_State::pGLContext->IsTransformFeedbackActive() && !MG_State::pGLContext->IsTransformFeedbackPaused() && !feedbackModeIsProgramDriven) { const GLenum feedbackMode = MG_State::pGLContext->GetTransformFeedbackPrimitiveMode(); diff --git a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp index fef1fc52..9fe2ef1e 100644 --- a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp +++ b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp @@ -700,7 +700,11 @@ namespace MobileGL::MG_Impl::GLImpl { MGLOG_D("%s: %s = %d", __func__, MG_Util::ConvertGLEnumToString(pname).c_str(), *params); break; case GL_COMPUTE_WORK_GROUP_SIZE: { // GL >= 4.3 - if (!programObject->GetLinkStatus() || programObject->GetShaderIndexByStage(ShaderStage::Compute) < 0) { + // "a linked program object with a compute shader" is one whose EXECUTABLE has the + // stage: the local size below is a link artifact, so an attached-but-not-yet-linked + // compute shader would answer this query with the previous link's (absent) value + // instead of the INVALID_OPERATION GL 4.6 core 7.13 asks for. + if (!programObject->GetLinkStatus() || !programObject->HasLinkedShaderStage(ShaderStage::Compute)) { MG_State::pGLContext->RecordError( ErrorCode::InvalidOperation, MakeUnique("MG_Impl/GLImpl", __func__, diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 476dca96..59ed3edc 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -88,6 +88,7 @@ add_executable(MobileGLIntegrationTest Scenarios/IoBlockNameCollisionScenario.cpp Scenarios/TessellationDrawModeScenario.cpp Scenarios/GeometryDrawModeScenario.cpp + Scenarios/PostLinkAttachScenario.cpp Scenarios/FormatlessImageBakeScenario.cpp Scenarios/FragmentOutputArrayIndexScenario.cpp Scenarios/BufferTextureScenario.cpp diff --git a/MobileGL/MG_IntegrationTest/Scenarios/GeometryDrawModeScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/GeometryDrawModeScenario.cpp index bf8a57c4..174fe25c 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/GeometryDrawModeScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/GeometryDrawModeScenario.cpp @@ -294,5 +294,120 @@ void main() } } + // The other half of "ask the stage": WHICH stage list is asked. gsInputPrimitive is a + // LINK artifact, so pairing it with the live attach list re-points the GL_NONE/GL_POINTS + // aliasing instead of removing it - inside the window between glAttachShader and the + // next link, the live list says "geometry present" while the artifact still reads + // GL_NONE, which is 0, which is GL_POINTS, so every mode but GL_POINTS is rejected. + // + // GL 4.6 core 7.3 makes that window legal and ordinary: an attach affects the program's + // executable only at the next link, and leaves LINK_STATUS alone. The attached shader + // need not even compile. Worse, it does not heal - glDetachShader defers the removal to + // the next Link() too, so the program would keep failing every non-POINTS draw until the + // application happened to relink for some unrelated reason. + TEST_F(GeometryDrawModeScenario, AttachingAGeometryStageAfterTheLinkDoesNotConstrainTheDrawMode) { + if (!Ready()) GTEST_SKIP(); + + // Deliberately NOT BuildProgram: the executable under test has no geometry stage. + const GLuint program = glCreateProgram(); + m_programs.push_back(program); + for (const auto& [stage, source] : + std::vector>{{GL_VERTEX_SHADER, kVertexSource}, + {GL_FRAGMENT_SHADER, kFragmentSource}}) { + const GLuint shader = glCreateShader(stage); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + glAttachShader(program, shader); + glDeleteShader(shader); + } + glLinkProgram(program); + GLint linked = GL_FALSE; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + ASSERT_EQ(linked, GL_TRUE) << "the vertex+fragment program did not link"; + + glUseProgram(program); + DrainErrors(); + glDrawArrays(GL_TRIANGLES, 0, 3); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)) + << "a program with no geometry stage must draw triangles"; + DrainErrors(); + + const GLuint geometry = glCreateShader(GL_GEOMETRY_SHADER); + glShaderSource(geometry, 1, &kPointsInGeometrySource, nullptr); + glCompileShader(geometry); + glAttachShader(program, geometry); + glDeleteShader(geometry); + DrainErrors(); + + // Same executable as three lines ago - no relink has happened. + glDrawArrays(GL_TRIANGLES, 0, 3); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)) + << "the attach does not reach the executable until the next link, so the geometry " + "shader's points input must not constrain this draw"; + DrainErrors(); + + // And once it IS linked in, the rule applies - the fix must not have simply disabled it. + glLinkProgram(program); + glGetProgramiv(program, GL_LINK_STATUS, &linked); + ASSERT_EQ(linked, GL_TRUE) << "the relink with the geometry stage failed"; + glUseProgram(program); + DrainErrors(); + glDrawArrays(GL_TRIANGLES, 0, 3); + EXPECT_EQ(glGetError(), static_cast(GL_INVALID_OPERATION)) + << "now that the points-in geometry shader is in the executable, triangles must be rejected"; + DrainErrors(); + } + + // The tessellation guard above the geometry one had the identical defect, and it does not + // even need the GL_NONE aliasing to misfire: it drives BOTH directions unconditionally, so + // reading the live attach list rejects every non-GL_PATCHES draw the moment an evaluation + // shader is attached, whether or not it was ever linked in. + TEST_F(GeometryDrawModeScenario, AttachingATessEvalStageAfterTheLinkDoesNotForceGlPatches) { + if (!Ready()) GTEST_SKIP(); + + GLint maxPatchVertices = 0; + glGetIntegerv(GL_MAX_PATCH_VERTICES, &maxPatchVertices); + DrainErrors(); + if (maxPatchVertices < 3) GTEST_SKIP() << "no tessellation stage on this backend"; + + const GLuint program = glCreateProgram(); + m_programs.push_back(program); + for (const auto& [stage, source] : + std::vector>{{GL_VERTEX_SHADER, kVertexSource}, + {GL_FRAGMENT_SHADER, kFragmentSource}}) { + const GLuint shader = glCreateShader(stage); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + glAttachShader(program, shader); + glDeleteShader(shader); + } + glLinkProgram(program); + GLint linked = GL_FALSE; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + ASSERT_EQ(linked, GL_TRUE) << "the vertex+fragment program did not link"; + + glUseProgram(program); + DrainErrors(); + + static const char* const kTessEvalSource = R"(#version 420 core +layout(triangles, equal_spacing, ccw) in; +void main() +{ + gl_Position = gl_in[0].gl_Position; +} +)"; + const GLuint tessEval = glCreateShader(GL_TESS_EVALUATION_SHADER); + glShaderSource(tessEval, 1, &kTessEvalSource, nullptr); + glCompileShader(tessEval); + glAttachShader(program, tessEval); + glDeleteShader(tessEval); + DrainErrors(); + + glDrawArrays(GL_TRIANGLES, 0, 3); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)) + << "the executable still has no tessellation stage, so GL_PATCHES must not be required"; + DrainErrors(); + } + } // namespace } // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PostLinkAttachScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PostLinkAttachScenario.cpp new file mode 100644 index 00000000..d8e78823 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/PostLinkAttachScenario.cpp @@ -0,0 +1,325 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/PostLinkAttachScenario.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// Scenario - A PROGRAM'S LIVE ATTACH LIST IS NOT ITS EXECUTABLE, AND THE BACKENDS MAY NOT +// INDEX ONE BY THE OTHER. +// +// GL 4.6 core 7.3: glAttachShader adds to the program's attach list immediately and affects +// what the program RUNS only at the next link (glDetachShader defers its removal the same +// way). So between an attach and the relink the two lists differ - the attach list is +// strictly longer - and the program stays perfectly drawable throughout, with the executable +// its last link produced. +// +// Both backends walked the attach list while indexing the LAST LINK's generated SPIR-V by +// the same running index: +// +// DirectGLES BackendProgramObjectImpl::SyncToBackend - `shaderSpirvs[index]` over +// `attachedShaders.size()` +// DirectVulkan ProgramFactory::GetOrCreateProgram - `spirv[i]` and `moduleSpirvs[i]` +// over `shaders.size()` +// +// One post-link attach therefore read one Vector past the end of the module array and +// copied it, which is the SIGSEGV this scenario is the regression test for (the source +// vector reported a capacity of 35177040171136). DirectGLES additionally derived +// "does this program tessellate" from the same wrong list, which would synthesize a +// pass-through tessellation control stage for an executable that does not tessellate. +// +// The repro needs the attach to land BEFORE the program's first backend build: the ES +// twin's rebuild is gated on the link version (which an attach does not move), so a program +// that was already drawn once keeps its built driver program and never re-reads the list. +// Every case below therefore attaches first and draws second. +// +// Deliberately pinned with a PIXEL and not just with glGetError. "Reject the draw earlier" +// would silence the crash while breaking the spec - GL requires this draw to execute - so +// the assertion has to be that the frame really came out, not merely that nothing complained. +// +// Needs a real context: the crash is in a backend program build, which the GPU-free suites +// never reach. + +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr int kFboWidth = 64; + constexpr int kFboHeight = 64; + + // A full-viewport triangle from gl_VertexID alone, so the scenario needs no vertex + // buffer and every pixel of the target is covered by the one draw. + const char* const kVertexSource = R"(#version 330 core +void main() +{ + vec2 corner = vec2(float((gl_VertexID << 1) & 2), float(gl_VertexID & 2)); + gl_Position = vec4(corner * 2.0 - 1.0, 0.0, 1.0); +} +)"; + + const char* const kFragmentSource = R"(#version 330 core +out vec4 fragColor; +void main() +{ + fragColor = vec4(0.0, 1.0, 0.0, 1.0); +} +)"; + + // The replacement fragment stage of the last case. A different colour, so "which + // executable did this draw run" is answerable from the frame alone. + const char* const kBlueFragmentSource = R"(#version 330 core +out vec4 fragColor; +void main() +{ + fragColor = vec4(0.0, 0.0, 1.0, 1.0); +} +)"; + + constexpr Rgba8 kGreen{0, 255, 0, 255}; + constexpr Rgba8 kBlue{0, 0, 255, 255}; + + // The extra attaches. Each declares a stage the executable ALREADY has and no main(), + // which is what a real shader library looks like and what makes the relink at the end + // of the second case legal. Their whole job here is to make the attach list longer + // than the module array. + const char* const kVertexHelperSource = R"(#version 330 core +vec4 mgPostLinkAttachVertexHelper() +{ + return vec4(0.0, 0.0, 0.0, 1.0); +} +)"; + + const char* const kFragmentHelperSource = R"(#version 330 core +vec4 mgPostLinkAttachFragmentHelper() +{ + return vec4(1.0, 0.0, 1.0, 1.0); +} +)"; + + // A pass-through, so that once it IS linked in the same full-viewport triangle still + // reaches the rasterizer and the final frame is still comparable to the first one. + const char* const kGeometrySource = R"(#version 330 core +layout(triangles) in; +layout(triangle_strip, max_vertices = 3) out; +void main() +{ + for (int i = 0; i < 3; ++i) { + gl_Position = gl_in[i].gl_Position; + EmitVertex(); + } + EndPrimitive(); +} +)"; + + class PostLinkAttachScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + glGenVertexArrays(1, &m_vao); + glBindVertexArray(m_vao); + m_target = MakeColorFbo(kFboWidth, kFboHeight); + ASSERT_NE(m_target.fbo, 0u) << "could not create the scenario's colour target"; + BindFbo(m_target); + DrainErrors(); + } + + void TearDown() override { + if (!Ready()) return; + glUseProgram(0); + for (const GLuint program : m_programs) glDeleteProgram(program); + m_programs.clear(); + for (const GLuint shader : m_shaders) glDeleteShader(shader); + m_shaders.clear(); + BindDefaultFramebuffer(); + DestroyColorFbo(m_target); + glBindVertexArray(0); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + m_vao = 0; + DrainErrors(); + } + + static void DrainErrors() { + for (int i = 0; i < 16 && glGetError() != GL_NO_ERROR; ++i) { + } + } + + static bool BackendHostsGeometry() { + GLint maxGeometryOutputVertices = 0; + glGetIntegerv(GL_MAX_GEOMETRY_OUTPUT_VERTICES, &maxGeometryOutputVertices); + DrainErrors(); + return maxGeometryOutputVertices >= 4; + } + + // Kept alive until TearDown rather than flagged for deletion at attach time: a + // deleted-but-attached shader is a second, unrelated lifetime rule, and this + // scenario is about which LIST the backend reads. + GLuint MakeShader(GLenum stage, const char* source) { + const GLuint shader = glCreateShader(stage); + if (shader == 0) return 0; + m_shaders.push_back(shader); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + return shader; + } + + // Vertex + fragment, linked. This is the executable every case draws with. + // `outFragmentShader` is the stage that paints green, which the last case needs a + // name for in order to detach it. + GLuint LinkBaseProgram(GLuint* outFragmentShader = nullptr) { + const GLuint program = glCreateProgram(); + m_programs.push_back(program); + const GLuint fragment = MakeShader(GL_FRAGMENT_SHADER, kFragmentSource); + glAttachShader(program, MakeShader(GL_VERTEX_SHADER, kVertexSource)); + glAttachShader(program, fragment); + glLinkProgram(program); + GLint linked = GL_FALSE; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + if (!linked) return 0; + if (outFragmentShader != nullptr) *outFragmentShader = fragment; + return program; + } + + // Clears to red, draws the full-viewport triangle, and hands back the frame. Red + // is deliberately the clear colour: a draw that silently did not execute leaves a + // red target, which is a different failure message from a draw that executed and + // painted the wrong thing. + // + // `outDrawError` is sampled between the draw and the readback, so a rejected draw + // is never confused with a readback that went wrong afterwards. + Image DrawFullViewportTriangle(GLuint program, GLenum mode, GLenum* outDrawError = nullptr) { + glUseProgram(program); + ClearTo(1.0f, 0.0f, 0.0f, 1.0f); + DrainErrors(); + glDrawArrays(mode, 0, 3); + if (outDrawError != nullptr) *outDrawError = glGetError(); + return ReadPixels(kFboWidth, kFboHeight); + } + + // The clear colour is red and no shader here ever writes red, so "still red" reads + // as "the draw did not execute" and any other wrong colour as "it executed against + // the wrong modules" - two failures worth telling apart. + static void ExpectFullyColored(const Image& frame, const Rgba8& expected, const char* what) { + ASSERT_FALSE(frame.Empty()) << what << ": nothing was read back"; + for (const int y : {0, kFboHeight / 2, kFboHeight - 1}) { + for (const int x : {0, kFboWidth / 2, kFboWidth - 1}) { + EXPECT_EQ(frame.At(x, y), expected) + << what << ": pixel (" << x << ", " << y << ") is " << frame.ColorName(x, y); + } + } + } + + GLuint m_vao = 0; + ColorFbo m_target{}; + std::vector m_programs; + std::vector m_shaders; + }; + + // THE REGRESSION. Up to four shaders attached after the link (the geometry one only + // where the backend has that stage), two of them duplicating a stage the executable + // already carries - so the attach list runs to five or six while the last link produced + // two modules, and the old loops read indices 2..5 of a 2-element array. + // + // Duplicating a stage is the sharp case on purpose: it is the one shape under which a + // "look the stage up in the attach list instead" repair still returns a valid-looking + // index for a module that does not exist. + TEST_F(PostLinkAttachScenario, DrawingAfterPostLinkAttachesStaysInsideTheGeneratedModules) { + if (!Ready()) GTEST_SKIP(); + + const GLuint program = LinkBaseProgram(); + ASSERT_NE(program, 0u) << "the vertex+fragment program did not link"; + + // Not drawn yet: the ES backend's rebuild is gated on the link version, so a draw + // here would build the driver program from the 2-module executable and the attaches + // below would never be re-read. The repro is the FIRST build seeing the long list. + glAttachShader(program, MakeShader(GL_VERTEX_SHADER, kVertexHelperSource)); + glAttachShader(program, MakeShader(GL_FRAGMENT_SHADER, kFragmentHelperSource)); + if (BackendHostsGeometry()) { + glAttachShader(program, MakeShader(GL_GEOMETRY_SHADER, kGeometrySource)); + } + // The stage that made DirectGLES synthesize a pass-through control stage for a + // program whose executable does not tessellate. Attached whether or not this + // backend can tessellate - an attach needs no support and no successful compile. + const GLuint tessEval = MakeShader(GL_TESS_EVALUATION_SHADER, R"(#version 420 core +layout(triangles, equal_spacing, ccw) in; +void main() +{ + gl_Position = gl_in[0].gl_Position; +} +)"); + if (tessEval != 0) glAttachShader(program, tessEval); + DrainErrors(); + + GLint attachedCount = 0; + glGetProgramiv(program, GL_ATTACHED_SHADERS, &attachedCount); + DrainErrors(); + ASSERT_GT(attachedCount, 2) << "the attaches did not land, so this case is not testing anything"; + + // Still the two-stage executable of three lines ago, and GL says it draws. + GLenum drawError = GL_NO_ERROR; + const Image frame = DrawFullViewportTriangle(program, GL_TRIANGLES, &drawError); + EXPECT_EQ(drawError, static_cast(GL_NO_ERROR)) + << "the attaches have not been linked in, so nothing about them may reject this draw"; + ExpectFullyColored(frame, kGreen, "the post-attach draw"); + DrainErrors(); + } + + // The same window, asked to prove something stronger than "it did not crash": WHICH + // modules the draw in that window ran. Between the detach+attach and the relink the + // program has three attached shaders and two modules, and GL 4.6 core 7.3 says the + // executable is still the one the last link produced - so the frame must come out in + // the OLD fragment shader's colour, not the newly attached one's and not garbage. + // + // This is also the other direction of the fix, so it cannot be "freeze the backend on + // the first link": the relink really does swap the executable, and the very next draw + // has to be rebuilt from it. + TEST_F(PostLinkAttachScenario, TheWindowKeepsTheOldExecutableAndTheRelinkSwapsIt) { + if (!Ready()) GTEST_SKIP(); + + GLuint greenFragment = 0; + const GLuint program = LinkBaseProgram(&greenFragment); + ASSERT_NE(program, 0u) << "the vertex+fragment program did not link"; + + // Both of these are deferred to the next link, in opposite directions: the green + // stage stays in the executable until then, and the blue one stays out of it. + const GLuint blueFragment = MakeShader(GL_FRAGMENT_SHADER, kBlueFragmentSource); + glDetachShader(program, greenFragment); + glAttachShader(program, blueFragment); + DrainErrors(); + + GLenum windowDrawError = GL_NO_ERROR; + const Image inTheWindow = DrawFullViewportTriangle(program, GL_TRIANGLES, &windowDrawError); + EXPECT_EQ(windowDrawError, static_cast(GL_NO_ERROR)) + << "neither the detach nor the attach has been linked in, so the draw must execute"; + ExpectFullyColored(inTheWindow, kGreen, "the draw inside the attach window"); + DrainErrors(); + + glLinkProgram(program); + GLint linked = GL_FALSE; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + ASSERT_EQ(linked, GL_TRUE) << "the relink onto the blue fragment stage failed"; + DrainErrors(); + + GLenum relinkedDrawError = GL_NO_ERROR; + const Image afterRelink = DrawFullViewportTriangle(program, GL_TRIANGLES, &relinkedDrawError); + EXPECT_EQ(relinkedDrawError, static_cast(GL_NO_ERROR)) << "the relinked program must draw"; + ExpectFullyColored(afterRelink, kBlue, "the draw after the relink"); + DrainErrors(); + } + + } // namespace +} // namespace MGITest diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index 13c50f78..ee92e265 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h @@ -155,6 +155,46 @@ namespace MobileGL::MG_State::GLState { // The last link's full input set; empty when this program has never linked (or its // last link had no shaders attached). GL-thread-owned, rebuilt in Link()'s prologue. const Vector& GetLinkedShaderSnapshot() const { return m_linkedShaderSnapshot; } + // "Does this program's EXECUTABLE have this stage" - the only form of the question a + // draw may ask. GetShaderIndexByStage answers it of the live attach list, which by the + // rule above is a different set: glAttachShader adds to that list immediately while + // leaving the executable (and LINK_STATUS) alone, and glDetachShader defers the removal + // to the next Link(), so between an attach and the relink the two disagree in both + // directions. A draw-time stage test that reads the live list therefore starts rejecting + // draws GL requires to execute, against an executable that does not carry the stage at + // all - and stays wrong until the application happens to relink. + Bool HasLinkedShaderStage(ShaderStage stage) const { + return std::any_of(m_linkedShaderSnapshot.begin(), m_linkedShaderSnapshot.end(), + [stage](const LinkedShaderRef& ref) { + return ref.shader && ref.shader->GetShaderStage() == stage; + }); + } + // The stage of each module of GetGeneratedSpirv(), at the SAME index and with the same + // size: phase B emits exactly one module per entry of the snapshot above, in that order + // (Link() fills ProgramLinkTask::in.shaders from the snapshot loop, phase A copies the + // stages straight across into SpirvHandoff::shaderTypes, and GetSpirvBinaryFromProgram + // walks that list). This - never GetAttachedShaders() - is what a consumer of the + // generated SPIR-V must size its loop by and index alongside. + // + // The two lists are NOT interchangeable and cannot be made so: the attach list is live + // and the SPIR-V is a link artifact, so a glAttachShader after a link grows one and not + // the other, with no link in between at which they could be reconciled. A loop that runs + // over the attach list and indexes the SPIR-V therefore reads off the end of it - which + // is a plain out-of-bounds Vector read, not a wrong answer. + // + // Deliberately a Vector and not the shader objects: every consumer wants + // only the stage, and a distinct type is what makes handing it the attach list by + // mistake a compile error rather than a segfault. Built on demand because these callers + // are program-BUILD paths (a backend rebuild, a pipeline cache miss), each of which then + // spends milliseconds compiling the very modules this indexes. + Vector GetLinkedShaderStages() const { + Vector stages; + stages.reserve(m_linkedShaderSnapshot.size()); + for (const LinkedShaderRef& ref : m_linkedShaderSnapshot) { + stages.push_back(ref.shader ? ref.shader->GetShaderStage() : ShaderStage::Unknown); + } + return stages; + } // Pipeline-composite attach: AttachShader plus a pin that makes THIS program's // Link() consume ref's (source, node) instead of the shader's current ones, so a // post-link recompile of the stage program's shader cannot leak into the composite.