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