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

Создание эмулятора для игр Xbox 360 на ПК — задача не из простых, и на каждом шагу можно столкнуться с коварными багами\. Сегодня рассмотрим типичные проблемы, которые можно обнаружить при разработке, на примере проекта Xenia\.

![1177_Xenia_ru/image1.png](https://import.viva64.com/docx/blog/1177_Xenia_ru/image1.png)

## Введение

Не так давно при поиске материалов на GameDev\-тематику я случайно наткнулась на [статью](https://habr.com/ru/articles/555554/) от разработчиков эмулятора [Xenia](https://github.com/xenia-project/xenia), в которой программист графики рассказал об особенностях эмуляции платформы Xbox 360 и их успехах в этом нелёгком деле\. Статья мне очень понравилась, поэтому я была особенно рада, что проект продолжает развиваться\. Хороший претендент для проверки c помощью PVS\-Studio, а заодно и написания своей первой статьи :\)

Итак, [Xenia](https://github.com/xenia-project/xenia) — экспериментальный эмулятор платформы Xbox 360\. Разработчики заявляют своей основной целью эксперименты, исследования и обучение по теме эмуляции современных устройств и операционных систем\. И никакого пиратства — только реверс\-инжиниринг легально купленных игр и устройств, а также чтение публично\-доступной информации\. 

Статический анализатор PVS\-Studio, кажется, в представлении не нуждается, поэтому просто упомяну, что для проверки я использовала свежий релиз 7\.33 \([release notes](https://pvs-studio.ru/ru/docs/manual/0010/#ID1EB8235B17)\) и плагин для Visual Studio\.

Кстати, раз уж мы говорим о GameDev'e, то будет не лишним упомянуть, что в свежем релизе мы сделали множество улучшений для повышения качество анализа проектов, использующих игровой движок Unreal Engine\. Подробнее об этом можно почитать [в отдельной заметке](https://habr.com/ru/companies/pvs-studio/articles/849896/)\.

Возвращаясь к проекту, хотелось бы упомянуть, что у него нет релизных тегов или веток, поэтому при проверке использовалось состояние репозитория на момент коммита [3d30b2e](https://github.com/xenia-project/xenia/tree/3d30b2eec3ab1f83140b09745bee881fb5d5dde2)\. 

В начале я хотела бы обратить внимание на наиболее серьёзные ошибки, а в конце — показать предупреждения, которые помогают улучшить код\. Что ж, не будем больше ждать и взглянем на найденные ошибки\.

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

**Фрагмент N1**

```cpp
void StfsContainerDevice::BlockToOffsetSVOD(size_t block, ....)
{
  ....
  const size_t BLOCK_SIZE = 0x800;
  const size_t HASH_BLOCK_SIZE = 0x1000;
  const size_t BLOCKS_PER_L0_HASH = 0x198;
  const size_t HASHES_PER_L1_HASH = 0xA1C4;
  const size_t BLOCKS_PER_FILE = 0x14388;
  const size_t MAX_FILE_SIZE = 0xA290000;
  const size_t BLOCK_OFFSET =
      header_.metadata.volume_descriptor.svod.start_data_block();
  ....

  // Resolve the true block address and file index
  size_t true_block = block - (BLOCK_OFFSET * 2);
  ....
  size_t file_block = true_block % BLOCKS_PER_FILE;
  size_t file_index = true_block / BLOCKS_PER_FILE;
  size_t offset = 0;

  // Calculate offset caused by Level0 Hash Tables
  size_t level0_table_count = (file_block / BLOCKS_PER_L0_HASH) + 1;
  offset += level0_table_count * HASH_BLOCK_SIZE;

  // Calculate offset caused by Level1 Hash Tables
  size_t level1_table_count = (level0_table_count / HASHES_PER_L1_HASH) + 1;
  offset += level1_table_count * HASH_BLOCK_SIZE;
  ....
}
```

Предупреждение PVS\-Studio:

[V1064](https://pvs-studio.ru/ru/docs/warnings/v1064/) The 'level0\_table\_count' operand of integer division is less than the 'HASHES\_PER\_L1\_HASH' one\. The result will always be zero\. [stfs\_container\_device\.cc 500](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/vfs/devices/stfs_container_device.cc#L500)

Анализатор выдаёт предупреждение, что значение _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\.

Может, мы где\-то ошиблись? Оказывается, что [нет](https://godbolt.org/z/79ereYfnj), так думает не только наш анализатор\.

Интересный пример, где ошибка абсолютно незаметна глазу\. Да и ревьювер легко пропустит ошибку, т\.к\. проводить все такие вычисления в голове долго и скучно\.

В коде есть [комментарий](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/vfs/devices/stfs_container_device.cc#L462-L473), который, возможно, сможет помочь в решении этой загадки\. Может, у вас уже есть какие\-то идеи?

**Фрагмент N2**

```cpp
if (unwind_info->CountOfCodes % 1)
{ 
  // Count of unwind codes must always be even.

  std::memset(&unwind_info->UnwindCode[unwind_info->CountOfCodes + 1], 0,
              sizeof(UNWIND_CODE));
  ...
}
```

Предупреждение PVS\-Studio:

[V1063](https://pvs-studio.ru/ru/docs/warnings/v1063/) The modulo by 1 operation is meaningless\. The result will always be zero\. x64\_code\_cache\_win\.cc 299

В комментарии написано, что условие служит проверкой на чётность переменной\. Однако остаток деления на 1 всегда равен 0, поэтому условие никогда не выполнится\. 

Обычно не думаешь, что ошибка может скрываться в такой простой задаче\. Для правильной работы программы следует использовать деление с остатком не на 1, а на 2:

```cpp
if (unwind_info->CountOfCodes % 2)
```

**Фрагмент N3**

Следует быть внимательным при использовании _union_, ведь тут легко словить [неопределённое поведение](https://pvs-studio.ru/ru/blog/terms/0066/)\.

Предупреждение PVS\-Studio:

[V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Uninitialized variable 'desc\.page\_count' used\. [xex\_module\.cc 594](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/cpu/xex_module.cc#L594C5-L594C55)

```cpp
int XexModule::ReadImageBasicCompressed(....)
{
  ....
  for (uint32_t i = 0; i < xex_security_info()->page_descriptor_count; i++)
  {
    // Byteswap the bitfield manually.

    xex2_page_descriptor desc;
    desc.value = xe::byte_swap(
                     xex_security_info()->page_descriptors[i].value);

    total_size += desc.page_count * heap->page_size();
  } 
  ....
}
```

В коде создаётся объект структуры _xex2\_page\_descriptor_, которая выглядит следующим образом:

```cpp
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\+\+ чтение можно производить только из активного поля, т\.е\. из того, в которое производилась запись последний раз\. Если происходит иное, то поведение такой операции [не определено](https://timsong-cpp.github.io/cppwp/n4950/class.union.general#5)\. Это отличает C\+\+ от C, в котором можно записать в одно поле, а прочитать из другого\.

Немногие знают про такое поведение, а те, кто знают, вполне могут забыть о нём\. Спасают ситуацию компиляторы, которые в качестве [нестандартного расширения](https://en.cppreference.com/w/cpp/language/union#Explanation) поддерживают такое поведение\. Однако полагаться на него не стоит, неопределённое поведение может проявиться в будущем при обновлении компилятора или его смене\.

Как же можно исправить ситуацию в C\+\+? Начиная с C\+\+20, можно и нужно использовать [_std::bit\_cast_](https://en.cppreference.com/w/cpp/numeric/bit_cast) в таких моментах:

```cpp
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_:

```cpp
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();
```



Читатель может возразить: "Прекрасно, раньше мы интерпретировали записанное значение как значение другого типа, а теперь делаем копирование"\. Не беспокойтесь, компиляторы знают об этом паттерне и [оптимизируют](https://godbolt.org/z/61r4bqrqh) его, никакого копирования происходить не будет\.

Ну и как вариант, можно до C\+\+20 имплементировать свой [_bit\_cast_](https://en.cppreference.com/w/cpp/numeric/bit_cast#Possible_implementation)\.

И вот ещё ряд таких же срабатываний:

* 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**

Порой и форматирование не помогает разобраться в коде\. Рассмотрим такой участок:

```cpp
void D3D12CommandProcessor::CheckSubmissionFence(....)
{
  ....
  if (SUCCEEDED(
        direct_queue->Signal(queue_operations_since_submission_fence_,
                             fence_value) &&
        SUCCEEDED(queue_operations_since_submission_fence_
                      ->SetEventOnCompletion(fence_value,
                                             fence_completion_event_))))
  {
    WaitForSingleObject(fence_completion_event_, INFINITE);
    queue_operations_done_since_submission_signal_ = false;
  }
  ....
}
```

Предупреждение PVS\-Studio:

[V716](https://pvs-studio.ru/ru/docs/warnings/v716/) Suspicious type conversion: bool \-\> HRESULT\. A cast is performed between semantically different types\. [d3d12\_command\_processor\.cc 2649](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/gpu/d3d12/d3d12_command_processor.cc#L2649)

Анализатор выдаёт предупреждение на странную логическую операцию с операндами типов_ HRESULT _и_ bool_\. Такая операция допустима, но не имеет смысла, т\.к\. _HRESULT _хранит статус и имеет сложный формат, который не имеет ничего общего с _bool_\.

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

1. Вызывается функция [_ID3D12CommandQueue::Signal_](https://learn.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12commandqueue-signal) объекта под указателем _direct\_queue_, которая возвращает _HRESULT_\.
1. Происходит преобразование левого операнда из типа _HRESULT_ в тип _bool_\. Любое ненулевое значение будет трактоваться как _true_, иначе _false_\.
1. Если левый операнд был _true_, то вызывается функция [_ID3D12Fence::SetEventOnCompletion_](https://learn.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12fence-seteventoncompletion) объекта под указателем _queue\_operations\_since\_submission\_fence\__\.
1. Результат предыдущей операции передаётся в макрос [_SUCCEEDED_](https://learn.microsoft.com/en-us/windows/win32/api/winerror/nf-winerror-succeeded), он производит корректную конвертацию _HRESULT_ в _bool_\.
1. Результат предыдущей конвертации передаётся в макрос _SUCCEEDED_\.
1. На основании результата работы макроса произойдёт ветвление\.

На самом деле разработчик просто ошибся с расстановкой скобок, а итоговый код должен использовать результаты двух _SUCCEEDED_ в качестве операндов логического "И":

```cpp
if (SUCCEEDED(direct_queue
                     ->Signal(queue_operations_since_submission_fence_,
                              fence_value))
 &&
    SUCCEEDED(queue_operations_since_submission_fence_
                      ->SetEventOnCompletion(fence_value,
                                             fence_completion_event_)))
{
  ....
}
```

А вообще, как мне кажется, смотреть на такую простыню в условии _if_ ещё то удовольствие, и я бы вынесла всё это дело в переменную, чтобы улучшить читаемость кода:

```cpp
bool res = SUCCEEDED(
 direct_queue->Signal(queue_operations_since_submission_fence_,
                      fence_value)
);

res = res
   && SUCCEEDED(
        queue_operations_since_submission_fence_
          ->SetEventOnCompletion(fence_value, fence_completion_event_)
       )
     );

if (res)
{
  ....
}
```

**Фрагмент N5**

Ошибки copy\-paste порой тяжело увидеть, именно поэтому мы тщательно проверяем код не только на code review, но и анализатором\.

```cpp
resolve_fsi_clear_32bpp_pipeline_ = 
                  ui::vulkan::util::CreateComputePipeline(....);

if (resolve_fsi_clear_32bpp_pipeline_ == VK_NULL_HANDLE) {
  XELOGE(
    "VulkanRenderTargetCache: Failed to create the 32bpp resolve EDRAM "
    "buffer clear pipeline");
  Shutdown();
  return false;
}


resolve_fsi_clear_64bpp_pipeline_ = 
                  ui::vulkan::util::CreateComputePipeline(....);

if (resolve_fsi_clear_32bpp_pipeline_ == VK_NULL_HANDLE) {        // <=
  XELOGE(
    "VulkanRenderTargetCache: Failed to create the 64bpp resolve EDRAM "
    "buffer clear pipeline");
  Shutdown();
  return false;
}
```

Можно заметить некоторые идентичные блоки для определения и проверки переменных _resolve\_fsi\_clear\_32bpp\_pipeline\__ и _resolve\_fsi\_clear\_64bpp\_pipeline\__, на которые анализатор PVS\-Studio выдаёт предупреждение:

[V1051](https://pvs-studio.ru/ru/docs/warnings/v1051/) Consider checking for misprints\. It's possible that the 'resolve\_fsi\_clear\_64bpp\_pipeline\_' should be checked here\. [vulkan\_render\_target\_cache\.cc 778](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/gpu/vulkan/vulkan_render_target_cache.cc#L778)

Суть ошибки в том, что переменная _resolve\_fsi\_clear\_32bpp\_pipeline\__ лишний раз проверяется на валидность вместо _resolve\_fsi\_clear\_64bpp\_pipeline\__\. Определить это не сложно — строка в теле второго условия как раз говорит о случившейся ошибке с переменной _64bpp_\. Решение также не сложное: во втором условии следует заменить переменную на _resolve\_fsi\_clear\_64bpp\_pipeline\__\.

**Фрагмент N6**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v591/) Non\-void function should return a value\. [chrono\.h 110](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/base/chrono.h#L110C1-L110C4)

Внутри функции проверяют поле _domain\__ на соответствие элементам _enum:_

```cpp
enum class Domain
{
  // boring host clock:
  Host,
  // adheres to guest scaling
  // (differrent speed, changing clock drift etc):
  Guest
};
```

Здесь, как и в проверке, всего два значения, но нельзя быть уверенным, что в будущем не появятся дополнительные элементы\. Поэтому следует сделать так, чтобы функция всегда возвращала значение для всех веток выполнения, либо чтобы код не компилировался\. В качестве исправления я могу предложить такой вариант \(до C\+\+23 он выглядит [так](https://godbolt.org/z/ds65YTonq)\):

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'extra' pointer was utilized before it was verified against nullptr\. Check lines: 51, 52\. [xam\_app\.cc 51](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/kernel/xam/apps/xam_app.cc#L51-L52)

```cpp
X_HRESULT XamApp::DispatchMessageSync(....){
  ....
  auto extra = memory_->TranslateVirtual<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_ на валидность происходит слишком поздно\.

Исправленный код:

```cpp
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? Давайте посмотрим пример:

```cpp
X_STATUS SDLAudioSystem::CreateDriver(
  size_t index,
  xe::threading::Semaphore* semaphore,
  AudioDriver** out_driver
)
{
  assert_not_null(out_driver);
  auto driver = new SDLAudioDriver(memory_, semaphore);

  if (!driver->Initialize())
  {
    driver->Shutdown();
    return X_STATUS_UNSUCCESSFUL;
  }

  *out_driver = driver;
  return X_STATUS_SUCCESS;
}
```

Предупреждение PVS\-Studio:

[V773](https://pvs-studio.ru/ru/docs/warnings/v773/) The function was exited without releasing the 'driver' pointer\. A memory leak is possible\. [sdl\_audio\_system\.cc 37](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/apu/sdl/sdl_audio_system.cc#L37)

Из функции сделали ранний возврат, [освободили](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/apu/sdl/sdl_audio_driver.cc#L110-L128) ресурсы, которые инициализировал драйвер при конструировании, но про сам объект _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\!

```cpp
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**

А сейчас посмотрим на весьма подозрительный код:

```cpp
static TextureExtent CalculateExtent(const FormatInfo* format_info,
                                     uint32_t pitch, uint32_t height,
                                     uint32_t depth, bool is_tiled,
                                     bool is_guest)
{
  TextureExtent extent; 
  extent.depth = depth;
  if (is_guest)
  {
    ....
    // Is depth special?
    extent.depth = extent.depth; 
  }

  return extent;
}
```

Предупреждение PVS\-Studio:

[V570](https://pvs-studio.ru/ru/docs/warnings/v570/) The 'extent\.depth' variable is assigned to itself\. [texture\_extent\.cc 58](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/gpu/texture_extent.cc#L58)

Здесь поле _TextureExtent::depth_ присваивается самому себе в _then_\-ветке\. Я затрудняюсь дать вариант исправления для этого кода, но что\-то здесь точно происходит не так\.

**Фрагмент N10**

Перед тем как использовать _memset_, лучше проверять, с какими данными он работает\.

```cpp
bool GetInfo(const std::filesystem::path& path, FileInfo* out_info)
{
  std::memset(out_info, 0, sizeof(FileInfo)); 
  ....
  if (....) return false;

  /* fill 'out_info' data members */

  return true;
}
```

Аргументом функции _memset_ передаётся структура _FileInfo_, которая выглядит следующим образом:

```cpp
struct FileInfo {
  enum class Type {
    kFile,
    kDirectory,
  };
  Type type;
  std::filesystem::path name;
  std::filesystem::path path;
  size_t total_size;
  uint64_t create_timestamp;
  uint64_t access_timestamp;
  uint64_t write_timestamp;
};
```

Она включает в себя такой тип, как _std::filesystem::path_, который не является [тривиально копируемым](https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable)_\. _Использование таких данных в функции _memset_ может привести к неопределённому поведению, о чём нас и предупреждает анализатор:

[V780](https://pvs-studio.ru/ru/docs/warnings/v780/) The object 'out\_info' of a non\-passive \(non\-PDS\) type cannot be initialized using the memset function\. [filesystem\_win\.cc 209](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/base/filesystem_win.cc#L209)

Я бы предложила переписать этот код на современном C\+\+, используя _std::optional_:

```cpp
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**

Расслабляться никогда не стоит\. Рассмотрим ситуацию, когда кажется, что ничего страшного нет, но это не так\.

```cpp
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](https://en.cppreference.com/w/cpp/language/reinterpret_cast#Type_aliasing)\. Их нарушение влечёт за собой неопределённое поведение\.

Об этом нас предупреждает анализатор PVS\-Studio:

[V615](https://pvs-studio.ru/ru/docs/warnings/v615/) An odd explicit conversion from 'double \*' type to 'float \*' type\. [emulator\.cc 595](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/emulator.cc#L595)

Способ исправления тот же, что и во фрагменте N4\.

**Фрагмент N12**

В разных проектах иногда встречается ошибка, при которой происходит безусловный выход из цикла на первой итерации\. Рассмотрим этот фрагмент кода:

```cpp
size_t SingleLayoutDescriptorSetPool::Allocate()
{
  ....

  // Two iterations so if vkAllocateDescriptorSets fails
  // even with a non-zero current_pool_sets_remaining_,
  // another attempt will be made in a new pool.
  for (uint32_t i = 0; i < 2; ++i)
  {
    if (    current_pool_ != VK_NULL_HANDLE
        && !current_pool_sets_remaining_)
    {
        full_pools_.push_back(current_pool_);
        current_pool_ = VK_NULL_HANDLE;
    }
    ....
    --current_pool_sets_remaining_;
    descriptor_sets_.push_back(descriptor_set); 
 
    return descriptor_sets_.size() - 1;
  }
  ....
}
```

Предупреждение PVS\-Studio:

[V612](https://pvs-studio.ru/ru/docs/warnings/v612/) An unconditional 'return' within a loop\. [single\_layout\_descriptor\_set\_pool\.cc 110](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/ui/vulkan/single_layout_descriptor_set_pool.cc#L110)

По комментарию понятно, что требуются две итерации\. Но в конце тела цикла стоит безусловный _return_, который и приведёт к незапланированному выходу\.

**Фрагмент N13**

Мы рассмотрели случаи, когда проверка нужна, но стоит в неправильном месте\. Сейчас же посмотрим на код, где проверка стоит в правильном месте, но оказалась ненужной\.

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v614/) The 'backend' smart pointer is utilized immediately after being declared or reset\. It is suspicious that no value was assigned to it\. [ppc\_testing\_main\.cc 201](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/cpu/ppc/testing/ppc_testing_main.cc#L201)

Как известно, конструктор _std::unique\_ptr_ по умолчанию создаёт объект и инициализирует его нулём\. Поэтому следующая за декларацией проверка бессмысленна — поток управления всегда попадёт в _then_\-ветку\.

Оказавшись там, мы встретим простынь из вложенных проверок и препроцессорных директив\. Читать такой код достаточно сложно\. Можно заметить, что инициализация умного указателя произойдёт лишь в том случае, если макрос _XE\_ARCH\_AMD64_ раскрывается в ненулевое значение\. На основе этого можно предложить следующий вариант для упрощения:

```cpp
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**

```cpp
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_ перехватывается по значению, а не по ссылке\. 

Перехватывать исключения лучше по ссылке, потому что это позволяет:

* не создавать копию объекта исключения;
* поймать всех публичных наследников этого класса исключения\. При перехвате по значению произойдёт [срезка типа](https://en.wikipedia.org/wiki/Object_slicing), из\-за которой потеряется информация от производных типов\.

Собственно, об этом и предупреждает анализатор:

[V746](https://pvs-studio.ru/ru/docs/warnings/v746/) Object slicing\. An exception should be caught by reference rather than by value\. [config\.cc 58](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/config.cc#L58)

**Фрагмент N15**

А теперь рассмотрим срабатывания, связанные с построением классов:

```cpp
class ImGuiDialog
{
 public:
  ~ImGuiDialog(); 
  ....
 protected:
  virtual void OnShow() {}
  virtual void OnClose() {}
  virtual void OnDraw(ImGuiIO& io) {}
};
```

Предупреждение PVS\-Studio:

[V599](https://pvs-studio.ru/ru/docs/warnings/v599/) The destructor was not declared as a virtual one, although the 'ImGuiDialog' class contains virtual functions\. [imgui\_dialog\.cc 46](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/ui/imgui_dialog.cc#L46)

Это срабатывание помогает избежать возможных проблем с использованием указателя на базовый класс\.

В классе _ImGuiDialog_ присутствуют виртуальные функции\. Это означает, что у него предполагаются наследники\. В таком случае деструктор также стоит сделать виртуальным\. В ином случае, при разрушении объекта класса\-наследника через указатель на базовый класс, [возникает](https://timsong-cpp.github.io/cppwp/n4950/expr.delete#3) неопределённое поведение\.

**Фрагмент N16**

Продолжая разговор о наследовании, отметим, что также важно помнить о правилах использования виртуальных функций в конструкторах и деструкторе класса\.

Предупреждение PVS\-Studio:

[V1053](https://pvs-studio.ru/ru/docs/warnings/v1053/) Calling the 'Reset' virtual function in the destructor may lead to unexpected result at runtime\. [assembler\.cc 18](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/cpu/backend/assembler.cc#L18)

```cpp
class Assembler
{
public:
  explicit Assembler(Backend* backend);
  virtual ~Assembler();
  virtual bool Initialize();
  virtual void Reset();
  ....
}

Assembler::~Assembler() { Reset(); }
```

В этом фрагменте кода представлен класс _Assembler_, который в своём деструкторе вызывает виртуальную функцию _Assembler::Reset_\.

```cpp
class X64Assembler : public Assembler
{
public:
  explicit X64Assembler(X64Backend* backend);
  ~X64Assembler() override;
  bool Initialize() override;
  void Reset() override;
  ....
}
```

Здесь представлен его наследник _X64Assembler_, переопределяющий виртуальную функцию _Reset_\. При удалении объекта класса _X64Assembler_ вызывается деструктор базового класса \(_Assembler\)_\. Внутри этого деструктора вызывается функция _Reset_ из базового класса, а не из наследника\. Возможно, автор ожидал, что будет вызываться переопределённая функция\.

Мой коллега подробно разбирал проблему такого паттерна в отдельной [статье](https://pvs-studio.ru/ru/blog/posts/cpp/1125/) и предложил следующее решение:

```cpp
class Assembler
{
private:
  void ResetImpl();

public:
  explicit Assembler(Backend* backend);
  virtual ~Assembler();
  virtual bool Initialize();
  virtual void Reset();
  ....
}

void Assembler::ResetImpl() { /* free only Assembler resources */ }
Assembler::~Assembler() { ResetImpl(); } 
void Assembler::Reset() { ResetImpl(); }

class X64Assembler : public Assembler
{
private:
  void ResetImpl();

public:
  explicit X64Assembler(X64Backend* backend);
  ~X64Assembler() override;
  bool Initialize() override;
  void Reset() override;
  ....
}

void X64Assembler::ResetImpl()
{
  /* free only X64Assembler resources */
}

X64Assembler::~X64Assembler() { ResetImpl(); }

void X64Assembler::Reset()
{
  ResetImpl();        // free X64Assembler resources
  Assembler::Reset(); // free resources of the base class
}
```

## Заключение

Мне хочется показать и остальные ошибки, найденные в проекте, но, боюсь, это будет интересно только для мейнтейнеров проекта\. Поэтому вместо этого я просто напомню, что у PVS\-Studio есть варианты бесплатного лицензирования как для [Open\-Source](https://pvs-studio.ru/ru/order/open-source-license/) проектов, так и для [студентов и преподавателей](https://pvs-studio.ru/ru/order/for-students/)\. Остальным же предлагаю получить [пробную версию](https://pvs-studio.ru/ru/pvs-studio/try-free/) анализатора и попробовать его в деле :\)