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

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

Предупреждение 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

Предупреждение 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:

Фрагмент N3

Следует быть внимательным при использовании union, ведь тут легко словить неопределённое поведение.

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

V614 Uninitialized variable 'desc.page_count' used. xex_module.cc 594

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

При работе с union в C++ чтение можно производить только из активного поля, т.е. из того, в которое производилась запись последний раз. Если происходит иное, то поведение такой операции не определено. Это отличает C++ от C, в котором можно записать в одно поле, а прочитать из другого.

Немногие знают про такое поведение, а те, кто знают, вполне могут забыть о нём. Спасают ситуацию компиляторы, которые в качестве нестандартного расширения поддерживают такое поведение. Однако полагаться на него не стоит, неопределённое поведение может проявиться в будущем при обновлении компилятора или его смене.

Как же можно исправить ситуацию в C++? Начиная с C++20, можно и нужно использовать std::bit_cast в таких моментах:

До C++20 можно воспользоваться memcpy:

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

Ну и как вариант, можно до 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

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

Предупреждение 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 ещё то удовольствие, и я бы вынесла всё это дело в переменную, чтобы улучшить читаемость кода:

Фрагмент N5

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

Можно заметить некоторые идентичные блоки для определения и проверки переменных 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

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

V591 Non-void function should return a value. chrono.h 110

Внутри функции проверяют поле domain_ на соответствие элементам enum:

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

Начиная с 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

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

Однако при создании указателя e используется extra. Если он был нулевым, то его разыменование ведёт к неопределённому поведению. К сожалению, проверка extra на валидность происходит слишком поздно.

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

Аналогичное предупреждение:

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

Предупреждение 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!

Фрагмент N9

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

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

V570 The 'extent.depth' variable is assigned to itself. texture_extent.cc 58

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

Фрагмент N10

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

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

Она включает в себя такой тип, как 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:

Фрагмент N11

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

В коде присутствуют два опасных преобразования указателя на double:

· сначала в указатель на uint64_t;

· затем в указатель на float.

Это довольно серьёзная ошибка, нарушающая правила strict aliasing. Их нарушение влечёт за собой неопределённое поведение.

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

V615 An odd explicit conversion from 'double *' type to 'float *' type. emulator.cc 595

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

Фрагмент N12

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

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

V612 An unconditional 'return' within a loop. single_layout_descriptor_set_pool.cc 110

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

Фрагмент N13

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

Предупреждение анализатора:

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

Фрагмент N14

Это фрагмент кода с блоком перехватывания исключений, но если присмотреться, то можно заметить неладное. Исключение в блоке catch перехватывается по значению, а не по ссылке.

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

· не создавать копию объекта исключения;

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

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

V746 Object slicing. An exception should be caught by reference rather than by value. config.cc 58

Фрагмент N15

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

Предупреждение 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

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

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

Мой коллега подробно разбирал проблему такого паттерна в отдельной статье и предложил следующее решение:

Мне хочется показать и остальные ошибки, найденные в проекте, но, боюсь, это будет интересно только для мейнтейнеров проекта. Поэтому вместо этого я просто напомню, что у PVS-Studio есть варианты бесплатного лицензирования как для Open-Source проектов, так и для студентов и преподавателей. Остальным же предлагаю получить пробную версию анализатора и попробовать его в деле :)