От винта! Смотрим движок War Thunder и говорим с его создателями

• serialize
• null
• shift
• delete
• munmap
• template_inst
• misprint
• overflow
• Промежуточный вывод
• Интервью
• В качестве заключения

Как говорил Юрий Гагарин: "В будущем мы будем летать, много летать". В определённой мере игровой движок Dagor Engine от Gaijin Entertainment позволяет это сделать. Давайте посмотрим, как он сделан, и поговорим с его создателями!

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

"— Уважаемый-многоуважаемый, не были ли бы Вы так любезны пояснить смысл Вашего тактического манёвра?"

"— Ну я впритирочку хотел, впритирочку...".

Это один из многих случаев — курьёзных и не слишком, но всегда запоминающихся, которые Всегда Ваш имел удовольствие пережить в полётах в старом-добром War Thunder. Давно это было, но новость о выходе движка игры в публичный доступ не смогла оставить Всегда Вашего равнодушным! В конце концов, как можно отказать себе в удовольствии покопаться в технологии, стоящей за такой игрой?

В этом небольшом путешествии нам составил компанию Антон Юдинцев, основатель Gaijin Entertainment. Он согласился дать пару комментариев о подозрительных местах в коде, которые мы нашли, и ответил на вопросы о некоторых моментах тестирования их продуктов в формате полноценного интервью. Оно находится во второй части материала и следует сразу за рассмотрением участков кода.

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

serialize

Давайте начнём с простого, но от того не менее примечательного момента.

void ExprField::serialize ( AstSerializer & ser ) {
  ....
  Module * module; ser << module;
  ....
}

Создаётся переменная module без её инициализации, после чего она передаётся в operator << сериализатора ser. Пока что проблем нет, переменная не читается. Но что же дальше?

AstSerializer & AstSerializer::operator << ( Module * & module ) {
  bool is_null = module == nullptr;
  ....
}

А дальше неинициализированная переменная сравнивается с nullptr, для чего производится операция чтения из этой переменной. Вроде бы ошибка небольшая, но UB по-прежнему страшное. Помни, программист, ночь темна и полнится ложными представлениями о неопределённом поведении!

Антон Юдинцев:

— Это новый код, и пока что не используется. Он пролез через нашу библиотеку, которую мы ещё не проверяем анализатором.

Сам же анализатор выдаёт ёмкое:

V614 Uninitialized pointer 'module' used. DagorEngine/prog/1stPartyLibs/daScript/src/builtin/module_builtin_ast_serialize.cpp 1303

null

А в примере ниже происходит гарантированное разыменовывание нулевого указателя.

void copy(const Node &n, int sz)
{
  ....
  for (int i = 0; i < 4; ++i)
    if (n.leaf_linear[i])
    {
      if (leaf_linear[i])
        leaf_linear[i] = new Leaf(*n.leaf_linear[i]);
      else
        *leaf_linear[i] = *n.leaf_linear[i];
    }
    else
    ....
   ....
}

Посудите сами: сначала идёт проверка на NULL в leaf_linear[i]. А что дальше? Если там NULL, то происходит его разыменование в операции присваивания. Что это ещё, если не UB!

Антон Юдинцев:

— Это правда ошибка. У нас есть несколько мест со старым кодом, который больше не используется. Мы подобные предупреждения сознательно фильтруем.

Анализатор сообщает:

V522 Dereferencing of the null pointer 'leaf_linear[i]' might take place. DagorEngine/prog/dagorInclude/generic/dag_hierGrid.h 71

shift

А в данной части мы собрали парочку подозрительных мест, так или иначе вовлекающих операции побитового сдвига.

Например, здесь происходит сдвиг на количество битов, превышающих размер переменной-операнда:

__forceinline bool is_char_in_set (int32_t ch, 
                                   const TDim<uint32_t,8> & bitset, 
                                   Context * ctx, LineInfoArg * at) 
{
  if ( ch<0 || ch>=256 ) ctx->throw_error_at(at,"invalid character %d", ch);
  return bitset[ch>>5] & (1u<<uint32_t(ch));
}

Обратите внимание на диапазон значений, которые переменная ch может принимать согласно условию в операторе if. Плюсы говорят на это однозначное UB.

Антон Юдинцев:

— char_in_set – очевидная ошибка (пропущен &31). Этот код тоже попал из нашей библиотеки.

Анализатор не дремлет:

V610 Undefined behavior. Check the shift operator '<<'. The right operand ('uint32_t(ch)' = [0..255]) is greater than or equal to the length in bits of the promoted left operand. DagorEngine/prog/1stPartyLibs/daScript/include/daScript/simulate/aot.h 820

Или, например, следующий вариант, в котором возможная проблема кроется в побитовом сдвиге вправо.

inline int does_line_intersect_box_side_two_points(....)
{
  int ret = -1;
  for (int j = 0; j < 3; ++j)
  {
    BBox2 box2;
    const int j1_mask = (j + 1 - 3) >> 4; //>3 == 0 <3 == -1
    const int j2_mask = (j + 2 - 3) >> 4;
    ....
  }
}

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

Антон Юдинцев:

— Кажется, это "правильный" код. В смысле, он, конечно, unspecified, но работает одинаково.

Анализатор держит в курсе:

V610 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('(j + 1 - 3)' = [-2..0]). DagorEngine/prog/dagorInclude/math/dag_mathUtils.h 612

V610 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('(j + 2 - 3)' = [-1..1]). /DagorEngine/prog/dagorInclude/math/dag_mathUtils.h 613

delete

Приведём ниже следующий небольшой фрагмент.

int SH3LightingManager::loadLtDataBinary(IGenLoad &crd, unsigned id)
{
  SH3LightingData *ltData = SH3LightingData::loadBinary(crd);
  if (!ltData)
  {
    delete ltData;
    return -1;
  }
  return addLtData(ltData, id);
}

Предположительно, на куче создаётся некоторый объект, указатель на который возвращается клиенту. Если клиенту вернулся NULL, то он удаляется.

Стоп, что?

Давайте удостоверимся, что происходит в вызываемой функции.

SH3LightingData *SH3LightingData::loadBinary(IGenLoad &crd)
{
  ....
  SH3LightingData *data =
      new (memalloc(sz, midmem), _NEW_INPLACE) SH3LightingData;
  ....
  return data;
}

Действительно, вызывается operator new, а в конце возвращается указатель на вновь созданный объект.

Строго говоря, язык не запрещает толкать NULL в оператор delete, но этим наш пример и интересен. Это один из тех моментов, когда статический анализатор может помочь в поиске и логических ошибок. В конце концов, точно ли мы хотим удалять NULL?

Но хорошая новость в том, что для команды движка какой-либо проблемы в этом нет.

Антон Юдинцев:

— Этот код не используется. Но не удалили, конечно, зря.

Анализатор же сообщает ёмкое:

V575 The null pointer is passed into 'operator delete'. Inspect the argument. DagorEngine/prog/engine/scene/sh3LtMgr.cpp 435

munmap

А сейчас приводим самый короткий пример за сегодня.

void builtin_map_file(const FILE* f, 
                      const TBlock<void, TTemporary<TArray<uint8_t>>>& blk, 
                      Context* context, LineInfoArg * at) {
  ....
  munmap(data, 0);
}

В man страницах видим следующее описание ошибки – "EINVAL The len argument is 0".

Антон Юдинцев:

— Ошибка, конечно, но код работает. В проект попала из 1st party библиотеки, в которой анализ пока что не ввели.

Анализатор говорит ёмкое:

V575 The 'munmap' function processes '0' elements. Inspect the second argument. DagorEngine/prog/1stPartyLibs/daScript/src/builtin/module_builtin_fio.cpp 214

P.S. Дорогой читатель, как насчёт дискуссии? Всегда Ваш для себя сформировал Дилемму Быстрого Квадратного Корня (названную в честь знаменитой функции из движка Quake 3). Вот она – если код приводит к UB или содержит другую ошибку, но гарантированно работает на наших платформах с нашими компиляторами, стоит ли её править? А вы как считаете? Жутко интересно будет узнать в комментариях!

template_inst

В примере ниже анализатор ругается на безобидную инициализацию переменной.

void DoInsertValues(const_iterator position, 
                    size_type n, 
                    const value_type& value) {
  const value_type temp  = value;
}

Интересно, а какой у неё тип? Давайте посмотрим.

template <typename T, ....>
class Vector
{
  ....
  typedef T value_type;
  ....
}

Хорошо, то есть тип задаётся при инстанцировании шаблона. Анализатор указывает, что в нашем случае T на самом деле является типом MountVromfsRec (см. предупреждение в конце этой главы).

Давайте посмотрим, как определён тип MountVromfsRec.

struct MountVromfsRec
{
  VirtualRomFsPack *vd;
  SimpleString fn;
  SimpleString mountPath;
  
  MountVromfsRec() : vd(NULL) {}
  ~MountVromfsRec()
  {
    remove_vromfs(vd);
    close_vromfs_pack(vd, inimem);
    vd = NULL;
  }
};

Единственным местом в кодовой базе, где он используется напрямую, является следующее место.

static Tab<MountVromfsRec> mnt_vromfs(inimem);

И это место ведёт нас обратно к пользовательскому типу dag::Vector.

template <typename T>
using Tab = dag::Vector<T, dag::MemPtrAllocator, false, uint32_t>;

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

Конкретно в этом случае тяжело сказать, является ли это ошибкой. Но мы бы хотели лишний раз обратить на него внимание – подобная ошибка (при наличии) может легко съесть много времени и сил при отладке. А мы же хотим получать удовольствие от программирования!

Антон Юдинцев:

— Ошибки, действительно, нет.

От анализатора получаем:

V1002 Instantiation of Vector < MountVromfsRec, MemPtrAllocator, bool, uint32_t >: The 'MountVromfsRec' class, containing pointers, constructor and destructor, is copied by the automatically generated copy constructor. DagorEngine/prog/1stPartyLibs/dag/dag_vector.h 567

misprint

Ниже будет достаточно много кода. Как насчёт небольшой игры: до того, как дочитаете до конца примера, попробуйте определить, что нас насторожило в коде.

GrassLod &lod = layer->lods[lodIdx];
unsigned int numInstancesInCell = (unsigned int)(
  baseNumInstances * lod.density / (GRID_SIZE * GRID_SIZE) + 0.5f
);
const ShaderMesh::RElem &elem = lod.mesh->getAllElems()[0];
lod.numInstancesInCell = min(
  numInstancesInCell, 
  (unsigned int)MAX_VERTICES_IN_CELL / elem.numv
);

if (numInstancesInCell == 0)
  return;

lod.singleVb = (lod.numInstancesInCell >= INSTANCES_IN_SINGLE_VB);
....

Переменной lod.numInstancesInCell присваивается минимальное значение от двух переменных. Одна из них, numInstancesInCell, может равняться нулю, о чём говорит проверка, следующая сразу за присваиванием. И именно она нас и насторожила. Если проверка на ноль происходит после присваивания, то не лучше бы было проверять то, куда присваивание происходит?

Теоретически elem.numv может быть немногим больше MAX_VERTICES_IN_CELL. Давайте проверим.

struct RElem
{
  ....
  int numv; // number of vertices
  ....
}

Действительно, при делении одного на другое получится 0, который не учитывается при проверке if (numInstancesInCell == 0).

Антон Юдинцев:

— Теоретически - да. На практике такого в этом месте не случится – там много порядков разницы.

Анализатор же выдаёт следующее:

V1051 Consider checking for misprints. It's possible that the 'lod.numInstancesInCell' should be checked here. DagorEngine/prog/gameLibs/render/editorGrass.cpp:135:1, DagorEngine/prog/gameLibs/render/editorGrass.cpp 133

overflow

А вот и такой небольшой пример с возможным выстрелом куда не надо.

int cache_idx = bd->cache->files.getNameId(fn);
....
fs->data[idx].init((void *)intptr_t(cache_idx + 1), fs->data[idx].size());

V1028 Possible overflow. Consider casting operands of the 'cache_idx + 1' operator to the 'intptr_t' type, not the result. DagorEngine/prog/engine/ioSys/vromfsBacked.cpp 44

Либо подобная его вариация.

const void *df_mmap(file_ptr_t fp, int *flen, int length, int offset)
{
  ....
  int pa_diff = (base + offset) - pa_offs;
  ....
  void *ret = mmap(NULL, 
                  (size_t)(length + pa_diff), 
                  PROT_READ, MAP_SHARED, fd, (off_t)pa_offs);
  ....
}

V1028 Possible overflow. Consider casting operands of the 'length + pa_diff' operator to the 'size_t' type, not the result. DagorEngine/prog/engine/osApiWrappers/mmap.cpp 77

Если результат операции приведён к большему типу, то почему к нему не были приведены операнды?

Антон Юдинцев:

— Этот код работает. На практике там все операнды и сумма влезают в int, потому что на 32бит надо работать всё равно. Ну лучше бы поправить на будущее.

Автор же может только добавить такую вот статью от бывшего сотрудника Google и автора некоторых частей Java. В ней он описывает, как простой mergesort в течение многих лет содержал потенциальный баг с переполнением, который долгое время физически не мог выстрелить, но всё-таки дождался своего. Читатель, будь бдителен!

Промежуточный вывод

Помните, как в самом начале мы предлагали сверить наши выводы об ошибках? Давайте попробуем! Если вы внимательно читали комментарии Антона, то обязательно заметили, что ошибки, найденные нами в коде, либо относятся к тем частям проекта, которые на данный момент не используются (что не мешает им служить отличным материалом для обучения и разбора возможностей анализатора), либо относятся к тем частям проекта, в которых статический анализ пока что не используется. Для нас это является ярким подтверждением того, что мы делаем нашу работу не зря, и польза от включения такой технологии видна даже невооружённым глазом.

Но как в Gaijin Entertainment обеспечивается сохранение такого высокого качества кода, как в их движке? Давайте перейдём к полноценному интервью с Антоном Юдинцевым и узнаем!

Интервью

— В двух словах, как у вас устроено тестирование проектов?

— PVS-Studio мы используем несколько лет вместе с другими статическими анализаторами. Используем связку Gerrit + Jenkins. В Gerrit делаем код ревью, в Jenkins запускаются тесты. Используем trunk-based подход, коммиты пускаются дальше в мастер ветку только если пройдены код ревью и тесты.

— Получается, через Gerrit делаются пул реквесты?

— Что-то вроде этого. Внутри там неонка со своими репозиториями, но для разработчика это выглядит как push в origin/master. Коммит не окажется у других людей, пока кто-то в веб-интерфейсе не нажмёт Merge. А его нажать нельзя, пока нет статуса Verified и Reviewed. Reviewed – это ставит человек после ревью кода. Verified ставит Jenkins после того, как удачно пройдут тесты.

— Trunk-based — это сознательный выбор, или так сложилось исторически?

— Сознательный выбор. На ветки для отдельных фич переходить пока не планируем. Наш пайплайн в целом благоволит trunk-based development. При этом некоторые фичи и сейчас всё-таки разрабатываются в бранчах.

— А какие тесты гоняете?

— Jenkins гоняет автотесты. Автотесты включают компиляцию под все платформы и проверку статическими анализаторами. Потом выдача анализаторов сравнивается с БД предупреждений, и, если их стало больше, – то тесты не проходят. К предупреждениям у нас нулевая терпимость, поэтому новых в БД не появляется.

— Смотрите каждый коммит?

— Да.

— Что будет, если два разработчика одновременно закоммитят?

— У нас есть ферма билднод. Коммиты попадают в очередь и последовательно анализируются. Этого хватает, чтобы работа не стояла, но если запушит сразу много людей, то придётся немного подождать. Но если ферма не справляется, то поднимаются ноды в облаке.

— Балансируете как-то нагрузку по нодам?

— Там же всё просто. Jenkins раздаёт всем желающим воркерам задачи в очереди. Предпочтение отдаётся тем, кто в прошлый раз делал таску. По сути, балансировкой занимается сам Jenkins. Но фактически балансировка не особо нужна, все ноды равнозначны. У нас стоит задача сделать как можно быстрее когда нужно, а не нагрузить ноды равномерно. Ночью ноды простаивают, днём пыхтят. Скриптом смотрим на размер очереди: если она слишком большая, то поднимаем ноды в облаке.

— А как конкретно анализ происходит?

— Каждый коммит тригеррит сборку изменённых компонентов на всех поддерживаемых платформах, линковку проекта так же на всех платформах и статанализ. Если что-то из этого не проходит, то и коммит, соответственно, не проходит тоже. Каждая проверка всегда ресетит на воркспейс билдноды текущий мастер, а потом происходит cherry-pick проверяемого коммита.

— Вы вроде бы много где поддерживаетесь. Представляю как обидно, если проект падает только на одной платформе.

— Слава богу, это робот говорит. Робота нельзя ненавидеть.

— А делаете проверки проектов целиком?

— Конечно. Раз в N часов делаются медленные проверки, которые не сделаешь на каждый коммит просто физически. Лучше бы всё было на каждый коммит, но таких ферм не бывает.

— Давайте затронем прод. Выкатываетесь как-то автоматизировано или по кнопке?

— Конечно кнопка. На прод — только руками и только из ветки. В мастере бинарники каждое утро собираются. Но на прод только из ветки, не из мастера. Из мастера делаем fast-forward в ветку, а потом уже только cherry-pick из мастера.

— То есть условный stable?

— Ну он именной, конечно, а не stable. У каждой прод версии есть бранч. Плюс у нас монорепо, а игр несколько. У всех свои прод бранчи.

— А если не секрет, монорепа сознательно делалась?

— Да, выбор сознательный, со своими плюсами. На сабмодули или аналог мы, видимо, скоро перейдём —– слишком много игр на одном движке делается. Движковой команде сложно минимизировать влияние на все проекты, это замедляет разработку.

— К слову, публикация Dagor Engine — это часть плана по выходу в сабмодули?

— Это независимые, но коррелированные вещи.

— Вы собираетесь делать полноценный Open Source проект с мейнтейнерами и пулреквестами? Видел, вы его апдейтите, и issues открытые тоже есть.

— И мы эти issues даже решаем! А так да, собираемся. Насколько сил хватит.

— Под конец расскажите, если я сильно захочу, то смогу собрать на нём свой Тарвандер?

— Очень сильно надо захотеть. Мы же исходники игры не выкладываем. Но проекты на нашем движке есть, например Nau. Ну и до этого давали доступ разным командам тоже, уже под FOSS.

В качестве заключения

Отличное приключение! Было приятно вновь вспомнить о хорошей игре и получить удовольствие от общения с ней, пусть в этот раз и в ином виде. Неожиданно приятно было найти в проекте с таким серьёзным подходом к тестированию несколько ошибок и ещё приятнее — помочь в их исправлении.

Надеемся, что и вы, читатель, получили удовольствие от нашего путешествия!

И, конечно же, хотим выразить благодарность Антону Юдинцеву и другим членам команды Gaijin Entertainment за участие в нашей статье. Как нам кажется, вы помогли сделать её намного интереснее!

Благодарим, что дошли до конца! El Psy Kongroo.