Our website uses cookies to enhance your browsing experience.
August 20 marked the 136th anniversary of Howard Phillips Lovecraft's birth, the man who created his own genre, Lovecraftian horror. Plenty of people have tried to copy or build on his ideas. Only a few have actually pulled it off.
Bloodborne is a good example. Its world draws on Lovecraft's work but stands on its own. For years, gamers could only dream of playing it on PC. Now that's finally possible, thanks to today's hero, the ShadPS4 emulator.
We checked ShadPS4's own code only, with no third-party libraries involved. The analysis was run on commit 24e68ba from the main branch. The findings in this article come from the High and Medium severity levels of the General Analysis group. The comments starting with //! are the author's.
The commit links are there to make it easier to verify the findings and fix the issues. They're not meant to discredit or criticize the people who wrote the code. I'm a big fan of both Lovecraft and Miyazaki's games, so a personal thank-you to everyone who contributes to the project.
Unlike my previous LLVM analysis, there won't be any weird merge artifacts here, such as two implementations getting mixed together. A hunter is never alone, and yet there are so few of them. May the hunt begin. Oh, I can't wait... hee hee...
Snippet 1. Fear your blindness
The PVS-Studio warning: 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;
}
}
}
}A textbook buffer overflow. The entire file was added in a massive commit packed with all kinds of new features, so it's easy to see how this could have slipped through.
As it turned out, while this article was being written, the bug was found and fixed by changing keys[3] = k1 to keys[2] = k1. Still, it had managed to survive in the project for a year and a half.
Snippet 2. No mercy for "Liar"
The PVS-Studio warning: 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;
}Let's see if there's actually a bug here. First of all, m_streams has always been a vector, so its size has always been unsigned.
The code was changed in this commit: the AvPlayerSource::FindStreams function replaced AvPlayerSource::HasStreams, and also took over some code from AvPlayerSource::Init.
bool AvPlayerSource::HasStreams() {
return m_streams.size() >= 0;
}If we dig even deeper, AvPlayerSource::HasStreams also had a predecessor:
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 is a smart pointer to the AVFormatContext type, and the nb_streams field is declared as follows:
unsigned int nb_streams;As we can see, things were a little less tautological back then.
If we look for where AvPlayerSource::FindStreams is used, we find it only in AvPlayerState::ProcessEvent:
void AvPlayerState::ProcessEvent() {
....
case AvEventType::AddSource: {
std::shared_lock lock(m_source_mutex);
if (m_up_source->FindStreams()) { //! Previously HasStreams(),
//! And before FindStreamInfo()
SetState(AvState::Ready);
OnPlaybackStateChanged(AvState::Ready);
} else {
OnWarning(ORBIS_AVPLAYER_ERROR_NOT_SUPPORTED);
SetState(AvState::Error);
}
break;
}
....
}So, everything points to a simple typo. The developer most likely used >= instead of > when creating AvPlayerSource::HasStreams, and then copied the same condition into the AvPlayerSource::FindStreams.
Snippet 3. Let us cleanse these foul streets
The PVS-Studio warnings:
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));
....
}The entire file was added in a single commit.
The expression sizeof(int) * 512 has type size_t, so sizeof(sizeof(int) * MAX_QUEUE)) is the size of size_t. Since size_t is usually 4 or 8 bytes, only part of the array will actually be zeroed out.
It's pretty safe to assume that the outer sizeof was added by mistake and that the intention was to zero out the entire buffer. It's hard to say for sure, though, because the array elements are only read by the sceKernelAio*** family of functions, where they're stored and called as pointers.
Hopefully, the hunters out there will take a closer look and either clean this whole thing up or leave a clear explanation of why it's actually needed.
Snippet 4. Beware of "Attack from behind"
The PVS-Studio warning: 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;
}
....
}The LOG_DEBUG macro is defined in the log.h file. After the macro is expanded, the code looks like this:
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);As we can see, when chunkIds == nullptr, we get a classic undefined behavior. The only thing that could save us here is having no logger for Common::Log::Class::Lib_PlayGo at that point. Unfortunately, all loggers are created in the main function at startup and remain active for the rest of the application's lifetime.
Even though this is only debug logging, the call to logger::log, and therefore the potential undefined behavior, still happens in a release build. The unnecessary log messages are filtered out inside the logger.
The macro has been there all along, with some variations in its form but with the dereference present in every version. The code that follows it was added later.
Judging by the commit message, the developer was expanding PlayGo support, the system that lets you start hunting before the whole game has finished downloading. The function was no longer just a placeholder and now had the necessary checks. The unconditional dereference of chunkIds however, seems to have slipped through.
There are several obvious ways to fix this, depending on the logging style used in the project. One option would be:
LOG_DEBUG(Lib_PlayGo, "called handle = {}, chunkIds = {},
numberOfEntries = {}", handle,
chunkIds ? *chunkIds : static_cast<32>(-1),
numberOfEntries);Snippet 5. Oh, Yourself, please, carry on in my stead
The PVS-Studio warning: 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 dense lines of code in which
//!commentStartLine and commentStartIndex may change
....
bool inComment =
(commentStartLine < currentLine ||
(commentStartLine == currentLine &&
commentStartIndex <= currentIndex));
.....
inComment = inComment =
(commentStartLine < currentLine ||
(commentStartLine == currentLine &&
commentStartIndex <= currentIndex));
....
}
}
}The entire thing was added in a single, massive commit.
Obviously, assigning inComment to itself makes no sense here. But how did it get there?
My guess is that the author wrote the inComment declaration, then copied the whole thing, initializer and all, from the line above and pasted it below without noticing the mistake.
Another possibility is that = was supposed to be == or !=, although neither really fits the logic of the algorithm. Of course, the final word here belongs to the developer or one of their fellow hunters.
Snippet 6. Treat "The unseen" with care
The PVS-Studio warning: 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;
}();The line
1 << amd_gpu_data_format_bit_size * 1 << amd_gpu_number_format_bit_sizeis equivalent to
1 << 6 * 1 << 4Because of operator precedence, this is interpreted as
(1 << (6 * 1)) << 4which gives us 1024.
According to GetSurfaceFormatTableIndex function, the maximum index in result is expected to be just under 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;
}This is consistent with the AMD documentation mentioned in the comment: 64 data formats and 14 ways for shaders to interpret them.
But the * 1 is puzzling. It looks like the intended expression might have been:
(1 << amd_gpu_data_format_bit_size) * ( 1 << amd_gpu_number_format_bit_size)That also gives 1024. So, the operator precedence may have been overlooked, but the expression still happens to produce the correct result.
All the functions and constants were added in a single commit, so we can rule out any coordination issues.
In the end, this doesn't seem to be a bug. Still, the code is easy to misread, and some unsuspecting developer may one day fall prey to the horrors of operator precedence.
Snippet 7. "Strong foe" waits ahead but "don't give up"
The PVS-Studio warning: V579 The ZydisDecoderDecodeFull function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. decoder.cpp 29
Previously, the file was named 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);
}
}The ZydisDecoderDecodeFull function has an explanatory comment:
/**
* @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.
**/Passing a pointer to some buffer along with the size of the pointer as the buffer length looks rather suspicious.
The length parameter takes quite a winding path through the code, but eventually ends up in a structure field:
state.buffer_len = length;That field is then used to limit instruction parsing, for example:
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;
}Previously, a constant was used instead of sizeof(code):
#define ZYDIS_MAX_INSTRUCTION_LENGTH 15That is the maximum instruction length in bytes on x86.
For the current code to be correct, there would have to be some guarantee that no instruction can be longer than the size of a pointer. I couldn't find one.
There is also some indirect evidence that this is a mistake in an example from the Zydis project, the disassembly framework used by the emulator:
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 0x007FFFFFFF42DB15Here, sizeof is used is used too, but this time on an array whose bounds are known at compile time, so it correctly gives the size of the buffer in bytes.
There is also almost identical code in the emulator's history. It was removed in 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;
}
....
}The first thing that stands out is the exact same sizeof(***) - offset pattern for length as in Zydis example. The pointer's size is also obtained in exactly the same way as in the current code. That makes the case for this being a real error even stronger.
There is more. Another, now-defunct PS4 emulator also used Zydis and called ZydisDecoderDecodeFull with an instruction length of 15:
ZyanStatus status =
ZydisDecoderDecodeFull(&m_decoder, code,
ZYDIS_MAX_INSTRUCTION_LENGTH,
&instruction, operands,
ZYDIS_MAX_OPERAND_COUNT_VISIBLE,
ZYDIS_DFLAG_VISIBLE_OPERANDS_ONLY);Still, to say for certain, we'd need to hear from the project's experienced developers.
Snippet 8. Despicable "Metamorphosis" therefore fear "Moon"
The PVS-Studio warning: 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;
}
}The function was added in its entirety, along with the scePlayGoInitialize code below it.
Since the emulator is currently built for the x86-64 architecture, where int is 4 bytes, any systemLang value below 32 will cause an overflow and undefined behavior.
But does that actually happen? Let's trace where the systemLang parameter comes from.
Our function is called only from 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;
}And system_lang gets its value from 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;
}As we can see, small system_lang values are entirely possible. In fact, they are the ones we are most likely to get. According to the documentation, each supported language has a number from 0 to 47. Japanese, for example, is 0, English is 1, and Russian is 8. So, any game using Japanese, for example, is guaranteed to trigger the overflow.
The conditions mentioned above also make more sense now. For example, when the SDK version is old enough, standard French is used instead of Canadian French:
if (lang == 0x16 && g_sdk_version < Common::ElfInfo::FW_250)
lang = 2;There's more evidence that this is a bug. The same function in another PlayGo implementation is virtually identical, but avoids the overflow:
static inline ScePlayGoLanguageMask scePlayGoConvertLanguage(int32_t systemLang)
{
return (systemLang >= 0 && systemLang < 48) ?
(1ULL << (64 - systemLang - 1)) :
0ULL;
}Snippet 9. Time for "Hidden path"
The PVS-Studio warning: 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);
....
}
....
}Earlier in this branch, there was an unconditional exit:
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;
}The commit message says:
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.
The commit says to ignore cases where a user contributes to the wrong operand. That means we shouldn't fall through when use.operand != 0, and a break is indeed missing here.
If that's not the case, a little hint like [[fallthrough]] would have helped guide those of us who weren't blessed with the same insight.
Snippet 10. Treat "Lever" with care or you must accept "Ignoring"
The PVS-Studio warning: 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;
}
....
}Once again, the entire file was added in a single commit (it had previously lived in a different directory).
Let me briefly recap how strncmp works:
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.
It looks like the author simply got the type wrong, using an unsigned type where a signed one was intended.
With our co-op in one world wrapped up, let's take a quick look at another. Besides the main repository, the ShadPS4 project has a fairly popular fork specifically tailored to Bloodborne: diegolix29/shadPS4.
It has drifted a couple thousand commits away from upstream and, alongside the usual problems, has a few of its own.
Snippet 11. The sky and the cosmos are one
The PVS-Studio warning: 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;
}
....
}The MAX_DELTA_TIME block and the orientation assignment came in a separate commit.
At first glance, it looks like deltaTime < 0.0001f should actually be deltaTime > 0.0001f. This is indirectly supported by a similar snippet in the upstream repository:
if (delta_time > 1.0f) {
orientation = last_orientation;
return;
}But then the MAX_DELTA_TIME block would make little sense. So I guess it's time to dig a little deeper into the lore of controller orientation calculations.
First, let's look at what delta_time actually is. Surprisingly, it's the time elapsed since the last update, for example:
const float delta_time = static_cast<float>
(timestamp - m_last_orientation_update) / 1'000'000.f;After looking into it, I realized that the upstream version with the 1.0f constant is there to guard against emulator slowdowns and pauses that could otherwise make things go haywire.
The fork takes a slightly different approach: in this case, it simply updates the orientation using a fixed delta of 0.1f. This means the questionable part is:
if (deltaTime <= 0.0f || deltaTime < 0.0001f) {
orientation = lastOrientation;
return;
}It seems to serve a different purpose: a paranoid safeguard against invalid data, since both negative and extremely small deltas can result in incorrect values.
So, with a fair degree of confidence, we can say that the left side of the check deltaTime <= 0.0f is unnecessary.
Snippet 12. Time for "jump"
The PVS-Studio warning: 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);
}The UNREACHABLE_MSG macro did indeed make the function 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");
}These lines were commented out in a commit with the message "SOTC hacks" (SOTC presumably stands for Shadow of the Colossus).
This change isn't present in the upstream version, so the analyzer had nothing to complain about there.
There's not much mystery here: the attribute was simply left behind. According to the C++ standard, this is undefined behavior, so [[noreturn]] should be removed.
Snippet 13. Treat hunter with care and don't be fooled
The PVS-Studio warning: 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;
}
....
}As we can see, the comparison with CUSA03014 appears twice, and both copies were added in the same commit.
This file doesn't exist in the upstream repository at all, so there is no corresponding warning there.
It's somewhat fitting that a CUSA ID is a five-digit serial number assigned to PS4 games, while our duplicate, CUSA03014, belongs to Bloodborne: The Old Hunters Edition.
CUSA003027 also stands out visually: it's longer than all the other IDs, breaking up the neat sequence, and being six digits long, it doesn't make sense.
Whether those redundant comparisons can simply be removed, or some of the IDs should be replaced with others, is something only the developer can say.
Snippet 14. Remember key but fear Malformed thing
The PVS-Studio warning: 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},
....
};The key eR8G8B8A8Srgb appears twice, with two different values: CompatibilityClass::_8BIT and CompatibilityClass::_32BIT. The C++ standard does not specify which entry will take precedence.
The link in the comment leads to the following entry:
{VK_FORMAT_R8G8B8A8_SRGB,
{FORMAT_COMPATIBILITY_CLASS::_32BIT, ....} }In other words, the 8BIT value doesn't match the pattern and also seems unnecessary. There is no corresponding entry between VK_FORMAT_R8_SRGB and VK_FORMAT_R8_SSCALED values in the reference map. The same map is present upstream, but, unsurprisingly, it doesn't contain the second 8BIT option, which was added separately later in the fork.
Only the developers can say for certain which of the two entries should be kept.
You will be freed... from this terrible hunter's dream.
Even with the power of C++ at their disposal, developers are still human. They can make mistakes, miss typos, and lose focus. Luckily, there are tools to help them survive the hunt, including those we craft in our Abandoned Old Workshop. One of them is PVS-Studio, which you can use for free on open-source projects.
Even if your project isn't open source, you can still try the analyzer for free. You know that, right?
As usual, we opened GitHub issues for what the check found: upstream, fork.
Farewell good hunter, may you find your worth in the waking world.
0
0
0
0