Игры — одни из самых массовых продуктов среди программного обеспечения. Это огромная индустрия, в которой появился новый игровой движок — Amazon Lumberyard. Проект ещё находится в статусе беты и у него есть время, чтобы исправить ошибки, повысить качество кода. Разработчикам движка предстоит проделать много работы, чтобы в ближайшее время не разочаровать миллионы игроманов и разработчиков игр.

Введение

Amazon Lumberyard ^[1] — бесплатный кроссплатформенный игровой движок класса AAA, разработанный компанией Amazon и основанный на архитектуре движка CryEngine ^[2], который был лицензирован у компании Crytek в 2015 году. Кстати, анализ CryEngine уже дважды проводился мною в августе 2016 ^[3] и апреле 2017 ^[4]. При этом я вынужден отметить, что код по прошествии года стал только хуже. И вот на днях я решил посмотреть, что сделал Amazon на основе этого игрового движка. Над окружением они очень хорошо поработали. Документация для разработчиков и софт для развёртывания рабочего окружения сделаны очень круто и на высоком уровне. Но с кодом снова беда! Я надеюсь, что у Amazon намного больше ресурсов для работы с проектом, и они всё-таки уделят внимание качеству кода. Этим обзором я надеюсь обратить внимание разработчиков на качество кода и подтолкнуть к новому подходу в разработке этого игрового движка. Текущее состояние кода оказалось в таком плачевном состоянии, что я несколько раз менял название статьи и перерисовывал титульную картинку по мере просмотра отчёта с результатами анализа. Первая версия картинки была менее эмоциональной:

Анализировались исходники Amazon Lumberyard последней доступной версии 1.14.0.1. Исходный код взят из репозитория на Github ^[5]. Одной из первых игр на движке Lumberyard должна стать Star Citizen ^[6]. Потенциальных игроков, которые её ждут, тоже приглашаю взглянуть, что на данный момент находится «под капотом» игры.

Интеграция с PVS-Studio

В качестве статического анализатора кода использовался PVS-Studio ^[7]. Он доступен для Windows, Linux и macOS. Т.е. для анализа кроссплатформенного проекта есть даже из чего выбрать для более комфортной работы. Кроме C и C++ поддерживается анализ проектов на языке C#. В планах Java ^[8]. На перечисленных языках написано подавляющее большинство кода в мире (не без ошибок, конечно же), так что пробуйте анализатор PVS-Studio на своём проекте, узнаете много интересного ;-).

В качестве сборочной системы Lumberyard используется WAF, которая был и в CryEngine. Специального способа для интеграции с этой сборочной системой у анализатора нет. Я решил поработать с проектом на Windows и выбрал такой способ запуска анализа: система мониторинга компиляции ^[9]. Проектный файл для Visual Studio является автогенерируемым. Им можно пользоваться для сборки проекта и просмотра отчёта анализатора.

Список команд для анализа выглядит примерно так:

cd /path/to/lumberyard/dev
lmbr_waf.bat ...
CLMonitor.exe monitor
MSBuild.exe ... LumberyardSDK_vs15.sln ...
CLMonitor.exe analyze --log /path/to/report.plog

Отчёт, как я уже говорил, можно просматривать в Visual Studio.

Про Игоря и Qualcomm

Amazon Lumberyard позиционирует себя как кроссплатформенный игровой движок. Продвигать проект в массы с такой фичей легко, а вот поддерживать очень трудно. Одно из предупреждений PVS-Studio было выдано на фрагмент кода, где программист Игорь боролся с компилятором Qualcomm. Возможно, он решил свою задачу, но оставил крайне подозрительный код. Я решил оформить его картинкой.

V523 ^[10] The 'then' statement is equivalent to the 'else' statement. toglsloperand.c 700

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

В целом по проекту это не единственное место, где условия нужно упростить для наглядности, либо исправить настоящую ошибку. Вот список таких мест:

• V523 The 'then' statement is equivalent to the 'else' statement. livingentity.cpp 1385
• V523 The 'then' statement is equivalent to the 'else' statement. tometalinstruction.c 4201
• V523 The 'then' statement is equivalent to the 'else' statement. scripttable.cpp 905
• V523 The 'then' statement is equivalent to the 'else' statement. budgetingsystem.cpp 701
• V523 The 'then' statement is equivalent to the 'else' statement. editorframeworkapplication.cpp 562
• V523 The 'then' statement is equivalent to the 'else' statement. particleitem.cpp 130
• V523 The 'then' statement is equivalent to the 'else' statement. trackviewnodes.cpp 1223
• V523 The 'then' statement is equivalent to the 'else' statement. propertyoarchive.cpp 447

Python++

Анализатор нашёл такой забавный код:

V709 ^[11] CWE-682 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. toglslinstruction.c 564

void CallBinaryOp(....)
{
  ....
  uint32_t src1SwizCount = GetNumSwizzleElements(....);
  uint32_t src0SwizCount = GetNumSwizzleElements(....);
  uint32_t dstSwizCount = GetNumSwizzleElements(....);

  ....
  if (src1SwizCount == src0SwizCount == dstSwizCount) // <=
  {
    ....
  }
  ....
}

В C++ такой код, к сожалению, компилируется успешно, но выполняется он совсем не так, как может показаться. Выражения в C++ вычисляются в порядке приоритета, выполняя неявные касты, если это необходимо.

Такая проверка была бы правильной, например, в языке Python. Но в этой ситуации разработчик «выстрелил себе в ногу».

Ещё 3 контрольных выстрела:

• V709 CWE-682 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. toglslinstruction.c 654
• V709 CWE-682 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. toglslinstruction.c 469
• V709 CWE-682 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. tometalinstruction.c 539

Первая и одна из лучших диагностик

Речь пойдёт о предупреждении V501 ^[12] — первой диагностике общего назначения в PVS-Studio. Ошибок, найденных только одной этой диагностикой, может быть достаточно для написания статьи. И проект Amazon Lumberyard отлично это демонстрирует.

К сожалению, долго просматривать однотипные ошибки скучно, поэтому в этом разделе прокомментирую только пару фрагментов кода, а остальное предупреждения приведу списком.

V501 ^[12] There are identical sub-expressions to the left and to the right of the '||' operator: hotX < 0 || hotX < 0 editorutils.cpp 166

QCursor CMFCUtils::LoadCursor(....)
{
  ....
  if (!pm.isNull() && (hotX < 0 || hotX < 0))
  {
    QFile f(path);
    f.open(QFile::ReadOnly);
    QDataStream stream(&f);
    stream.setByteOrder(QDataStream::LittleEndian);
    f.read(10);
    quint16 x;
    stream >> x;
    hotX = x;
    stream >> x;
    hotY = x;
  }
  ....
}

В условии не хватает переменной hotY. Классическая опечатка.

V501 ^[12] There are identical sub-expressions 'sp.m_pTexture == m_pTexture' to the left and to the right of the '&&' operator. shadercomponents.h 487

V501 ^[12] There are identical sub-expressions 'sp.m_eCGTextureType == m_eCGTextureType' to the left and to the right of the '&&' operator. shadercomponents.h 487

bool operator != (const SCGTexture& sp) const
{
  if (sp.m_RegisterOffset == m_RegisterOffset &&
      sp.m_Name == m_Name &&
      sp.m_pTexture == m_pTexture &&              // <= 1
      sp.m_RegisterCount == m_RegisterCount &&
      sp.m_eCGTextureType == m_eCGTextureType &&  // <= 2
      sp.m_BindingSlot == m_BindingSlot &&
      sp.m_Flags == m_Flags &&
      sp.m_pAnimInfo == m_pAnimInfo &&
      sp.m_pTexture == m_pTexture &&              // <= 1
      sp.m_eCGTextureType == m_eCGTextureType &&  // <= 2
      sp.m_bSRGBLookup == m_bSRGBLookup &&
      sp.m_bGlobal == m_bGlobal)
  {
      return false;
  }
  return true;
}

В этом фрагменте найдено сразу две копипасты. Для наглядности подрисовал стрелочки.

V501 ^[12] There are identical sub-expressions to the left and to the right of the '==' operator: pSrc.GetLen() == pSrc.GetLen() fbxpropertytypes.h 978

inline bool FbxTypeCopy(FbxBlob& pDst, const FbxString& pSrc)
{
    bool lCastable = pSrc.GetLen() == pSrc.GetLen();
    FBX_ASSERT( lCastable );
    if( lCastable )
        pDst.Assign(pSrc.Buffer(), (int)pSrc.GetLen());
    return lCastable;
}

Тут я хочу передать привет разработчикам из AUTODESK ^[13]. Эта ошибка из их библиотеки FBX SDK ^[14]. Перепутали переменные pSrc и pDst. Я думаю, кроме Lumberyard полно и других пользователей, чьи проекты зависят от кода с этой ошибкой.

V501 ^[12] There are identical sub-expressions to the left and to the right of the '&&' operator: pTS->pRT_ALD_1 && pTS->pRT_ALD_1 d3d_svo.cpp 857

void CSvoRenderer::ConeTracePass(SSvoTargetsSet* pTS)
{
  ....
  if (pTS->pRT_ALD_1 && pTS->pRT_ALD_1)
  {
    static int nPrevWidth = 0;
    if (....)
    {
      ....
    }
    else
    {
      pTS->pRT_ALD_1->Apply(10, m_nTexStateLinear);
      pTS->pRT_RGB_1->Apply(11, m_nTexStateLinear);
    }
  }
  ....
}

Вернёмся к коду Lumberyard. В условии проверяется один и тот же указатель pTS->pRT_ALD_1. Один из них должен был быть pTS->pRT_RGB_1. Возможно, даже после объяснения не сразу можно увидеть разницу, а она есть: разница в коротеньких подстроках ALD и RGB. Если вам скажут, что ручного Code Review достаточно, то покажите этот пример.

А если этого примера недостаточно, то есть ещё 5 похожих:

О позиции камеры в играх

Есть вторая по крутости диагностика в PVS-Studio — V502 ^[15]. Эта диагностика старше некоторых новых языков программирования, в которых уже нельзя допустить такую ошибку. А для С++ эта ошибка будет актуальная, пожалуй, всегда.

Начнём с маленького простого примера.

V502 ^[15] Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. zipencryptor.cpp 217

bool ZipEncryptor::ParseKey(....)
{
  ....
  size_t pos = i * 2 + (v1 == 0xff) ? 1 : 2;
  RCLogError("....", pos);
  return false;
  ....
}

Операция сложения имеет более высокий приоритет, чем тернарный оператор. Следовательно, у этого выражения совсем другая логика вычисления, чем предполагал автор.

Исправить ошибку можно таким образом:

size_t pos = i * 2 + (v1 == 0xff ? 1 : 2);

V502 ^[15] Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. 3dengine.cpp 1898

float C3DEngine::GetDistanceToSectorWithWater()
{
  ....
  return (bCameraInTerrainBounds && (m_pTerrain &&
          m_pTerrain->GetDistanceToSectorWithWater() > 0.1f)) ?
          m_pTerrain->GetDistanceToSectorWithWater() :
          max(camPostion.z - OceanToggle::IsActive() ?
          OceanRequest::GetOceanLevel() : GetWaterLevel(), 0.1f);
}

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

Ошибка присутствует в этой подстроке:

camPostion.z - OceanToggle::IsActive() ? .... : ....

Если камера в Вашей игре вдруг начала вести себя неадекватно, то знайте, разработчики сэкономили на статическом анализе кода :D.

Другие примеры с похожими предупреждениями:

• V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. scriptbind_ai.cpp 5203
• V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. qcolumnwidget.cpp 136
• V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '&&' operator. shapetool.h 98

Наследие CryEngine

Amazon Lumberyard основан на коде CryEngine ^[2]. Причём не на самой лучшей его версии. Такой вывод я сделал, посмотрев отчёт анализатора. Некоторые найденные ошибки уже исправлены в последней версии CryEngine по двум моим обзорам кода, но до сих пор присутствуют в коде Lumberyard. Также за последний год анализатор был значительно улучшен и удалось найти дополнительные ошибки, которые теперь присутствуют в двух игровых движках. Но с Lumberyard всё же ситуация похуже. В наследство Amazon по сути достался весь технический долг CryEngine. Ну а свой собственный технический долг, само собой, в каждой компании появляется своими силами :).

В этом разделе я приведу всего парочку ошибок, которые были исправлены в последней версии CryEngine, и теперь остались только проблемой проекта Lumberyard.

V519 ^[16] The 'BlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1283, 1284. ccrydxgldevicecontext.cpp 1284

Примерно такие эмоции будут испытывать разработчики Lumberyard, когда узнают, что эта ошибка осталась только у них.

Кстати таких ещё две:

• V519 The 'm_auBlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 919, 920. ccrydxgldevicecontext.cpp 920
• V519 The 'm_auBlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 926, 927. ccrydxgldevicecontext.cpp 927

Есть такая ошибка:

V546 ^[17] Member of a class is initialized by itself: 'eConfigMax(eConfigMax.VeryHigh)'. particleparams.h 1837

ParticleParams() :
  ....
  fSphericalApproximation(1.f),
  fVolumeThickness(1.0f),
  fSoundFXParam(1.f),
  eConfigMax(eConfigMax.VeryHigh), // <=
  fFadeAtViewCosAngle(0.f)
{}

В CryEngine этот класс вообще переписали, а тут ошибка с инициализацией осталась.

V521 ^[18] Such expressions using the ',' operator are dangerous. Make sure the expression '!sWords[iWord].empty(), iWord ++' is correct. tacticalpointsystem.cpp 3376

bool CTacticalPointSystem::Parse(....) const
{
  string sInput(sSpec);
  const int MAXWORDS = 8;
  string sWords[MAXWORDS];

  int iC = 0, iWord = 0;
  for (; iWord < MAXWORDS; !sWords[iWord].empty(), iWord++)
  {
      sWords[iWord] = sInput.Tokenize("_", iC);
  }
  ....
}

Подозрительный цикл, который в CryEngine тоже уже переписали.

Ошибки живут дольше, чем вы думаете

У пользователей, которые начинают использовать PVS-Studio впервые, возникает примерно одинаковая ситуация: находят ошибку, выясняют, что её добавили несколько месяцев назад и с радостью осознают, что чудом избегали проявления проблемы у своих пользователей. Многие наши клиенты пришли к регулярному использованию PVS-Studio именно после таких историй.

Иногда для запуска процессов контроля качества кода компания должна побывать в таких ситуациях несколько раз. Вот пример про CryEngine и Lumberyard:

V557 ^[19] CWE-119 Array overrun is possible. The 'id' index is pointing beyond array bound. gameobjectsystem.cpp 113

uint32 CGameObjectSystem::GetExtensionSerializationPriority(....)
{
  if (id > m_extensionInfo.size())
  {
    return 0xffffffff; // minimum possible priority
  }
  else
  {
    return m_extensionInfo[id].serializationPriority;
  }
}

Как известно, Amazon Lumberyard основан на не самой новой версии CryEngine. Тем не менее, с помощью анализатора PVS-Studio удалось найти ошибку, которая присутствует сейчас в двух игровых движках. Надо было с помощью оператора '>=' индекс проверять…

Ошибка с индексацией серьёзная. Более того, всего таких мест шесть! Вот ещё пример:

V557 ^[19] CWE-119 Array overrun is possible. The 'index' index is pointing beyond array bound. vehicleseatgroup.cpp 73

CVehicleSeat* CVehicleSeatGroup::GetSeatByIndex(unsigned index)
{
  if (index >= 0 && index <= m_seats.size())
  {
    return m_seats[index];
  }

  return NULL;
}

Кто-то наделал однотипных ошибок, и они не были исправлены только потому, что когда-то не попали в обзоры ошибок CryEngine.

Оставшиеся предупреждения:

• V557 CWE-119 Array overrun is possible. The 'id' index is pointing beyond array bound. gameobjectsystem.cpp 195
• V557 CWE-119 Array overrun is possible. The 'id' index is pointing beyond array bound. gameobjectsystem.cpp 290
• V557 CWE-119 Array overrun is possible. The 'stateId' index is pointing beyond array bound. vehicleanimation.cpp 311
• V557 CWE-119 Array overrun is possible. The 'stateId' index is pointing beyond array bound. vehicleanimation.cpp 354

Долгое существование ошибок в коде можно объяснить только соответствующим уровнем тестирования проекта. Есть мнение, что статический анализ находит ошибки только в неиспользуемом коде. Так вот, это не так. Разработчики забывают, что большинство пользователей молча страдает от неочевидных багов в программах, а проявление этих самых редких багов часто пагубно сказывается на работе всей компании, репутации и её продажах, если таковые имеются.

Разные оттенки Copy-Paste программирования

Дойдя до этого раздела статьи, вы наверняка заметили, что программирование методом Copy-Paste — причина многих проблем. В PVS-Studio поиск таких ошибок реализован в разных диагностиках. В этом разделе будут приведены примеры копипасты, находимые с помощью V561 ^[20].

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

V561 ^[20] CWE-563 It's probably better to assign value to 'pLibrary' variable than to declare it anew. Previous declaration: entityobject.cpp, line 4703. entityobject.cpp 4706

void CEntityObject::OnMenuConvertToPrefab()
{
  ....
  IDataBaseLibrary* pLibrary = GetIEditor()->Get....;
  if (pLibrary == NULL)
  {
    IDataBaseLibrary* pLibrary = GetIEditor()->Get....;
  }

  if (pLibrary == NULL)
  {
    QString sError = tr(....);
    CryMessageBox(....);
    return;
  }
  ....
}

Указатель 'pLibrary' не перезаписывается, как ожидалось. Инициализация этого указателя была полностью скопирована под условие вместе с объявлением типа.

Приведу списком все похожие места:

• V561 CWE-563 It's probably better to assign value to 'eType' variable than to declare it anew. Previous declaration: toglsloperand.c, line 838. toglsloperand.c 1224
• V561 CWE-563 It's probably better to assign value to 'eType' variable than to declare it anew. Previous declaration: toglsloperand.c, line 838. toglsloperand.c 1305
• V561 CWE-563 It's probably better to assign value to 'rSkelPose' variable than to declare it anew. Previous declaration: attachmentmanager.cpp, line 409. attachmentmanager.cpp 458
• V561 CWE-563 It's probably better to assign value to 'nThreadID' variable than to declare it anew. Previous declaration: d3dmeshbaker.cpp, line 797. d3dmeshbaker.cpp 867
• V561 CWE-563 It's probably better to assign value to 'directoryNameList' variable than to declare it anew. Previous declaration: assetimportermanager.cpp, line 720. assetimportermanager.cpp 728
• V561 CWE-563 It's probably better to assign value to 'pNode' variable than to declare it anew. Previous declaration: breakpointsctrl.cpp, line 340. breakpointsctrl.cpp 349
• V561 CWE-563 It's probably better to assign value to 'pLibrary' variable than to declare it anew. Previous declaration: prefabobject.cpp, line 1443. prefabobject.cpp 1446
• V561 CWE-563 It's probably better to assign value to 'pLibrary' variable than to declare it anew. Previous declaration: prefabobject.cpp, line 1470. prefabobject.cpp 1473
• V561 CWE-563 It's probably better to assign value to 'cmdLine' variable than to declare it anew. Previous declaration: fileutil.cpp, line 110. fileutil.cpp 130
• V561 CWE-563 It's probably better to assign value to 'sfunctionArgs' variable than to declare it anew. Previous declaration: attributeitemlogiccallbacks.cpp, line 291. attributeitemlogiccallbacks.cpp 303
• V561 CWE-563 It's probably better to assign value to 'curveName' variable than to declare it anew. Previous declaration: qgradientselectorwidget.cpp, line 475. qgradientselectorwidget.cpp 488

Большой список… некоторые из перечисленных мест являются даже полными копиями описанного примера.

Инициализация собственным значением

В коде игрового движка найдено очень много мест, где переменная присваивается сама себе. Где-то этот код остался для отладки, где-то код просто красиво оформлен (тоже часто является источником ошибок), поэтому приведу один самый подозрительный для меня фрагмент кода.

V570 ^[21] The 'behaviorParams.ignoreOnVehicleDestroyed' variable is assigned to itself. vehiclecomponent.cpp 168

bool CVehicleComponent::Init(....)
{
  ....
  if (!damageBehaviorTable.getAttr(....)
  {
    behaviorParams.ignoreOnVehicleDestroyed = false;
  }
  else
  {
    behaviorParams.ignoreOnVehicleDestroyed =      // <=
      behaviorParams.ignoreOnVehicleDestroyed;     // <=
  }
  ....
}

В текущем виде ветвь else вообще не нужна. Но, возможно, этот фрагмент кода содержит ошибку: хотели присвоить переменной противоположное значение:

bValue = !bValue

Но с результатами этой диагностики разработчиками лучше ознакомиться самим.

Обработка проблемных ситуаций

В этом разделе будет приведено много примеров, когда при обработке ошибок что-то пошло не так.

Пример 1.

V606 ^[22] Ownerless token 'nullptr'. dx12rootsignature.cpp 599

RootSignature* RootSignatureCache::AcquireRootSignature(....)
{
  ....
  RootSignature* result = new RootSignature(m_pDevice);
  if (!result->Init(params))
  {
    DX12_ERROR("Could not create root signature!");
    nullptr;
  }
  
  m_RootSignatureMap[hash] = result;
    return result;
  }
}

Забыли написать return nullptr;. Теперь невалидное значение переменной result будет использовано в других местах кода.

Точь-в-точь такой же код скопировали ещё в одно место:

• V606 Ownerless token 'nullptr'. dx12rootsignature.cpp 621

Пример 2.

V606 ^[22] Ownerless token 'false'. fillspacetool.cpp 191

bool FillSpaceTool::FillHoleBasedOnSelectedElements()
{
  ....

  if (validEdgeList.size() == 2)
  {
    ....
  }

  if (validEdgeList.empty())
  {
      ....
      for (int i = 0, iVertexSize(....); i < iVertexSize; ++i)
      {
          validEdgeList.push_back(....);
      }
  }

  if (validEdgeList.empty())                  // <=
  {
      false;                                  // <= fail
  }
  
  std::vector<BrushEdge3D> linkedEdgeList;
  std::set<int> usedEdgeSet;

  linkedEdgeList.push_back(validEdgeList[0]); // <= fail
  ....
}

Очень интересный пример ошибки с пропущенным оператором return. Теперь возможна ситуация обращения по индексу к пустому контейнеру.

Пример 3.

V564 ^[23] CWE-480 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. toglslinstruction.c 2914

void SetDataTypes(....)
{
 ....
 // Check assumption that both the values which MOVC might pick
 // have the same basic data type.
 if(!psContext->flags & HLSLCC_FLAG_AVOID_TEMP_REGISTER_ALIASING)
 {
   ASSERT(GetOperandDataType(psContext, &psInst->asOperands[2])
     == GetOperandDataType(psContext, &psInst->asOperands[3]));
 }
 ....
}

Неправильно проверили наличие битиков во флаге. Оператор отрицания применяется к значению флага, а не всего выражения. Правильно написать так:

if(!(psContext->flags & ....))

Ещё подобные предупреждения:

• V564 CWE-480 The '|' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '||' operator. d3dhwshader.cpp 1832
• V564 CWE-480 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. trackviewdialog.cpp 2112
• V564 CWE-480 The '|' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '||' operator. imagecompiler.cpp 1039

Пример 4.

V596 ^[24] CWE-390 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); prefabobject.cpp 1491

static std::vector<std::string> PyGetPrefabLibrarys()
{
  CPrefabManager* pPrefabManager = GetIEditor()->GetPrefabMa....;
  if (!pPrefabManager)
  {
      std::runtime_error("Invalid Prefab Manager.");
  }
  ....
}

Ошибка с генерацией исключения. Надо было писать так:

throw std::runtime_error("Invalid Prefab Manager.");

Весь список таких ошибок:

• V596 CWE-390 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); prefabobject.cpp 1515
• V596 CWE-390 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); prefabobject.cpp 1521
• V596 CWE-390 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); prefabobject.cpp 1543
• V596 CWE-390 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); prefabobject.cpp 1549
• V596 CWE-390 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); prefabobject.cpp 1603
• V596 CWE-390 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); prefabobject.cpp 1619
• V596 CWE-390 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); prefabobject.cpp 1644

Пара проблем при работе с памятью

V549 ^[25] CWE-688 The first argument of 'memcmp' function is equal to the second argument. meshutils.h 894

struct VertexLess
{
 ....
 bool operator()(int a, int b) const
 {
   ....
   if (m.m_links[a].links.size() != m.m_links[b].links.size())
   {
     res = (m.m_links[a].links.size() <
            m.m_links[b].links.size()) ? -1 : +1;
   }
   else
   {
     res = memcmp(&m.m_links[a].links[0], &m.m_links[a].links[0],
     sizeof(m.m_links[a].links[0]) * m.m_links[a].links.size());
   }
   ....
 }
 ....
};

В условии сравниваются размеры двух векторов. Если они равны, то в ветви else значения первых элементов векторов сравниваются с помощью функции memcmp(). Но первый и второй аргументы этой функции одинаковы! Доступ к элементам массива достаточно громозднкий. Там есть индексы a и b. Скорее всего, опечатка именно в них.

V611 ^[26] CWE-762 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] data;'. vectorn.h 102

~vectorn_tpl()
{
  if (!(flags & mtx_foreign_data))
  {
    delete[] data;
  }
}

vectorn_tpl& operator=(const vectorn_tpl<ftype>& src)
{
  if (src.len != len && !(flags & mtx_foreign_data))
  {
    delete data;  // <=
    data = new ftype[src.len];
  }
  ....
}

Память по указателю datа освобождается с помощью неправильного оператора. Везде должен использоваться оператор delete[].

Недостижимый код

V779 ^[27] CWE-561 Unreachable code detected. It is possible that an error is present. fbxskinimporter.cpp 67

Events::ProcessingResult FbxSkinImporter::ImportSkin(....)
{
  ....
  if (BuildSceneMeshFromFbxMesh(....)
  {
    context.m_createdData.push_back(std::move(createdData));
    return Events::ProcessingResult::Success;   // <=
  }
  else
  {
    return Events::ProcessingResult::Failure;   // <=
  }

  context.m_createdData.push_back();            // <= fail

  return Events::ProcessingResult::Success;
}

Все ветки условного оператора завершаются выходом из функции. При этом некий код не выполняется.

V779 ^[27] CWE-561 Unreachable code detected. It is possible that an error is present. dockablelibrarytreeview.cpp 153

bool DockableLibraryTreeView::Init(IDataBaseLibrary* lib)
{
  ....
  if (m_treeView && m_titleBar && m_defaultView)
  {
    if (m_treeView->topLevelItemCount() > 0)
    {
      ShowTreeView();
    }
    else
    {
      ShowDefaultView();
    }
    return true;                // <=
  }
  else
  {
    return false;               // <=
  }

  emit SignalFocused(this);     // <= fail
}

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

V622 ^[28] CWE-478 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. datum.cpp 872

AZ_INLINE bool IsDataGreaterEqual(....)
{
  switch (type.GetType())
  {
    AZ_Error("ScriptCanvas", false, "....");
    return false;

  case Data::eType::Number:
    return IsDataGreaterEqual<Data::NumberType>(lhs, rhs);

  ....

  case Data::eType::AABB:
    AZ_Error("ScriptCanvas", false, "....",
      Data::Traits<Data::AABBType>::GetName());
    return false;

  case Data::eType::OBB:
    AZ_Error("ScriptCanvas", false, "....",
      Data::Traits<Data::OBBType>::GetName());
    return false;
  ....
}

Если в switch присутствует код вне оператора case/default, то он никогда не выполняется.

Заключение

В статью вошли 95 предупреждений анализатора, из них 25 с примерами кода. Сколько это материала от всего отчёта анализатора? Я быстро прокрутил всего треть предупреждений уровня High. Есть ещё Medium и Low, группа диагностик для поиска оптимизаций и другие неосвоенные возможности анализатора — это ещё сотни очевидных ошибок и тысячи неисследованных предупреждений.

И тут читателю надо задать себе вопрос: «Возможно ли с таким подходом к проекту выпустить хороший игровой движок?». Контроля качества кода нет. За основу был взят код CryEngine со старыми ошибками, добавлены новые ошибки. Сам CryEngine дорабатывается только после очередного обзора кода. У компании Amazon есть все шансы с её ресурсами поработать в направлении качества кода и выпустить самый крутой игровой движок!

Не стоит сильно расстраиваться. Среди клиентов PVS-Studio есть более тридцати других компаний, которые занимаются играми. С ними и их продуктами вы можете познакомиться на странице нашего сайта "Клиенты ^[29]", выбрав фильтр «Разработка игр». Так что мы постепенно улучшаем мир. Возможно, мы сможем улучшить и Amazon Lumberyard :).

На тему качества игрового ПО коллега недавно написал статью, предлагаю заинтересованным ознакомиться: "Статический анализ в видеоигровой индустрии: топ-10 программных ошибок ^[30]".

Ссылка на загрузку анализатора PVS-Studio ^[7], как же без неё ;-)

Автор: SvyatoslavMC

Источник ^[31]