Merge pull request #17399 from hrydgard/more-crash-fixes

More crash fixes

Common/GPU/OpenGL/GLRenderManager.cpp

@@ -554,6 +554,11 @@ void GLPushBuffer::Flush() {
 	// Must be called from the render thread.
 	_dbg_assert_(OnRenderThread());
 
+	if (buf_ >= buffers_.size()) {
+		_dbg_assert_msg_(false, "buf_ somehow got out of sync: %d vs %d", (int)buf_, (int)buffers_.size());
+		return;
+	}
+
 	buffers_[buf_].flushOffset = offset_;
 	if (!buffers_[buf_].deviceMemory && writePtr_) {
 		auto &info = buffers_[buf_];
@@ -589,6 +594,7 @@ bool GLPushBuffer::AddBuffer() {
 	if (!info.localMemory)
 		return false;
 	info.buffer = render_->CreateBuffer(target_, size_, GL_DYNAMIC_DRAW);
+	info.size = size_;
 	buf_ = buffers_.size();
 	buffers_.push_back(info);
 	return true;
@@ -605,7 +611,6 @@ void GLPushBuffer::Destroy(bool onRenderThread) {
 		} else {
 			render_->DeleteBuffer(info.buffer);
 		}
-
 		FreeAlignedMemory(info.localMemory);
 	}
 	buffers_.clear();
@@ -640,7 +645,7 @@ void GLPushBuffer::Defragment() {
 	_dbg_assert_msg_(!OnRenderThread(), "Defragment must not run on the render thread");
 
 	if (buffers_.size() <= 1) {
-		// Let's take this chance to jetison localMemory we don't need.
+		// Let's take this opportunity to jettison any localMemory we don't need.
 		for (auto &info : buffers_) {
 			if (info.deviceMemory) {
 				FreeAlignedMemory(info.localMemory);
@@ -652,18 +657,31 @@ void GLPushBuffer::Defragment() {
 	}
 
 	// Okay, we have more than one.  Destroy them all and start over with a larger one.
-	size_t newSize = size_ * buffers_.size();
+
+	// When calling AddBuffer, we sometimes increase size_. So if we allocated multiple buffers in a frame,
+	// they won't all have the same size. Sum things up properly.
+	size_t newSize = 0;
+	for (int i = 0; i < (int)buffers_.size(); i++) {
+		newSize += buffers_[i].size;
+	}
+
 	Destroy(false);
 
-	size_ = newSize;
+	// Set some sane but very free limits. If there's another spike, we'll just allocate more anyway.
+	size_ = std::min(std::max(newSize, (size_t)65536), (size_t)(512 * 1024 * 1024));
 	bool res = AddBuffer();
 	_assert_msg_(res, "AddBuffer failed");
 }
 
 size_t GLPushBuffer::GetTotalSize() const {
 	size_t sum = 0;
-	if (buffers_.size() > 1)
-		sum += size_ * (buffers_.size() - 1);
+	// When calling AddBuffer, we sometimes increase size_. So if we allocated multiple buffers in a frame,
+	// they won't all have the same size. Sum things up properly.
+	if (buffers_.size() > 1) {
+		for (int i = 0; i < (int)buffers_.size() - 1; i++) {
+			sum += buffers_[i].size;
+		}
+	}
 	sum += offset_;
 	return sum;
 }

Common/GPU/OpenGL/GLRenderManager.h

@@ -242,6 +242,7 @@ public:
 		uint8_t *localMemory = nullptr;
 		uint8_t *deviceMemory = nullptr;
 		size_t flushOffset = 0;
+		size_t size;
 	};
 
 	GLPushBuffer(GLRenderManager *render, GLuint target, size_t size);

Common/UI/PopupScreens.h

@@ -272,7 +272,9 @@ public:
 
 protected:
 	void PostChoiceCallback(int num) override {
-		*valueStr_ = choices_[num];
+		if (valueStr_) {
+			*valueStr_ = choices_[num];
+		}
 	}
 
 private:

Common/UI/UIScreen.cpp

@@ -111,6 +111,7 @@ void UIScreen::postRender() {
 	if (!draw) {
 		return;
 	}
+	screenManager()->getUIContext()->Flush();
 	draw->EndFrame();
 }
 

GPU/Common/DrawEngineCommon.cpp

@@ -66,6 +66,7 @@ VertexDecoder *DrawEngineCommon::GetVertexDecoder(u32 vtype) {
 	if (dec)
 		return dec;
 	dec = new VertexDecoder();
+	_assert_(dec);
 	dec->SetVertexType(vtype, decOptions_, decJitCache_);
 	decoderMap_.Insert(vtype, dec);
 	return dec;
@@ -809,7 +810,7 @@ void DrawEngineCommon::SubmitPrim(const void *verts, const void *inds, GEPrimiti
 	}
 
 	// If vtype has changed, setup the vertex decoder.
-	if (vertTypeID != lastVType_) {
+	if (vertTypeID != lastVType_ || !dec_) {
 		dec_ = GetVertexDecoder(vertTypeID);
 		lastVType_ = vertTypeID;
 	}

GPU/D3D11/DrawEngineD3D11.h

@@ -149,7 +149,11 @@ public:
 		DecodeVerts(decoded);
 	}
 
-	void DispatchFlush() override { Flush(); }
+	void DispatchFlush() override {
+		if (!numDrawCalls)
+			return;
+		Flush();
+	}
 
 	void ClearTrackedVertexArrays() override;
 

GPU/Directx9/DrawEngineDX9.h

@@ -139,7 +139,11 @@ public:
 		DecodeVerts(decoded);
 	}
 
-	void DispatchFlush() override { Flush(); }
+	void DispatchFlush() override {
+		if (!numDrawCalls)
+			return;
+		Flush();
+	}
 
 protected:
 	// Not currently supported.

GPU/GLES/DrawEngineGLES.cpp

@@ -249,9 +249,20 @@ void DrawEngineGLES::Invalidate(InvalidationCallbackFlags flags) {
 void DrawEngineGLES::DoFlush() {
 	PROFILE_THIS_SCOPE("flush");
 	FrameData &frameData = frameData_[render_->GetCurFrame()];
-	
-	// Attempt to gather some information (asserts now upload the game name).
-	_assert_(render_->IsInRenderPass());
+	VShaderID vsid;
+
+	if (!render_->IsInRenderPass()) {
+		// Something went badly wrong. Try to survive by simply skipping the draw, though.
+		_dbg_assert_msg_(false, "Trying to DoFlush while not in a render pass. This is bad.");
+		// can't goto bail here, skips too many variable initializations. So let's wipe the most important stuff.
+		indexGen.Reset();
+		decodedVerts_ = 0;
+		numDrawCalls = 0;
+		vertexCountInDrawCalls_ = 0;
+		decodeCounter_ = 0;
+		dcid_ = 0;
+		return;
+	}
 
 	bool textureNeedsApply = false;
 	if (gstate_c.IsDirty(DIRTY_TEXTURE_IMAGE | DIRTY_TEXTURE_PARAMS) && !gstate.isModeClear() && gstate.isTextureMapEnabled()) {
@@ -265,7 +276,6 @@ void DrawEngineGLES::DoFlush() {
 
 	GEPrimitiveType prim = prevPrim_;
 
-	VShaderID vsid;
 	Shader *vshader = shaderManager_->ApplyVertexShader(CanUseHardwareTransform(prim), useHWTessellation_, dec_, decOptions_.expandAllWeightsToFloat, decOptions_.applySkinInDecode || !CanUseHardwareTransform(prim), &vsid);
 
 	GLRBuffer *vertexBuffer = nullptr;

GPU/GLES/DrawEngineGLES.h

@@ -97,7 +97,11 @@ public:
 		DoFlush();
 	}
 
-	void DispatchFlush() override { Flush(); }
+	void DispatchFlush() override {
+		if (!numDrawCalls)
+			return;
+		Flush();
+	}
 
 	GLPushBuffer *GetPushVertexBuffer() {
 		return frameData_[render_->GetCurFrame()].pushVertex;

GPU/GPUCommonHW.h

@@ -62,6 +62,7 @@ public:
 
 	void Execute_TexFlush(u32 op, u32 diff);
 
+	// TODO: Have these return an error code if they jump to a bad address. If bad, stop the FastRunLoop.
 	typedef void (GPUCommonHW::*CmdFunc)(u32 op, u32 diff);
 
 	void FastRunLoop(DisplayList &list) override;

GPU/Vulkan/DrawEngineVulkan.h

@@ -177,7 +177,11 @@ public:
 		DoFlush();
 	}
 
-	void DispatchFlush() override { Flush(); }
+	void DispatchFlush() override {
+		if (!numDrawCalls)
+			return;
+		Flush();
+	}
 
 	VkPipelineLayout GetPipelineLayout() const {
 		return pipelineLayout_;

GPU/Vulkan/GPU_Vulkan.cpp

@@ -182,8 +182,10 @@ void GPU_Vulkan::SaveCache(const Path &filename) {
 }
 
 GPU_Vulkan::~GPU_Vulkan() {
-	VulkanRenderManager *rm = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER);
-	rm->DrainCompileQueue();
+	if (draw_) {
+		VulkanRenderManager *rm = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER);
+		rm->DrainCompileQueue();
+	}
 
 	SaveCache(shaderCachePath_);
 	// Note: We save the cache in DeviceLost

UI/GameInfoCache.cpp

@@ -197,9 +197,12 @@ bool GameInfo::LoadFromPath(const Path &gamePath) {
 	std::lock_guard<std::mutex> guard(lock);
 	// No need to rebuild if we already have it loaded.
 	if (filePath_ != gamePath) {
-		fileLoader.reset(ConstructFileLoader(gamePath));
-		if (!fileLoader)
-			return false;
+		{
+			std::lock_guard<std::mutex> guard(loaderLock);
+			fileLoader.reset(ConstructFileLoader(gamePath));
+			if (!fileLoader)
+				return false;
+		}
 		filePath_ = gamePath;
 
 		// This is a fallback title, while we're loading / if unable to load.
@@ -215,13 +218,18 @@ std::shared_ptr<FileLoader> GameInfo::GetFileLoader() {
 		// because Priority() calls GetFileLoader()... gnarly.
 		return fileLoader;
 	}
+
+	std::lock_guard<std::mutex> guard(loaderLock);
 	if (!fileLoader) {
-		fileLoader.reset(ConstructFileLoader(filePath_));
+		FileLoader *loader = ConstructFileLoader(filePath_);
+		fileLoader.reset(loader);
+		return fileLoader;
 	}
 	return fileLoader;
 }
 
 void GameInfo::DisposeFileLoader() {
+	std::lock_guard<std::mutex> guard(loaderLock);
 	fileLoader.reset();
 }
 
@@ -365,7 +373,7 @@ public:
 		}
 
 		// In case of a remote file, check if it actually exists before locking.
-		if (!info_->GetFileLoader()->Exists()) {
+		if (!info_->GetFileLoader() || !info_->GetFileLoader()->Exists()) {
 			return;
 		}
 

UI/GameInfoCache.h

@@ -119,6 +119,9 @@ public:
 	// to it.
 	std::mutex lock;
 
+	// Controls access to the fileLoader pointer.
+	std::mutex loaderLock;
+
 	std::string id;
 	std::string id_version;
 	int disc_total = 0;