From 607d2486eac29f331fb41f05729b00e497358a3d Mon Sep 17 00:00:00 2001 From: mp-t Date: Tue, 1 Aug 2017 19:22:33 +0200 Subject: [PATCH] Code review (#3114) * Fix always-true conditions in sceNp module * gl_render_targets: useless check on unsigned variable, possible bug * fixed UB in crypto utility functions * copy-paste error in vk::init_default_resources * pass strings by const ref * Dont copy vectors. Make sure copies are not needed because functions are used in a multi-threaded context. --- Utilities/StrFmt.cpp | 4 ++-- rpcs3/Crypto/unedat.cpp | 16 ++++++++-------- rpcs3/Crypto/utils.cpp | 4 ++-- rpcs3/Emu/Cell/Modules/cellL10n.cpp | 6 +++--- rpcs3/Emu/Cell/Modules/sceNp.cpp | 12 ++++++------ rpcs3/Emu/Cell/Modules/sys_libc_.cpp | 2 +- rpcs3/Emu/Cell/SPUASMJITRecompiler.cpp | 2 +- rpcs3/Emu/RSX/Common/VertexProgramDecompiler.cpp | 2 +- rpcs3/Emu/RSX/Common/VertexProgramDecompiler.h | 2 +- rpcs3/Emu/RSX/D3D12/D3D12Buffer.cpp | 2 +- rpcs3/Emu/RSX/GL/GLRenderTargets.h | 2 +- rpcs3/Emu/RSX/GL/GLVertexBuffers.cpp | 2 +- rpcs3/Emu/RSX/RSXVertexProgram.h | 2 +- rpcs3/Emu/RSX/VK/VKCommonDecompiler.cpp | 2 +- rpcs3/Emu/RSX/VK/VKHelpers.h | 2 +- rpcs3/Emu/RSX/VK/VKTexture.cpp | 2 +- 16 files changed, 32 insertions(+), 32 deletions(-) diff --git a/Utilities/StrFmt.cpp b/Utilities/StrFmt.cpp index 51ff654c17..4407b46a0e 100644 --- a/Utilities/StrFmt.cpp +++ b/Utilities/StrFmt.cpp @@ -257,8 +257,8 @@ struct fmt::cfmt_src void skip(std::size_t extra) { - ++sup += extra; - ++args += extra; + sup += extra + 1; + args += extra + 1; } std::size_t fmt_string(std::string& out, std::size_t extra) const diff --git a/rpcs3/Crypto/unedat.cpp b/rpcs3/Crypto/unedat.cpp index 18d58cc76a..178ec33f28 100644 --- a/rpcs3/Crypto/unedat.cpp +++ b/rpcs3/Crypto/unedat.cpp @@ -150,8 +150,8 @@ s64 decrypt_block(const fs::file* in, u8* out, EDAT_HEADER *edat, NPD_HEADER *np const int metadata_section_size = ((edat->flags & EDAT_COMPRESSED_FLAG) != 0 || (edat->flags & EDAT_FLAG_0x20) != 0) ? 0x20 : 0x10; const int metadata_offset = 0x100; - std::unique_ptr enc_data; - std::unique_ptr dec_data; + std::unique_ptr enc_data; + std::unique_ptr dec_data; u8 hash[0x10] = { 0 }; u8 key_result[0x10] = { 0 }; u8 hash_result[0x14] = { 0 }; @@ -315,7 +315,7 @@ int decrypt_data(const fs::file* in, const fs::file* out, EDAT_HEADER *edat, NPD { const int total_blocks = (int)((edat->file_size + edat->block_size - 1) / edat->block_size); u64 size_left = (int)edat->file_size; - std::unique_ptr data(new u8[edat->block_size]); + std::unique_ptr data(new u8[edat->block_size]); for (int i = 0; i < total_blocks; i++) { @@ -427,8 +427,8 @@ int check_data(unsigned char *key, EDAT_HEADER *edat, NPD_HEADER *npd, const fs: long bytes_read = 0; long bytes_to_read = metadata_size; - std::unique_ptr metadata(new u8[metadata_size]); - std::unique_ptr empty_metadata(new u8[metadata_size]); + std::unique_ptr metadata(new u8[metadata_size]); + std::unique_ptr empty_metadata(new u8[metadata_size]); while (bytes_to_read > 0) { @@ -487,7 +487,7 @@ int check_data(unsigned char *key, EDAT_HEADER *edat, NPD_HEADER *npd, const fs: if ((edat->flags & EDAT_FLAG_0x20) != 0) //Sony failed again, they used buffer from 0x100 with half size of real metadata. { int metadata_buf_size = block_num * 0x10; - std::unique_ptr metadata_buf(new u8[metadata_buf_size]); + std::unique_ptr metadata_buf(new u8[metadata_buf_size]); f->seek(file_offset + metadata_offset); f->read(metadata_buf.get(), metadata_buf_size); sha1(metadata_buf.get(), metadata_buf_size, signature_hash); @@ -516,7 +516,7 @@ int check_data(unsigned char *key, EDAT_HEADER *edat, NPD_HEADER *npd, const fs: { // Setup header signature hash. memset(signature_hash, 0, 20); - std::unique_ptr header_buf(new u8[0xD8]); + std::unique_ptr header_buf(new u8[0xD8]); f->seek(file_offset); f->read(header_buf.get(), 0xD8); sha1(header_buf.get(), 0xD8, signature_hash ); @@ -577,7 +577,7 @@ int validate_npd_hashes(const char* file_name, const u8* klicensee, NPD_HEADER * int dev_hash_result = 0; const int file_name_length = (int) strlen(file_name); - std::unique_ptr buf(new u8[0x30 + file_name_length]); + std::unique_ptr buf(new u8[0x30 + file_name_length]); // Build the title buffer (content_id + file_name). memcpy(buf.get(), npd->content_id, 0x30); diff --git a/rpcs3/Crypto/utils.cpp b/rpcs3/Crypto/utils.cpp index 6a31a7e766..0b07abf692 100644 --- a/rpcs3/Crypto/utils.cpp +++ b/rpcs3/Crypto/utils.cpp @@ -118,7 +118,7 @@ void aesecb128_encrypt(unsigned char *key, unsigned char *in, unsigned char *out bool hmac_hash_compare(unsigned char *key, int key_len, unsigned char *in, int in_len, unsigned char *hash, int hash_len) { - std::unique_ptr out(new u8[key_len]); + std::unique_ptr out(new u8[key_len]); sha1_hmac(key, key_len, in, in_len, out.get()); @@ -132,7 +132,7 @@ void hmac_hash_forge(unsigned char *key, int key_len, unsigned char *in, int in_ bool cmac_hash_compare(unsigned char *key, int key_len, unsigned char *in, int in_len, unsigned char *hash, int hash_len) { - std::unique_ptr out(new u8[key_len]); + std::unique_ptr out(new u8[key_len]); aes_context ctx; aes_setkey_enc(&ctx, key, 128); diff --git a/rpcs3/Emu/Cell/Modules/cellL10n.cpp b/rpcs3/Emu/Cell/Modules/cellL10n.cpp index b78749d563..dc5f93945e 100644 --- a/rpcs3/Emu/Cell/Modules/cellL10n.cpp +++ b/rpcs3/Emu/Cell/Modules/cellL10n.cpp @@ -161,7 +161,7 @@ bool _L10nCodeParse(s32 code, HostCode& retCode) #ifdef _MSC_VER // Use code page to transform std::string to std::wstring. -s32 _OEM2Wide(HostCode oem_code, const std::string src, std::wstring& dst) +s32 _OEM2Wide(HostCode oem_code, const std::string& src, std::wstring& dst) { //Such length returned should include the '\0' character. s32 length = MultiByteToWideChar(oem_code, 0, src.c_str(), -1, NULL, 0); @@ -178,7 +178,7 @@ s32 _OEM2Wide(HostCode oem_code, const std::string src, std::wstring& dst) } // Use Code page to transform std::wstring to std::string. -s32 _Wide2OEM(HostCode oem_code, const std::wstring src, std::string& dst) +s32 _Wide2OEM(HostCode oem_code, const std::wstring& src, std::string& dst) { //Such length returned should include the '\0' character. s32 length = WideCharToMultiByte(oem_code, 0, src.c_str(), -1, NULL, 0, NULL, NULL); @@ -195,7 +195,7 @@ s32 _Wide2OEM(HostCode oem_code, const std::wstring src, std::string& dst) } // Convert Codepage to Codepage (all char*) -std::string _OemToOem(HostCode src_code, HostCode dst_code, const std::string str) +std::string _OemToOem(HostCode src_code, HostCode dst_code, const std::string& str) { std::wstring wide; std::string result; _OEM2Wide(src_code, str, wide); diff --git a/rpcs3/Emu/Cell/Modules/sceNp.cpp b/rpcs3/Emu/Cell/Modules/sceNp.cpp index de17964847..f6d0bfe195 100644 --- a/rpcs3/Emu/Cell/Modules/sceNp.cpp +++ b/rpcs3/Emu/Cell/Modules/sceNp.cpp @@ -972,7 +972,7 @@ s32 sceNpManagerGetOnlineId(vm::ptr onlineId) return SCE_NP_ERROR_OFFLINE; } - if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) + if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) { return SCE_NP_ERROR_INVALID_STATE; } @@ -994,7 +994,7 @@ s32 sceNpManagerGetNpId(ppu_thread& ppu, vm::ptr npId) return SCE_NP_ERROR_OFFLINE; } - if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) + if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) { return SCE_NP_ERROR_INVALID_STATE; } @@ -1082,7 +1082,7 @@ s32 sceNpManagerGetAccountRegion(vm::ptr countryCode, vm::ptr< return SCE_NP_ERROR_OFFLINE; } - if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) + if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) { return SCE_NP_ERROR_INVALID_STATE; } @@ -1104,7 +1104,7 @@ s32 sceNpManagerGetAccountAge(vm::ptr age) return SCE_NP_ERROR_OFFLINE; } - if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) + if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) { return SCE_NP_ERROR_INVALID_STATE; } @@ -1126,7 +1126,7 @@ s32 sceNpManagerGetContentRatingFlag(vm::ptr isRestricted, vm::ptr age return SCE_NP_ERROR_OFFLINE; } - if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) + if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) { return SCE_NP_ERROR_INVALID_STATE; } @@ -1152,7 +1152,7 @@ s32 sceNpManagerGetChatRestrictionFlag(vm::ptr isRestricted) return SCE_NP_ERROR_OFFLINE; } - if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) + if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE) { return SCE_NP_ERROR_INVALID_STATE; } diff --git a/rpcs3/Emu/Cell/Modules/sys_libc_.cpp b/rpcs3/Emu/Cell/Modules/sys_libc_.cpp index 8c8679033d..88963734d6 100644 --- a/rpcs3/Emu/Cell/Modules/sys_libc_.cpp +++ b/rpcs3/Emu/Cell/Modules/sys_libc_.cpp @@ -31,7 +31,7 @@ struct ps3_fmt_src void skip(std::size_t extra) { - ++g_count += (u32)extra; + g_count += (u32)extra + 1; } std::size_t fmt_string(std::string& out, std::size_t extra) const diff --git a/rpcs3/Emu/Cell/SPUASMJITRecompiler.cpp b/rpcs3/Emu/Cell/SPUASMJITRecompiler.cpp index 29b1104790..3444f4a2e3 100644 --- a/rpcs3/Emu/Cell/SPUASMJITRecompiler.cpp +++ b/rpcs3/Emu/Cell/SPUASMJITRecompiler.cpp @@ -166,7 +166,7 @@ void spu_recompiler::compile(spu_function_t& f) dis_asm.dump_pc = m_pos; dis_asm.disasm(m_pos); compiler.comment(dis_asm.last_opcode.c_str()); - log += dis_asm.last_opcode.c_str(); + log += dis_asm.last_opcode; log += '\n'; } diff --git a/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.cpp b/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.cpp index d56696a2bd..f800788b3a 100644 --- a/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.cpp +++ b/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.cpp @@ -373,7 +373,7 @@ void VertexProgramDecompiler::SetDSTSca(const std::string& code) SetDST(true, code); } -std::string VertexProgramDecompiler::NotZeroPositive(const std::string code) +std::string VertexProgramDecompiler::NotZeroPositive(const std::string& code) { return "max(" + code + ", 1.E-10)"; } diff --git a/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.h b/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.h index 7f1b446902..14a5b31ee4 100644 --- a/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.h +++ b/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.h @@ -61,7 +61,7 @@ struct VertexProgramDecompiler const std::vector& m_data; ParamArray m_parr; - std::string NotZeroPositive(const std::string code); + std::string NotZeroPositive(const std::string& code); std::string GetMask(bool is_sca); std::string GetVecMask(); std::string GetScaMask(); diff --git a/rpcs3/Emu/RSX/D3D12/D3D12Buffer.cpp b/rpcs3/Emu/RSX/D3D12/D3D12Buffer.cpp index 72e01918fd..dc4d5d8ecb 100644 --- a/rpcs3/Emu/RSX/D3D12/D3D12Buffer.cpp +++ b/rpcs3/Emu/RSX/D3D12/D3D12Buffer.cpp @@ -42,7 +42,7 @@ namespace fmt::throw_exception("Wrong vector size %d" HERE, size); } - u32 get_vertex_count(const std::vector > first_count_commands) + u32 get_vertex_count(const std::vector >& first_count_commands) { u32 vertex_count = 0; for (const auto &pair : first_count_commands) diff --git a/rpcs3/Emu/RSX/GL/GLRenderTargets.h b/rpcs3/Emu/RSX/GL/GLRenderTargets.h index e88879e308..51fac654b8 100644 --- a/rpcs3/Emu/RSX/GL/GLRenderTargets.h +++ b/rpcs3/Emu/RSX/GL/GLRenderTargets.h @@ -340,7 +340,7 @@ private: return false; u32 offset = texaddr - surface_address; - if (offset >= 0) + if (texaddr >= surface_address) { std::tie(is_subslice, x_offset, y_offset) = surface->get_texture_subresource(offset, scale_to_fit); if (is_subslice) diff --git a/rpcs3/Emu/RSX/GL/GLVertexBuffers.cpp b/rpcs3/Emu/RSX/GL/GLVertexBuffers.cpp index 4bd4eb27c2..3681a68092 100644 --- a/rpcs3/Emu/RSX/GL/GLVertexBuffers.cpp +++ b/rpcs3/Emu/RSX/GL/GLVertexBuffers.cpp @@ -164,7 +164,7 @@ namespace return std::make_tuple(element_count, mapping.second); } - std::tuple upload_index_buffer(gsl::span raw_index_buffer, void *ptr, rsx::index_array_type type, rsx::primitive_type draw_mode, const std::vector> first_count_commands, u32 initial_vertex_count) + std::tuple upload_index_buffer(gsl::span raw_index_buffer, void *ptr, rsx::index_array_type type, rsx::primitive_type draw_mode, const std::vector>& first_count_commands, u32 initial_vertex_count) { u32 min_index, max_index, vertex_draw_count = initial_vertex_count; diff --git a/rpcs3/Emu/RSX/RSXVertexProgram.h b/rpcs3/Emu/RSX/RSXVertexProgram.h index 9167420c78..13cbda6d41 100644 --- a/rpcs3/Emu/RSX/RSXVertexProgram.h +++ b/rpcs3/Emu/RSX/RSXVertexProgram.h @@ -216,7 +216,7 @@ struct rsx_vertex_input bool int_type; u32 flags; //Initially zero, to be optionally filled by the backend - bool operator==(const rsx_vertex_input other) const + bool operator==(const rsx_vertex_input& other) const { return location == other.location && size == other.size && frequency == other.frequency && is_modulo == other.is_modulo && is_array == other.is_array && int_type == other.int_type && flags == other.flags; diff --git a/rpcs3/Emu/RSX/VK/VKCommonDecompiler.cpp b/rpcs3/Emu/RSX/VK/VKCommonDecompiler.cpp index cece520e31..8ae1f0ef4a 100644 --- a/rpcs3/Emu/RSX/VK/VKCommonDecompiler.cpp +++ b/rpcs3/Emu/RSX/VK/VKCommonDecompiler.cpp @@ -163,7 +163,7 @@ namespace vk rsc.maxFragmentUniformVectors = 16; rsc.maxVertexOutputVectors = 16; rsc.maxFragmentInputVectors = 15; - rsc.maxProgramTexelOffset = -8; + rsc.minProgramTexelOffset = -8; rsc.maxProgramTexelOffset = 7; rsc.maxClipDistances = 8; rsc.maxComputeWorkGroupCountX = 65535; diff --git a/rpcs3/Emu/RSX/VK/VKHelpers.h b/rpcs3/Emu/RSX/VK/VKHelpers.h index 60bd53d5ba..d7bebee8a7 100644 --- a/rpcs3/Emu/RSX/VK/VKHelpers.h +++ b/rpcs3/Emu/RSX/VK/VKHelpers.h @@ -1463,6 +1463,6 @@ namespace vk * dst_image must be in TRANSFER_DST_OPTIMAL layout and upload_buffer have TRANSFER_SRC_BIT usage flag. */ void copy_mipmaped_image_using_buffer(VkCommandBuffer cmd, VkImage dst_image, - const std::vector subresource_layout, int format, bool is_swizzled, u16 mipmap_count, + const std::vector& subresource_layout, int format, bool is_swizzled, u16 mipmap_count, vk::vk_data_heap &upload_heap, vk::buffer* upload_buffer); } diff --git a/rpcs3/Emu/RSX/VK/VKTexture.cpp b/rpcs3/Emu/RSX/VK/VKTexture.cpp index 21d8cad213..fcc92ef756 100644 --- a/rpcs3/Emu/RSX/VK/VKTexture.cpp +++ b/rpcs3/Emu/RSX/VK/VKTexture.cpp @@ -136,7 +136,7 @@ namespace vk } void copy_mipmaped_image_using_buffer(VkCommandBuffer cmd, VkImage dst_image, - const std::vector subresource_layout, int format, bool is_swizzled, u16 mipmap_count, + const std::vector& subresource_layout, int format, bool is_swizzled, u16 mipmap_count, vk::vk_data_heap &upload_heap, vk::buffer* upload_buffer) { u32 mipmap_level = 0;