diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 3b157bfe..77c231b2 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -5268,31 +5268,41 @@ namespace MobileGL::MG_Backend::DirectGLES { if (boundFormat == 0) continue; if (!MG_Util::ShaderTranspiler::ShaderCompiler::GLInternalFormatIsCoreEsslImageFormat(boundFormat)) { - if (!g_GLESCapabilities.SupportsExtendedImageFormats) { - // Outside the GLSL ES core set and with no GL_NV_image_formats to spell - // it. If the format widens exactly it is still baked HERE, narrow, and - // WidenImageFormatsForEssl re-declares it in its core carrier immediately - // afterwards - both routes into that pass end in the same place. - // - // If it does not widen there is no legal ESSL for this stage at all. - // Leaving the image format-LESS is NOT a softer failure: all three test - // devices reject a format-less image declaration outright ("all images - // have to define layout format"), readonly and writeonly alike, so it - // trades one hard compile error for another. The unit stays in the - // rebuild key either way, so a rebind to a spellable format still - // rebuilds and works. - if (!ImageFormatWillBeWidened(boundFormat)) { - MGLOG_D("Image uniform '%s' has no declared format and its unit %d holds 0x%x, which " - "GLSL ES core cannot spell, this driver has no GL_NV_image_formats for, and " - "no core format carries exactly.", - name.c_str(), unit, boundFormat); - recordUnspellableFormat( - name, MG_Util::ShaderTranspiler::ShaderCompiler::EsslImageFormatSpelling(boundFormat)); - continue; - } + // The same three-way split the DECLARED branch above makes, and it has to be + // the same one: a format-less image is baked with the bound format, so from + // WidenImageFormatsForEssl's point of view the two routes hand it identical + // modules and must arm it identically. + if (ImageFormatWillBeWidened(boundFormat)) { // The bake writes this format INTO the module, so the widening that runs - // straight after has to be armed for it even though nothing DECLARED it. + // straight after has to be armed for it even though nothing DECLARED it - + // and armed WHETHER OR NOT the driver has GL_NV_image_formats. SPIRV-Cross + // throws for its is_desktop_only_format set the moment it targets ESSL, + // however willing the driver was, so the extension decides HOW MUCH gets + // widened (widenOnlyUnprintableImageFormats) and never WHETHER. Arming + // this only on the no-extension path left the shader half of the widening + // switched off while TextureImpl's storage/bind half - which keys on + // SpirvCrossCanPrintEsslImageFormat, not on the driver bit - still ran: + // the stage threw, the program linked without it, and every dispatch + // silently did nothing. That is the whole of + // KHR-GL43.stencil_texturing.functional's compute half, whose uni_image is + // a format-less uimage2D bound to an R8UI texture. inputs.declaresWidenableImageFormat = true; + } else if (!g_GLESCapabilities.SupportsExtendedImageFormats) { + // Outside the GLSL ES core set, with no GL_NV_image_formats to spell it + // and no core format that carries it exactly: there is no legal ESSL for + // this stage at all. Leaving the image format-LESS is NOT a softer + // failure: all three test devices reject a format-less image declaration + // outright ("all images have to define layout format"), readonly and + // writeonly alike, so it trades one hard compile error for another. The + // unit stays in the rebuild key either way, so a rebind to a spellable + // format still rebuilds and works. + MGLOG_D("Image uniform '%s' has no declared format and its unit %d holds 0x%x, which " + "GLSL ES core cannot spell, this driver has no GL_NV_image_formats for, and " + "no core format carries exactly.", + name.c_str(), unit, boundFormat); + recordUnspellableFormat( + name, MG_Util::ShaderTranspiler::ShaderCompiler::EsslImageFormatSpelling(boundFormat)); + continue; } else { inputs.needsExtendedImageFormats = true; } diff --git a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp index fe5cc80b..e73c1dd6 100644 --- a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp +++ b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp @@ -230,8 +230,18 @@ namespace MobileGL::MG_Impl::GLImpl { // input primitive (GL 4.6 core 11.3.1); anything else is INVALID_OPERATION. GL_PATCHES // is the tessellation pipeline's input and reaches the geometry stage already // converted, so it is not constrained here. - const GLenum gsInput = currentProgram ? currentProgram->GetGeometryInputType() : GL_NONE; - if (gsInput != GL_NONE && mode != GL_PATCHES) { + // + // "Is there a geometry stage at all" has to be asked of the STAGE, never of the input + // primitive: GL_NONE and GL_POINTS are both 0, so a `layout(points) in` geometry shader + // is indistinguishable from no geometry shader by its reflected input type alone. The + // sentinel test this replaces therefore skipped the whole rule for exactly the geometry + // 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; + const GLenum gsInput = geometryActive ? currentProgram->GetGeometryInputType() : GL_NONE; + if (geometryActive && mode != GL_PATCHES) { Bool compatible = false; switch (gsInput) { case GL_POINTS: diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 04a76493..476dca96 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -87,6 +87,8 @@ add_executable(MobileGLIntegrationTest Scenarios/Glsl420DeclarationScenario.cpp Scenarios/IoBlockNameCollisionScenario.cpp Scenarios/TessellationDrawModeScenario.cpp + Scenarios/GeometryDrawModeScenario.cpp + Scenarios/FormatlessImageBakeScenario.cpp Scenarios/FragmentOutputArrayIndexScenario.cpp Scenarios/BufferTextureScenario.cpp Scenarios/VertexAttribBindingScenario.cpp diff --git a/MobileGL/MG_IntegrationTest/Scenarios/FormatlessImageBakeScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/FormatlessImageBakeScenario.cpp new file mode 100644 index 00000000..68b9a186 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/FormatlessImageBakeScenario.cpp @@ -0,0 +1,211 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/FormatlessImageBakeScenario.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 FORMAT-LESS IMAGE UNIFORM WHOSE UNIT HOLDS A NON-CORE FORMAT. +// +// GLSL 4.20 lets a write-only image uniform omit its layout format; GLSL ES demands one, so +// DirectGLES BAKES the format of whatever glBindImageTexture put on the unit into the +// declaration. When that format is outside the GLSL ES core thirteen, the bake alone is not +// enough - the baked declaration then has to go through the same channel-widening +// WidenImageFormatsForEssl gives a DECLARED non-core format (see NonCoreImageFormatScenario for +// the widening itself). +// +// The two routes had different arming. The declared route armed the widening on the format +// alone; the baked route armed it only when the driver lacked GL_NV_image_formats. That reads +// like an optimisation and is not one: SPIRV-Cross throws for its is_desktop_only_format set the +// moment it targets ESSL, whatever the driver would have accepted, so on a driver that HAS the +// extension the shader half of the widening stayed switched off while TextureImpl's storage/bind +// half - which keys on SpirvCrossCanPrintEsslImageFormat, not on the driver bit - still ran. The +// stage threw, the program linked without it, and every dispatch silently did nothing. +// +// KHR-GL43.stencil_texturing.functional is where it surfaced: its compute half writes through a +// format-less `uimage2D` bound to an R8UI texture, and returned zeros for every texel. +// +// DISCRIMINATING ONLY WHERE THE DRIVER ADVERTISES GL_NV_image_formats - Mesa does, which is what +// the software lanes run and where this was found. On Adreno 830 and both Malis the extension is +// absent, the old code already armed the widening, and these cases pass before and after; they +// are kept running there as a guard against the opposite mistake. + +#include +#include +#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 kExtent = 8; + + // No layout format on uni_image on purpose: that is the whole subject. uni_source is a + // plain integer texture so nothing but the image declaration is in play. + const char* const kComputeSource = R"(#version 430 core +layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; +writeonly uniform uimage2D uni_image; +uniform usampler2D uni_source; +void main() +{ + ivec2 at = ivec2(gl_GlobalInvocationID.xy); + imageStore(uni_image, at, uvec4(texelFetch(uni_source, at, 0).r, 0u, 0u, 0u)); +} +)"; + + class FormatlessImageBakeScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + if (!BackendHostsCompute()) { + GTEST_SKIP() << "no compute stage on " << Gl().BackendName() << " (" + << Gl().RendererString() << ")"; + } + } + + static bool BackendHostsCompute() { + GLint maxImageUnits = 0; + glGetIntegerv(GL_MAX_IMAGE_UNITS, &maxImageUnits); + DrainErrors(); + return maxImageUnits >= 2; + } + + static void DrainErrors() { + for (int i = 0; i < 16 && glGetError() != GL_NO_ERROR; ++i) { + } + } + + static GLuint BuildCompute(const char* source, std::string& log) { + const GLuint cs = glCreateShader(GL_COMPUTE_SHADER); + glShaderSource(cs, 1, &source, nullptr); + glCompileShader(cs); + GLint ok = 0; + glGetShaderiv(cs, GL_COMPILE_STATUS, &ok); + if (!ok) { + char buffer[2048] = ""; + glGetShaderInfoLog(cs, sizeof(buffer), nullptr, buffer); + log = buffer; + glDeleteShader(cs); + return 0; + } + const GLuint program = glCreateProgram(); + glAttachShader(program, cs); + glLinkProgram(program); + glGetProgramiv(program, GL_LINK_STATUS, &ok); + glDeleteShader(cs); + if (!ok) { + char buffer[2048] = ""; + glGetProgramInfoLog(program, sizeof(buffer), nullptr, buffer); + log = buffer; + glDeleteProgram(program); + return 0; + } + return program; + } + + // internalFormat is the NON-CORE image format under test; the destination texture and + // the glBindImageTexture argument both use it, and the shader declares nothing. + void RunCopy(GLenum internalFormat, GLenum uploadFormat, GLenum uploadType) { + std::vector expected(kExtent * kExtent); + for (int i = 0; i < kExtent * kExtent; ++i) { + expected[i] = static_cast(1 + i); + } + + // Source: a core-format integer texture holding 1..64. + std::vector sourceBytes(kExtent * kExtent); + for (int i = 0; i < kExtent * kExtent; ++i) { + sourceBytes[i] = static_cast(expected[i]); + } + GLuint sourceTexture = 0; + glGenTextures(1, &sourceTexture); + glBindTexture(GL_TEXTURE_2D, sourceTexture); + glTexStorage2D(GL_TEXTURE_2D, 1, GL_R8UI, kExtent, kExtent); + glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, kExtent, kExtent, GL_RED_INTEGER, GL_UNSIGNED_BYTE, + sourceBytes.data()); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + + // Destination: the format under test, zero-filled so "the dispatch did nothing" + // and "the dispatch wrote zeros" are the same observation the CTS made. + GLuint destTexture = 0; + glGenTextures(1, &destTexture); + glBindTexture(GL_TEXTURE_2D, destTexture); + glTexStorage2D(GL_TEXTURE_2D, 1, internalFormat, kExtent, kExtent); + const std::vector zeros(static_cast(kExtent) * kExtent * 8, 0); + glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, kExtent, kExtent, uploadFormat, uploadType, zeros.data()); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)) << "destination storage"; + + std::string log; + const GLuint program = BuildCompute(kComputeSource, log); + ASSERT_NE(program, 0u) << "the format-less image program did not build: " << log; + + glUseProgram(program); + glBindImageTexture(1, destTexture, 0, GL_FALSE, 0, GL_WRITE_ONLY, internalFormat); + glUniform1i(glGetUniformLocation(program, "uni_image"), 1); + glActiveTexture(GL_TEXTURE1); + glBindTexture(GL_TEXTURE_2D, sourceTexture); + glUniform1i(glGetUniformLocation(program, "uni_source"), 1); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)) << "binding"; + + glDispatchCompute(kExtent, kExtent, 1); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)) << "dispatch"; + + std::vector readback(kExtent * kExtent, 0xFFFFFFFFu); + glActiveTexture(GL_TEXTURE0); + glBindTexture(GL_TEXTURE_2D, destTexture); + glGetTexImage(GL_TEXTURE_2D, 0, GL_RED_INTEGER, GL_UNSIGNED_INT, readback.data()); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)) << "readback"; + + int offenders = 0; + for (int i = 0; i < kExtent * kExtent; ++i) { + if (readback[i] != expected[i]) ++offenders; + } + EXPECT_EQ(offenders, 0) << "the dispatch wrote " << offenders << " of " + << (kExtent * kExtent) << " texels wrongly; texel 0 was " + << readback[0] << ", expected " << expected[0] + << ". A whole stage lost to the ESSL emitter looks exactly like this."; + + glUseProgram(0); + glDeleteProgram(program); + glDeleteTextures(1, &sourceTexture); + glDeleteTextures(1, &destTexture); + DrainErrors(); + } + }; + + // R8UI: one of the seven formats GLSL ES reaches only through GL_NV_image_formats AND one + // SPIRV-Cross refuses to print for ESSL, so it needs the widening in both driver modes. + TEST_F(FormatlessImageBakeScenario, R8uiBakedFromTheBoundUnitStillReachesTheDriver) { + if (!Ready()) GTEST_SKIP(); + RunCopy(GL_R8UI, GL_RED_INTEGER, GL_UNSIGNED_BYTE); + } + + // R16UI, from the same set, carried in RGBA16UI: the fix must not be R8UI-shaped. + TEST_F(FormatlessImageBakeScenario, R16uiBakedFromTheBoundUnitStillReachesTheDriver) { + if (!Ready()) GTEST_SKIP(); + RunCopy(GL_R16UI, GL_RED_INTEGER, GL_UNSIGNED_SHORT); + } + + // The control: R32UI is in the GLSL ES core thirteen, so it is baked and never widened. + // It passed before the fix and has to keep passing. + TEST_F(FormatlessImageBakeScenario, CoreFormatBakedFromTheBoundUnitIsUnaffected) { + if (!Ready()) GTEST_SKIP(); + RunCopy(GL_R32UI, GL_RED_INTEGER, GL_UNSIGNED_INT); + } + + } // namespace +} // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/GeometryDrawModeScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/GeometryDrawModeScenario.cpp new file mode 100644 index 00000000..bf8a57c4 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/GeometryDrawModeScenario.cpp @@ -0,0 +1,298 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/GeometryDrawModeScenario.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 GEOMETRY SHADER'S INPUT PRIMITIVE CONSTRAINS THE DRAW MODE, AND +// GL_NONE IS NOT A USABLE "NO GEOMETRY SHADER" SENTINEL. +// +// GL 4.6 core 11.3.1: mode must be one of the primitive types that decomposes into the +// geometry shader's declared input primitive, or the draw is GL_INVALID_OPERATION. The +// validator asked "is there a geometry stage?" by comparing the REFLECTED INPUT PRIMITIVE +// against GL_NONE - and GL_NONE and GL_POINTS are both 0, so a `layout(points) in` geometry +// shader answered "no geometry stage" and every mode sailed through. The rule was therefore +// dead for exactly the geometry shaders whose input primitive rejects the most modes. +// +// KHR-GL43.transform_feedback.api_errors_test is where it showed: it draws a points-in +// geometry program with GL_LINES through glDrawTransformFeedbackInstanced and requires +// INVALID_OPERATION. The bug is not specific to that entry point - every draw shares this +// validator - so the ordinary glDrawArrays spelling is pinned here too, and the lines-in +// program is the control that proves the rule was not simply widened. +// +// Needs a real context: the validator returns before this rule when no backend object is +// active, so the GPU-free negative-API suite cannot reach it. + +#include +#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 { + + const char* const kVertexSource = R"(#version 420 core +void main() +{ + gl_Position = vec4(0.0, 0.0, 0.0, 1.0); +} +)"; + + // The input primitive the CTS case uses, and the one the GL_NONE sentinel erased. + // `result` is here so the same program can be captured with transform feedback. + const char* const kPointsInGeometrySource = R"(#version 420 core +layout(points) in; +layout(points, max_vertices = 1) out; +out float result; +void main() +{ + gl_Position = gl_in[0].gl_Position; + result = 1.0; + EmitVertex(); +} +)"; + + const char* const kLinesInGeometrySource = R"(#version 420 core +layout(lines) in; +layout(points, max_vertices = 1) out; +void main() +{ + gl_Position = gl_in[0].gl_Position; + EmitVertex(); +} +)"; + + const char* const kFragmentSource = R"(#version 420 core +out vec4 fragColor; +void main() +{ + fragColor = vec4(0.0, 1.0, 0.0, 1.0); +} +)"; + + class GeometryDrawModeScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + glGenVertexArrays(1, &m_vao); + glBindVertexArray(m_vao); + if (!BackendHostsGeometry()) { + GTEST_SKIP() << "no geometry stage on " << Gl().BackendName() << " (" + << Gl().RendererString() << "); there is no input primitive to validate"; + } + } + + void TearDown() override { + if (!Ready()) return; + glUseProgram(0); + for (const GLuint program : m_programs) { + glDeleteProgram(program); + } + m_programs.clear(); + glBindVertexArray(0); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + m_vao = 0; + } + + // The same real-backend probe IoBlockNameCollisionScenario uses: 0 on a DirectGLES + // driver without GL_EXT_geometry_shader and on a DirectVulkan device without the + // geometryShader feature. + static bool BackendHostsGeometry() { + GLint maxGeometryOutputVertices = 0; + glGetIntegerv(GL_MAX_GEOMETRY_OUTPUT_VERTICES, &maxGeometryOutputVertices); + DrainErrors(); + return maxGeometryOutputVertices >= 4; + } + + static void DrainErrors() { + for (int i = 0; i < 16 && glGetError() != GL_NO_ERROR; ++i) { + } + } + + GLuint BuildProgram(const char* geometrySource, const char* capturedVarying = nullptr) { + const std::vector> stages = { + {GL_VERTEX_SHADER, kVertexSource}, + {GL_GEOMETRY_SHADER, geometrySource}, + {GL_FRAGMENT_SHADER, kFragmentSource}}; + + std::vector shaders; + bool ok = true; + for (const auto& [stage, source] : stages) { + const GLuint shader = glCreateShader(stage); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + GLint compiled = 0; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + shaders.push_back(shader); + if (!compiled) { + m_buildLog = InfoLog(shader, true); + ok = false; + break; + } + } + if (!ok) { + for (const GLuint shader : shaders) glDeleteShader(shader); + return 0; + } + + const GLuint program = glCreateProgram(); + for (const GLuint shader : shaders) glAttachShader(program, shader); + if (capturedVarying != nullptr) { + glTransformFeedbackVaryings(program, 1, &capturedVarying, GL_INTERLEAVED_ATTRIBS); + } + glLinkProgram(program); + GLint linked = 0; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + for (const GLuint shader : shaders) glDeleteShader(shader); + if (!linked) { + m_buildLog = InfoLog(program, false); + glDeleteProgram(program); + return 0; + } + m_programs.push_back(program); + return program; + } + + static std::string InfoLog(GLuint object, bool isShader) { + GLint length = 0; + if (isShader) { + glGetShaderiv(object, GL_INFO_LOG_LENGTH, &length); + } else { + glGetProgramiv(object, GL_INFO_LOG_LENGTH, &length); + } + std::vector buffer(static_cast(length) + 1, '\0'); + if (isShader) { + glGetShaderInfoLog(object, length + 1, nullptr, buffer.data()); + } else { + glGetProgramInfoLog(object, length + 1, nullptr, buffer.data()); + } + return buffer.data(); + } + + const std::string& BuildLog() const { return m_buildLog; } + + GLuint m_vao = 0; + std::vector m_programs; + std::string m_buildLog; + }; + + // GL_POINTS is the only mode that decomposes into a points input primitive. + TEST_F(GeometryDrawModeScenario, PointsInGeometryProgramRejectsEveryOtherMode) { + if (!Ready()) GTEST_SKIP(); + + const GLuint program = BuildProgram(kPointsInGeometrySource); + ASSERT_NE(program, 0u) << "the points-in geometry program did not build: " << BuildLog(); + + glUseProgram(program); + DrainErrors(); + + for (const GLenum mode : + {static_cast(GL_LINES), static_cast(GL_LINE_STRIP), + static_cast(GL_TRIANGLES), static_cast(GL_TRIANGLE_STRIP)}) { + glDrawArrays(mode, 0, 3); + EXPECT_EQ(glGetError(), static_cast(GL_INVALID_OPERATION)) + << "mode " << mode << " does not decompose into the geometry shader's points input"; + DrainErrors(); + } + + // The one mode that IS compatible still draws. + glDrawArrays(GL_POINTS, 0, 1); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + DrainErrors(); + } + + // The same rule reached through glDrawTransformFeedback*, which is the spelling the CTS + // case asks about. The capture span is really completed first, so GL_POINTS comes back + // GL_NO_ERROR: without that the draw would report INVALID_OPERATION for the + // never-ended-a-span reason instead and the case could not tell the two apart. + TEST_F(GeometryDrawModeScenario, PointsInGeometryProgramRejectsNonPointModesOnFeedbackDraws) { + if (!Ready()) GTEST_SKIP(); + + const GLuint program = BuildProgram(kPointsInGeometrySource, "result"); + ASSERT_NE(program, 0u) << "the points-in geometry program did not build: " << BuildLog(); + + GLuint feedback = 0; + glGenTransformFeedbacks(1, &feedback); + glBindTransformFeedback(GL_TRANSFORM_FEEDBACK, feedback); + GLuint captureBuffer = 0; + glGenBuffers(1, &captureBuffer); + glBindBuffer(GL_TRANSFORM_FEEDBACK_BUFFER, captureBuffer); + glBufferData(GL_TRANSFORM_FEEDBACK_BUFFER, 64, nullptr, GL_STATIC_DRAW); + glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, captureBuffer); + glUseProgram(program); + DrainErrors(); + + glBeginTransformFeedback(GL_POINTS); + glDrawArrays(GL_POINTS, 0, 1); + glEndTransformFeedback(); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)) << "the capture span did not complete"; + + glDrawTransformFeedbackInstanced(GL_LINES, feedback, 1); + EXPECT_EQ(glGetError(), static_cast(GL_INVALID_OPERATION)) + << "glDrawTransformFeedbackInstanced must honour the geometry input primitive"; + DrainErrors(); + + glDrawTransformFeedbackStreamInstanced(GL_LINES, feedback, 0, 1); + EXPECT_EQ(glGetError(), static_cast(GL_INVALID_OPERATION)) + << "glDrawTransformFeedbackStreamInstanced must honour the geometry input primitive"; + DrainErrors(); + + // The compatible mode replays the captured span with no error at all, which is what + // makes the two assertions above about the MODE and not about the span. + glDrawTransformFeedbackInstanced(GL_POINTS, feedback, 1); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)) + << "a compatible mode must still replay the captured span"; + DrainErrors(); + + glUseProgram(0); + glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, 0); + glBindBuffer(GL_TRANSFORM_FEEDBACK_BUFFER, 0); + glDeleteBuffers(1, &captureBuffer); + glBindTransformFeedback(GL_TRANSFORM_FEEDBACK, 0); + glDeleteTransformFeedbacks(1, &feedback); + DrainErrors(); + } + + // The control: a lines-in geometry shader is a NON-zero input primitive, so it exercised + // the rule even before the fix. It must still accept the line modes and still reject the + // others - a fix that widened the rule instead of repairing its guard breaks this. + TEST_F(GeometryDrawModeScenario, LinesInGeometryProgramStillAcceptsLineModesOnly) { + if (!Ready()) GTEST_SKIP(); + + const GLuint program = BuildProgram(kLinesInGeometrySource); + ASSERT_NE(program, 0u) << "the lines-in geometry program did not build: " << BuildLog(); + + glUseProgram(program); + DrainErrors(); + + for (const GLenum mode : {static_cast(GL_LINES), static_cast(GL_LINE_STRIP), + static_cast(GL_LINE_LOOP)}) { + glDrawArrays(mode, 0, 2); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)) + << "mode " << mode << " decomposes into lines and must be accepted"; + DrainErrors(); + } + + for (const GLenum mode : {static_cast(GL_POINTS), static_cast(GL_TRIANGLES)}) { + glDrawArrays(mode, 0, 3); + EXPECT_EQ(glGetError(), static_cast(GL_INVALID_OPERATION)) + << "mode " << mode << " does not decompose into lines"; + DrainErrors(); + } + } + + } // namespace +} // namespace MGITest diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index 69dbfc2a..3860b355 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -1023,6 +1023,12 @@ namespace MobileGL::MG_State::GLState { return uniform.index >= 0 && uniform.index < static_cast(artifacts.tProgramBlockIndexToGl.size()) && artifacts.tProgramBlockIndexToGl[uniform.index] < 0; }; + // Member of a block GL can see - a named uniform block, a buffer block, or the + // synthesized atomic-counter block. GL locations are a property of the DEFAULT uniform + // block alone (GL 4.6 core 7.6.1), so these take none. + const auto isNamedBlockMember = [&isGlobalUboMember](const glslang::TObjectReflection& uniform) { + return uniform.index >= 0 && !isGlobalUboMember(uniform); + }; for (Int i = 0; i < tProgramUniformCount; i++) { const auto& uniform = artifacts.program->getUniform(i); if (isGlobalUboMember(uniform) && uniform.stages == 0) { @@ -1069,8 +1075,7 @@ namespace MobileGL::MG_State::GLState { for (const Int i : artifacts.glUniformIndexToTProgram) { const auto& uniform = artifacts.program->getUniform(i); const glslang::TType* type = uniform.getType(); - const Bool inNamedBlock = uniform.index >= 0 && !isGlobalUboMember(uniform); - if (inNamedBlock) continue; // block members never take glUniform locations + if (isNamedBlockMember(uniform)) continue; // block members never take glUniform locations if (const Int* explicitLocation = findExplicitLocation(uniform.name)) { effectiveLocation[i] = static_cast(*explicitLocation); @@ -1145,20 +1150,23 @@ namespace MobileGL::MG_State::GLState { in.externalIndex, uniform.name.c_str(), location, location + locationSpan - 1); } + // Counts ONLY default-block uniforms, which is the whole of what a GL uniform location + // is and the whole of what GL_MAX_UNIFORM_LOCATIONS bounds (GL 4.6 core 7.6.1). A + // named-block member used to be counted here too and used to be handed a location by the + // first-fit pass below, which is a spec violation twice over: glGetUniformLocation must + // answer -1 for it (glGetProgramResourceLocation already did), and every slot it took + // pushed a real default-block uniform one location further up. On a program with a + // buffer block that is exactly how a location EQUAL to the advertised maximum got minted + // - the table's ceiling is raised to hold this count, so one extra block member raised it + // to MAX and the first-fit pass then filled the last slot + // (KHR-GL43.explicit_uniform_location.uniform-loc-mix-with-implicit-max, whose compute + // program carries an SSBO; its -max-array sibling ran the pool out and failed to link). Int requiredUniformLocations = deadReservedLocationCount; - // The same count restricted to DEFAULT-BLOCK uniforms, which is the only thing - // GL_MAX_UNIFORM_LOCATIONS bounds. requiredUniformLocations cannot serve: it also carries - // named-block members, which take a slot in this allocator's table (an implementation - // detail) but consume no GL uniform location at all, so a big UBO array would otherwise - // fail a link the spec allows. - Int defaultBlockLocationDemand = deadReservedLocationCount; for (const Int i : artifacts.glUniformIndexToTProgram) { auto& uniform = artifacts.program->getUniform(i); const Uint location = effectiveLocation[i]; const Int locationSpan = GetUniformLocationSpan(uniform); - requiredUniformLocations += locationSpan; - const Bool inNamedBlock = uniform.index >= 0 && !isGlobalUboMember(uniform); - if (!inNamedBlock) defaultBlockLocationDemand += locationSpan; + if (!isNamedBlockMember(uniform)) requiredUniformLocations += locationSpan; if (location != kNoLocation) { artifacts.maxUniformLocation = std::max(artifacts.maxUniformLocation, location + locationSpan - 1); } @@ -1177,11 +1185,11 @@ namespace MobileGL::MG_State::GLState { // (KHR-GL43.explicit_uniform_location.uniform-loc-negative-link-max-num-of-locations). // A single uniform whose own span passes the ceiling was already rejected above; this is // the aggregate half of the same rule. - if (defaultBlockLocationDemand > static_cast(kMaxUniformLocations)) { + if (requiredUniformLocations > static_cast(kMaxUniformLocations)) { artifacts.infoLog = std::format("Uniform locations exhausted: the default-block uniforms need {} locations but " "GL_MAX_UNIFORM_LOCATIONS is {}.", - defaultBlockLocationDemand, kMaxUniformLocations); + requiredUniformLocations, kMaxUniformLocations); DeferLog(std::format("ProgramObject {}: Link failed - {}", in.externalIndex, artifacts.infoLog)); ProgramObject::ResetLinkArtifacts(artifacts); return false; @@ -1254,6 +1262,10 @@ namespace MobileGL::MG_State::GLState { // is demoted to the first-fit pass below instead of failing the link. for (const Int i : artifacts.glUniformIndexToTProgram) { auto& uniform = artifacts.program->getUniform(i); + // Same rule the effective-location loop applies: a block member has no GL location, + // so it must not reach the first-fit pass either. Its uniformLocations entry stays + // at kNoLocation, which glGetUniformLocation reads back as the -1 the spec wants. + if (isNamedBlockMember(uniform)) continue; if (locationIsSourceExplicit[i]) continue; const Uint location = effectiveLocation[i]; if (location == kNoLocation) { diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index ce58c551..13c50f78 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h @@ -373,6 +373,13 @@ namespace MobileGL::MG_State::GLState { const auto& uniform = UniformAt(TProgramUniformIndex(index)); if (GlBlockIndexFromTProgram(uniform.index) < 0) return -1; if (!uniform.type.isArray) return 0; + // An atomic counter reaches the std140 branch below only because the transpiler + // lowered it onto a synthesized block; the buffer it actually addresses is an + // ATOMIC COUNTER buffer, whose elements are tightly packed uints (GL 4.6 core 7.6: + // "each counter is a single 4-byte value"). Its array stride is therefore 4, not the + // vec4 round-up std140 would apply + // (KHR-GL43.shader_atomic_counters.basic-program-query wants 4 for ac_counter67[0]). + if (IsActiveUniformAtomicCounter(index)) return 4; if (uniform.type.isMatrix) { const bool rowMajor = GetActiveUniformIsRowMajor(index) != 0; const int vectors = rowMajor ? uniform.type.matrixRows : uniform.type.matrixCols; diff --git a/MobileGL/MG_Test/Program/ProgramTest.cpp b/MobileGL/MG_Test/Program/ProgramTest.cpp index 0383837a..da9c2e1b 100644 --- a/MobileGL/MG_Test/Program/ProgramTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramTest.cpp @@ -3401,3 +3401,182 @@ void main() { fragColor = vec4(1.0); } } EXPECT_EQ(GetError(), GL_NO_ERROR); } + +// GL 4.6 core 7.6: an atomic counter is a default-block uniform that addresses an ATOMIC COUNTER +// buffer, where every counter is a tightly packed 4-byte value. MobileGL lowers each atomic_uint +// onto a synthesized block, which used to drag the whole array-stride query onto the std140 rule +// that rounds an element stride up to a vec4 - so an atomic counter array reported 16 +// (KHR-GL43.shader_atomic_counters.basic-program-query: "GL_UNIFORM_ARRAY_STRIDE is 16 should be +// 4"). The offsets, matrix stride and row-major flag are pinned alongside it because the same +// synthesized block feeds all four queries. +TEST_F(ProgramTest, AtomicCounterArrayReportsThePackedFourByteStride) { + const char* vsSource = R"(#version 430 core +void main() { gl_Position = vec4(1.0); } +)"; + const char* fsSource = R"(#version 430 core +layout(location = 0) out vec4 o_color; +layout(binding = 0, offset = 0) uniform atomic_uint ac_counter0; +layout(binding = 0, offset = 4) uniform atomic_uint ac_counter1; +layout(binding = 0) uniform atomic_uint ac_counter2; +layout(binding = 0) uniform atomic_uint ac_counter67[2]; +layout(binding = 0) uniform atomic_uint ac_counter3; +void main() { + uint c = 0u; + c += atomicCounterIncrement(ac_counter0); + c += atomicCounterIncrement(ac_counter1); + c += atomicCounterIncrement(ac_counter2); + c += atomicCounterIncrement(ac_counter3); + c += atomicCounterIncrement(ac_counter67[0]); + c += atomicCounterIncrement(ac_counter67[1]); + o_color = vec4(float(c)); +} +)"; + const GLuint vs = CompileShaderChecked(GL_VERTEX_SHADER, vsSource); + const GLuint fs = CompileShaderChecked(GL_FRAGMENT_SHADER, fsSource); + const GLuint program = LinkVsFs(vs, fs, GL_TRUE); + + GLint activeUniforms = 0; + GetProgramiv(program, GL_ACTIVE_UNIFORMS, &activeUniforms); + ASSERT_EQ(activeUniforms, 5); + + // Declared offset -> expected {array size, array stride}. layout(offset=) pins the first two; + // the rest are packed after them in declaration order, the array taking two 4-byte slots. + struct Expectation { + const char* name; + GLint size; + GLint offset; + GLint arrayStride; + }; + const Expectation expectations[] = { + {"ac_counter0", 1, 0, 0}, {"ac_counter1", 1, 4, 0}, {"ac_counter2", 1, 8, 0}, + {"ac_counter67[0]", 2, 12, 4}, {"ac_counter3", 1, 20, 0}, + }; + + for (const auto& expected : expectations) { + const char* queryName = expected.name; + GLuint index = GL_INVALID_INDEX; + GetUniformIndices(program, 1, &queryName, &index); + ASSERT_NE(index, GL_INVALID_INDEX) << expected.name << " is not an active uniform"; + + GLint value = -2; + GetActiveUniformsiv(program, 1, &index, GL_UNIFORM_TYPE, &value); + EXPECT_EQ(value, static_cast(GL_UNSIGNED_INT_ATOMIC_COUNTER)) << expected.name; + GetActiveUniformsiv(program, 1, &index, GL_UNIFORM_SIZE, &value); + EXPECT_EQ(value, expected.size) << expected.name; + // An atomic counter is a default-block uniform however it was lowered. + GetActiveUniformsiv(program, 1, &index, GL_UNIFORM_BLOCK_INDEX, &value); + EXPECT_EQ(value, -1) << expected.name; + GetActiveUniformsiv(program, 1, &index, GL_UNIFORM_OFFSET, &value); + EXPECT_EQ(value, expected.offset) << expected.name; + GetActiveUniformsiv(program, 1, &index, GL_UNIFORM_ARRAY_STRIDE, &value); + EXPECT_EQ(value, expected.arrayStride) << expected.name; + GetActiveUniformsiv(program, 1, &index, GL_UNIFORM_MATRIX_STRIDE, &value); + EXPECT_EQ(value, 0) << expected.name; + GetActiveUniformsiv(program, 1, &index, GL_UNIFORM_IS_ROW_MAJOR, &value); + EXPECT_EQ(value, 0) << expected.name; + GetActiveUniformsiv(program, 1, &index, GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX, &value); + EXPECT_EQ(value, 0) << expected.name; + } + EXPECT_EQ(GetError(), GL_NO_ERROR); +} + +// GL 4.6 core 7.6.1: a uniform LOCATION is a property of the default uniform block. A member of +// a named uniform block or a buffer block has none, and glGetUniformLocation must answer -1 for +// it - which is what glGetProgramResourceLocation(GL_UNIFORM, ...) already did, so the two used +// to disagree. The location such a member was handed was not merely reported, it was CONSUMED: +// it came out of the same first-fit table the default-block uniforms draw from. +TEST_F(ProgramTest, BlockMembersConsumeNoUniformLocation) { + const char* csSource = R"(#version 430 core +layout(local_size_x = 1) in; +layout(std430, binding = 1) buffer ResultBuffer { vec4 bufferMember; }; +layout(std140, binding = 2) uniform SettingsBlock { vec4 blockMember; }; +layout(location = 0) uniform float uDead[3]; +uniform float uImplicit; +void main() { bufferMember = blockMember * uImplicit; } +)"; + const GLuint cs = CompileShaderChecked(GL_COMPUTE_SHADER, csSource); + const GLuint program = CreateProgram(); + AttachShader(program, cs); + LinkProgram(program); + GLint linkStatus = GL_FALSE; + GetProgramiv(program, GL_LINK_STATUS, &linkStatus); + char infoLog[1024] = ""; + GetProgramInfoLog(program, sizeof(infoLog), nullptr, infoLog); + ASSERT_EQ(linkStatus, GL_TRUE) << infoLog; + + for (const char* member : {"bufferMember", "blockMember"}) { + EXPECT_EQ(GetUniformLocation(program, member), -1) << member << " is a block member, not a GL uniform"; + EXPECT_EQ(GetProgramResourceLocation(program, GL_UNIFORM, member), -1) + << member << ": the two location queries must agree"; + } + + // uDead[3] reserves 0..2 without becoming visible, so the first location left for the one + // default-block uniform is 3. It used to be 4, because a block member took 3 first. + EXPECT_EQ(GetUniformLocation(program, "uImplicit"), 3) + << "a block member consumed a location the default-block uniform was entitled to"; + EXPECT_EQ(GetUniformLocation(program, "uDead"), -1); + EXPECT_EQ(GetError(), GL_NO_ERROR); +} + +// The same defect at the boundary, which is where the conformance suite catches it. The location +// table's ceiling is raised to hold every uniform it must place; counting block members into that +// raise pushed the ceiling to GL_MAX_UNIFORM_LOCATIONS itself, and the first-fit pass then handed +// out the one location past the legal 0..MAX-1 range +// (KHR-GL43.explicit_uniform_location.uniform-loc-mix-with-implicit-max, whose compute program +// carries an SSBO: "Uniform u2 returned location (4095) is greater than implementation dependent +// limit (4095)"). Its -array sibling shares the root cause and failed one step further along, with +// the pool reported exhausted and no link at all. +TEST_F(ProgramTest, ImplicitLocationStaysInRangeWhenABufferBlockSharesTheProgram) { + GLint maxLocations = 0; + GetIntegerv(GL_MAX_UNIFORM_LOCATIONS, &maxLocations); + ASSERT_GE(maxLocations, 1024) << "GL 4.3 requires at least 1024 uniform locations"; + + // The CTS shape: explicit unused arrays fill the pool except for a hole of `implicitCount` + // locations at `holeBase`, and the one implicit uniform must land exactly in that hole. + const auto runCase = [&](int holeBase, int implicitCount) { + String decls; + int nextName = 0; + if (holeBase > 0) { + decls += "layout(location = 0) uniform float u" + std::to_string(nextName++) + "[" + + std::to_string(holeBase) + "];\n"; + } + const int tailBase = holeBase + implicitCount; + if (tailBase < maxLocations) { + decls += "layout(location = " + std::to_string(tailBase) + ") uniform float u" + + std::to_string(nextName++) + "[" + std::to_string(maxLocations - tailBase) + "];\n"; + } + const String implicitName = "u" + std::to_string(nextName); + decls += "uniform float " + implicitName + "[" + std::to_string(implicitCount) + "];\n"; + + // The buffer block is the whole point: it is one more uniform the table has to seat, and + // seating it inside the location space is what used to push the implicit uniform out. + const String csSource = "#version 430 core\n" + "layout(local_size_x = 1) in;\n" + "layout(std430, binding = 1) buffer ResultBuffer { vec4 cs_result; };\n" + + decls + "void main() { cs_result = vec4(" + implicitName + "[0]); }\n"; + const GLuint cs = CompileShaderChecked(GL_COMPUTE_SHADER, csSource.c_str()); + const GLuint program = CreateProgram(); + AttachShader(program, cs); + LinkProgram(program); + GLint linkStatus = GL_FALSE; + GetProgramiv(program, GL_LINK_STATUS, &linkStatus); + char infoLog[1024] = ""; + GetProgramInfoLog(program, sizeof(infoLog), nullptr, infoLog); + ASSERT_EQ(linkStatus, GL_TRUE) << "hole at " << holeBase << " x" << implicitCount << ": " << infoLog; + + const GLint location = GetUniformLocation(program, implicitName.c_str()); + EXPECT_EQ(location, holeBase) << "the implicit uniform must take the one free span left"; + EXPECT_LT(location + implicitCount, maxLocations + 1) + << "locations " << location << ".." << (location + implicitCount - 1) + << " must stay inside 0.." << (maxLocations - 1); + EXPECT_EQ(GetUniformLocation(program, "cs_result"), -1); + }; + + // The three holes the CTS walks, for its single-uniform and its 3-element-array subcase. + for (const int implicitCount : {1, 3}) { + runCase(0, implicitCount); + runCase(3, implicitCount); + runCase(maxLocations - implicitCount, implicitCount); + } + EXPECT_EQ(GetError(), GL_NO_ERROR); +}