diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index 0df2f5f9..28100a2c 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -1875,15 +1875,45 @@ namespace MobileGL::MG_State::GLState { const Int location = explicitLocation != in.explicitFragDataLocation.end() ? static_cast(explicitLocation->second) : static_cast(output.layoutLocation()); - // glBindFragDataLocationIndexed wins over the shader's own qualifier, the same - // precedence the location above follows and the same one ProgramInterface applies. + // The colour INDEX, under the one precedence rule the whole codebase uses: a NON-ZERO + // glBindFragDataLocationIndexed index wins, and a zero (or absent) one falls back to + // the shader's own layout(index = N). + // + // Zero has to mean "no override" rather than "index 0", because glBindFragDataLocation + // IS glBindFragDataLocationIndexed with index 0 (GL_Program.cpp) and writes a real 0 + // into this map. Reading that 0 as an override made a blanket + // `glBindFragDataLocation(prog, 0, "b")` over a shader that declares + // `layout(location = 0, index = 1) out vec4 b;` collapse b onto slot (0,0) next to the + // index-0 output and fail the link as an alias - while the IO resolver had left b's + // qualifier at 1, the SPIR-V still carried Index 1, and glGetProgramResourceLocationIndex + // still answered 1. Validation was rejecting a program the backend had already emitted + // correctly, which is the one case where this branch can change the answer at all: this + // runs AFTER ShaderCompiler::LinkProgram/mapIO, so for every other shape the qualifier + // already carries the resolver's verdict. + // + // The two other consumers spell the same rule: TMglGlslIoResolver only writes the API + // index into the qualifier when it is non-zero, and ProgramInterface falls back to + // type.layoutIndex when GetFragmentDataIndex answers 0. All three now agree. + // + // Against the spec (GL 4.6 core 15.2.3): where a fragment output's index is given by a + // shader layout qualifier, that value is used and anything bound through + // BindFragDataLocation(Indexed) is IGNORED - the same precedence layout(location) has + // over glBindAttribLocation. That is stricter than "non-zero API wins", and the two + // differ in exactly one shape: an explicit `index = 0` in the shader against an API + // index of 1, where the spec keeps 0 and this codebase takes 1. That divergence lives + // in the resolver (it decides what is emitted); it is pre-existing, out of scope here, + // and deliberately not re-litigated in a third place - matching the resolver is what + // keeps validation checking what was actually built. Int colorIndex = 0; if (const auto explicitIndex = in.explicitFragDataIndex.find(outputName); explicitIndex != in.explicitFragDataIndex.end()) { colorIndex = static_cast(explicitIndex->second); - } else if (const glslang::TType* outputType = output.getType(); - outputType != nullptr && outputType->getQualifier().hasIndex()) { - colorIndex = static_cast(outputType->getQualifier().layoutIndex); + } + if (colorIndex == 0) { + if (const glslang::TType* outputType = output.getType(); + outputType != nullptr && outputType->getQualifier().hasIndex()) { + colorIndex = static_cast(outputType->getQualifier().layoutIndex); + } } const Int span = std::max(output.size, 1); diff --git a/MobileGL/MG_Test/Program/ProgramTest.cpp b/MobileGL/MG_Test/Program/ProgramTest.cpp index 107062cc..5d838b82 100644 --- a/MobileGL/MG_Test/Program/ProgramTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramTest.cpp @@ -4097,6 +4097,57 @@ void main() { fragColorA = vec4(1.0); fragColorB = vec4(0.5); } } } +// An API colour index of ZERO is "no override", not "index 0". glBindFragDataLocation is +// glBindFragDataLocationIndexed with index 0 (GL_Program.cpp), so the blanket-bind pattern - +// portable code that binds every output name it knows about, without caring about dual-source - +// writes a real 0 into the frag-data index map for an output whose shader qualifier says 1. +// Reading that 0 as an override collapsed both outputs onto slot (0,0) and failed the link as an +// alias, while the IO resolver had left the qualifier at 1 and the emitted SPIR-V still carried +// Index 1 - validation rejecting a program the backend had already built correctly. +// +// The rule pinned here is the codebase's (non-zero API index wins, zero falls back to the shader +// qualifier), which is also what GL 4.6 core 15.2.3 gives for THIS shape: a shader layout +// qualifier is used and the bound value ignored. +TEST_F(ProgramTest, AnApiColorIndexOfZeroDoesNotOverrideTheShaderIndexQualifier) { + constexpr const char* dualSourceFs = R"(#version 460 core +layout(location = 0, index = 0) out vec4 fragColor0; +layout(location = 0, index = 1) out vec4 fragColor1; +void main() { fragColor0 = vec4(1.0); fragColor1 = vec4(0.5); } +)"; + const GLuint program = CreateProgram(); + const GLuint vs = CreateShader(GL_VERTEX_SHADER); + ShaderSource(vs, 1, &kPassthroughVs, nullptr); + CompileShader(vs); + AttachShader(program, vs); + DeleteShader(vs); + const GLuint fs = CreateShader(GL_FRAGMENT_SHADER); + ShaderSource(fs, 1, &dualSourceFs, nullptr); + CompileShader(fs); + AttachShader(program, fs); + DeleteShader(fs); + + // The blanket bind: colour number 0, index 0, on the output the shader put at index 1. + BindFragDataLocation(program, 0, "fragColor0"); + BindFragDataLocation(program, 0, "fragColor1"); + LinkProgram(program); + GLint linkStatus = GL_FALSE; + GetProgramiv(program, GL_LINK_STATUS, &linkStatus); + EXPECT_EQ(linkStatus, GL_TRUE) << [&] { + char log[512] = ""; + GetProgramInfoLog(program, sizeof(log), nullptr, log); + return std::string(log); + }(); + + // The explicit indexed form with a NON-zero index is still an override, and still links. + BindFragDataLocationIndexed(program, 0, 1, "fragColor1"); + LinkProgram(program); + GetProgramiv(program, GL_LINK_STATUS, &linkStatus); + EXPECT_EQ(linkStatus, GL_TRUE); + EXPECT_EQ(GetFragDataIndex(program, "fragColor1"), 1); + for (int i = 0; i < 32 && GetError() != GL_NO_ERROR; ++i) { + } +} + TEST_F(ProgramTest, GetProgramivReportsTheGeometryStageLinkProperties) { constexpr const char* gs = R"(#version 460 core layout(triangles, invocations = 3) in;