Creating an emulator for Xbox 360 games on a PC is challenging, and developers may encounter treacherous bugs at each stage of development. Let's explore some common issues that may arise during the process using the Xenia project as a case study.
from the developers behind the is a research emulator for the Xbox 360 platform. The project aims to experiment, research, and educate on the topic of emulating modern devices and operating systems. Lower your black flags, pirates! All information is obtained via reverse engineering of legally purchased devices, games, and content published online.
The PVS-Studio static analyzer likely needs no introduction, so I'll just note that I used the latest 7.33 release along with the plugin for Visual Studio.
Btw, since I'm talking about GameDev, I'd like to highlight that in the latest release, my team has significantly fostered the quality of analysis for Unreal Engine-driven projects. You can learn more about it commit.
I'll start with the major issues and smoothly move on to suggestions on how to enhance the code. Let's drive and delve into the detected errors!
Errors? He-he, errors :)
Fragment N1
void StfsContainerDevice::BlockToOffsetSVOD(size_t block, ....)
{
....
const size_t BLOCK_SIZE = 0x800;
const size_t HASH_BLOCK_SIZE = 0x1000;
const size_t BLOCKS_PER_L0_HASH = 0x198;
const size_t HASHES_PER_L1_HASH = 0xA1C4;
const size_t BLOCKS_PER_FILE = 0x14388;
const size_t MAX_FILE_SIZE = 0xA290000;
const size_t BLOCK_OFFSET =
header_.metadata.volume_descriptor.svod.start_data_block();
....
// Resolve the true block address and file index
size_t true_block = block - (BLOCK_OFFSET * 2);
....
size_t file_block = true_block % BLOCKS_PER_FILE;
size_t file_index = true_block / BLOCKS_PER_FILE;
size_t offset = 0;
// Calculate offset caused by Level0 Hash Tables
size_t level0_table_count = (file_block / BLOCKS_PER_L0_HASH) + 1;
offset += level0_table_count * HASH_BLOCK_SIZE;
// Calculate offset caused by Level1 Hash Tables
size_t level1_table_count = (level0_table_count / HASHES_PER_L1_HASH) + 1;
offset += level1_table_count * HASH_BLOCK_SIZE;
....
}
The PVS-Studio warning:
The analyzer warns that the level1_table_count value always equals 0 because the level0_table_count left operand is less than the HASHES_PER_L1_HASH right operand during an integer division operation. The value of_ HASHES_PER_L1_HASH_ is 41412, so to determine the level0_table_count value, take a look at the code above.
The file_block variable is computed by dividing the true_block by BLOCKS_PER_FILE, so it's within the range of [0 .. 82823].
The BLOCKS_PER_L0_HASH variable divides the value by 408, and the 1 is added to the result. When file_block reaches its maximum value, we'll get 202, so the level0_table_count variable value is within the range of [1 .. 203].
So, the level1_table_count variable is computed as 203/41412+1, and equals to 1 for any true_block values.
Could we get it wrong somewhere? It appears that might shed light on this mystery. Perhaps someone has already got some thoughts on the matter?
Fragment N2
if (unwind_info->CountOfCodes % 1)
{
// Count of unwind codes must always be even.
std::memset(&unwind_info->UnwindCode[unwind_info->CountOfCodes + 1], 0,
sizeof(UNWIND_CODE));
...
}
The PVS-Studio warning:
here.
The PVS-Studio warning:
int XexModule::ReadImageBasicCompressed(....)
{
....
for (uint32_t i = 0; i < xex_security_info()->page_descriptor_count; i++)
{
// Byteswap the bitfield manually.
xex2_page_descriptor desc;
desc.value = xe::byte_swap(
xex_security_info()->page_descriptors[i].value);
total_size += desc.page_count * heap->page_size();
}
....
}
The code creates an object of the xex2_page_descriptor structure, which looks like this:
struct xex2_page_descriptor
{
union
{
xe::be<uint32_t> value; // 0x0
struct
{
xex2_section_type info : 4;
uint32_t page_count : 28;
};
};
char data_digest[0x14]; // 0x4
};
When working with union in C++, we can read only from the active data member that was last written to. Otherwise, the behavior is . However, don't rely on it; undefined behavior may arise in the future once we update or change a compiler.
How can we address the issue in C++? Starting with C++20, we can use it, no copying will occur.
Alternatively, we can implement Suspicious type conversion: bool -> HRESULT. A cast is performed between semantically different types. member function has been called via the direct_queue pointer and returns HRESULT.
Developers might simply have misplaced the parentheses. So, the final code should use two SUCCEEDED results as operands of a logical AND:
if (SUCCEEDED(direct_queue
->Signal(queue_operations_since_submission_fence_,
fence_value))
&&
SUCCEEDED(queue_operations_since_submission_fence_
->SetEventOnCompletion(fence_value,
fence_completion_event_)))
{
....
}
I think it's still a pleasure to look at such a wall of code within the if statement. To enhance readability, I'd put it in a variable:
bool res = SUCCEEDED(
direct_queue->Signal(queue_operations_since_submission_fence_,
fence_value)
);
res = res
&& SUCCEEDED(
queue_operations_since_submission_fence_
->SetEventOnCompletion(fence_value, fence_completion_event_)
)
);
if (res)
{
....
}
Fragments N5
Copy-paste errors can often be difficult to spot, that's why we thoroughly check the code, both via code reviews and using the analyzer.
resolve_fsi_clear_32bpp_pipeline_ =
ui::vulkan::util::CreateComputePipeline(....);
if (resolve_fsi_clear_32bpp_pipeline_ == VK_NULL_HANDLE) {
XELOGE(
"VulkanRenderTargetCache: Failed to create the 32bpp resolve EDRAM "
"buffer clear pipeline");
Shutdown();
return false;
}
resolve_fsi_clear_64bpp_pipeline_ =
ui::vulkan::util::CreateComputePipeline(....);
if (resolve_fsi_clear_32bpp_pipeline_ == VK_NULL_HANDLE) { // <=
XELOGE(
"VulkanRenderTargetCache: Failed to create the 64bpp resolve EDRAM "
"buffer clear pipeline");
Shutdown();
return false;
}
We can observe some similar code blocks for defining and checking the resolve_fsi_clear_32bpp_pipeline_ and resolve_fsi_clear_64bpp_pipeline_ variables, for which the PVS-Studio analyzer issues a warning:
The developers redundantly checked resolve_fsi_clear_32bpp_pipeline_ _for validity instead of _resolve_fsi_clear_64bpp_pipeline_. It's a pretty straightforward case—the string in the second condition indicates an error related to the 64bpp variable. The fix is simple: just replace the variable in the second condition with resolve_fsi_clear_64bpp_pipeline_.
Fragment N6
template <Domain domain_>
struct NtSystemClock
{
....
[[nodiscard]] static time_point now() noexcept
{
if constexpr (domain_ == Domain::Host)
{
// QueryHostSystemTime() returns
// windows epoch times even on POSIX
return from_file_time(Clock::QueryHostSystemTime());
}
else if constexpr (domain_ == Domain::Guest)
{
return from_file_time(Clock::QueryGuestSystemTime());
}
}
....
};
The PVS-Studio warning:
Inside the function, the domain_ data member is checked against the enum elements:
enum class Domain
{
// boring host clock:
Host,
// adheres to guest scaling
// (differrent speed, changing clock drift etc):
Guest
};
While there are only two values, just like in the check, we can't be sure that there won't be extra elements in the future. Therefore, we should make the function always return a value for all execution branches, or the code should not compile. As a fix, we can use the following (before C++23, it looks like The 'extra' pointer was utilized before it was verified against nullptr. Check lines: 51, 52. The function was exited without releasing the 'driver' pointer. A memory leak is possible. the resources that the driver had initialized during construction, but they missed the SDLAudioDriver object. It results in a memory leak, and it's not the only one:
- V773 The function was exited without releasing the 'driver' pointer. A memory leak is possible. sdl_audio_system.cc 37
- V773 The function was exited without releasing the 'driver' pointer. A memory leak is possible. xaudio2_audio_system.cc 38
- V773 The function was exited without releasing the 'module' pointer. A memory leak is possible. user_module.cc 376
- V773 The function was exited without releasing the 'sem' pointer. A memory leak is possible. xsemaphore.cc 80
Down with manual control, use the RAII idiom!
assert_not_null(out_driver);
auto driver = std::make_unique<SDLAudioDriver>(memory_, semaphore);
if (!driver->Initialize())
{
driver->Shutdown();
return X_STATUS_UNSUCCESSFUL;
}
*out_driver = driver.release();
return X_STATUS_SUCCESS;
Fragments N9
Let's move on to a very suspicious code fragment:
static TextureExtent CalculateExtent(const FormatInfo* format_info,
uint32_t pitch, uint32_t height,
uint32_t depth, bool is_tiled,
bool is_guest)
{
TextureExtent extent;
extent.depth = depth;
if (is_guest)
{
....
// Is depth special?
extent.depth = extent.depth;
}
return extent;
}
The PVS-Studio warning:
The TextureExtent::depth data member is assigned to itself in the then branch. I find it hard to come up with a solution here, but something is wrong.
Fragment N10
Before using memset, it's better to check what data it handles.
bool GetInfo(const std::filesystem::path& path, FileInfo* out_info)
{
std::memset(out_info, 0, sizeof(FileInfo));
....
if (....) return false;
/* fill 'out_info' data members */
return true;
}
An object of the FileInfo type is passed to the_ memset_ function as an argument, which looks as follows:
struct FileInfo {
enum class Type {
kFile,
kDirectory,
};
Type type;
std::filesystem::path name;
std::filesystem::path path;
size_t total_size;
uint64_t create_timestamp;
uint64_t access_timestamp;
uint64_t write_timestamp;
};
It includes the std::filesystem::path type, which isn't The object 'out_info' of a non-passive (non-PDS) type cannot be initialized using the memset function. . It leads to undefined behavior.
The PVS-Studio analyzer warns us about it:
We can fix it here as in fragment N4.
Fragment N12
In different projects, we can encounter an error that causes an unconditional return within a loop at the first iteration. Let's take a look at the following code snippet:
size_t SingleLayoutDescriptorSetPool::Allocate()
{
....
// Two iterations so if vkAllocateDescriptorSets fails
// even with a non-zero current_pool_sets_remaining_,
// another attempt will be made in a new pool.
for (uint32_t i = 0; i < 2; ++i)
{
if ( current_pool_ != VK_NULL_HANDLE
&& !current_pool_sets_remaining_)
{
full_pools_.push_back(current_pool_);
current_pool_ = VK_NULL_HANDLE;
}
....
--current_pool_sets_remaining_;
descriptor_sets_.push_back(descriptor_set);
return descriptor_sets_.size() - 1;
}
....
}
The PVS-Studio warning:
The comment makes it clear that we need two iterations. At the end of the loop body, there is an unconditional return, which leads to an unexpected return.
Fragment N13
We've examined scenarios where a check is necessary but incorrectly placed. Now, let's consider the code where the check is in the right place yet redundant.
bool Setup(TestSuite& suite)
{
// Reset memory.
memory_->Reset();
std::unique_ptr<xe::cpu::backend::Backend> backend;
if (!backend)
{
#if XE_ARCH_AMD64
if (cvars::cpu == "x64")
{
backend.reset(new xe::cpu::backend::x64::X64Backend());
}
#endif // XE_ARCH
if (cvars::cpu == "any")
{
if (!backend)
{
#if XE_ARCH_AMD64
backend.reset(new xe::cpu::backend::x64::X64Backend());
#endif // XE_ARCH
}
}
}
....
}
The analyzer warnings:
As we know, the std::unique_ptr constructor creates an object and initializes it to null by default. That's why the check after the declaration doesn't matter; the control flow will proceed to the then branch.
Once there, we encounter a wall of nested checks and preprocessor directives. It can be tricky to read code like this. We may notice that the smart pointer will be initialized only if the XE_ARCH_AMD64 macro is expanded to a non-zero value. We can facilitate it this way:
bool Setup(TestSuite& suite)
{
// Reset memory.
memory_->Reset();
std::unique_ptr<xe::cpu::backend::Backend> backend;
#if XE_ARCH_AMD64
if (cvars::cpu == "x64" || cvars::cpu == "any")
{
backend.reset(new xe::cpu::backend::x64::X64Backend());
}
#endif // XE_ARCH
....
}
Fragment N14
std::shared_ptr<cpptoml::table>
ParseConfig(const std::filesystem::path& config_path)
{
try
{
return ParseFile(config_path);
}
catch (cpptoml::parse_exception e)
{
xe::FatalError(
fmt::format("Failed to parse config file '{}':\n\n{}",
xe::path_to_utf8(config_path),
e.what())
);
return nullptr;
}
}
Here is the exception catching block but look closely — something strange is going on here. The exception in the catch block is caught by value, not by reference.
It's better to catch exceptions by reference because it enables us to:
- avoid creating the exception object copy;
- catch all publicly inherited exceptions from this class. Catching by value results in Object slicing. An exception should be caught by reference rather than by value. The destructor was not declared as a virtual one, although the 'ImGuiDialog' class contains virtual functions. when we destroy a derived class object via the pointer to the base class.
Fragment N16
Speaking of inheritance, it's also important to remember the rules of using virtual functions in the class constructors and destructors.
The PVS-Studio warning:
CODEclass Assembler
{
public:
explicit Assembler(Backend* backend);
virtual ~Assembler();
virtual bool Initialize();
virtual void Reset();
....
}
Assembler::~Assembler() { Reset(); }
The code fragment contains the Assembler class, which calls the Assembler::Reset virtual function in its destructor.
CODEclass X64Assembler : public Assembler
{
public:
explicit X64Assembler(X64Backend* backend);
~X64Assembler() override;
bool Initialize() override;
void Reset() override;
....
}
Here's its derived class, X64Assembler, that overrides the Reset virtual function. If we delete an object of the X64Assembler class, the destructor of the base class, Assembler, will be called. In the destructor, the Reset function is called from the base class, not from the derived. Developers might've expected an overridden function to be called.
My colleague described the pattern in more detail in a separate projects and :)
↗ Original-Artikel auf dev.to lesenVollständiger Original-BerichtAusführliche Details, Code-Beispiele & Hersteller-Stellungnahme auf dev.to.
SOCIAL SHARE CARD GENERATOR