Merge pull request #17374 from hrydgard/crash-fixes

Assorted crash fixes and asserts

Common/GPU/OpenGL/GLRenderManager.h

@@ -532,6 +532,10 @@ public:
 		pushbuffer->End();
 	}
 
+	bool IsInRenderPass() const {
+		return curRenderStep_ && curRenderStep_->stepType == GLRStepType::RENDER;
+	}
+
 	// This starts a new step (like a "render pass" in Vulkan).
 	//
 	// After a "CopyFramebuffer" or the other functions that start "steps", you need to call this before

Common/GPU/Vulkan/VulkanMemory.cpp

@@ -372,6 +372,8 @@ void VulkanPushPool::BeginFrame() {
 }
 
 void VulkanPushPool::NextBlock(VkDeviceSize allocationSize) {
+	_dbg_assert_(allocationSize != 0);  // If so, the logic in the caller is wrong, should never need a new block for this case.
+
 	int curFrameIndex = vulkan_->GetCurFrame();
 	curBlockIndex_++;
 	while (curBlockIndex_ < blocks_.size()) {

Common/GPU/Vulkan/VulkanMemory.h

@@ -121,6 +121,7 @@ public:
 	void GetDebugString(char *buffer, size_t bufSize) const override;
 
 	// When using the returned memory, make sure to bind the returned vkbuf.
+	// It is okay to allocate 0 bytes.
 	uint8_t *Allocate(VkDeviceSize numBytes, VkDeviceSize alignment, VkBuffer *vkbuf, uint32_t *bindOffset) {
 		_dbg_assert_(curBlockIndex_ >= 0);
 		

Common/GPU/Vulkan/thin3d_vulkan.cpp

@@ -1487,6 +1487,11 @@ void VKContext::DrawIndexed(int vertexCount, int offset) {
 }
 
 void VKContext::DrawUP(const void *vdata, int vertexCount) {
+	_dbg_assert_(vertexCount >= 0);
+	if (vertexCount <= 0) {
+		return;
+	}
+
 	VkBuffer vulkanVbuf, vulkanUBObuf;
 	size_t vbBindOffset = push_->Push(vdata, vertexCount * curPipeline_->stride[0], 4, &vulkanVbuf);
 	uint32_t ubo_offset = (uint32_t)curPipeline_->PushUBO(push_, vulkan_, &vulkanUBObuf);

Core/ELF/ParamSFO.cpp

@@ -60,20 +60,22 @@ void ParamSFOData::SetValue(std::string key, const u8* value, unsigned int size,
 	values[key].max_size = max_size;
 }
 
-int ParamSFOData::GetValueInt(std::string key) {
-	std::map<std::string,ValueData>::iterator it = values.find(key);
+int ParamSFOData::GetValueInt(std::string key) const {
+	std::map<std::string,ValueData>::const_iterator it = values.find(key);
 	if(it == values.end() || it->second.type != VT_INT)
 		return 0;
 	return it->second.i_value;
 }
-std::string ParamSFOData::GetValueString(std::string key) {
-	std::map<std::string,ValueData>::iterator it = values.find(key);
+
+std::string ParamSFOData::GetValueString(std::string key) const {
+	std::map<std::string,ValueData>::const_iterator it = values.find(key);
 	if(it == values.end() || (it->second.type != VT_UTF8))
 		return "";
 	return it->second.s_value;
 }
-u8* ParamSFOData::GetValueData(std::string key, unsigned int *size) {
-	std::map<std::string,ValueData>::iterator it = values.find(key);
+
+const u8 *ParamSFOData::GetValueData(std::string key, unsigned int *size) const {
+	std::map<std::string,ValueData>::const_iterator it = values.find(key);
 	if(it == values.end() || (it->second.type != VT_UTF8_SPE))
 		return 0;
 	if(size)
@@ -83,7 +85,7 @@ u8* ParamSFOData::GetValueData(std::string key, unsigned int *size) {
 	return it->second.u_value;
 }
 
-std::vector<std::string> ParamSFOData::GetKeys() {
+std::vector<std::string> ParamSFOData::GetKeys() const {
 	std::vector<std::string> result;
 	for (const auto &pair : values) {
 		result.push_back(pair.first);
@@ -109,43 +111,70 @@ bool ParamSFOData::ReadSFO(const u8 *paramsfo, size_t size) {
 
 	const u8 *data_start = paramsfo + header->data_table_start;
 
+	auto readStringCapped = [paramsfo, size](size_t offset, size_t maxLen) -> std::string {
+		std::string str;
+		while (offset < size) {
+			char c = (char)(paramsfo[offset]);
+			if (c) {
+				str.push_back(c);
+			} else {
+				break;
+			}
+			offset++;
+			if (maxLen != 0 && str.size() == maxLen)
+				break;
+		}
+		return str;
+	};
+
 	for (u32 i = 0; i < header->index_table_entries; i++)
 	{
 		size_t key_offset = header->key_table_start + indexTables[i].key_table_offset;
 		if (key_offset >= size) {
 			return false;
 		}
+
 		size_t data_offset = header->data_table_start + indexTables[i].data_table_offset;
 		if (data_offset >= size) {
 			return false;
 		}
 
-		const char *key = (const char *)(paramsfo + key_offset);
+		std::string key = readStringCapped(key_offset, 0);
+		if (key.empty())
+			continue;  // Likely ran into a truncated PARAMSFO.
+
 		switch (indexTables[i].param_fmt) {
 		case 0x0404:
 			{
+				if (data_offset + 4 > size)
+					continue;
 				// Unsigned int
 				const u32_le *data = (const u32_le *)(paramsfo + data_offset);
 				SetValue(key, *data, indexTables[i].param_max_len);
-				VERBOSE_LOG(LOADER, "%s %08x", key, *data);
+				VERBOSE_LOG(LOADER, "%s %08x", key.c_str(), *data);
 			}
 			break;
 		case 0x0004:
 			// Special format UTF-8
 			{
+				if (data_offset + indexTables[i].param_len > size)
+					continue;
 				const u8 *utfdata = (const u8 *)(paramsfo + data_offset);
-				VERBOSE_LOG(LOADER, "%s %s", key, utfdata);
+				VERBOSE_LOG(LOADER, "%s %s", key.c_str(), utfdata);
 				SetValue(key, utfdata, indexTables[i].param_len, indexTables[i].param_max_len);
 			}
 			break;
 		case 0x0204:
 			// Regular UTF-8
 			{
-				const char *utfdata = (const char *)(paramsfo + data_offset);
-				VERBOSE_LOG(LOADER, "%s %s", key, utfdata);
-				SetValue(key, std::string(utfdata /*, indexTables[i].param_len*/), indexTables[i].param_max_len);
+				// TODO: Likely should use param_len here, but there's gotta be a reason we avoided it before.
+				std::string str = readStringCapped(data_offset, indexTables[i].param_max_len);
+				VERBOSE_LOG(LOADER, "%s %s", key.c_str(), str.c_str());
+				SetValue(key, str, indexTables[i].param_max_len);
 			}
 			break;
+		default:
+			break;
 		}
 	}
 
@@ -176,7 +205,7 @@ int ParamSFOData::GetDataOffset(const u8 *paramsfo, std::string dataName) {
 	return -1;
 }
 
-bool ParamSFOData::WriteSFO(u8 **paramsfo, size_t *size) {
+bool ParamSFOData::WriteSFO(u8 **paramsfo, size_t *size) const {
 	size_t total_size = 0;
 	size_t key_size = 0;
 	size_t data_size = 0;
@@ -198,7 +227,7 @@ bool ParamSFOData::WriteSFO(u8 **paramsfo, size_t *size) {
 	}
 
 	// Padding
-	while((key_size%4)) key_size++;
+	while ((key_size % 4) != 0) key_size++;
 
 	header.key_table_start = sizeof(Header) + header.index_table_entries * sizeof(IndexTable);
 	header.data_table_start = header.key_table_start + (u32)key_size;
@@ -266,20 +295,18 @@ void ParamSFOData::Clear() {
 }
 
 void ParamSFOData::ValueData::SetData(const u8* data, int size) {
-	if(u_value)
-	{
+	if (u_value) {
 		delete[] u_value;
 		u_value = 0;
 	}
-	if(size > 0)
-	{
+	if (size > 0) {
 		u_value = new u8[size];
 		memcpy(u_value, data, size);
 	}
 	u_size = size;
 }
 
-std::string ParamSFOData::GenerateFakeID(std::string filename) {
+std::string ParamSFOData::GenerateFakeID(std::string filename) const {
 	// Generates fake gameID for homebrew based on it's folder name.
 	// Should probably not be a part of ParamSFO, but it'll be called in same places.
 	std::string file = PSP_CoreParameter().fileToStart.ToString();

Core/ELF/ParamSFO.h

@@ -31,12 +31,12 @@ public:
 	void SetValue(std::string key, std::string value, int max_size);
 	void SetValue(std::string key, const u8 *value, unsigned int size, int max_size);
 
-	int GetValueInt(std::string key);
-	std::string GetValueString(std::string key);
-	u8* GetValueData(std::string key, unsigned int *size);
+	int GetValueInt(std::string key) const;
+	std::string GetValueString(std::string key) const;
+	const u8 *GetValueData(std::string key, unsigned int *size) const;
 
-	std::vector<std::string> GetKeys();
-	std::string GenerateFakeID(std::string filename = "");
+	std::vector<std::string> GetKeys() const;
+	std::string GenerateFakeID(std::string filename = "") const;
 
 	std::string GetDiscID() {
 		const std::string discID = GetValueString("DISC_ID");
@@ -53,7 +53,7 @@ public:
 	}
 
 	bool ReadSFO(const u8 *paramsfo, size_t size);
-	bool WriteSFO(u8 **paramsfo, size_t *size);
+	bool WriteSFO(u8 **paramsfo, size_t *size) const;
 
 	bool ReadSFO(const std::vector<u8> &paramsfo) {
 		if (!paramsfo.empty()) {

Core/HLE/Plugins.cpp

@@ -16,6 +16,7 @@
 // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/.
 
 #include <set>
+#include <mutex>
 
 #include "Common/Data/Format/IniFile.h"
 #include "Common/File/FileUtil.h"
@@ -29,6 +30,8 @@
 #include "Core/HLE/sceKernelModule.h"
 
 namespace HLEPlugins {
+
+std::mutex g_inputMutex;
 float PluginDataAxis[JOYSTICK_AXIS_MAX];
 std::map<int, uint8_t> PluginDataKeys;
 
@@ -189,6 +192,8 @@ bool Load() {
 		started = true;
 	}
 
+	std::lock_guard<std::mutex> guard(g_inputMutex);
+	PluginDataKeys.clear();
 	return started;
 }
 
@@ -199,6 +204,8 @@ void Unload() {
 void Shutdown() {
 	prxPlugins.clear();
 	anyEnabled = false;
+	std::lock_guard<std::mutex> guard(g_inputMutex);
+	PluginDataKeys.clear();
 }
 
 void DoState(PointerWrap &p) {
@@ -214,4 +221,16 @@ bool HasEnabled() {
 	return anyEnabled;
 }
 
-};
+void SetKey(int key, uint8_t value) {
+	if (anyEnabled) {
+		std::lock_guard<std::mutex> guard(g_inputMutex);
+		PluginDataKeys[key] = value;
+	}
+}
+
+uint8_t GetKey(int key) {
+	std::lock_guard<std::mutex> guard(g_inputMutex);
+	return PluginDataKeys[key];
+}
+
+}  // namespace

Core/HLE/Plugins.h

@@ -17,6 +17,7 @@
 
 #pragma once
 
+#include <cstdint>
 #include "Input/KeyCodes.h"
 
 class PointerWrap;
@@ -33,6 +34,9 @@ void DoState(PointerWrap &p);
 
 bool HasEnabled();
 
+void SetKey(int key, uint8_t value);
+uint8_t GetKey(int key);
+
 extern float PluginDataAxis[JOYSTICK_AXIS_MAX];
-extern std::map<int, uint8_t> PluginDataKeys;
-};
+
+}  // namespace

Core/HLE/sceIo.cpp

@@ -2072,7 +2072,7 @@ static u32 sceIoDevctl(const char *name, int cmd, u32 argAddr, int argLen, u32 o
 			return 0;
 		case EMULATOR_DEVCTL__GET_VKEY:
 			if (Memory::IsValidAddress(outPtr) && (argAddr >= 0 && argAddr < NKCODE_MAX)) {
-				Memory::Write_U8(HLEPlugins::PluginDataKeys[argAddr], outPtr);
+				Memory::Write_U8(HLEPlugins::GetKey(argAddr), outPtr);
 			}
 			return 0;
 		}

Core/Reporting.cpp

@@ -480,6 +480,8 @@ namespace Reporting
 	{
 		SetCurrentThreadName("Report");
 
+		AndroidJNIThreadContext jniContext;  // destructor detaches
+
 		Payload &payload = payloadBuffer[pos];
 		Buffer output;
 

GPU/GLES/DrawEngineGLES.cpp

@@ -250,6 +250,9 @@ 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());
+
 	bool textureNeedsApply = false;
 	if (gstate_c.IsDirty(DIRTY_TEXTURE_IMAGE | DIRTY_TEXTURE_PARAMS) && !gstate.isModeClear() && gstate.isTextureMapEnabled()) {
 		textureCache_->SetTexture();
@@ -411,7 +414,11 @@ void DrawEngineGLES::DoFlush() {
 
 		ApplyDrawStateLate(result.setStencil, result.stencilValue);
 
-		shaderManager_->ApplyFragmentShader(vsid, vshader, pipelineState_, framebufferManager_->UseBufferedRendering());
+		LinkedShader *linked = shaderManager_->ApplyFragmentShader(vsid, vshader, pipelineState_, framebufferManager_->UseBufferedRendering());
+		if (!linked) {
+			// Not much we can do here. Let's skip drawing.
+			goto bail;
+		}
 
 		if (result.action == SW_DRAW_PRIMITIVES) {
 			if (result.drawIndexed) {
@@ -455,6 +462,7 @@ void DrawEngineGLES::DoFlush() {
 		decOptions_.applySkinInDecode = g_Config.bSoftwareSkinning;
 	}
 
+bail:
 	gpuStats.numFlushes++;
 	gpuStats.numDrawCalls += numDrawCalls;
 	gpuStats.numVertsSubmitted += vertexCountInDrawCalls_;

GPU/GLES/ShaderManagerGLES.cpp

@@ -76,13 +76,15 @@ LinkedShader::LinkedShader(GLRenderManager *render, VShaderID VSID, Shader *vs,
 		: render_(render), useHWTransform_(useHWTransform) {
 	PROFILE_THIS_SCOPE("shaderlink");
 
+	_assert_(vs);
+	_assert_(fs);
+
 	vs_ = vs;
 
 	std::vector<GLRShader *> shaders;
 	shaders.push_back(vs->shader);
 	shaders.push_back(fs->shader);
 
-
 	std::vector<GLRProgram::Semantic> semantics;
 	semantics.reserve(7);
 	semantics.push_back({ ATTR_POSITION, "position" });
@@ -874,6 +876,11 @@ LinkedShader *ShaderManagerGLES::ApplyFragmentShader(VShaderID VSID, Shader *vs,
 	if (ls == nullptr) {
 		_dbg_assert_(FSID.Bit(FS_BIT_FLATSHADE) == VSID.Bit(VS_BIT_FLATSHADE));
 
+		if (vs == nullptr || fs == nullptr) {
+			// Can't draw. This shouldn't really happen.
+			return nullptr;
+		}
+
 		// Check if we can link these.
 		ls = new LinkedShader(render_, VSID, vs, FSID, fs, vs->UseHWTransform());
 		ls->use(VSID);

UI/MemStickScreen.cpp

@@ -633,18 +633,16 @@ void ConfirmMemstickMoveScreen::update() {
 }
 
 UI::EventReturn ConfirmMemstickMoveScreen::OnConfirm(UI::EventParams &params) {
-	auto sy = GetI18NCategory(I18NCat::SYSTEM);
-	auto iz = GetI18NCategory(I18NCat::MEMSTICK);
-
 	// Transfer all the files in /PSP from the original directory.
 	// Should probably be done on a background thread so we can show some UI.
 	// So we probably need another screen for this with a progress bar..
 	// If the directory itself is called PSP, don't go below.
 
 	if (moveData_) {
-		progressReporter_.Set(iz->T("Starting move..."));
+		progressReporter_.Set(T(I18NCat::MEMSTICK, "Starting move..."));
 
 		moveDataTask_ = Promise<MoveResult *>::Spawn(&g_threadManager, [&]() -> MoveResult * {
+			auto ms = GetI18NCategory(I18NCat::MEMSTICK);
 			Path moveSrc = g_Config.memStickDirectory;
 			Path moveDest = newMemstickFolder_;
 			if (moveSrc.GetFilename() != "PSP") {
@@ -666,7 +664,7 @@ UI::EventReturn ConfirmMemstickMoveScreen::OnConfirm(UI::EventParams &params) {
 				// TODO: Handle failure listing files.
 				std::string error = "Failed to read old directory";
 				INFO_LOG(SYSTEM, "%s", error.c_str());
-				progressReporter_.Set(iz->T(error.c_str()));
+				progressReporter_.Set(ms->T(error.c_str()));
 				return new MoveResult{ false, error };
 			}
 
@@ -757,12 +755,12 @@ UI::EventReturn ConfirmMemstickMoveScreen::OnConfirm(UI::EventParams &params) {
 }
 
 void ConfirmMemstickMoveScreen::FinishFolderMove() {
-	auto iz = GetI18NCategory(I18NCat::MEMSTICK);
+	auto ms = GetI18NCategory(I18NCat::MEMSTICK);
 
 	// Successful so far, switch the memstick folder.
 	if (!SwitchMemstickFolderTo(newMemstickFolder_)) {
 		// TODO: More precise errors.
-		error_ = iz->T("That folder doesn't work as a memstick folder.");
+		error_ = ms->T("That folder doesn't work as a memstick folder.");
 		return;
 	}
 
@@ -779,7 +777,7 @@ void ConfirmMemstickMoveScreen::FinishFolderMove() {
 			// TriggerFinish(DialogResult::DR_OK);
 			screenManager()->switchScreen(new MainScreen());
 		} else {
-			error_ = iz->T("Failed to save config");
+			error_ = ms->T("Failed to save config");
 			RecreateViews();
 		}
 	}

UI/MiscScreens.cpp

@@ -171,6 +171,7 @@ public:
 			int n = i & 3;
 			ui_draw2d.DrawImageRotated(symbols[n], x, y, 1.0f, angle, colorAlpha(colors[n], alpha * 0.1f));
 		}
+		dc.Flush();
 	}
 
 private:

UI/NativeApp.cpp

@@ -1257,7 +1257,7 @@ bool NativeKey(const KeyInput &key) {
 	// INFO_LOG(SYSTEM, "Key code: %i flags: %i", key.keyCode, key.flags);
 #if !defined(MOBILE_DEVICE)
 	if (g_Config.bPauseExitsEmulator) {
-		static std::vector<int> pspKeys;
+		std::vector<int> pspKeys;
 		pspKeys.clear();
 		if (KeyMap::InputMappingToPspButton(InputMapping(key.deviceId, key.keyCode), &pspKeys)) {
 			if (std::find(pspKeys.begin(), pspKeys.end(), VIRTKEY_PAUSE) != pspKeys.end()) {
@@ -1267,10 +1267,10 @@ bool NativeKey(const KeyInput &key) {
 		}
 	}
 #endif
+
 	bool retval = false;
-	if (g_screenManager)
-	{
-		HLEPlugins::PluginDataKeys[key.keyCode] = (key.flags & KEY_DOWN) ? 1 : 0;
+	if (g_screenManager) {
+		HLEPlugins::SetKey(key.keyCode, (key.flags & KEY_DOWN) ? 1 : 0);
 		retval = g_screenManager->key(key);
 	}