# Amazon Lumberyard: крик души Игры \- одни из самых массовых продуктов среди программного обеспечения\. Это огромная индустрия, в которой появился новый игровой движок \- Amazon Lumberyard\. Проект ещё находится в статусе беты и у него есть время, чтобы исправить ошибки, повысить качество кода\. Разработчикам движка предстоит проделать много работы, чтобы в ближайшее время не разочаровать миллионы игроманов и разработчиков игр\. ![0574_AmazonLumberyard_ru/image1.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image1.png) ## Введение [Amazon Lumberyard](https://aws.amazon.com/ru/lumberyard/) \- бесплатный кроссплатформенный игровой движок класса AAA, разработанный компанией Amazon и основанный на архитектуре движка [CryEngine](https://www.cryengine.com/), который был лицензирован у компании Crytek в 2015 году\. Кстати, анализ CryEngine уже дважды проводился мною в [августе 2016](https://pvs-studio.ru/ru/blog/posts/cpp/0417/) и [апреле 2017](https://pvs-studio.ru/ru/blog/posts/cpp/0495/)\. При этом я вынужден отметить, что код по прошествии года стал только хуже\. И вот на днях я решил посмотреть, что сделал Amazon на основе этого игрового движка\. Над окружением они очень хорошо поработали\. Документация для разработчиков и софт для развёртывания рабочего окружения сделаны очень круто и на высоком уровне\. Но с кодом снова беда\! Я надеюсь, что у Amazon намного больше ресурсов для работы с проектом, и они всё\-таки уделят внимание качеству кода\. Этим обзором я надеюсь обратить внимание разработчиков на качество кода и подтолкнуть к новому подходу в разработке этого игрового движка\. Текущее состояние кода оказалось в таком плачевном состоянии, что я несколько раз менял название статьи и перерисовывал титульную картинку по мере просмотра отчёта с результатами анализа\. Первая версия картинки была менее эмоциональной: ![0574_AmazonLumberyard_ru/image2.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image2.png) Анализировались исходники Amazon Lumberyard последней доступной версии 1\.14\.0\.1\. Исходный код взят из репозитория на [GitHub](https://github.com/aws/lumberyard/)\. Одной из первых игр на движке Lumberyard должна стать [Star Citizen](https://robertsspaceindustries.com/)\. Потенциальных игроков, которые её ждут, тоже приглашаю взглянуть, что на данный момент находится "под капотом" игры\. ## Интеграция с PVS\-Studio В качестве статического анализатора кода использовался [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Он доступен для Windows, Linux и macOS\. Т\.е\. для анализа кроссплатформенного проекта есть даже из чего выбрать для более комфортной работы\. Кроме C и C\+\+ поддерживается анализ проектов на языке C\#\. [В планах Java](https://pvs-studio.ru/ru/blog/posts/java/0572/)\. На перечисленных языках написано подавляющее большинство кода в мире \(не без ошибок, конечно же\), так что пробуйте анализатор PVS\-Studio на своём проекте, узнаете много интересного ;\-\)\. В качестве сборочной системы Lumberyard используется WAF, которая была и в CryEngine\. Специального способа для интеграции с этой сборочной системой у анализатора нет\. Я решил поработать с проектом на Windows и выбрал такой способ запуска анализа: [система мониторинга компиляции](https://pvs-studio.ru/ru/docs/manual/0031/)\. Проектный файл для Visual Studio является автогенерируемым\. Им можно пользоваться для сборки проекта и просмотра отчёта анализатора\. Список команд для анализа выглядит примерно так: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. toglsloperand\.c 700 ![0574_AmazonLumberyard_ru/image3.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image3.png) Тут выполняется одинаковый код, независимо от вычисленного условия\. На фоне оставленных комментариев такое решение выглядит подозрительно\. В целом по проекту это не единственное место, где условия нужно упростить для наглядности, либо исправить настоящую ошибку\. Вот список таких мест: * 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\+\+ ![0574_AmazonLumberyard_ru/image4.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image4.png) Анализатор нашёл такой забавный код: [V709](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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 ## Первая и одна из лучших диагностик ![0574_AmazonLumberyard_ru/image5.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image5.png) Речь пойдёт о предупреждении [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) \- первой диагностике общего назначения в PVS\-Studio\. Ошибок, найденных только одной этой диагностикой, может быть достаточно для написания статьи\. И проект Amazon Lumberyard отлично это демонстрирует\. К сожалению, долго просматривать однотипные ошибки скучно, поэтому в этом разделе прокомментирую только пару фрагментов кода, а остальное предупреждения приведу списком\. [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '\|\|' operator: hotX < 0 \|\| hotX < 0 editorutils\.cpp 166 ```cpp 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; } .... } ``` В условии не хватает переменной _hot**Y**_\. Классическая опечатка\. [V501](https://pvs-studio.ru/ru/docs/warnings/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](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions 'sp\.m\_eCGTextureType \=\= m\_eCGTextureType' to the left and to the right of the '&&' operator\. shadercomponents\.h 487 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '\=\=' operator: pSrc\.GetLen\(\) \=\= pSrc\.GetLen\(\) fbxpropertytypes\.h 978 ```cpp 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](https://www.autodesk.com)\. Эта ошибка из их библиотеки [FBX SDK](https://www.autodesk.com/products/fbx/overview)\. Перепутали переменные _pSrc_ и _pDst_\. Я думаю, кроме Lumberyard полно и других пользователей, чьи проекты зависят от кода с этой ошибкой\. [V501](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 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 ## О позиции камеры в играх ![0574_AmazonLumberyard_ru/image6.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image6.png) Есть вторая по крутости диагностика в PVS\-Studio \- [V502](https://pvs-studio.ru/ru/docs/warnings/v502/)\. Эта диагностика старше некоторых новых языков программирования, в которых уже нельзя допустить такую ошибку\. А для С\+\+ эта ошибка будет актуальная, пожалуй, всегда\. Начнём с маленького простого примера\. [V502](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp bool ZipEncryptor::ParseKey(....) { .... size_t pos = i * 2 + (v1 == 0xff) ? 1 : 2; RCLogError("....", pos); return false; .... } ``` Операция сложения имеет более высокий приоритет, чем тернарный оператор\. Следовательно, у этого выражения совсем другая логика вычисления, чем предполагал автор\. Исправить ошибку можно таким образом: ```cpp size_t pos = i * 2 + (v1 == 0xff ? 1 : 2); ``` [V502](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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); } ``` А вот пример кода, где работают с позицией камеры\. Пример сложный для восприятия глазами и в нём присутствует ошибка\. Для публикации форматирование кода было изменено, но в исходном файле этот код ещё более нечитабелен\. Ошибка присутствует в этой подстроке: ```cpp 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](https://www.cryengine.com/)\. Причём не на самой лучшей его версии\. Такой вывод я сделал, посмотрев отчёт анализатора\. Некоторые найденные ошибки уже исправлены в последней версии CryEngine по двум моим обзорам кода, но до сих пор присутствуют в коде Lumberyard\. Также за последний год анализатор был значительно улучшен и удалось найти дополнительные ошибки, которые теперь присутствуют в двух игровых движках\. Но с Lumberyard всё же ситуация похуже\. В наследство Amazon по сути достался весь технический долг CryEngine\. Ну а свой собственный технический долг, само собой, в каждой компании появляется своими силами :\)\. В этом разделе я приведу всего парочку ошибок, которые были исправлены в последней версии CryEngine, и теперь остались только проблемой проекта Lumberyard\. [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'BlendFactor\[2\]' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 1283, 1284\. ccrydxgldevicecontext\.cpp 1284 ![0574_AmazonLumberyard_ru/image7.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image7.png) Примерно такие эмоции будут испытывать разработчики 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](https://pvs-studio.ru/ru/docs/warnings/v546/) Member of a class is initialized by itself: 'eConfigMax\(eConfigMax\.VeryHigh\)'\. particleparams\.h 1837 ```cpp ParticleParams() : .... fSphericalApproximation(1.f), fVolumeThickness(1.0f), fSoundFXParam(1.f), eConfigMax(eConfigMax.VeryHigh), // <= fFadeAtViewCosAngle(0.f) {} ``` В CryEngine этот класс вообще переписали, а тут ошибка с инициализацией осталась\. [V521](https://pvs-studio.ru/ru/docs/warnings/v521/) Such expressions using the ',' operator are dangerous\. Make sure the expression '\!sWords\[iWord\]\.empty\(\), iWord \+\+' is correct\. tacticalpointsystem\.cpp 3376 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v557/) CWE\-119 Array overrun is possible\. The 'id' index is pointing beyond array bound\. gameobjectsystem\.cpp 113 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v557/) CWE\-119 Array overrun is possible\. The 'index' index is pointing beyond array bound\. vehicleseatgroup\.cpp 73 ```cpp 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 программирования ![0574_AmazonLumberyard_ru/image8.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image8.png) Дойдя до этого раздела статьи, вы наверняка заметили, что программирование методом Copy\-Paste \- причина многих проблем\. В PVS\-Studio поиск таких ошибок реализован в разных диагностиках\. В этом разделе будут приведены примеры копипасты, находимые с помощью [V561](https://pvs-studio.ru/ru/docs/warnings/v561/)\. Вот пример подозрительного кода, когда объявляют переменные с одинаковым именем в пересекающихся областях видимости\. [V561](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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 Большой список\.\.\. некоторые из перечисленных мест являются даже полными копиями описанного примера\. ## Инициализация собственным значением ![0574_AmazonLumberyard_ru/image9.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image9.png) В коде игрового движка найдено очень много мест, где переменная присваивается сама себе\. Где\-то этот код остался для отладки, где\-то код просто красиво оформлен \(тоже часто является источником ошибок\), поэтому приведу один самый подозрительный для меня фрагмент кода\. [V570](https://pvs-studio.ru/ru/docs/warnings/v570/) The 'behaviorParams\.ignoreOnVehicleDestroyed' variable is assigned to itself\. vehiclecomponent\.cpp 168 ```cpp bool CVehicleComponent::Init(....) { .... if (!damageBehaviorTable.getAttr(....) { behaviorParams.ignoreOnVehicleDestroyed = false; } else { behaviorParams.ignoreOnVehicleDestroyed = // <= behaviorParams.ignoreOnVehicleDestroyed; // <= } .... } ``` В текущем виде ветвь _else_ вообще не нужна\. Но, возможно, этот фрагмент кода содержит ошибку: хотели присвоить переменной противоположное значение: ```cpp bValue = !bValue ``` Но с результатами этой диагностики разработчиками лучше ознакомиться самим\. ## Обработка проблемных ситуаций ![0574_AmazonLumberyard_ru/image10.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image10.png) В этом разделе будет приведено много примеров, когда при обработке ошибок что\-то пошло не так\. **Пример 1**\. [V606](https://pvs-studio.ru/ru/docs/warnings/v606/) Ownerless token 'nullptr'\. dx12rootsignature\.cpp 599 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v606/) Ownerless token 'false'\. fillspacetool\.cpp 191 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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])); } .... } ``` Неправильно проверили наличие битиков во флаге\. Оператор отрицания применяется к значению флага, а не всего выражения\. Правильно написать так: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp static std::vector PyGetPrefabLibrarys() { CPrefabManager* pPrefabManager = GetIEditor()->GetPrefabMa....; if (!pPrefabManager) { std::runtime_error("Invalid Prefab Manager."); } .... } ``` Ошибка с генерацией исключения\. Надо было писать так: ```cpp 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 ## Пара проблем при работе с памятью ![0574_AmazonLumberyard_ru/image11.png](https://import.viva64.com/docx/blog/0574_AmazonLumberyard_ru/image11.png) [V549](https://pvs-studio.ru/ru/docs/warnings/v549/) CWE\-688 The first argument of 'memcmp' function is equal to the second argument\. meshutils\.h 894 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp ~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](https://pvs-studio.ru/ru/docs/warnings/v779/) CWE\-561 Unreachable code detected\. It is possible that an error is present\. fbxskinimporter\.cpp 67 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v779/) CWE\-561 Unreachable code detected\. It is possible that an error is present\. dockablelibrarytreeview\.cpp 153 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v622/) CWE\-478 Consider inspecting the 'switch' statement\. It's possible that the first 'case' operator is missing\. datum\.cpp 872 ```cpp 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 есть более тридцати других компаний, которые занимаются играми\. С ними и их продуктами вы можете познакомиться на странице нашего сайта "[Клиенты](https://pvs-studio.ru/ru/about/customers/)", выбрав фильтр "Разработка игр"\. Так что мы постепенно улучшаем мир\. Возможно, мы сможем улучшить и Amazon Lumberyard :\)\. На тему качества игрового ПО коллега недавно написал статью, предлагаю заинтересованным ознакомиться: "[Статический анализ в видеоигровой индустрии: топ\-10 программных ошибок](https://pvs-studio.ru/ru/blog/posts/cpp/0570/)"\. Ссылка на загрузку анализатора [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/), как же без неё ;\-\)