From 68f2c10cdf51396aa7fa1c331bed3b17f50a3c26 Mon Sep 17 00:00:00 2001 From: Shahbaz Youssefi Date: Tue, 15 Aug 2023 22:47:32 -0400 Subject: [PATCH] Remove program executable backup during link This was added in https://chromium-review.googlesource.com/c/angle/angle/+/2181450/30 to support the case where a program fails to relink, but needs to still be usable. However, this does not seem to be an issue anymore. New tests are specifically added for this, and they, along with every other test, pass. If this needs to be reintroduced, it needs to be rethought. It does not play well with parallel link as it changes the executable pointer while link is in progress (and it was done on the assumption that everything needing the executable is linked serially). A better solution would likely be an `mLastSuccessfullyLinkedExecutable` that normally points to `mExecutable`, but not during link. On `resolveLink`, it would either make `mExecutable` point back to `mLastSuccessfullyLinkedExecutable`, or the other way around based on whether the link was successful or not. Bug: angleproject:8297 Change-Id: Ic9d55bccb75fff0253fe299a244bf1e4bbc416a6 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4781632 Reviewed-by: Yuxin Hu Auto-Submit: Shahbaz Youssefi Commit-Queue: Shahbaz Youssefi Reviewed-by: Charlie Lao --- src/libANGLE/Program.cpp | 19 --- src/tests/gl_tests/LinkAndRelinkTest.cpp | 199 ++++++++++++++++++++++- 2 files changed, 193 insertions(+), 25 deletions(-) diff --git a/src/libANGLE/Program.cpp b/src/libANGLE/Program.cpp index 605a176af..c1e7e6042 100644 --- a/src/libANGLE/Program.cpp +++ b/src/libANGLE/Program.cpp @@ -563,7 +563,6 @@ void LoadInterfaceBlock(BinaryInputStream *stream, InterfaceBlock *block) // Saves the linking context for later use in resolveLink(). struct Program::LinkingState { - std::shared_ptr linkedExecutable; ProgramLinkedResources resources; egl::BlobCache::Key programHash; std::unique_ptr linkEvent; @@ -1242,13 +1241,6 @@ angle::Result Program::link(const Context *context) angle::Result result = linkImpl(context); - // Avoid having two ProgramExecutables if the link failed and the Program had successfully - // linked previously. - if (mLinkingState && mLinkingState->linkedExecutable) - { - mState.mExecutable = mLinkingState->linkedExecutable; - } - return result; } @@ -1435,11 +1427,6 @@ angle::Result Program::linkImpl(const Context *context) mState.updateProgramInterfaceInputs(); mState.updateProgramInterfaceOutputs(); - if (mState.mSeparable) - { - mLinkingState->linkedExecutable = mState.mExecutable; - } - return angle::Result::Continue; } @@ -1584,12 +1571,6 @@ void ProgramState::updateProgramInterfaceOutputs() // Returns the program object to an unlinked state, before re-linking, or at destruction void Program::unlink() { - if (mLinkingState && mLinkingState->linkedExecutable) - { - // The new ProgramExecutable that we'll attempt to link with needs to start from a copy of - // the last successfully linked ProgramExecutable, so we don't lose any state information. - mState.mExecutable.reset(new ProgramExecutable(*mLinkingState->linkedExecutable)); - } mState.mExecutable->reset(true); mState.mUniformLocations.clear(); diff --git a/src/tests/gl_tests/LinkAndRelinkTest.cpp b/src/tests/gl_tests/LinkAndRelinkTest.cpp index c828513c8..e244913da 100644 --- a/src/tests/gl_tests/LinkAndRelinkTest.cpp +++ b/src/tests/gl_tests/LinkAndRelinkTest.cpp @@ -21,18 +21,18 @@ class LinkAndRelinkTest : public ANGLETest<> LinkAndRelinkTest() {} }; +class LinkAndRelinkTestES3 : public ANGLETest<> +{ + protected: + LinkAndRelinkTestES3() {} +}; + class LinkAndRelinkTestES31 : public ANGLETest<> { protected: LinkAndRelinkTestES31() {} }; -class LinkAndRelinkTestES32 : public ANGLETest<> -{ - protected: - LinkAndRelinkTestES32() {} -}; - // When a program link or relink fails, if you try to install the unsuccessfully // linked program (via UseProgram) and start rendering or dispatch compute, // We can not always report INVALID_OPERATION for rendering/compute pipeline. @@ -611,8 +611,195 @@ TEST_P(LinkAndRelinkTest, AttachShaderThenCompile) ASSERT_GL_NO_ERROR(); } +// If a program is linked successfully once, it should retain its executable if a relink fails. +TEST_P(LinkAndRelinkTestES3, SucessfullLinkThenFailingRelink) +{ + // Install a render program in current GL state via UseProgram, then render. + // It should succeed. + constexpr char kVS[] = R"(#version 300 es +out vec4 color; +void main() +{ + vec2 position = vec2(-1, -1); + if (gl_VertexID == 1) + position = vec2(3, -1); + else if (gl_VertexID == 2) + position = vec2(-1, 3); + + gl_Position = vec4(position, 0, 1); + color = vec4(0, 1, 0, 1); +})"; + constexpr char kBadFS[] = R"(#version 300 es +flat in uvec2 color; +out mediump vec4 colorOut; +void main() +{ + colorOut = vec4(1, color, 1); +})"; + + GLuint program = glCreateProgram(); + GLuint vs = CompileShader(GL_VERTEX_SHADER, kVS); + GLuint fs = CompileShader(GL_FRAGMENT_SHADER, essl3_shaders::fs::Green()); + GLuint badfs = CompileShader(GL_FRAGMENT_SHADER, kBadFS); + + EXPECT_NE(0u, vs); + EXPECT_NE(0u, fs); + EXPECT_NE(0u, badfs); + + glAttachShader(program, vs); + glAttachShader(program, fs); + + glLinkProgram(program); + + GLint linkStatus; + glGetProgramiv(program, GL_LINK_STATUS, &linkStatus); + EXPECT_GL_TRUE(linkStatus); + + EXPECT_GL_NO_ERROR(); + + const int w = getWindowWidth(); + const int h = getWindowHeight(); + + glViewport(0, 0, w, h); + + glClearColor(0, 0, 0, 1); + glClear(GL_COLOR_BUFFER_BIT); + + glEnable(GL_SCISSOR_TEST); + glScissor(0, 0, w / 2, h / 2); + + glUseProgram(program); + glDrawArrays(GL_TRIANGLES, 0, 3); + EXPECT_GL_NO_ERROR(); + + // Cause the program to fail linking + glDetachShader(program, fs); + glAttachShader(program, badfs); + + glLinkProgram(program); + + glGetProgramiv(program, GL_LINK_STATUS, &linkStatus); + EXPECT_GL_FALSE(linkStatus); + + // Link failed, but the program should still be usable. + glScissor(w / 2, h / 2, w / 2, h / 2); + glDrawArrays(GL_TRIANGLES, 0, 3); + EXPECT_GL_NO_ERROR(); + + EXPECT_PIXEL_RECT_EQ(0, 0, w / 2, h / 2, GLColor::green); + EXPECT_PIXEL_RECT_EQ(w / 2, 0, w / 2, h / 2, GLColor::black); + EXPECT_PIXEL_RECT_EQ(0, h / 2, w / 2, h / 2, GLColor::black); + EXPECT_PIXEL_RECT_EQ(w / 2, h / 2, w / 2, h / 2, GLColor::green); + + glDeleteShader(vs); + glDeleteShader(fs); + glDeleteShader(badfs); + glDeleteProgram(program); +} + +// Same as SucessfullLinkThenFailingRelink, but with PPOs. +TEST_P(LinkAndRelinkTestES31, SucessfullLinkThenFailingRelinkWithPPO) +{ + // Only the Vulkan backend supports PPOs. + ANGLE_SKIP_TEST_IF(!IsVulkan()); + ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_geometry_shader")); + + // Install a render program in current GL state via UseProgram, then render. + // It should succeed. + constexpr char kVS[] = R"(#version 300 es +void main() +{ + vec2 position = vec2(-1, -1); + if (gl_VertexID == 1) + position = vec2(3, -1); + else if (gl_VertexID == 2) + position = vec2(-1, 3); + + gl_Position = vec4(position, 0, 1); +})"; + constexpr char kBadGS[] = R"(#version 310 es +#extension GL_EXT_geometry_shader : require +layout (invocations = 3, triangles) in; +layout (triangle_strip, max_vertices = 3) out; +void main() +{ +})"; + + GLuint vs = CompileShader(GL_VERTEX_SHADER, kVS); + GLuint fs = CompileShader(GL_FRAGMENT_SHADER, essl3_shaders::fs::Green()); + GLuint badgs = CompileShader(GL_GEOMETRY_SHADER, kBadGS); + + EXPECT_NE(0u, vs); + EXPECT_NE(0u, fs); + EXPECT_NE(0u, badgs); + + GLuint vsProg = glCreateProgram(); + GLuint fsProg = glCreateProgram(); + + glProgramParameteri(vsProg, GL_PROGRAM_SEPARABLE, GL_TRUE); + glAttachShader(vsProg, vs); + glLinkProgram(vsProg); + EXPECT_GL_NO_ERROR(); + + glProgramParameteri(fsProg, GL_PROGRAM_SEPARABLE, GL_TRUE); + glAttachShader(fsProg, fs); + glLinkProgram(fsProg); + EXPECT_GL_NO_ERROR(); + + GLuint pipeline; + glGenProgramPipelines(1, &pipeline); + glUseProgramStages(pipeline, GL_VERTEX_SHADER_BIT, vsProg); + glUseProgramStages(pipeline, GL_FRAGMENT_SHADER_BIT, fsProg); + EXPECT_GL_NO_ERROR(); + + const int w = getWindowWidth(); + const int h = getWindowHeight(); + + glViewport(0, 0, w, h); + + glClearColor(0, 0, 0, 1); + glClear(GL_COLOR_BUFFER_BIT); + + glEnable(GL_SCISSOR_TEST); + glScissor(0, 0, w / 2, h / 2); + + glUseProgram(0); + glBindProgramPipeline(pipeline); + glDrawArrays(GL_TRIANGLES, 0, 3); + EXPECT_GL_NO_ERROR(); + + // Cause the fs program to fail linking + glAttachShader(fsProg, badgs); + glLinkProgram(fsProg); + glUseProgramStages(pipeline, GL_GEOMETRY_SHADER_BIT, fsProg); + EXPECT_GL_ERROR(GL_INVALID_OPERATION); + + GLint linkStatus; + glGetProgramiv(fsProg, GL_LINK_STATUS, &linkStatus); + EXPECT_GL_FALSE(linkStatus); + + // Program link failed, but the program pipeline should still be usable. + glScissor(w / 2, h / 2, w / 2, h / 2); + glDrawArrays(GL_TRIANGLES, 0, 3); + EXPECT_GL_NO_ERROR(); + + EXPECT_PIXEL_RECT_EQ(0, 0, w / 2, h / 2, GLColor::green); + EXPECT_PIXEL_RECT_EQ(w / 2, 0, w / 2, h / 2, GLColor::black); + EXPECT_PIXEL_RECT_EQ(0, h / 2, w / 2, h / 2, GLColor::black); + EXPECT_PIXEL_RECT_EQ(w / 2, h / 2, w / 2, h / 2, GLColor::green); + + glDeleteShader(vs); + glDeleteShader(fs); + glDeleteShader(badgs); + glDeleteProgram(vsProg); + glDeleteProgram(fsProg); + glDeleteProgramPipelines(1, &pipeline); +} + ANGLE_INSTANTIATE_TEST_ES2_AND_ES3(LinkAndRelinkTest); +ANGLE_INSTANTIATE_TEST_ES3(LinkAndRelinkTestES3); + GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(LinkAndRelinkTestES31); ANGLE_INSTANTIATE_TEST_ES31(LinkAndRelinkTestES31);