OpenGL: Guard init step recording, which happens on three threads

GLRenderManager is documented as "emu thread records, render thread executes",
but GL has to record device object creation as init steps rather than just doing
it, and InitGPU() runs on the ExecLoader thread - GPU_GLES's constructor builds
DrawEngineGLES, whose InitDeviceObjects() reaches initSteps_ through
CreatePushBuffer and CreateInputLayout. The emu thread is still drawing the
loading screen into the same FastVec until the loader thread is joined, so two
concurrent push_uninitialized() can both reallocate, and one writes its step into
a freed buffer - losing a shader or buffer creation, or scribbling an owned
pointer into freed memory.

frameData_[].activePushBuffers is genuinely three-threaded too: inserted into by
whoever creates a push buffer, erased on the render thread via GLDeleter, and
walked on the render thread each frame.

A mutex each, uncontended in practice. Note this makes the existing access safe
rather than fixing the layering - Vulkan avoids the problem by creating objects
directly and deferring the rest to FinishInitOnMainThread, which GPU_GLES has
never had. Moving GL's device object creation there would be the better fix.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Vd8ntC2brCUtCrDJMqLbs8

Common/GPU/OpenGL/GLRenderManager.cpp

@@ -397,10 +397,13 @@ void GLRenderManager::Finish() {
 	task->frame = curFrame;
 	{
 		std::unique_lock<std::mutex> lock(pushMutex_);
-		task->initSteps = std::move(initSteps_);
+		{
+			std::lock_guard<std::mutex> initLock(initStepsMutex_);
+			task->initSteps = std::move(initSteps_);
+			initSteps_.clear();
+		}
 		task->steps = std::move(steps_);
 		renderThreadQueue_.push(task);
-		initSteps_.clear();
 		steps_.clear();
 		pushCondVar_.notify_one();
 	}
@@ -474,6 +477,7 @@ bool GLRenderManager::Run(GLRRenderThreadTask &task) {
 
 	// Run this after RunInitSteps so any fresh GLRBuffers for the pushbuffers can get created.
 	if (!skipGLCalls_) {
+		std::lock_guard<std::mutex> lock(pushBuffersMutex_);
 		for (auto iter : frameData.activePushBuffers) {
 			iter->Flush();
 			iter->UnmapDevice();
@@ -500,6 +504,7 @@ bool GLRenderManager::Run(GLRRenderThreadTask &task) {
 	}
 
 	if (!skipGLCalls_) {
+		std::lock_guard<std::mutex> lock(pushBuffersMutex_);
 		for (auto iter : frameData.activePushBuffers) {
 			iter->MapDevice(bufferStrategy_);
 		}
@@ -537,7 +542,11 @@ void GLRenderManager::FlushSync() {
 
 		std::unique_lock<std::mutex> lock(pushMutex_);
 		renderThreadQueue_.push(task);
-		renderThreadQueue_.back()->initSteps = std::move(initSteps_);
+		{
+			std::lock_guard<std::mutex> initLock(initStepsMutex_);
+			renderThreadQueue_.back()->initSteps = std::move(initSteps_);
+			initSteps_.clear();
+		}
 		renderThreadQueue_.back()->steps = std::move(steps_);
 		pushCondVar_.notify_one();
 		steps_.clear();

Common/GPU/OpenGL/GLRenderManager.h

@@ -263,6 +263,7 @@ public:
 	// and then we'll also need formats and stuff.
 	GLRTexture *CreateTexture(GLenum target, int width, int height, int depth, int numMips) {
 		_dbg_assert_(target != 0);
+		std::lock_guard<std::mutex> lock(initStepsMutex_);
 		GLRInitStep &step = initSteps_.push_uninitialized();
 		step.stepType = GLRInitStepType::CREATE_TEXTURE;
 		step.create_texture.texture = new GLRTexture(caps_, width, height, depth, numMips);
@@ -271,6 +272,7 @@ public:
 	}
 
 	GLRBuffer *CreateBuffer(GLuint target, size_t size, GLuint usage) {
+		std::lock_guard<std::mutex> lock(initStepsMutex_);
 		GLRInitStep &step = initSteps_.push_uninitialized();
 		step.stepType = GLRInitStepType::CREATE_BUFFER;
 		step.create_buffer.buffer = new GLRBuffer(target, size);
@@ -280,6 +282,7 @@ public:
 	}
 
 	GLRShader *CreateShader(GLuint stage, const std::string &code, std::string_view desc) {
+		std::lock_guard<std::mutex> lock(initStepsMutex_);
 		GLRInitStep &step = initSteps_.push_uninitialized();
 		step.stepType = GLRInitStepType::CREATE_SHADER;
 		step.create_shader.shader = new GLRShader(desc);
@@ -292,6 +295,7 @@ public:
 	GLRFramebuffer *CreateFramebuffer(int width, int height, bool z_stencil, const char *tag) {
 		_dbg_assert_(width > 0 && height > 0 && tag != nullptr);
 
+		std::lock_guard<std::mutex> lock(initStepsMutex_);
 		GLRInitStep &step = initSteps_.push_uninitialized();
 		step.stepType = GLRInitStepType::CREATE_FRAMEBUFFER;
 		step.create_framebuffer.framebuffer = new GLRFramebuffer(caps_, width, height, z_stencil, tag);
@@ -303,6 +307,7 @@ public:
 	GLRProgram *CreateProgram(
 		std::vector<GLRShader *> shaders, std::vector<GLRProgram::Semantic> semantics, std::vector<GLRProgram::UniformLocQuery> queries,
 		std::vector<GLRProgram::Initializer> initializers, GLRProgramLocData *locData, const GLRProgramFlags &flags) {
+		std::lock_guard<std::mutex> lock(initStepsMutex_);
 		GLRInitStep &step = initSteps_.push_uninitialized();
 		step.stepType = GLRInitStepType::CREATE_PROGRAM;
 		_assert_(shaders.size() <= ARRAY_SIZE(step.create_program.shaders));
@@ -332,6 +337,7 @@ public:
 	}
 
 	GLRInputLayout *CreateInputLayout(const std::vector<GLRInputLayout::Entry> &entries, int stride) {
+		std::lock_guard<std::mutex> lock(initStepsMutex_);
 		GLRInitStep &step = initSteps_.push_uninitialized();
 		step.stepType = GLRInitStepType::CREATE_INPUT_LAYOUT;
 		step.create_input_layout.inputLayout = new GLRInputLayout();
@@ -411,6 +417,7 @@ public:
 	void BufferSubdata(GLRBuffer *buffer, size_t offset, size_t size, uint8_t *data, bool deleteData = true) {
 		// TODO: Maybe should be a render command instead of an init command? When possible it's better as
 		// an init command, that's for sure.
+		std::lock_guard<std::mutex> lock(initStepsMutex_);
 		GLRInitStep &step = initSteps_.push_uninitialized();
 		step.stepType = GLRInitStepType::BUFFER_SUBDATA;
 		_dbg_assert_(offset <= buffer->size_ - size);
@@ -423,6 +430,7 @@ public:
 
 	// Takes ownership over the data pointer and delete[]-s it.
 	void TextureImage(GLRTexture *texture, int level, int width, int height, int depth, Draw::DataFormat format, uint8_t *data, GLRAllocType allocType = GLRAllocType::NEW, bool linearFilter = false) {
+		std::lock_guard<std::mutex> lock(initStepsMutex_);
 		GLRInitStep &step = initSteps_.push_uninitialized();
 		step.stepType = GLRInitStepType::TEXTURE_IMAGE;
 		step.texture_image.texture = texture;
@@ -453,6 +461,7 @@ public:
 	}
 
 	void FinalizeTexture(GLRTexture *texture, int loadedLevels, bool genMips) {
+		std::lock_guard<std::mutex> lock(initStepsMutex_);
 		GLRInitStep &step = initSteps_.push_uninitialized();
 		step.stepType = GLRInitStepType::TEXTURE_FINALIZE;
 		step.texture_finalize.texture = texture;
@@ -800,6 +809,7 @@ public:
 	}
 
 	void UnregisterPushBuffer(GLPushBuffer *buffer) {
+		std::lock_guard<std::mutex> lock(pushBuffersMutex_);
 		int foundCount = 0;
 		for (int i = 0; i < MAX_INFLIGHT_FRAMES; i++) {
 			auto iter = frameData_[i].activePushBuffers.find(buffer);
@@ -850,6 +860,7 @@ private:
 	// When using legacy functionality for push buffers (glBufferData), we need to flush them
 	// before actually making the glDraw* calls. It's best if the render manager handles that.
 	void RegisterPushBuffer(int frame, GLPushBuffer *buffer) {
+		std::lock_guard<std::mutex> lock(pushBuffersMutex_);
 		frameData_[frame].activePushBuffers.insert(buffer);
 	}
 
@@ -860,8 +871,18 @@ private:
 
 	GLRStep *curRenderStep_ = nullptr;
 	std::vector<GLRStep *> steps_;
+	// Guards initSteps_. Recorded into from the emu thread, but also from the loader thread during
+	// boot (InitGPU runs there, and GL has to record device object creation rather than just doing
+	// it), and moved out on the emu thread in Finish/FlushSync. Uncontended in practice.
+	// Lock ordering: taken while pushMutex_ is held, never the other way around.
+	std::mutex initStepsMutex_;
 	FastVec<GLRInitStep> initSteps_;
 
+	// Guards frameData_[].activePushBuffers, which is inserted into from whichever thread creates a
+	// push buffer, erased from on the render thread (GLDeleter), and walked on the render thread.
+	// Lock ordering: taken before initStepsMutex_ (Flush() below records init steps), never after.
+	std::mutex pushBuffersMutex_;
+
 	// Execution time state
 
 	// Thread is managed elsewhere, and should call ThreadFrame.