Сегодня ночью ShadPS4 присоединится к охоте: проверяем самый популярный эмулятор PS4

20 августа исполнилось 136 лет Говарду Филлипсу Лавкрафту, человеку, создавшему свой собственный жанр "лавкрафтовских ужасов". Многие пытались копировать или развивать его идеи, но получалось только у считанных единиц.

Удачным примером можно назвать популярную игру Bloodborne, мир которой вдохновлён творчеством Лавкрафта, но имеет свою уникальную идентичность. Долгое время геймеры лишь мечтали запустить её на ПК, и вот, наконец, это стало возможным. А всё благодаря нашему сегодняшнему герою — эмулятору ShadPS4.

В нашей проверке участвовал код непосредственно ShadPS4 без всяческих сторонних библиотек. За основу взят коммит 24e68ba main-ветки. Ошибки брались из High и Medium уровней группы общего назначения. Комментарии вида `//!` добавлены мной.

Приведённые ссылки на конкретные коммиты добавлены для упрощения верифицирования и исправления найденных ошибок и не имеют цели скомпрометировать или каким-то образом принизить их авторов. Всем контрибуторам проекта большая личная благодарность от фаната Лавкрафта и игр Миядзаки.
Автор статьи Никита Терентьев

В отличие от моей прошлой проверки llvm здесь не будет примеров "кривого мёржа", наложения двух реализаций и прочих артефактов совместной разработки. Охотник никогда не одинок, но всё же их всегда немного. Начнём же охоту, о, я не могу дождаться... хи-хи...

Фрагмент N1. Fear your blindness

Предупреждение PVS-Studio: V557 Array overrun is possible. The `3` index is pointing beyond array bound. input_handler.h 397

class InputBinding { public: InputID keys[3]; InputBinding(....) { .... if (k1 <= k2 && k1 <= k3) { .... } else if (k2 <= k1 && k2 <= k3) { .... } else { keys[0] = k3; if (k1 <= k2) { keys[1] = k1; keys[2] = k2; } else { keys[1] = k2; keys[3] = k1; } } } }

Выход за границу буфера в чистейшем виде. Весь файл добавили в огромном коммите с горой разных фич и, скорее всего, глаз автора просто замылился.

Так вышло, что пока писалась статья, эту ошибку нашли и исправили, заменив `keys[3] = k1` на `keys[2] = k1`. Но полтора года зловреду всё же удалось прожить в проекте.

Предупреждение PVS-Studio: V547 Expression `m_streams.size() >= 0` is always true. Unsigned type value is always >= 0. avplayer_source.cpp 132

class AvPlayerSource{ .... std::vector<Stream> m_streams; } bool AvPlayerSource::FindStreams() { .... return m_streams.size() >= 0; }

Давайте попробуем разобраться, присутствует ли здесь ошибка. Сразу замечу, что `m_streams` всегда был вектором, а значит его `size` всегда был беззнаковым.

Фрагмент менялся в этом коммите: функция `AvPlayerSource::FindStreams` заменила собой `AvPlayerSource::HasStreams`, утащив также часть кода из `AvPlayerSource::Init`.

bool AvPlayerSource::HasStreams() { return m_streams.size() >= 0; }

Если копнуть ещё глубже, то у `AvPlayerSource::HasStreams` тоже был предшественник:

bool AvPlayerSource::FindStreamInfo() { if (m_avformat_context == nullptr) { LOG_ERROR(Lib_AvPlayer, "Could not find stream info. NULL context."); return false; } if (m_avformat_context->nb_streams > 0) { return true; } return avformat_find_stream_info(m_avformat_context.get(), nullptr) == 0; }

`m_avformat_context` — умный указатель на тип `AVFormatContext`, а поле `nb_streams` объявлено так:

unsigned int nb_streams;

Как видим, в прошлом действия были менее тавтологичны.

Если же поискать использование `AvPlayerSource::FindStreams`, то оно присутствует только в функции `AvPlayerState::ProcessEvent`:

void AvPlayerState::ProcessEvent() { .... case AvEventType::AddSource: { std::shared_lock lock(m_source_mutex); if (m_up_source->FindStreams()) { //!HasStreams() ранее, //!FindStreamInfo() ещё ранее SetState(AvState::Ready); OnPlaybackStateChanged(AvState::Ready); } else { OnWarning(ORBIS_AVPLAYER_ERROR_NOT_SUPPORTED); SetState(AvState::Error); } break; } .... }

Как итог, всё указывает на то, что разработчик сделал опечатку `>=` при создании функции `AvPlayerSource::HasStreams`, а потом скопипастил её в функцию `AvPlayerSource::FindStreams`.

Предупреждения PVS-Studio:

V568 It's odd that the argument of sizeof() operator is the `sizeof (int) * 512` expression. aio.cpp 318

V1086 A call of the `memset` function will lead to underflow of the buffer `id_state`. aio.cpp 318

namespace Libraries::Kernel { #define MAX_QUEUE 512 static s32* id_state; static s32 id_index; .... } void RegisterAio(Core::Loader::SymbolsResolver* sym) { id_index = 1; id_state = (int*)malloc(sizeof(int) * MAX_QUEUE); memset(id_state, 0, sizeof(sizeof(int) * MAX_QUEUE)); .... }

Файл добавлен целиком.

Выражение `sizeof(int) * 512` имеет тип `size_t`, соответственно `sizeof(sizeof(int) * MAX_QUEUE))` будет равно размеру `size_t`. Поскольку его размер обычно 4 или 8 байт, обнулится в итоге только часть массива. С большой долей уверенности можно предположить, что внешний `sizeof` лишний и предполагалось заполнить нулями весь буфер. Однако найти примеры использования с конкретными данными сложно, т. к. читаются элементы массива только в семействе функций `sceKernelAio***`, которые сохраняются и вызывается как указатели. Надеюсь, "охотники" этого мира обратят внимание и либо очистят всё целиком, либо вставят чёткий знак, почему это не нужно.

Фрагмент N4. Beware of "Attack from behind"

Предупреждение PVS-Studio: V595 The `chunkIds` pointer was utilized before it was verified against nullptr. Check lines: 173, 179.

s32 PS4_SYSV_ABI scePlayGoGetProgress(...., const OrbisPlayGoChunkId* chunkIds, ....) { LOG_DEBUG(Lib_PlayGo, "called handle = {}, chunkIds = {}, numberOfEntries = {}", handle, *chunkIds, numberOfEntries); if (handle != PlaygoHandle) { return ORBIS_PLAYGO_ERROR_BAD_HANDLE; } if (chunkIds == nullptr || outProgress == nullptr) { return ORBIS_PLAYGO_ERROR_BAD_POINTER; } .... }

Макрос `LOG_DEBUG` определён в файле `log.h`,и после подстановки код выглядит так:

do { if (auto logger = Common::Log::ALL_LOGGERS[Common::Log::Class::Lib_PlayGo]){ logger->log( spdlog::level::debug, "[{}] <{}> ({}) {}:{} {}: " "called handle = {}, chunkIds = {}, numberOfEntries = {}", Common::Log::Class::Lib_PlayGo, Common::Log::to_string_view(spdlog::level::debug), Common::GetCurrentThreadName(), spdlog::source_loc::basename( "path to playgo.cpp"), 174, std::string_view(__func__) == "operator()" ? "lambda" : __func__, handle, *chunkIds, numberOfEntries); } } while (false);

Как видим, при `chunkIds == nullptr` получаем классическое UB, от которого может спасти только отсутствие в таком случае логера для `Common::Log::Class::Lib_PlayGo`. Но, увы, все логеры создаются в функции main на старте приложения и живут вместе с ним.

Несмотря на то, что логгируется только debug информация, вызов функции `logger::log`, а с ним и потенциальное UB, произойдёт и в release — излишние записи отсекаются уже внутри.

Сам макрос — немного разный по форме, но с разыменованием во всех версиях, — был всегда, а вот код после него добавлен позже.

Судя по тексту коммита, разработчик расширил поддержку системы PlayGo, той самой, что позволяет начать охоту, не дожидаясь скачивания игры целиком. Функция перестала быть просто заглушкой, получив и необходимые проверки, а безусловное разыменование `chunkIds`, видимо, выпало из поля зрения автора. Возможных исправлений тут множество, и они довольно очевидны: всё зависит от стилистики и формата логов, принятых в проекте. Я бы предложил такой вариант:

LOG_DEBUG(Lib_PlayGo, "called handle = {}, chunkIds = {}, numberOfEntries = {}", handle, chunkIds ? *chunkIds : static_cast<32>(-1), numberOfEntries);

Фрагмент N5. Oh, Yourself, please, carry on in my stead

Предупреждение PVS-Studio: V570 The same value is assigned twice to the `inComment` variable. text_editor.cpp 2109

void TextEditor::ColorizeInternal() { .... if (mCheckComments) { .... while (currentLine < endLine || currentIndex < endIndex) { .... //!~50 насыщенных строк кода, в которых могут измениться //!commentStartLine и commentStartIndex .... bool inComment = (commentStartLine < currentLine || (commentStartLine == currentLine && commentStartIndex <= currentIndex)); ..... inComment = inComment = (commentStartLine < currentLine || (commentStartLine == currentLine && commentStartIndex <= currentIndex)); .... } } }

Добавлено единым огромным коммитом.

Очевидно, что присваивание `inComment` самой себе здесь бессмысленно, но как это произошло?

Как мне кажется, автор написал декларацию `inComment`, затем скопировал декларатор с инициализатором из верхней строки целиком и всё вставил ниже, не заметив подвоха. Поэтому повторное присваивание здесь просто лишнее. Другой возможный вариант: вместо `=` должно было быть `==` или `!=`, но это не особо вяжется с логикой алгоритма. Хотя, конечно, последнее слово может сказать только сам разработчик или его "боевые товарищи".

Предупреждение PVS-Studio: V634 The priority of the `*` operation is higher than that of the `<<` operation. It`s possible that parentheses should be used in the expression. liverpool_to_vk.cpp 759

// Table 8.13 Data and Image Formats //[Sea Islands Series Instruction Set Architecture] //All values are under 64 static const size_t amd_gpu_data_format_bit_size = 6; //All values are under 16 static const size_t amd_gpu_number_format_bit_size = 4; static auto surface_format_table = []() constexpr { std::array<vk::Format, 1 << amd_gpu_data_format_bit_size * 1 << amd_gpu_number_format_bit_size> result; for (auto& entry : result) { entry = vk::Format::eUndefined; } for (const auto& supported_format : SurfaceFormats()) { result[GetSurfaceFormatTableIndex(supported_format.data_format, supported_format.number_format)] = supported_format.vk_format; } return result; }();

Строка:

1 << amd_gpu_data_format_bit_size * 1 << amd_gpu_number_format_bit_size

Равносильна:

1 << 6 * 1 << 4

Что из-за приоритета операций будет интерпретировано как:

(1 << (6 * 1)) << 4

Что равно 1024.

Судя по функции `GetSurfaceFormatTableIndex`, ожидается, что максимальный индекс в `result` как раз меньше 1024:

static size_t GetSurfaceFormatTableIndex(AmdGpu::DataFormat data_format, AmdGpu::NumberFormat num_format) { DEBUG_ASSERT(u32(data_format) < 1 << amd_gpu_data_format_bit_size); DEBUG_ASSERT(u32(num_format) < 1 << amd_gpu_number_format_bit_size); size_t result = static_cast<size_t>(num_format) | (static_cast<size_t>(data_format) << amd_gpu_number_format_bit_size); return result; }

Что вполне согласуется с документацией AMD из комментария: 64 формата данных и 14 способов их интерпретации шейдерами.

Но смущает `* 1`, как будто хотели написать:

(1 << amd_gpu_data_format_bit_size) * ( 1 << amd_gpu_number_format_bit_size)

Что тоже равно 1024, однако забыли про приоритет операции и при этом всё равно получили правильный результат.

Все функции и константы добавлены единым коммитом, поэтому проблемы кооперации исключаем.

В итоге ошибки вроде бы и нет, но код получился хрупким, и какой-нибудь неискушённый разработчик в будущем может здесь легко оступиться и пасть перед ужасами приоритета операций.

Фрагмент N7. "Strong foe" waits ahead but "don't give up"

Предупреждение PVS-Studio: V579 The ZydisDecoderDecodeFull function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. decoder.cpp 29

Ранее файл назывался `Disassembler.cpp`:

void DecoderImpl::printInstruction(void* code, u64 address) { ZydisDecodedInstruction instruction; ZydisDecodedOperand operands[ZYDIS_MAX_OPERAND_COUNT_VISIBLE]; ZyanStatus status = ZydisDecoderDecodeFull(&m_decoder, code, sizeof(code), &instruction, operands); if (!ZYAN_SUCCESS(status)) { fmt::print("decode instruction failed at {}\n", fmt::ptr(code)); } else { printInst(instruction, operands, address); } }

Для функции `ZydisDecoderDecodeFull` есть поясняющий комментарий:

/** * @param buffer A pointer to the input buffer. * @param length The length of the input buffer. * Note that this can be bigger than the * actual size of the instruction -- you don`t have to know * the size up front. This length is merely used to prevent * Zydis from doing out-of-bounds reads on your buffer. **/

Довольно подозрительно выглядит передача в функцию указателя на некий буфер и размера указателя в качестве длины этого буфера.

Путь параметра `length` довольно извилистый, но в какой-то момент он сохраняется в поле структуры:

state.buffer_len = length;

И далее используется для ограничения парсинга инструкции, например:

static ZyanStatus ZydisInputPeek(ZydisDecoderState* state, ZydisDecodedInstruction* instruction, ZyanU8* value) { .... if (state->buffer_len > 0) { *value = state->buffer[0]; return ZYAN_STATUS_SUCCESS; } return ZYDIS_STATUS_NO_MORE_DATA; } static ZyanStatus ZydisInputNext(ZydisDecoderState* state, ZydisDecodedInstruction* instruction, ZyanU8* value) { .... if (state->buffer_len > 0) { *value = state->buffer++[0]; ++instruction->length; --state->buffer_len; return ZYAN_STATUS_SUCCESS; } return ZYDIS_STATUS_NO_MORE_DATA; }

Ранее вместо `sizeof(code)` была константа:

#define ZYDIS_MAX_INSTRUCTION_LENGTH 15

Что соответствует максимальной длине инструкции в байтах на х86.

Чтобы текущий код был корректным, должна быть гарантия, что нет инструкций длиннее, чем размер указателя. И мне такую найтись не удалось.

Также в пользу ошибки косвенно говорит пример в проекте Zydis — фреймворке для дисассемблирования, используемом в эмуляторе:

ZyanU8 data[] = { 0x51, 0x8D, 0x45, 0xFF, 0x50, 0xFF, 0x75, 0x0C, 0xFF, 0x75, 0x08, 0xFF, 0x15, 0xA0, 0xA5, 0x48, 0x76, 0x85, 0xC0, 0x0F, 0x88, 0xFC, 0xDA, 0x02, 0x00 }; // The runtime address (instruction pointer) was chosen // arbitrarily here in order to better // visualize relative addressing. In your actual program, // set this to e.g. the memory address // that the code being disassembled was read from. ZyanU64 runtime_address = 0x007FFFFFFF400000; // Loop over the instructions in our buffer. ZyanUSize offset = 0; ZydisDisassembledInstruction instruction; while (ZYAN_SUCCESS(ZydisDisassembleIntel( /* machine_mode: */ ZYDIS_MACHINE_MODE_LONG_64, /* runtime_address: */ runtime_address, /* buffer: */ data + offset, /* length: */ sizeof(data) - offset, /* instruction: */ &instruction ))) { printf("%016" PRIX64 " %s\n", runtime_address, instruction.text); offset += instruction.info.length; runtime_address += instruction.info.length; } output: 007FFFFFFF400000 push rcx 007FFFFFFF400001 lea eax, [rbp-0x01] 007FFFFFFF400004 push rax 007FFFFFFF400005 push qword ptr [rbp+0x0C] 007FFFFFFF400008 push qword ptr [rbp+0x08] 007FFFFFFF40000B call [0x008000007588A5B1] 007FFFFFFF400011 test eax, eax 007FFFFFFF400013 js 0x007FFFFFFF42DB15

Здесь тоже используется `sizeof`, но для массива с известными во время компиляции границами он действительно даст размер буфера в байтах.

Также можно заметить почти такой же код в истории коммитов и в самом эмуляторе. Его выпилили в 2023 году:

void Linker::LoadModuleToMemory(Module* m){ .... auto* rt1 = reinterpret_cast<uint8_t*> (m->elf.GetElfEntry() + m->base_virtual_addr); ZyanU64 runtime_address = m->elf.GetElfEntry() + m->base_virtual_addr; // Loop over the instructions in our buffer. ZyanUSize offset = 0; ZydisDisassembledInstruction instruction; while (ZYAN_SUCCESS(ZydisDisassembleIntel( /* machine_mode: */ ZYDIS_MACHINE_MODE_LONG_64, /* runtime_address: */ runtime_address, /* buffer: */ rt1 + offset, /* length: */ sizeof(rt1) - offset, /* instruction: */ &instruction ))) { fmt::print("{:#x}" PRIX64 " {}\n", runtime_address, instruction.text); offset += instruction.info.length; runtime_address += instruction.info.length; } .... }

Сразу бросается в глаза точно такая же структура `sizeof(***) - offset` для `length`, что и в Zydis, и такое же взятие размера указателя, что и в текущем коде, что выглядит как чёткий сигнал о реальности ошибки.

Вдобавок в другом, уже почившем эмуляторе PS4, тоже использовался Zydis и функция `ZydisDecoderDecodeFull` с длиной инструкции, равной 15:

Но для полной определённости, конечно же, необходимы пояснения от опытных разработчиков проекта.

Фрагмент N8. Despicable "Metamorphosis" therefore fear "Moon"

Предупреждение PVS-Studio: V610 Undefined behavior. Check the shift operator `<<`. The right operand (`(64 - systemLang - 1)` = [16..63]) is greater than or equal to the length in bits of the promoted left operand. playgo.cpp 233

int scePlayGoConvertLanguage(int systemLang) { if (systemLang >= 0 && systemLang < 48) { //! systemLang: [0..47] return (1 << (64 - systemLang - 1)); //! 1 << [16..63] } else { return 0; } }

Функция добавлена целиком вместе с нижележащим куском `scePlayGoInitialize`.

С учётом того, что сейчас эмулятор собирается для архитектуры x86-64, где `int` занимает 4 байта, то при значениях `systemLang` меньше 32 получим переполнение и UB.

Но происходит ли такое в действительности? Отследим происхождение параметра `systemLang`.

Наша функция вызывается только из `scePlayGoInitialize`:

s32 PS4_SYSV_ABI scePlayGoInitialize(OrbisPlayGoInitParams* param) { .... s32 system_lang = 0; sceSystemServiceParamGetInt(OrbisSystemServiceParamId::Lang, &system_lang); playgo->langMask = scePlayGoConvertLanguage(system_lang); return ORBIS_OK; }

А `system_lang` туда попадает после функции `sceSystemServiceParamGetInt`:

s32 PS4_SYSV_ABI sceSystemServiceParamGetInt( OrbisSystemServiceParamId param_id, int* value ) { // TODO this probably should be stored in config for UI configuration LOG_DEBUG(Lib_SystemService, "called param_id {}", u32(param_id)); if (value == nullptr) { LOG_ERROR(Lib_SystemService, "value is null"); return ORBIS_SYSTEM_SERVICE_ERROR_PARAMETER; } switch (param_id) { case OrbisSystemServiceParamId::Lang: { s32 lang = EmulatorSettings.GetConsoleLanguage(); if (lang == 0x15 && g_sdk_version < Common::ElfInfo::FW_200) { lang = 0x12; } if (lang == 0x16 && g_sdk_version < Common::ElfInfo::FW_250) { lang = 2; } if ((lang >= 0x17 && lang <= 0x1a) && g_sdk_version < Common::ElfInfo::FW_500) { lang = 0x12; } if ((lang >= 0x1b && lang <= 0x1d) && g_sdk_version < Common::ElfInfo::FW_500) { lang = 1; } if (lang == 0x1e && g_sdk_version < Common::ElfInfo::FW_1000) { lang = 0x12; } *value = lang; break; } .... } return ORBIS_OK; }

Как видим, небольшие значения `system_lang` вполне возможны. Кроме того, они чаще всего и будут! Ведь по документации каждому поддерживаемому языку соответствует свой номер от 0 до 47. Например, японский — 0 (ожидаемо от Sony), английский — 1, а русский — 8. Так что любая игра, скажем, на японском, гарантировано вызовет переполнение.

Заодно теперь стали понятны условия выше. Вот тут, например, если sdk довольно старой версии, , то вместо канадского французского используется обычный:

if (lang == 0x16 && g_sdk_version < Common::ElfInfo::FW_250) lang = 2;

Кроме вышеперечисленного, дополнительным аргументом в пользу ошибки служит реализация этой же функции в другой имплементации PlayGo, которая практически идентична, но лишена переполнения:

static inline ScePlayGoLanguageMask scePlayGoConvertLanguage(int32_t systemLang) { return (systemLang >= 0 && systemLang < 48) ? (1ULL << (64 - systemLang - 1)) : 0ULL; }

Фрагмент N9. Time for "Hidden path"

Предупреждение PVS-Studio: V796 It is possible that `break` statement is missing in switch statement. hull_shader_transform.cpp 247

void WalkUsersOfTessConstantHelper(IR::Use use, u32 inc,bool propagateError){ IR::Inst* inst = use.user; switch (use.user->GetOpcode()) { case IR::Opcode::LoadSharedU32: case IR::Opcode::LoadSharedU64: case IR::Opcode::WriteSharedU32: case IR::Opcode::WriteSharedU64: { bool is_addr_operand = use.operand == 0; if (is_addr_operand) { u32 counter = inst->Flags<u32>(); inst->SetFlags<u32>(counter + inc); ASSERT_MSG(!propagateError, "LDS instruction {} accesses ambiguous attribute type", fmt::ptr(use.user)); // Stop here return; } } case IR::Opcode::Phi: { auto it = phi_infos.find(use.user); .... } .... }

Ранее в этой ветке был безусловный выход:

switch (use.user->GetOpcode()) { case IR::Opcode::LoadSharedU32: case IR::Opcode::LoadSharedU64: case IR::Opcode::WriteSharedU32: case IR::Opcode::WriteSharedU64: { u32 counter = inst->Flags<u32>(); inst->SetFlags<u32>(counter + inc); // Stop here return; }

В тексте коммита говорится:

Ignore when a user contributes to the wrong operand of an LDS inst, for example the data operand of WriteShared* instead of the address operand. This can mistakenly happen due to phi nodes.

Из "ignore when a user contributes to the wrong operand" можно сделать вывод, что нежелательно продолжать выполнение, когда `use.operand != 0`, и здесь действительно пропущен `break`. Если же наши предположения неверны, то разработчику стоило воспользоваться блокнотом и оставить подсказку в виде `[[fallthrough]]` своим менее благословлённым озарением коллегам.

Фрагмент N10. Treat "Lever" with care or you must accept "Ignoring"

Предупреждение PVS-Studio: V547 Expression `compare < 0` is always false. Unsigned type value is never < 0. np_common.cpp 49

s32 PS4_SYSV_ABI sceNpCmpNpIdInOrder(OrbisNpId* np_id1, OrbisNpId* np_id2, u32* out_result) { .... // Compare data u32 compare = std::strncmp(np_id1->handle.data, np_id2->handle.data, ORBIS_NP_ONLINEID_MAX_LENGTH); if (compare < 0) { *out_result = -1; return ORBIS_OK; } else if (compare > 0) { *out_result = 1; return ORBIS_OK; } .... }

Весь файл был добавлен целиком (ранее он располагался в другой папке).

Напомню семантику функции `strncmp`:

int strncmp( const char* lhs, const char* rhs, std::size_t count );

The sign of the result is the sign of the difference between the values of the first pair of characters (both interpreted as unsigned char) that differ in the arrays being compared.

Похоже, автор просто опечатался в типе, написав беззнаковый вместо знакового.

Прервав кооперацию в одном мире, заглянем ненадолго в другой. Кроме основного репозитория у проекта ShadPS4 существует довольно популярный форк, заточенный конкретно под Bloodborne: diegolix29/shadPS4.

Он ушёл от upstream на пару тысяч коммитов и, помимо общих болячек, имеет и свои уникальные.

Фрагмент N11. The sky and the cosmos are one

Предупреждение PVS-Studio: V590 Consider inspecting the 'deltaTime <= 0.0f || deltaTime < 0.0001f' expression. The expression is excessive or contains a misprint. controller.cpp 201

void GameController::CalculateOrientation(/*....*/ float deltaTime, Libraries::Pad::OrbisFQuaternion& lastOrientation, Libraries::Pad::OrbisFQuaternion& orientation) { constexpr float MAX_DELTA_TIME = 0.1f; if (deltaTime > MAX_DELTA_TIME) { deltaTime = MAX_DELTA_TIME; } if (deltaTime <= 0.0f || deltaTime < 0.0001f) { orientation = lastOrientation; return; } .... }

Кусок с `MAX_DELTA_TIME` и `orientation` добавлен отдельно.

На первый взгляд кажется, будто вместо `deltaTime < 0.0001f` должно было `deltaTime > 0.0001f`, и это косвенно подтверждается тем, что в upstream репозитории есть похожий фрагмент:

if (delta_time > 1.0f) { orientation = last_orientation; return; }

Но тогда бессмысленным становится кусок с `MAX_DELTA_TIME`. Что ж, придётся углубиться в лор вычисления ориентации контролёра.

Сперва посмотрим, что же такое `delta_time`. И, неожиданно, это дельта по времени с момента последнего обновления, например:

const float delta_time = static_cast<float> (timestamp - m_last_orientation_update) / 1'000'000.f;

Поискав, удалось понять, что вариант из upstream с константой `1.0f` служит защитой от пролагов и пауз эмулятора, при которых в результате получалась бы ерунда.

В форке поступили немного иначе и в таком случае просто обновляют ориентацию, но с фиксированной дельтой `0.1f`. Отсюда вытекает, что проблемное место:

if (deltaTime <= 0.0f || deltaTime < 0.0001f) { orientation = lastOrientation; return; }

видимо, служит другой цели — параноидальной защите от невалидных данных, поскольку и при отрицательных, и при крайне малых дельтах мы получаем некорректные значения.

В итоге со значительной долей уверенности можно сказать, что левая часть проверки `deltaTime <= 0.0f` — лишняя.

Фрагмент N12. Time for "jump"

Предупреждение PVS-Studio: V1082 Function marked as 'noreturn' may return control. This will result in undefined behavior. ir_emitter.cpp 15

[[noreturn]] void ThrowInvalidType(Type type, std::source_location loc = std::source_location::current()) { const std::string functionName = loc.function_name(); const int lineNumber = loc.line(); // UNREACHABLE_MSG("Invalid type = {}, functionName = {}, line = {}", // u32(type), functionName, // lineNumber); }

Как видим, `UNREACHABLE_MSG` действительно делал функцию `noreturn`:

#define UNREACHABLE_MSG(...) \ do { \ LOG_CRITICAL(Debug, "Unreachable code!\n" __VA_ARGS__);\ unreachable_impl(); \ } while (0) [[noreturn]] void unreachable_impl() { Common::Log::Stop(); std::fflush(stdout); Crash(); throw std::runtime_error("Unreachable code"); }

Закомментировали эти строки с сообщением "SOTC hacks" (вероятно, SOTC — Shadow Of The Colossus).

В upstream этого изменения нет, соответственно, и анализатору негде было срабатывать.

Здесь особо вопросов нет: очевидно, атрибут просто забыли удалить. Согласно стандарту C++, это — UB, и `[[noreturn]]` всё же нужно убрать.

Фрагмент N13. Treat hunter with care and don't be fooled

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'serial == "CUSA03014"' to the left and to the right of the '||' operator. storage_image_sync.cpp 42

void StorageImageSync::Sync(VideoCore::ImageId image_id) { const auto& serial = Common::ElfInfo::Instance().GameSerial(); if (serial == "CUSA11227" || serial == "CUSA12982" || serial == "CUSA00093" || serial == "CUSA03173" || serial == "CUSA00900" || serial == "CUSA00208" || serial == "CUSA01363" || serial == "CUSA01322" || serial == "CUSA003027" || serial == "CUSA00299" || serial == "CUSA00207" || serial == "CUSA03014" || serial == "CUSA03023" || serial == "CUSA03014" || serial == "CUSA00900" || serial == "CUSA00003" || serial == "CUSA01627" || serial == "CUSA01778" || serial == "CUSA03388" || serial == "CUSA01589" || serial == "CUSA01760" || serial == "CUSA07439" || serial == "CUSA07339" || serial == "CUSA08692" || serial == "CUSA08495" || serial == "CUSA50617" || serial == "CUSA18723" || serial == "CUSA28863" || serial == "CUSA00093" || serial == "CUSA00003") { return; } .... }

Как видим, сравнение с `CUSA03014` дублируется, и обе копии добавили одновременно.

В upstream вообще нет этого файла, поэтому там нет и срабатывания.

Символично, что `CUSA ID` — это пятизначный серийный номер игр для PS4 и наш дубль `CUSA03014` соответствует "Bloodborne: The Old Hunters Edition".

Также можно заметить, что `CUSA003027` визуально выделяется большей длиной, ломая стройный ряд линий, и не имеет смысла, т. к. шестизначен.

Но просто ли удалить излишние сравнения или заменить ID на другие, может сказать только сам разработчик.

Фрагмент N14. Remember key but fear Malformed thing

Предупреждение PVS-Studio: V766 An item with the same key 'vk::Format::eR8G8B8A8Srgb' has already been added. host_compatibility.cpp 202

/** * @brief The format compatibility class according to the Vulkan specification * @url * https://registry.khronos.org/vulkan/specs/ .* 1.3-extensions/html/vkspec.html#formats-compatibility-classes * @url * https://github.com/KhronosGroup/VulkanValidationLayers/ * blob/d37c676f/layers/generated/vk_format_utils.cpp#L70-L812 **/ static const std::unordered_map<vk::Format,CompatibilityClass> FORMAT_TABLE ={ .... {vk::Format::eR64G64Uint, CompatibilityClass::_128BIT}, {vk::Format::eR64Sfloat, CompatibilityClass::_64BIT}, {vk::Format::eR64Sint, CompatibilityClass::_64BIT}, {vk::Format::eR64Uint, CompatibilityClass::_64BIT}, {vk::Format::eR8G8B8A8Sint, CompatibilityClass::_32BIT}, {vk::Format::eR8G8B8A8Snorm, CompatibilityClass::_32BIT}, {vk::Format::eR8G8B8A8Srgb, CompatibilityClass::_32BIT}, // <= 32BIT {vk::Format::eR8G8B8A8Sscaled, CompatibilityClass::_32BIT}, {vk::Format::eR8G8B8A8Uint, CompatibilityClass::_32BIT}, {vk::Format::eR8G8B8A8Unorm, CompatibilityClass::_32BIT}, {vk::Format::eR8G8B8A8Uscaled, CompatibilityClass::_32BIT}, .... {vk::Format::eR8G8Uscaled, CompatibilityClass::_16BIT}, {vk::Format::eR8Sint, CompatibilityClass::_8BIT}, {vk::Format::eR8Snorm, CompatibilityClass::_8BIT}, {vk::Format::eR8Srgb, CompatibilityClass::_8BIT}, {vk::Format::eR8G8B8A8Srgb, CompatibilityClass::_8BIT}, // <= 8BIT {vk::Format::eR8Sscaled, CompatibilityClass::_8BIT}, {vk::Format::eR8Uint, CompatibilityClass::_8BIT}, .... };

Ключ `eR8G8B8A8Srgb` повторяется дважды с разными значениями: `CompatibilityClass::_8BIT` и `CompatibilityClass::_32BIT`. По стандарту C++ не специфицировано, какое из них останется в итоге.

{VK_FORMAT_R8G8B8A8_SRGB, {FORMAT_COMPATIBILITY_CLASS::_32BIT, ....} }

Т. е. значение `8BIT` не совпадает с образцом, да и просто, похоже, лишнее, с учётом того, что в референсной мапе между значениями `VK_FORMAT_R8_SRGB` и `VK_FORMAT_R8_SSCALED` ничего нет.В upstream есть эта мапа, но, естественно, нет второго варианта с 8BIT, который в форке был добавлен позже отдельно от других.

Какой из двух вариантов в итоге нужно оставить, могут уверенно сказать только разработчики.

Вот и конец ночи охоты, и пора увидеть свет дня. Пусть и наделённые силой C++, но разработчики остаются людьми и тоже могут совершать ошибки, пропускать опечатки и утрачивать бдительность. В помощь им создано множество инструментов, и в нашей мастерской в том числе. Например, бесплатная версия PVS-Studio для open source проектов.

Если у вас не open source проект, то вы все равно можете попробовать анализатор бесплатно. Вы ведь в курсе, да?

Ссылки на открытые по итогам проверки issue на GitHub: upstream, fork.

Сегодня ночью ShadPS4 присоединится к охоте: проверяем самый популярный эмулятор PS4

Добро пожаловать на ПК, добрый охотник. Желаешь проверить на баги эмулятор ShadPS4?

Результаты проверки

Фрагмент N2. No mercy for "Liar"

Фрагмент N3. Let us cleanse these foul streets

Фрагмент N6. Treat "The unseen" with care

Заключение