Amazon Lumberyard: крик души09.07.2018 11:48

94c9b74f00eae5bc8d344548dd744585.png


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

Введение


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

Picture 2

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

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


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

В качестве сборочной системы Lumberyard используется WAF, которая был и в CryEngine. Специального способа для интеграции с этой сборочной системой у анализатора нет. Я решил поработать с проектом на Windows и выбрал такой способ запуска анализа: система мониторинга компиляции. Проектный файл для 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 The 'then' statement is equivalent to the 'else' statement. toglsloperand.c 700

Picture 3

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

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

  • 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++

Picture 9

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

V709 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

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

Picture 5

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

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

V501 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 There are identical sub-expressions 'sp.m_pTexture == m_pTexture' to the left and to the right of the '&&' operator. shadercomponents.h 487

V501 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 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. Эта ошибка из их библиотеки FBX SDK. Перепутали переменные pSrc и pDst. Я думаю, кроме Lumberyard полно и других пользователей, чьи проекты зависят от кода с этой ошибкой.

V501 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 похожих:

  • V501 There are identical sub-expressions to the left and to the right of the '||' operator: ! pTS→pRT_ALD_0 ||! pTS→pRT_ALD_0 d3d_svo.cpp 1041
  • V501 There are identical sub-expressions to the left and to the right of the '&&' operator: m_pRT_AIR_MIN && m_pRT_AIR_MIN d3d_svo.cpp 1808
  • V501 There are identical sub-expressions to the left and to the right of the '&&' operator: m_pRT_AIR_MAX && m_pRT_AIR_MAX d3d_svo.cpp 1819
  • V501 There are identical sub-expressions to the left and to the right of the '&&' operator: m_pRT_AIR_SHAD && m_pRT_AIR_SHAD d3d_svo.cpp 1830
  • V501 There are identical sub-expressions to the left and to the right of the '&&' operator: s_pPropertiesPanel && s_pPropertiesPanel entityobject.cpp 1700


И как я обещал, вот вам список оставшихся предупреждений V501 без примеров кода:

  • V501 There are identical sub-expressions 'MaxX < 0' to the left and to the right of the '||' operator. czbufferculler.h 128
  • V501 There are identical sub-expressions 'm_joints[op[1]].limits[1][i]' to the left and to the right of the '-' operator. articulatedentity.cpp 795
  • V501 There are identical sub-expressions 'm_joints[i].limits[1][j]' to the left and to the right of the '-' operator. articulatedentity.cpp 2044
  • V501 There are identical sub-expressions 'irect[0].x + 1 — irect[1].x >> 31' to the left and to the right of the '|' operator. trimesh.cpp 4029
  • V501 There are identical sub-expressions 'b→mlen <= 0' to the left and to the right of the '||' operator. bstrlib.c 1779
  • V501 There are identical sub-expressions 'b→mlen <= 0' to the left and to the right of the '||' operator. bstrlib.c 1827
  • V501 There are identical sub-expressions 'b→mlen <= 0' to the left and to the right of the '||' operator. bstrlib.c 1865
  • V501 There are identical sub-expressions 'b→mlen <= 0' to the left and to the right of the '||' operator. bstrlib.c 1779
  • V501 There are identical sub-expressions 'b→mlen <= 0' to the left and to the right of the '||' operator. bstrlib.c 1827
  • V501 There are identical sub-expressions 'b→mlen <= 0' to the left and to the right of the '||' operator. bstrlib.c 1865
  • V501 There are identical sub-expressions to the left and to the right of the '-' operator: dd — dd finalizingspline.h 669
  • V501 There are identical sub-expressions 'pVerts[2] — pVerts[3]' to the left and to the right of the '^' operator. roadrendernode.cpp 307
  • V501 There are identical sub-expressions '! pGroup→GetStatObj ()' to the left and to the right of the '||' operator. terrain_node.cpp 594
  • V501 There are identical sub-expressions to the left and to the right of the '||' operator: val == 0 || val == — 0 xmlcpb_attrwriter.cpp 367
  • V501 There are identical sub-expressions 'geom_colltype_solid' to the left and to the right of the '|' operator. attachmentmanager.cpp 1058
  • V501 There are identical sub-expressions '(TriMiddle — RMWPosition)' to the left and to the right of the '|' operator. modelmesh.cpp 174
  • V501 There are identical sub-expressions '(goal — pAbsPose[b3].t)' to the left and to the right of the '|' operator. posemodifierhelper.cpp 115
  • V501 There are identical sub-expressions '(goal — pAbsPose[b4].t)' to the left and to the right of the '|' operator. posemodifierhelper.cpp 242
  • V501 There are identical sub-expressions '(m_eTFSrc == eTF_BC6UH)' to the left and to the right of the '||' operator. texturestreaming.cpp 983
  • V501 There are identical sub-expressions to the left and to the right of the '-' operator: q2.v.z — q2.v.z azentitynode.cpp 102
  • V501 There are identical sub-expressions to the left and to the right of the '-' operator: q2.v.z — q2.v.z entitynode.cpp 107
  • V501 There are identical sub-expressions 'm_listRect.contains (event→pos ())' to the left and to the right of the '||' operator. aidebuggerview.cpp 463
  • V501 There are identical sub-expressions to the left and to the right of the '&&' operator: pObj→GetParent () && pObj→GetParent () designerpanel.cpp 253

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

Picture 6

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

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

V502 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 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. Причём не на самой лучшей его версии. Такой вывод я сделал, посмотрев отчёт анализатора. Некоторые найденные ошибки уже исправлены в последней версии CryEngine по двум моим обзорам кода, но до сих пор присутствуют в коде Lumberyard. Также за последний год анализатор был значительно улучшен и удалось найти дополнительные ошибки, которые теперь присутствуют в двух игровых движках. Но с Lumberyard всё же ситуация похуже. В наследство Amazon по сути достался весь технический долг CryEngine. Ну, а свой собственный технический долг, само собой, в каждой компании появляется своими силами :).

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

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

Picture 4

Примерно такие эмоции будут испытывать разработчики 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 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 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 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 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 программирования

Picture 7

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

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

V561 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


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

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

Picture 8

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

V570 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


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

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

Picture 11

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

Пример 1.

V606 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 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 linkedEdgeList;
  std::set usedEdgeSet;

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


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

Пример 3.

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

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

Picture 10

V549 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 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& src)
{
  if (src.len != len && !(flags & mtx_foreign_data))
  {
    delete data;  // <=
    data = new ftype[src.len];
  }
  ....
}


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

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


V779 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 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 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(lhs, rhs);

  ....

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

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


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

Заключение


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

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

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

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

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

tsz9kmyjtteajhd4x1au60rsrvq.png

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Amazon Lumberyard: A Scream of Anguish

© Habrahabr.ru