# Realm of gaming experiments: potential developer errors in emulator creating 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\. ![1177_Xenia/image1.png](https://import.viva64.com/docx/blog/1177_Xenia/image1.png) ## Intro While searching for GameDev content, I found an [article](https://xenia.jp/updates/2021/04/27/leaving-no-pixel-behind-new-render-target-cache-3x3-resolution-scaling.html) from the developers behind the [Xenia](https://github.com/xenia-project/xenia) emulator\. The graphics developer shared their experiences with emulating Xbox 360 and how they've managed to make it work\. I really enjoyed the article, so I was particularly pleased to see that the project is still going strong\. I think this is a great project to check using PVS\-Studio\. Yet, It's also a cool opportunity for me to write my first article—a true win\-win situation :\) [Xenia](https://github.com/xenia-project/xenia) 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 [here](https://pvs-studio.com/en/blog/posts/cpp/1168/)\. This project has no release tags or branches, so the code matches the [3d30b2e](https://github.com/xenia-project/xenia/tree/3d30b2eec3ab1f83140b09745bee881fb5d5dde2) 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** ```cpp 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: [V1064](https://pvs-studio.com/en/docs/warnings/v1064/) The 'level0\_table\_count' operand of integer division is less than the 'HASHES\_PER\_L1\_HASH' one\. The result will always be zero\. [stfs\_container\_device\.cc 500](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/vfs/devices/stfs_container_device.cc#L500) 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 [not](https://godbolt.org/z/79ereYfnj), it's not just our analyzer that warns about it\. It's an interesting case where we can't even spot the error at a glance\. A reviewer might easily miss it because it's time\-consuming and tedious to calculate manually\. There is a code [comment](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/vfs/devices/stfs_container_device.cc#L462-L473) that might shed light on this mystery\. Perhaps someone has already got some thoughts on the matter? **Fragment N2** ```cpp 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: [V1063](https://pvs-studio.com/en/docs/warnings/v1063/) The modulo by 1 operation is meaningless\. The result will always be zero\. x64\_code\_cache\_win\.cc 299 The comment says that the condition is a check whether the variable is even\. However, the modulo by 1 always yields 0, so the condition won't be executed\. It seems that a bug can't lurk in such a straightforward task\. To ensure the program operates correctly, it'd be better to use division with modulo by 2 instead of 1: ```cpp if (unwind_info->CountOfCodes % 2) ``` **Fragment N3** Be careful when using _union_, because we can easily get [undefined behavior](https://pvs-studio.com/en/blog/terms/0066/) here\. The PVS\-Studio warning: [V614](https://pvs-studio.com/en/docs/warnings/v614/) Uninitialized variable 'desc\.page\_count' used\. [xex\_module\.cc 594](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/cpu/xex_module.cc#L594C5-L594C55) ```cpp 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: ```cpp struct xex2_page_descriptor { union { xe::be 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 [undefined](https://timsong-cpp.github.io/cppwp/n4950/class.union.general#5)\. This sets C\+\+ apart from C, where we can write to one data member and read from another\. Some may not be aware of the behavior or may simply forget about it\. Compilers come to the rescue here—they support this behavior as a [non\-standard extension](https://en.cppreference.com/w/cpp/language/union#Explanation)\. 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 [_std::bit\_cast_](https://en.cppreference.com/w/cpp/numeric/bit_cast) for these purposes: ```cpp struct xex2_section_info { xex2_section_type info : 4; uint32_t page_count : 28; }; .... xe::be value = xe::byte_swap( xex_security_info()->page_descriptors[i].value ); auto section_info = std::bit_cast(value); total_size += section_info.page_count * heap->page_size(); ``` Before C\+\+20, we can use _memcry_: ```cpp struct xex2_section_info { xex2_section_type info : 4; uint32_t page_count : 28; }; .... xe::be value = xe::byte_swap( xex_security_info()->page_descriptors[i].value ); xex2_section_info section_info; memcpy(§ion_info, &value, sizeof(section_info); total_size += section_info.page_count * heap->page_size(); ``` The reader may argue, "Fine, we used to treat the written value as a value of a different type, and now we copy it\."No worries\! Compilers are set up to detect the pattern and [optimize](https://godbolt.org/z/61r4bqrqh) it, no copying will occur\. Alternatively, we can implement [_bit\_cast_](https://en.cppreference.com/w/cpp/numeric/bit_cast#Possible_implementation) before C\+\+20\. Here are some other similar warnings: * V614 Uninitialized variable 'desc\.page\_count' used\. xex\_module\.h 89 * V614 Uninitialized variable 'desc\.page\_count' used\. xex\_module\.cc 594 * V614 Uninitialized variable 'desc\.page\_count' used\. xex\_module\.cc 995 * V614 Uninitialized variable 'desc\.info' used\. xex\_module\.cc 996 * V614 Uninitialized variable 'desc\.page\_count' used\. xex\_module\.cc 1071 * V614 Uninitialized variable 'desc\.page\_count' used\. xex\_module\.cc 1472 * V614 Uninitialized variable 'desc\.info' used\. xex\_module\.cc 1474 * V614 Uninitialized variable 'page\_descriptor\.page\_count' used\. user\_module\.cc 687 **Fragment N4** Sometimes, the formatting doesn't help us understand the code either\. Let's take a look at such a case: ```cpp void D3D12CommandProcessor::CheckSubmissionFence(....) { .... if (SUCCEEDED( direct_queue->Signal(queue_operations_since_submission_fence_, fence_value) && SUCCEEDED(queue_operations_since_submission_fence_ ->SetEventOnCompletion(fence_value, fence_completion_event_)))) { WaitForSingleObject(fence_completion_event_, INFINITE); queue_operations_done_since_submission_signal_ = false; } .... } ``` The PVS\-Studio warning: [V716](https://pvs-studio.com/en/docs/warnings/v716/) Suspicious type conversion: bool \-\> HRESULT\. A cast is performed between semantically different types\. [d3d12\_command\_processor\.cc 2649](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/gpu/d3d12/d3d12_command_processor.cc#L2649) The analyzer detected a suspicious logical operation with operands of the _HRESULT_ and _bool_ types\. We can write such an operation but like for what? It makes no sense because _HRESULT_ represents a status and has a complex format that's unrelated to _bool_\. The code operates as follows: 1. The [_ID3D12CommandQueue::Signal_](https://learn.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12commandqueue-signal) member function has been called via the _direct\_queue_ pointer and returns _HRESULT_\. 1. The left operand is converted from _HRESULT_ to _bool_\. So, any non\-zero value will be _true_, otherwise—_false_\. 1. If the left operand is _true_, the [_ID3D12Fence::SetEventOnCompletion_](https://learn.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12fence-seteventoncompletion) member function is called via the _queue\_operations\_since\_submission\_fence\__ pointer\. 1. The result of the previous operation is passed to the [_SUCCEEDED_](https://learn.microsoft.com/en-us/windows/win32/api/winerror/nf-winerror-succeeded) macro that correctly converts _HRESULT_ to _bool_\. 1. The result of the previous conversion is passed to the _SUCCEEDED_ macro\. 1. A branch will be selected based on the macro result\. Developers might simply have misplaced the parentheses\. So, the final code should use two _SUCCEEDED_ results as operands of a logical AND: ```cpp 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: ```cpp 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\. ```cpp 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: [V1051](https://pvs-studio.com/en/docs/warnings/v1051/) Consider checking for misprints\. It's possible that the 'resolve\_fsi\_clear\_64bpp\_pipeline\_' should be checked here\. [vulkan\_render\_target\_cache\.cc 778](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/gpu/vulkan/vulkan_render_target_cache.cc#L778) 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** ```cpp template 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: [V591](https://pvs-studio.com/en/docs/warnings/v591/) Non\-void function should return a value\. [chrono\.h 110](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/base/chrono.h#L110C1-L110C4) Inside the function, the _domain\__ data member is checked against the _enum_ elements: ```cpp 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 [this](https://godbolt.org/z/ds65YTonq)\): ```cpp template struct always_false : std::false_type {}; template constexpr auto always_false_v = always_false::value; [[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()); } else { static_assert(always_false_v, "Your message."); } } ``` Starting with C\+\+23, we can greatly streamline the code using _static\_assert\(false, "\.\.\.\."\)_ without an extra entity like the _always\_false_ class template\. **Fragment N7** Speaking about errors, we often recall null dereferencing\. The key to fixing such an error is to put the check in the right place\. The PVS\-Studio warning: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'extra' pointer was utilized before it was verified against nullptr\. Check lines: 51, 52\. [xam\_app\.cc 51](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/kernel/xam/apps/xam_app.cc#L51-L52) ```cpp X_HRESULT XamApp::DispatchMessageSync(....){ .... auto extra = memory_->TranslateVirtual( data->extra_ptr ); auto buffer = memory_->TranslateVirtual(data->buffer_ptr); auto e = kernel_state_->object_table() ->LookupObject(extra->handle); if (!e || !buffer || !extra) { return X_E_INVALIDARG; } .... } ``` On the surface, all goes well: developers created three pointers and assumed that they could be null\. To make code below operate correctly, devs added the checks for validity with an early return from the function\. However, once developers created the _e_ pointer, they used _extra\._ If it's null, its dereferencing leads to undefined behavior\. Unfortunately, checking _extra_ for validity occurres too late\. Here's the fixed code: ```cpp auto extra = memory_->TranslateVirtual( data->extra_ptr ); auto buffer = memory_->TranslateVirtual(data->buffer_ptr); if (!buffer || !extra) { return X_E_INVALIDARG; } auto e = kernel_state_->object_table() ->LookupObject(extra->handle); if (!e) { return X_E_INVALIDARG; } ``` This is a similar warning: * V595 The 'writable\_first\_' pointer was utilized before it was verified against nullptr\. Check lines: 100, 105\. graphics\_upload\_buffer\_pool\.cc 100 **Fragment N8** As we know, memory is allocated via the operator _new_, and we should deallocate memory at the end\. Do the Xenia developers use this approach consistently across the project? Let's peek at an example: ```cpp X_STATUS SDLAudioSystem::CreateDriver( size_t index, xe::threading::Semaphore* semaphore, AudioDriver** out_driver ) { assert_not_null(out_driver); auto driver = new SDLAudioDriver(memory_, semaphore); if (!driver->Initialize()) { driver->Shutdown(); return X_STATUS_UNSUCCESSFUL; } *out_driver = driver; return X_STATUS_SUCCESS; } ``` The PVS\-Studio warning: [V773](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'driver' pointer\. A memory leak is possible\. [sdl\_audio\_system\.cc 37](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/apu/sdl/sdl_audio_system.cc#L37) Devs made an early return from a function, and [deallocated](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/apu/sdl/sdl_audio_driver.cc#L110-L128) 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\! ```cpp assert_not_null(out_driver); auto driver = std::make_unique(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: ```cpp 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: [V570](https://pvs-studio.com/en/docs/warnings/v570/) The 'extent\.depth' variable is assigned to itself\. [texture\_extent\.cc 58](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/gpu/texture_extent.cc#L58) 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\. ```cpp 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: ```cpp 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 [trivially copyable](https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable)_\. _Using such data in the _memset_ function may lead to undefined behavior, and the analyzer warns us about it: [V780](https://pvs-studio.com/en/docs/warnings/v780/) The object 'out\_info' of a non\-passive \(non\-PDS\) type cannot be initialized using the memset function\. [filesystem\_win\.cc 209](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/base/filesystem_win.cc#L209) I'd suggest rewriting this code in modern C\+\+ using _std::optional_: ```cpp std::optional GetInfo(const std::filesystem::path &path) { if (....) return {}; FileInfo out_info {}; /* fill 'out_info' data members */ return std::move(out_info); } ``` **Fragment N11** We always need to stay cautious\. Here's a deceptively simple case where things go wrong: ```cpp bool Emulator::ExceptionCallback(....) { .... double f[32]; .... for (int i = 0; i < 32; i++) { XELOGE(" f{:<3} = {:016X} = (double){} = (float){}", i, *reinterpret_cast(&context->f[i]), context->f[i], *(float*)&context->f[i]); } .... } ``` There are two dangerous conversions of the _double_ pointer: * to the _uint64\_t_ pointer; * to the _float_ pointer\. This is a pretty serious error that violates the rules of [strict aliasing](https://en.cppreference.com/w/cpp/language/reinterpret_cast#Type_aliasing)\. It leads to undefined behavior\. The PVS\-Studio analyzer warns us about it: [V615](https://pvs-studio.com/en/docs/warnings/v615/) An odd explicit conversion from 'double \*' type to 'float \*' type\. [emulator\.cc 595](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/emulator.cc#L595) 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: ```cpp 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: [V612](https://pvs-studio.com/en/docs/warnings/v612/) An unconditional 'return' within a loop\. [single\_layout\_descriptor\_set\_pool\.cc 110](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/ui/vulkan/single_layout_descriptor_set_pool.cc#L110) 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\. ```cpp bool Setup(TestSuite& suite) { // Reset memory. memory_->Reset(); std::unique_ptr 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: [V614](https://pvs-studio.com/en/docs/warnings/v614/) The 'backend' smart pointer is utilized immediately after being declared or reset\. It is suspicious that no value was assigned to it\. [ppc\_testing\_main\.cc 201](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/cpu/ppc/testing/ppc_testing_main.cc#L201) 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: ```cpp bool Setup(TestSuite& suite) { // Reset memory. memory_->Reset(); std::unique_ptr 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** ```cpp std::shared_ptr 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 [type slicing](https://en.wikipedia.org/wiki/Object_slicing), which leads to the loss of information from derived types\. The analyzer reports: [V746](https://pvs-studio.com/en/docs/warnings/v746/) Object slicing\. An exception should be caught by reference rather than by value\. [config\.cc 58](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/config.cc#L58) **Fragment N15** Now let's look at the warnings related to the class construction: ```cpp class ImGuiDialog { public: ~ImGuiDialog(); .... protected: virtual void OnShow() {} virtual void OnClose() {} virtual void OnDraw(ImGuiIO& io) {} }; ``` The PVS\-Studio warning: [V599](https://pvs-studio.com/en/docs/warnings/v599/) The destructor was not declared as a virtual one, although the 'ImGuiDialog' class contains virtual functions\. [imgui\_dialog\.cc 46](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/ui/imgui_dialog.cc#L46) The warning helps us prevent possible issues that may arise from using a pointer to the base class\. There are virtual functions in the _ImGuiDialog_ class\. This means that there should be derived classes\. It'd be better to introduce the destructor as virtual\. Otherwise, undefined behavior [arises](https://timsong-cpp.github.io/cppwp/n4950/expr.delete#3) 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: [V1053](https://pvs-studio.com/en/docs/warnings/v1053/) Calling the 'Reset' virtual function in the destructor may lead to unexpected result at runtime\. [assembler\.cc 18](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/cpu/backend/assembler.cc#L18) ```cpp class 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\. ```cpp class 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 [article](https://pvs-studio.com/en/blog/posts/cpp/1125/) and offered the following solution: ```cpp class Assembler { private: void ResetImpl(); public: explicit Assembler(Backend* backend); virtual ~Assembler(); virtual bool Initialize(); virtual void Reset(); .... } void Assembler::ResetImpl() { /* free only Assembler resources */ } Assembler::~Assembler() { ResetImpl(); } void Assembler::Reset() { ResetImpl(); } class X64Assembler : public Assembler { private: void ResetImpl(); public: explicit X64Assembler(X64Backend* backend); ~X64Assembler() override; bool Initialize() override; void Reset() override; .... } void X64Assembler::ResetImpl() { /* free only X64Assembler resources */ } X64Assembler::~X64Assembler() { ResetImpl(); } void X64Assembler::Reset() { ResetImpl(); // free X64Assembler resources Assembler::Reset(); // free resources of the base class } ``` ## Outro I'd like to show you the other bugs detected in the project, but I suppose this may interest only to the project maintainers\. Just a quick note to remind you that PVS\-Studio has a free license for [open\-source](https://pvs-studio.com/en/order/open-source-license/) projects and [educational purposes](https://pvs-studio.com/en/order/for-students/)\. If you haven't had a chance to try it yet, I highly recommend getting a [trial](https://pvs-studio.com/en/pvs-studio/try-free/) :\)