Мы используем куки, чтобы пользоваться сайтом
было удобно.
Игры - одни из самых массовых продуктов среди программного обеспечения. Это огромная индустрия, в которой появился новый игровой движок - Amazon Lumberyard. Проект ещё находится в статусе беты и у него есть время, чтобы исправить ошибки, повысить качество кода. Разработчикам движка предстоит проделать много работы, чтобы в ближайшее время не разочаровать миллионы игроманов и разработчиков игр.
Amazon Lumberyard - бесплатный кроссплатформенный игровой движок класса AAA, разработанный компанией Amazon и основанный на архитектуре движка CryEngine, который был лицензирован у компании Crytek в 2015 году. Кстати, анализ CryEngine уже дважды проводился мною в августе 2016 и апреле 2017. При этом я вынужден отметить, что код по прошествии года стал только хуже. И вот на днях я решил посмотреть, что сделал Amazon на основе этого игрового движка. Над окружением они очень хорошо поработали. Документация для разработчиков и софт для развёртывания рабочего окружения сделаны очень круто и на высоком уровне. Но с кодом снова беда! Я надеюсь, что у Amazon намного больше ресурсов для работы с проектом, и они всё-таки уделят внимание качеству кода. Этим обзором я надеюсь обратить внимание разработчиков на качество кода и подтолкнуть к новому подходу в разработке этого игрового движка. Текущее состояние кода оказалось в таком плачевном состоянии, что я несколько раз менял название статьи и перерисовывал титульную картинку по мере просмотра отчёта с результатами анализа. Первая версия картинки была менее эмоциональной:
Анализировались исходники Amazon Lumberyard последней доступной версии 1.14.0.1. Исходный код взят из репозитория на GitHub. Одной из первых игр на движке Lumberyard должна стать Star Citizen. Потенциальных игроков, которые её ждут, тоже приглашаю взглянуть, что на данный момент находится "под капотом" игры.
В качестве статического анализатора кода использовался 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.
Amazon Lumberyard позиционирует себя как кроссплатформенный игровой движок. Продвигать проект в массы с такой фичей легко, а вот поддерживать очень трудно. Одно из предупреждений PVS-Studio было выдано на фрагмент кода, где программист Игорь боролся с компилятором Qualcomm. Возможно, он решил свою задачу, но оставил крайне подозрительный код. Я решил оформить его картинкой.
V523 The 'then' statement is equivalent to the 'else' statement. toglsloperand.c 700
Тут выполняется одинаковый код, независимо от вычисленного условия. На фоне оставленных комментариев такое решение выглядит подозрительно.
В целом по проекту это не единственное место, где условия нужно упростить для наглядности, либо исправить настоящую ошибку. Вот список таких мест:
Анализатор нашёл такой забавный код:
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 контрольных выстрела:
Речь пойдёт о предупреждении 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 без примеров кода:
Есть вторая по крутости диагностика в 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.
Другие примеры с похожими предупреждениями:
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
Примерно такие эмоции будут испытывать разработчики Lumberyard, когда узнают, что эта ошибка осталась только у них.
Кстати таких ещё две:
Есть такая ошибка:
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.
Оставшиеся предупреждения:
Долгое существование ошибок в коде можно объяснить только соответствующим уровнем тестирования проекта. Есть мнение, что статический анализ находит ошибки только в неиспользуемом коде. Так вот, это не так. Разработчики забывают, что большинство пользователей молча страдает от неочевидных багов в программах, а проявление этих самых редких багов часто пагубно сказывается на работе всей компании, репутации и её продажах, если таковые имеются.
Дойдя до этого раздела статьи, вы наверняка заметили, что программирование методом 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' не перезаписывается, как ожидалось. Инициализация этого указателя была полностью скопирована под условие вместе с объявлением типа.
Приведу списком все похожие места:
Большой список... некоторые из перечисленных мест являются даже полными копиями описанного примера.
В коде игрового движка найдено очень много мест, где переменная присваивается сама себе. Где-то этот код остался для отладки, где-то код просто красиво оформлен (тоже часто является источником ошибок), поэтому приведу один самый подозрительный для меня фрагмент кода.
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Но с результатами этой диагностики разработчиками лучше ознакомиться самим.
В этом разделе будет приведено много примеров, когда при обработке ошибок что-то пошло не так.
Пример 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 будет использовано в других местах кода.
Точь-в-точь такой же код скопировали ещё в одно место:
Пример 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<BrushEdge3D> linkedEdgeList;
std::set<int> 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 & ....))Ещё подобные предупреждения:
Пример 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<std::string> PyGetPrefabLibrarys()
{
CPrefabManager* pPrefabManager = GetIEditor()->GetPrefabMa....;
if (!pPrefabManager)
{
std::runtime_error("Invalid Prefab Manager.");
}
....
}Ошибка с генерацией исключения. Надо было писать так:
throw std::runtime_error("Invalid Prefab Manager.");Весь список таких ошибок:
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<ftype>& 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<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 есть более тридцати других компаний, которые занимаются играми. С ними и их продуктами вы можете познакомиться на странице нашего сайта "Клиенты", выбрав фильтр "Разработка игр". Так что мы постепенно улучшаем мир. Возможно, мы сможем улучшить и Amazon Lumberyard :).
На тему качества игрового ПО коллега недавно написал статью, предлагаю заинтересованным ознакомиться: "Статический анализ в видеоигровой индустрии: топ-10 программных ошибок".
Ссылка на загрузку анализатора PVS-Studio, как же без неё ;-)
0
0
0
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
0