Александра Уварова

31 Окт 2024

Теги:

#Cpp

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

31 Окт 2024

Автор: Александра Уварова

Введение
Ошибочки? Ошибочки :)
Заключение

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

#Cpp