Создание эмулятора для игр Xbox 360 на ПК — задача не из простых, и на каждом шагу можно столкнуться с коварными багами. Сегодня рассмотрим типичные проблемы, которые можно обнаружить при разработке, на примере проекта Xenia.
Введение
Не так давно при поиске материалов на GameDev-тематику я случайно наткнулась на статью от разработчиков эмулятора Xenia, в которой программист графики рассказал об особенностях эмуляции платформы Xbox 360 и их успехах в этом нелёгком деле. Статья мне очень понравилась, поэтому я была особенно рада, что проект продолжает развиваться. Хороший претендент для проверки c помощью PVS-Studio, а заодно и написания своей первой статьи 🙂
Итак, Xenia — экспериментальный эмулятор платформы Xbox 360. Разработчики заявляют своей основной целью эксперименты, исследования и обучение по теме эмуляции современных устройств и операционных систем. И никакого пиратства — только реверс-инжиниринг легально купленных игр и устройств, а также чтение публично-доступной информации.
Статический анализатор PVS-Studio, кажется, в представлении не нуждается, поэтому просто упомяну, что для проверки я использовала свежий релиз 7.33 (release notes) и плагин для Visual Studio.
Кстати, раз уж мы говорим о GameDev’e, то будет не лишним упомянуть, что в свежем релизе мы сделали множество улучшений для повышения качество анализа проектов, использующих игровой движок Unreal Engine. Подробнее об этом можно почитать в отдельной заметке.
Возвращаясь к проекту, хотелось бы упомянуть, что у него нет релизных тегов или веток, поэтому при проверке использовалось состояние репозитория на момент коммита 3d30b2e.
В начале я хотела бы обратить внимание на наиболее серьёзные ошибки, а в конце — показать предупреждения, которые помогают улучшить код. Что ж, не будем больше ждать и взглянем на найденные ошибки.
Ошибочки? Ошибочки 🙂
Фрагмент 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; .... } Предупреждение PVS-Studio:
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
Анализатор выдаёт предупреждение, что значение level1_table_count будет всегда равно 0, т.к. при целочисленном делении левый операнд level0_table_count меньше, чем правый HASHES_PER_L1_HASH. Значение последнего равняется 41412, а чтобы узнать значение первого, поднимемся чуть выше.
Переменная file_block вычисляется через остаток от деления переменной true_block на BLOCKS_PER_FILE и поэтому лежит в диапазоне [0 .. 82823].
Переменная BLOCKS_PER_L0_HASH делит это значение на 408, и затем к результату прибавляется 1. При наибольшем значении file_block в результате деления получится 202, поэтому значение переменной level0_table_count будет лежать в диапазоне [1 .. 203].
Переменная level1_table_count по итогу будет вычисляться как 203/41412+1, и при любых значениях переменной true_block будет равна 1.
Может, мы где-то ошиблись? Оказывается, что нет, так думает не только наш анализатор.
Интересный пример, где ошибка абсолютно незаметна глазу. Да и ревьювер легко пропустит ошибку, т.к. проводить все такие вычисления в голове долго и скучно.
В коде есть комментарий, который, возможно, сможет помочь в решении этой загадки. Может, у вас уже есть какие-то идеи?
Фрагмент 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)); ... } Предупреждение PVS-Studio:
V1063 The modulo by 1 operation is meaningless. The result will always be zero. x64_code_cache_win.cc 299
В комментарии написано, что условие служит проверкой на чётность переменной. Однако остаток деления на 1 всегда равен 0, поэтому условие никогда не выполнится.
Обычно не думаешь, что ошибка может скрываться в такой простой задаче. Для правильной работы программы следует использовать деление с остатком не на 1, а на 2:
if (unwind_info->CountOfCodes % 2) Фрагмент N3
Следует быть внимательным при использовании union, ведь тут легко словить неопределённое поведение.
Предупреждение PVS-Studio:
V614 Uninitialized variable ‘desc.page_count’ used. xex_module.cc 594
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(); } .... } В коде создаётся объект структуры xex2_page_descriptor, которая выглядит следующим образом:
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 }; При работе с union в C++ чтение можно производить только из активного поля, т.е. из того, в которое производилась запись последний раз. Если происходит иное, то поведение такой операции не определено. Это отличает C++ от C, в котором можно записать в одно поле, а прочитать из другого.
Немногие знают про такое поведение, а те, кто знают, вполне могут забыть о нём. Спасают ситуацию компиляторы, которые в качестве нестандартного расширения поддерживают такое поведение. Однако полагаться на него не стоит, неопределённое поведение может проявиться в будущем при обновлении компилятора или его смене.
Как же можно исправить ситуацию в C++? Начиная с C++20, можно и нужно использовать std::bit_cast в таких моментах:
struct xex2_section_info { xex2_section_type info : 4; uint32_t page_count : 28; }; .... xe::be<uint32_t> value = xe::byte_swap( xex_security_info()->page_descriptors[i].value ); auto section_info = std::bit_cast<xex2_section_info>(value); total_size += section_info.page_count * heap->page_size(); До C++20 можно воспользоваться memcpy:
struct xex2_section_info { xex2_section_type info : 4; uint32_t page_count : 28; }; .... xe::be<uint32_t> 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(); Читатель может возразить: «Прекрасно, раньше мы интерпретировали записанное значение как значение другого типа, а теперь делаем копирование». Не беспокойтесь, компиляторы знают об этом паттерне и оптимизируют его, никакого копирования происходить не будет.
Ну и как вариант, можно до C++20 имплементировать свой bit_cast.
И вот ещё ряд таких же срабатываний:
-
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
Фрагмент N4
Порой и форматирование не помогает разобраться в коде. Рассмотрим такой участок:
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; } .... } Предупреждение PVS-Studio:
V716 Suspicious type conversion: bool -> HRESULT. A cast is performed between semantically different types. d3d12_command_processor.cc 2649
Анализатор выдаёт предупреждение на странную логическую операцию с операндами типов HRESULT и bool. Такая операция допустима, но не имеет смысла, т.к. HRESULT хранит статус и имеет сложный формат, который не имеет ничего общего с bool.
Сейчас код делает следующее:
-
Вызывается функция ID3D12CommandQueue::Signal объекта под указателем direct_queue, которая возвращает HRESULT.
-
Происходит преобразование левого операнда из типа HRESULT в тип bool. Любое ненулевое значение будет трактоваться как true, иначе false.
-
Если левый операнд был true, то вызывается функция ID3D12Fence::SetEventOnCompletion объекта под указателем queue_operations_since_submission_fence_.
-
Результат предыдущей операции передаётся в макрос SUCCEEDED, он производит корректную конвертацию HRESULT в bool.
-
Результат предыдущей конвертации передаётся в макрос SUCCEEDED.
-
На основании результата работы макроса произойдёт ветвление.
На самом деле разработчик просто ошибся с расстановкой скобок, а итоговый код должен использовать результаты двух SUCCEEDED в качестве операндов логического «И»:
if (SUCCEEDED(direct_queue ->Signal(queue_operations_since_submission_fence_, fence_value)) && SUCCEEDED(queue_operations_since_submission_fence_ ->SetEventOnCompletion(fence_value, fence_completion_event_))) { .... } А вообще, как мне кажется, смотреть на такую простыню в условии if ещё то удовольствие, и я бы вынесла всё это дело в переменную, чтобы улучшить читаемость кода:
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) { .... } Фрагмент N5
Ошибки copy-paste порой тяжело увидеть, именно поэтому мы тщательно проверяем код не только на code review, но и анализатором.
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; } Можно заметить некоторые идентичные блоки для определения и проверки переменных resolve_fsi_clear_32bpp_pipeline_ и resolve_fsi_clear_64bpp_pipeline_, на которые анализатор PVS-Studio выдаёт предупреждение:
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
Суть ошибки в том, что переменная resolve_fsi_clear_32bpp_pipeline_ лишний раз проверяется на валидность вместо resolve_fsi_clear_64bpp_pipeline_. Определить это не сложно — строка в теле второго условия как раз говорит о случившейся ошибке с переменной 64bpp. Решение также не сложное: во втором условии следует заменить переменную на resolve_fsi_clear_64bpp_pipeline_.
Фрагмент 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()); } } .... }; Предупреждение PVS-Studio:
V591 Non-void function should return a value. chrono.h 110
Внутри функции проверяют поле domain_ на соответствие элементам enum:
enum class Domain { // boring host clock: Host, // adheres to guest scaling // (differrent speed, changing clock drift etc): Guest }; Здесь, как и в проверке, всего два значения, но нельзя быть уверенным, что в будущем не появятся дополнительные элементы. Поэтому следует сделать так, чтобы функция всегда возвращала значение для всех веток выполнения, либо чтобы код не компилировался. В качестве исправления я могу предложить такой вариант (до C++23 он выглядит так):
template <typename> struct always_false : std::false_type {}; template <typename T> constexpr auto always_false_v = always_false<T>::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<decltype(domain_)>, "Your message."); } } Начиная с C++23, можно сильно упростить код, просто написав static_assert(false, «….») без необходимости дополнительной сущности в виде шаблона класса always_false.
Фрагмент N7
Когда говорят об ошибках в коде, часто вспоминают про разыменование нулевого указателя. Главное при исправлении такой ошибки — правильно поставить проверку.
Предупреждение PVS-Studio:
V595 The ‘extra’ pointer was utilized before it was verified against nullptr. Check lines: 51, 52. xam_app.cc 51
X_HRESULT XamApp::DispatchMessageSync(....){ .... auto extra = memory_->TranslateVirtual<X_KENUMERATOR_CONTENT_AGGREGATE*>( data->extra_ptr ); auto buffer = memory_->TranslateVirtual(data->buffer_ptr); auto e = kernel_state_->object_table() ->LookupObject<XEnumerator>(extra->handle); if (!e || !buffer || !extra) { return X_E_INVALIDARG; } .... } На первый взгляд всё нормально: создаются три указателя, и предполагается, что они могут быть нулевыми. Чтобы дальнейший код работал исправно, добавили проверки на их валидность с ранним возвратом из функции.
Однако при создании указателя e используется extra. Если он был нулевым, то его разыменование ведёт к неопределённому поведению. К сожалению, проверка extra на валидность происходит слишком поздно.
Исправленный код:
auto extra = memory_->TranslateVirtual<X_KENUMERATOR_CONTENT_AGGREGATE*>( data->extra_ptr ); auto buffer = memory_->TranslateVirtual(data->buffer_ptr); if (!buffer || !extra) { return X_E_INVALIDARG; } auto e = kernel_state_->object_table() ->LookupObject<XEnumerator>(extra->handle); if (!e) { return X_E_INVALIDARG; } Аналогичное предупреждение:
-
V595 The ‘writable_first_’ pointer was utilized before it was verified against nullptr. Check lines: 100, 105. graphics_upload_buffer_pool.cc 100
Фрагмент N8
Мы знаем про выделение памяти с помощью оператора new, и о том, что память в конце следует очищать самостоятельно. Но делается ли это везде в проекте Xenia? Давайте посмотрим пример:
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; } Предупреждение PVS-Studio:
V773 The function was exited without releasing the ‘driver’ pointer. A memory leak is possible. sdl_audio_system.cc 37
Из функции сделали ранний возврат, освободили ресурсы, которые инициализировал драйвер при конструировании, но про сам объект SDLAudioDriver забыли. В итоге имеем утечку, и такое повторяется не раз:
-
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
Долой ручное управление ресурсами — используйте идиому RAII!
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; Фрагмент N9
А сейчас посмотрим на весьма подозрительный код:
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; } Предупреждение PVS-Studio:
V570 The ‘extent.depth’ variable is assigned to itself. texture_extent.cc 58
Здесь поле TextureExtent::depth присваивается самому себе в then-ветке. Я затрудняюсь дать вариант исправления для этого кода, но что-то здесь точно происходит не так.
Фрагмент N10
Перед тем как использовать memset, лучше проверять, с какими данными он работает.
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; } Аргументом функции memset передаётся структура FileInfo, которая выглядит следующим образом:
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; }; Она включает в себя такой тип, как std::filesystem::path, который не является тривиально копируемым. Использование таких данных в функции memset может привести к неопределённому поведению, о чём нас и предупреждает анализатор:
V780 The object ‘out_info’ of a non-passive (non-PDS) type cannot be initialized using the memset function. filesystem_win.cc 209
Я бы предложила переписать этот код на современном C++, используя std::optional:
std::optional<FileInfo> GetInfo(const std::filesystem::path &path) { if (....) return {}; FileInfo out_info {}; /* fill 'out_info' data members */ return std::move(out_info); } Фрагмент N11
Расслабляться никогда не стоит. Рассмотрим ситуацию, когда кажется, что ничего страшного нет, но это не так.
bool Emulator::ExceptionCallback(....) { .... double f[32]; .... for (int i = 0; i < 32; i++) { XELOGE(" f{:<3} = {:016X} = (double){} = (float){}", i, *reinterpret_cast<uint64_t*>(&context->f[i]), context->f[i], *(float*)&context->f[i]); } .... } В коде присутствуют два опасных преобразования указателя на double:
-
сначала в указатель на uint64_t;
-
затем в указатель на float.
Это довольно серьёзная ошибка, нарушающая правила strict aliasing. Их нарушение влечёт за собой неопределённое поведение.
Об этом нас предупреждает анализатор PVS-Studio:
V615 An odd explicit conversion from ‘double’ type to ‘float’ type. emulator.cc 595
Способ исправления тот же, что и во фрагменте N4.
Фрагмент N12
В разных проектах иногда встречается ошибка, при которой происходит безусловный выход из цикла на первой итерации. Рассмотрим этот фрагмент кода:
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; } .... } Предупреждение PVS-Studio:
V612 An unconditional ‘return’ within a loop. single_layout_descriptor_set_pool.cc 110
По комментарию понятно, что требуются две итерации. Но в конце тела цикла стоит безусловный return, который и приведёт к незапланированному выходу.
Фрагмент N13
Мы рассмотрели случаи, когда проверка нужна, но стоит в неправильном месте. Сейчас же посмотрим на код, где проверка стоит в правильном месте, но оказалась ненужной.
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 } } } .... } Предупреждение анализатора:
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
Как известно, конструктор std::unique_ptr по умолчанию создаёт объект и инициализирует его нулём. Поэтому следующая за декларацией проверка бессмысленна — поток управления всегда попадёт в then-ветку.
Оказавшись там, мы встретим простынь из вложенных проверок и препроцессорных директив. Читать такой код достаточно сложно. Можно заметить, что инициализация умного указателя произойдёт лишь в том случае, если макрос XE_ARCH_AMD64 раскрывается в ненулевое значение. На основе этого можно предложить следующий вариант для упрощения:
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 .... } Фрагмент 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; } } Это фрагмент кода с блоком перехватывания исключений, но если присмотреться, то можно заметить неладное. Исключение в блоке catch перехватывается по значению, а не по ссылке.
Перехватывать исключения лучше по ссылке, потому что это позволяет:
-
не создавать копию объекта исключения;
-
поймать всех публичных наследников этого класса исключения. При перехвате по значению произойдёт срезка типа, из-за которой потеряется информация от производных типов.
Собственно, об этом и предупреждает анализатор:
V746 Object slicing. An exception should be caught by reference rather than by value. config.cc 58
Фрагмент N15
А теперь рассмотрим срабатывания, связанные с построением классов:
class ImGuiDialog { public: ~ImGuiDialog(); .... protected: virtual void OnShow() {} virtual void OnClose() {} virtual void OnDraw(ImGuiIO& io) {} }; Предупреждение PVS-Studio:
V599 The destructor was not declared as a virtual one, although the ‘ImGuiDialog’ class contains virtual functions. imgui_dialog.cc 46
Это срабатывание помогает избежать возможных проблем с использованием указателя на базовый класс.
В классе ImGuiDialog присутствуют виртуальные функции. Это означает, что у него предполагаются наследники. В таком случае деструктор также стоит сделать виртуальным. В ином случае, при разрушении объекта класса-наследника через указатель на базовый класс, возникает неопределённое поведение.
Фрагмент N16
Продолжая разговор о наследовании, отметим, что также важно помнить о правилах использования виртуальных функций в конструкторах и деструкторе класса.
Предупреждение PVS-Studio:
V1053 Calling the ‘Reset’ virtual function in the destructor may lead to unexpected result at runtime. assembler.cc 18
class Assembler { public: explicit Assembler(Backend* backend); virtual ~Assembler(); virtual bool Initialize(); virtual void Reset(); .... } Assembler::~Assembler() { Reset(); } В этом фрагменте кода представлен класс Assembler, который в своём деструкторе вызывает виртуальную функцию Assembler::Reset.
class X64Assembler : public Assembler { public: explicit X64Assembler(X64Backend* backend); ~X64Assembler() override; bool Initialize() override; void Reset() override; .... } Здесь представлен его наследник X64Assembler, переопределяющий виртуальную функцию Reset. При удалении объекта класса X64Assembler вызывается деструктор базового класса (Assembler). Внутри этого деструктора вызывается функция Reset из базового класса, а не из наследника. Возможно, автор ожидал, что будет вызываться переопределённая функция.
Мой коллега подробно разбирал проблему такого паттерна в отдельной статье и предложил следующее решение:
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 } Заключение
Мне хочется показать и остальные ошибки, найденные в проекте, но, боюсь, это будет интересно только для мейнтейнеров проекта. Поэтому вместо этого я просто напомню, что у PVS-Studio есть варианты бесплатного лицензирования как для Open-Source проектов, так и для студентов и преподавателей. Остальным же предлагаю получить пробную версию анализатора и попробовать его в деле 🙂
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Aleksandra Uvarova. Realm of gaming experiments: potential developer errors in emulator creating.
ссылка на оригинал статьи https://habr.com/ru/articles/855108/
Добавить комментарий