Игровое поле экспериментов: какие ошибки могут подстерегать программиста при создании эмулятора

Создание эмулятора для игр 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(&section_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.

Сейчас код делает следующее:

1. Вызывается функция ID3D12CommandQueue::Signal объекта под указателем direct_queue, которая возвращает HRESULT.

2. Происходит преобразование левого операнда из типа HRESULT в тип bool. Любое ненулевое значение будет трактоваться как true, иначе false.

3. Если левый операнд был true, то вызывается функция ID3D12Fence::SetEventOnCompletion объекта под указателем queue_operations_since_submission_fence_.

4. Результат предыдущей операции передаётся в макрос SUCCEEDED, он производит корректную конвертацию HRESULT в bool.

5. Результат предыдущей конвертации передаётся в макрос SUCCEEDED.

6. На основании результата работы макроса произойдёт ветвление.

На самом деле разработчик просто ошибся с расстановкой скобок, а итоговый код должен использовать результаты двух 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 проектов, так и для студентов и преподавателей. Остальным же предлагаю получить пробную версию анализатора и попробовать его в деле :)

3 комментария

Кайфолог III

6 нояб

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

Ответить

WhoLetTheDogOut

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

НекроПоршень

Поставил лайк, типа понял что там написано, шайтаны

Игровое поле экспериментов: какие ошибки могут подстерегать программиста при создании эмулятора

Введение

Ошибочки? Ошибочки :)

Заключение