Amazon Lumberyard: le cri de l'âme

Les jeux sont l'un des logiciels les plus populaires. Il s'agit d'une énorme industrie dans laquelle un nouveau moteur de jeu - Amazon Lumberyard. Le projet est toujours en version bêta et il a le temps de corriger les erreurs et d'améliorer la qualité du code. Les développeurs du moteur ont beaucoup de travail à faire dans un avenir proche pour ne pas décevoir des millions de joueurs et de développeurs de jeux.

Présentation

Amazon Lumberyard est un moteur de jeu multiplateforme AAA gratuit développé par Amazon et basé sur l'architecture du moteur CryEngine , qui a été autorisé par Crytek en 2015. Soit dit en passant, l'analyse de CryEngine a déjà été effectuée deux fois par moi en août 2016 et avril 2017 . En même temps, je dois noter qu'après un an, le code n'a fait qu'empirer. Et l'autre jour, j'ai décidé de voir ce qu'Amazon a fait sur la base de ce moteur de jeu. Ils ont très bien travaillé sur l'environnement. La documentation pour les développeurs et les logiciels pour déployer un environnement de travail est rendue très cool et à un niveau élevé. Mais le problème est à nouveau avec le code! J'espère qu'Amazon a beaucoup plus de ressources pour travailler avec le projet, et qu'ils font toujours attention à la qualité du code. Avec cette revue, j'espère attirer l'attention des développeurs sur la qualité du code et pousser pour une nouvelle approche dans le développement de ce moteur de jeu. L'état actuel du code était dans un état tellement déplorable que j'ai changé plusieurs fois le titre de l'article et redessiné l'image du titre en parcourant le rapport avec les résultats de l'analyse. La première version de l'image était moins émotionnelle:

Nous avons analysé les sources d'Amazon Lumberyard de la dernière version disponible 1.14.0.1. Le code source est extrait du référentiel sur Github . L'un des premiers jeux sur le moteur Lumberyard devrait être Star Citizen . Joueuses potentielles qui l'attendent, je vous invite également à jeter un œil à ce qui est actuellement "sous le capot" du jeu.

Intégration avec PVS-Studio

PVS-Studio a été utilisé comme analyseur de code statique. Il est disponible pour Windows, Linux et macOS. C'est-à-dire pour l'analyse d'un projet multiplateforme, il y a même quelque chose à choisir pour un travail plus confortable. En plus de C et C ++, l'analyse des projets en langage C # est prise en charge. Plans Java . La grande majorité du code dans le monde est écrit dans les langues répertoriées (non sans erreurs, bien sûr), alors essayez l'analyseur PVS-Studio sur votre projet, vous apprendrez beaucoup de choses intéressantes ;-).

En tant que système d'assemblage Lumberyard, WAF est utilisé, ce qui était également dans CryEngine. L'analyseur n'a pas de moyen spécial pour s'intégrer à ce système d'assemblage. J'ai décidé de travailler avec un projet sur Windows et j'ai choisi cette méthode de démarrage de l'analyse: un système de surveillance de compilation . Le fichier de projet pour Visual Studio est généré automatiquement. Il peut être utilisé pour créer le projet et afficher le rapport de l'analyseur.

La liste des commandes d'analyse ressemble à ceci:

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

Comme je l'ai dit, le rapport peut être consulté dans Visual Studio.

À propos d'Igor et de Qualcomm

Amazon Lumberyard se positionne comme un moteur de jeu multiplateforme. Promouvoir un projet auprès des masses avec une telle fonctionnalité est facile, mais le maintenir est très difficile. L'un des avertissements de PVS-Studio a été émis sur un fragment de code où le programmeur Igor s'est battu avec le compilateur Qualcomm. Peut-être qu'il a résolu son problème, mais a laissé un code extrêmement suspect. J'ai décidé de le dessiner avec une photo.

V523 L' instruction 'then' est équivalente à l'instruction 'else'. toglsloperand.c 700

Ici, le même code est exécuté, quelle que soit la condition calculée. Dans le contexte des commentaires laissés, une telle décision semble suspecte.

En général, le projet n'est pas le seul endroit où les conditions doivent être simplifiées pour plus de clarté ou pour corriger une erreur réelle. Voici une liste de ces endroits:

• V523 L'instruction 'then' est équivalente à l'instruction 'else'. livingentity.cpp 1385
• V523 L'instruction 'then' est équivalente à l'instruction 'else'. tometalinstruction.c 4201
• V523 L'instruction 'then' est équivalente à l'instruction 'else'. scripttable.cpp 905
• V523 L'instruction 'then' est équivalente à l'instruction 'else'. budgetingsystem.cpp 701
• V523 L'instruction 'then' est équivalente à l'instruction 'else'. editorframeworkapplication.cpp 562
• V523 L'instruction 'then' est équivalente à l'instruction 'else'. particuleitem.cpp 130
• V523 L'instruction 'then' est équivalente à l'instruction 'else'. trackviewnodes.cpp 1223
• V523 L'instruction 'then' est équivalente à l'instruction 'else'. propertyoarchive.cpp 447

Python ++

L'analyseur a trouvé un code aussi amusant:

V709 CWE-682 Comparaison suspecte trouvée: 'a == b == c'. N'oubliez pas que «a == b == c» n'est pas égal à «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) // <= { .... } .... } 

En C ++, un tel code, malheureusement, se compile avec succès, mais il ne s'exécute pas du tout comme cela peut sembler. Les expressions en C ++ sont évaluées par ordre de priorité, exécutant des castes implicites, si nécessaire.

Une telle vérification serait correcte, par exemple, dans le langage Python. Mais dans cette situation, le développeur "s'est tiré une balle dans le pied".

3 autres coups de contrôle:

• V709 CWE-682 Comparaison suspecte trouvée: 'a == b == c'. N'oubliez pas que «a == b == c» n'est pas égal à «a == b && b == c». toglslinstruction.c 654
• V709 CWE-682 Comparaison suspecte trouvée: 'a == b == c'. N'oubliez pas que «a == b == c» n'est pas égal à «a == b && b == c». toglslinstruction.c 469
• V709 CWE-682 Comparaison suspecte trouvée: 'a == b == c'. N'oubliez pas que «a == b == c» n'est pas égal à «a == b && b == c». tometalinstruction.c 539

Le premier et l'un des meilleurs diagnostics

Il s'agira d'avertissement V501 - les premiers diagnostics à usage général dans PVS-Studio. Les erreurs trouvées par ce diagnostic à elles seules peuvent suffire à écrire un article. Et le projet Amazon Lumberyard le démontre très bien.

Malheureusement, il est ennuyeux de regarder les erreurs du même type pendant longtemps, donc dans cette section, je ne commenterai que quelques fragments de code, et le reste des avertissements seront répertoriés.

V501 Il existe des sous-expressions identiques à gauche et à droite de '||' opérateur: 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; } .... } 

La condition n'a pas la variable Y chaud . Typo classique.

V501 Il existe des sous-expressions identiques 'sp.m_pTexture == m_pTexture' à gauche et à droite de l'opérateur '&&'. shadercomponents.h 487

V501 Il existe des sous-expressions identiques «sp.m_eCGTextureType == m_eCGTextureType» à gauche et à droite de l'opérateur «&&». 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; } 

Deux copies-pâtes ont été trouvées dans ce fragment à la fois. Pour plus de clarté, j'ai peint des flèches.

V501 Il existe des sous-expressions identiques à gauche et à droite de l'opérateur '==': 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; } 

Ici, je veux dire bonjour aux développeurs d' AUTODESK . Cette erreur provient de leur bibliothèque SDK FBX . Variables confuses pSrc et pDst . Je pense que, outre Lumberyard, il y a aussi beaucoup d'autres utilisateurs dont les projets dépendent du code avec cette erreur.

V501 Il existe des sous-expressions identiques à gauche et à droite de l'opérateur '&&': 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); } } .... } 

Revenons au code Lumberyard. Dans la condition, le même pointeur pTS-> pRT_ALD_1 est vérifié . L'un d'eux aurait dû être pTS-> pRT_RGB_1 . Peut-être même après l'explication, il n'est pas immédiatement possible de voir la différence, mais il y en a une: la différence est dans les sous-chaînes courtes ALD et RGB . Si on vous dit que la révision manuelle du code est suffisante, montrez cet exemple.

Et si cet exemple ne suffit pas, il y en a 5 de plus similaires.

• V501 Il existe des sous-expressions identiques à gauche et à droite de '||' opérateur:! pTS-> pRT_ALD_0 ||! pTS-> pRT_ALD_0 d3d_svo.cpp 1041
• V501 Il existe des sous-expressions identiques à gauche et à droite de l'opérateur '&&': m_pRT_AIR_MIN && m_pRT_AIR_MIN d3d_svo.cpp 1808
• V501 Il existe des sous-expressions identiques à gauche et à droite de l'opérateur '&&': m_pRT_AIR_MAX && m_pRT_AIR_MAX d3d_svo.cpp 1819
• V501 Il existe des sous-expressions identiques à gauche et à droite de l'opérateur '&&': m_pRT_AIR_SHAD && m_pRT_AIR_SHAD d3d_svo.cpp 1830
• V501 Il existe des sous-expressions identiques à gauche et à droite de l'opérateur '&&': s_pPropertiesPanel && s_pPropertiesPanel entityobject.cpp 1700

Et comme je l'ai promis, voici une liste des avertissements V501 restants sans exemples de code:

À propos de la position de la caméra dans les jeux

Il existe le deuxième diagnostic le plus rapide dans PVS-Studio - V502 . Ce diagnostic est plus ancien que certains nouveaux langages de programmation, dans lesquels une telle erreur ne peut plus être faite. Et pour C ++, cette erreur sera pertinente, peut-être toujours.

Commençons par un petit exemple simple.

V502 L'opérateur '?:' Fonctionne peut-être d'une manière différente de celle attendue. L'opérateur '?:' A une priorité inférieure à l'opérateur '+'. zipencryptor.cpp 217

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

L'opération d'addition a une priorité plus élevée que l'opérateur ternaire. Par conséquent, cette expression a une logique de calcul complètement différente de celle supposée par l'auteur.

Vous pouvez corriger l'erreur de cette façon:

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

V502 L'opérateur '?:' Fonctionne peut-être d'une manière différente de celle attendue. L'opérateur '?:' A une priorité inférieure à l'opérateur '-'. 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); } 

Et voici un exemple de code où ils travaillent avec la position de la caméra. Un exemple est difficile à percevoir avec les yeux et contient une erreur. Pour la publication, la mise en forme du code a été modifiée, mais dans le fichier source, ce code est encore plus illisible.

Une erreur est présente dans cette sous-chaîne:

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

Si la caméra de votre jeu a soudainement commencé à se comporter de manière inappropriée, vous devez savoir que les développeurs ont économisé sur l'analyse de code statique: D.

Autres exemples avec des avertissements similaires:

• V502 L'opérateur '?:' Fonctionne peut-être d'une manière différente de celle attendue. L'opérateur '?:' A une priorité inférieure à l'opérateur '-'. scriptbind_ai.cpp 5203
• V502 L'opérateur '?:' Fonctionne peut-être d'une manière différente de celle attendue. L'opérateur '?:' A une priorité inférieure à l'opérateur '+'. qcolumnwidget.cpp 136
• V502 L'opérateur '?:' Fonctionne peut-être d'une manière différente de celle attendue. L'opérateur '?:' A une priorité plus faible que l'opérateur '&&'. shapetool.h 98

CryEngine Legacy

Amazon Lumberyard est basé sur le code CryEngine . Et pas sur sa meilleure version. J'ai fait une telle conclusion en regardant le rapport de l'analyseur. Certains bogues trouvés ont déjà été corrigés dans la dernière version de CryEngine pour mes deux revues de code, mais sont toujours présents dans le code Lumberyard. De plus, au cours de la dernière année, l'analyseur a été considérablement amélioré et il a été possible de trouver des erreurs supplémentaires qui sont maintenant présentes dans deux moteurs de jeu. Mais avec Lumberyard la situation est pire. Amazon a essentiellement hérité de la totalité de la dette technique de CryEngine. Mais son propre devoir technique, bien sûr, apparaît dans chaque entreprise de son propre chef :).

Dans cette section, je vais vous donner quelques erreurs qui ont été corrigées dans la dernière version de CryEngine, et qui ne restent maintenant qu'un problème du projet Lumberyard.

V519 La variable 'BlendFactor [2]' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 1283, 1284. ccrydxgldevicecontext.cpp 1284

De telles émotions seront ressenties par les développeurs de Lumberyard lorsqu'ils découvriront que cette erreur n'est restée qu'en eux.

Soit dit en passant, il y en a deux autres:

• V519 La variable 'm_auBlendFactor [2]' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 919, 920. ccrydxgldevicecontext.cpp 920
• V519 La variable 'm_auBlendFactor [2]' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 926, 927. ccrydxgldevicecontext.cpp 927

Il y a une telle erreur:

V546 Le membre d'une classe est initialisé par lui-même: 'eConfigMax (eConfigMax.VeryHigh)'. particulesparams.h 1837

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

Dans CryEngine, cette classe était généralement réécrite, mais ici l'erreur d'initialisation est restée.

V521 De telles expressions utilisant l'opérateur ',' sont dangereuses. Assurez-vous que l'expression «! SWords [iWord] .empty (), iWord ++» est correcte. 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); } .... } 

Le cycle suspect que CryEngine a également réécrit.

Les erreurs vivent plus longtemps que vous ne le pensez

Les utilisateurs qui commencent à utiliser PVS-Studio pour la première fois ont une situation à peu près identique: ils trouvent une erreur, découvrent qu'elle a été ajoutée il y a plusieurs mois et sont heureux de réaliser qu'ils ont miraculeusement évité la manifestation du problème chez leurs utilisateurs. Beaucoup de nos clients ont commencé à utiliser régulièrement PVS-Studio précisément après de telles histoires.

Parfois, afin de démarrer des processus de contrôle de la qualité du code, une entreprise doit visiter de telles situations plusieurs fois. Voici un exemple sur CryEngine et Lumberyard:

V557 CWE-119 Le dépassement de matrice est possible. L'index 'id' pointe au-delà de la limite du tableau. gameobjectsystem.cpp 113

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

Comme vous le savez, Amazon Lumberyard n'est pas basé sur la dernière version de CryEngine. Néanmoins, à l'aide de l'analyseur PVS-Studio, il a été possible de trouver une erreur qui est maintenant présente dans deux moteurs de jeu. Il fallait vérifier l'index à l'aide de l'opérateur '> =' ...

L'erreur d'indexation est grave. De plus, il y en a six! Voici un autre exemple:

V557 CWE-119 Le dépassement de matrice est possible. L'index «index» pointe au-delà de la limite du tableau. vehicleseatgroup.cpp 73

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

Quelqu'un a fait des erreurs du même type, et elles n'ont pas été corrigées simplement parce qu'elles n'avaient pas été incluses une fois dans les critiques de bogues de CryEngine.

Avertissements restants:

• V557 CWE-119 Le dépassement de matrice est possible. L'index 'id' pointe au-delà de la limite du tableau. gameobjectsystem.cpp 195
• V557 CWE-119 Le dépassement de matrice est possible. L'index 'id' pointe au-delà de la limite du tableau. gameobjectsystem.cpp 290
• V557 CWE-119 Le dépassement de matrice est possible. L'index «stateId» pointe au-delà de la limite du tableau. vehicleanimation.cpp 311
• V557 CWE-119 Le dépassement de matrice est possible. L'index «stateId» pointe au-delà de la limite du tableau. vehicleanimation.cpp 354

La longue existence d'erreurs dans le code ne peut être expliquée que par le niveau correspondant de test de projet. On pense que l'analyse statique ne trouve des erreurs que dans le code inutilisé. Ce n'est donc pas le cas. Les développeurs oublient que la plupart des utilisateurs souffrent silencieusement de bogues non évidents dans les programmes, et la manifestation de ces bogues très rares affecte souvent le travail de toute l'entreprise, sa réputation et ses ventes, le cas échéant.

Différentes nuances de programmation par copier-coller

Lorsque vous avez atteint cette section de l'article, vous avez probablement remarqué que la programmation de copier-coller est la cause de nombreux problèmes. Dans PVS-Studio, la recherche de ces erreurs est implémentée dans divers diagnostics. Cette section donne des exemples de copier-coller trouvés avec le V561 .

Voici un exemple de code suspect lors de la déclaration de variables portant le même nom dans des étendues qui se chevauchent.

V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'pLibrary' que de la déclarer à nouveau. Déclaration précédente: entityobject.cpp, ligne 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; } .... } 

Le pointeur pLibrary ne remplace pas comme prévu. L'initialisation de ce pointeur a été complètement copiée sous la condition avec la déclaration de type.

Je vais lister tous les endroits similaires:

• V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'eType' que de la déclarer à nouveau. Déclaration précédente: toglsloperand.c, ligne 838. toglsloperand.c 1224
• V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'eType' que de la déclarer à nouveau. Déclaration précédente: toglsloperand.c, ligne 838. toglsloperand.c 1305
• V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'rSkelPose' que de la déclarer à nouveau. Déclaration précédente: attachmentmanager.cpp, ligne 409. attachmentmanager.cpp 458
• V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'nThreadID' que de la déclarer à nouveau. Déclaration précédente: d3dmeshbaker.cpp, ligne 797. d3dmeshbaker.cpp 867
• V561 CWE-563 Il vaut probablement mieux attribuer une valeur à la variable 'directoryNameList' que de la déclarer à nouveau. Déclaration précédente: assetimportermanager.cpp, ligne 720. assetimportermanager.cpp 728
• V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'pNode' que de la déclarer à nouveau. Déclaration précédente: breakpointsctrl.cpp, ligne 340. breakpointsctrl.cpp 349
• V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'pLibrary' que de la déclarer à nouveau. Déclaration précédente: prefabobject.cpp, ligne 1443. prefabobject.cpp 1446
• V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'pLibrary' que de la déclarer à nouveau. Déclaration précédente: prefabobject.cpp, ligne 1470. prefabobject.cpp 1473
• V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'cmdLine' que de la déclarer à nouveau. Déclaration précédente: fileutil.cpp, ligne 110. fileutil.cpp 130
• V561 CWE-563 Il vaut probablement mieux attribuer une valeur à la variable 'sfunctionArgs' que de la déclarer à nouveau. Déclaration précédente: attributeitemlogiccallbacks.cpp, ligne 291. attributeitemlogiccallbacks.cpp 303
• V561 CWE-563 Il est probablement préférable d'attribuer une valeur à la variable 'curveName' que de la déclarer à nouveau. Déclaration précédente: qgradientselectorwidget.cpp, ligne 475. qgradientselectorwidget.cpp 488

Une longue liste ... certains des endroits énumérés sont même des copies complètes de l'exemple décrit.

Initialisation des valeurs propres

Dans le code du moteur de jeu, de nombreux endroits ont été trouvés où la variable est assignée à elle-même. Quelque part ce code a été laissé pour le débogage, quelque part le code est simplement magnifiquement conçu (c'est aussi souvent une source d'erreurs), donc je vais vous donner un morceau de code qui me semble le plus suspect.

V570 La variable 'behaviorParams.ignoreOnVehicleDestroyed' est assignée à elle-même. vehiclecomponent.cpp 168

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

Dans la vue actuelle, la branche else n'est pas du tout nécessaire. Mais peut-être que ce morceau de code contient une erreur: ils voulaient assigner la valeur opposée à la variable:

 bValue = !bValue 

Mais les développeurs connaissent mieux les résultats de ce diagnostic.

Gestion des problèmes

Cette section donne de nombreux exemples en cas de problème lors de la gestion des erreurs.

Exemple 1

V606 Jeton sans propriétaire '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; } } 

Oublié d'écrire return nullptr; . Maintenant, la valeur non valide de la variable de résultat sera utilisée ailleurs dans le code.

Le même code exact a été copié dans un autre emplacement:

• V606 Jeton sans propriétaire 'nullptr'. dx12rootsignature.cpp 621

Exemple 2

V606 Jeton sans propriétaire «faux». 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 .... } 

Un exemple très intéressant d'une erreur avec une instruction de retour manquante. Maintenant, une situation d'accès à un conteneur vide de l'index est possible.

Exemple 3

V564 CWE-480 L'opérateur '&' est appliqué à la valeur de type bool. Vous avez probablement oublié d'inclure des parenthèses ou avez l'intention d'utiliser l'opérateur '&&'. 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])); } .... } 

Vérification incorrecte de la présence de bits dans l'indicateur. L'opérateur de négation s'applique à la valeur de l'indicateur, pas à l'expression entière. Écrivez correctement comme ceci:

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

Avertissements plus similaires:

• V564 CWE-480 Le '|' L'opérateur est appliqué à la valeur de type bool. Vous avez probablement oublié d'inclure des parenthèses ou avez l'intention d'utiliser le '||' opérateur. d3dhwshader.cpp 1832
• V564 CWE-480 L'opérateur '&' est appliqué à la valeur de type bool. Vous avez probablement oublié d'inclure des parenthèses ou avez l'intention d'utiliser l'opérateur '&&'. trackviewdialog.cpp 2112
• V564 CWE-480 Le '|' L'opérateur est appliqué à la valeur de type bool. Vous avez probablement oublié d'inclure des parenthèses ou avez l'intention d'utiliser le '||' opérateur. imagecompiler.cpp 1039

Exemple 4

V596 CWE-390 L'objet a été créé mais il n'est pas utilisé. Le mot clé «throw» peut être manquant: 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."); } .... } 

Erreur lors de la levée de l'exception. Il fallait écrire comme ceci:

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

La liste complète de ces erreurs:

• V596 CWE-390 L'objet a été créé mais il n'est pas utilisé. Le mot clé «throw» peut être manquant: throw runtime_error (FOO); prefabobject.cpp 1515
• V596 CWE-390 L'objet a été créé mais il n'est pas utilisé. Le mot clé «throw» peut être manquant: throw runtime_error (FOO); prefabobject.cpp 1521
• V596 CWE-390 L'objet a été créé mais il n'est pas utilisé. Le mot clé «throw» peut être manquant: throw runtime_error (FOO); prefabobject.cpp 1543
• V596 CWE-390 L'objet a été créé mais il n'est pas utilisé. Le mot clé «throw» peut être manquant: throw runtime_error (FOO); prefabobject.cpp 1549
• V596 CWE-390 L'objet a été créé mais il n'est pas utilisé. Le mot clé «throw» peut être manquant: throw runtime_error (FOO); prefabobject.cpp 1603
• V596 CWE-390 L'objet a été créé mais il n'est pas utilisé. Le mot clé «throw» peut être manquant: throw runtime_error (FOO); prefabobject.cpp 1619
• V596 CWE-390 L'objet a été créé mais il n'est pas utilisé. Le mot clé «throw» peut être manquant: throw runtime_error (FOO); prefabobject.cpp 1644

Quelques problèmes lors de l'utilisation de la mémoire

V549 CWE-688 Le premier argument de la fonction 'memcmp' est égal au deuxième 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()); } .... } .... }; 

La condition compare les tailles de deux vecteurs. S'ils sont égaux, alors dans la branche else , les valeurs des premiers éléments des vecteurs sont comparées à l'aide de la fonction memcmp () . Mais les premier et deuxième arguments de cette fonction sont les mêmes! L'accès aux éléments du tableau est assez lourd. Il existe des indices a et b . Très probablement, une faute de frappe est en eux.

V611 CWE-762 La mémoire a été allouée à l'aide de l'opérateur 'new T []' mais a été libérée à l'aide de l'opérateur 'delete'. Pensez à inspecter ce code. Il vaut probablement mieux utiliser '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]; } .... } 

La mémoire du pointeur de données est libérée avec une instruction non valide. L'opérateur delete [] doit être utilisé partout .

Code inaccessible

V779 CWE-561 Code inaccessible détecté. Il est possible qu'une erreur soit présente. 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; } 

Toutes les branches de l'instruction conditionnelle se terminent par la sortie de la fonction. Cependant, certains codes ne sont pas exécutés.

V779 CWE-561 Code inaccessible détecté. Il est possible qu'une erreur soit présente. 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 } 

Il est facile de repérer une erreur sur ce morceau de code. Mais si vous écrivez le code pendant une longue période, l'attention diminue fortement et de telles erreurs tombent facilement dans le projet.

V622 CWE-478 Envisagez d'inspecter l'instruction 'switch'. Il est possible que le premier opérateur «case» soit manquant. 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; .... } 

Si le commutateur contient du code en dehors de l' instruction case / default , il n'est jamais exécuté.

Conclusion

L'article comprend 95 avertissements d'analyseur, dont 25 avec des exemples de code. Quelle est la quantité de matériel de l'ensemble du rapport de l'analyseur? J'ai fait défiler rapidement un tiers seulement des alertes de haut niveau . Il existe également Medium et Low, un groupe de diagnostics pour rechercher des optimisations, et d'autres opportunités inexploitées de l'analyseur - ce sont des centaines d'erreurs plus évidentes et des milliers d'avertissements inexplorés.

Et puis le lecteur doit se poser la question: "Est-il possible avec cette approche du projet de sortir un bon moteur de jeu?". Il n'y a pas de contrôle de qualité du code. Le code CryEngine avec d'anciennes erreurs a été pris comme base, de nouvelles erreurs ont été ajoutées. CryEngine lui-même n'est finalisé qu'après la prochaine révision du code. Amazon a toutes les chances avec ses ressources de travailler dans le sens de la qualité du code et de sortir le moteur de jeu le plus cool!

Ne soyez pas très contrarié. Parmi les clients de PVS-Studio, plus de trente autres sociétés sont impliquées dans les jeux. Vous pouvez vous familiariser avec eux et leurs produits sur la page de notre site " Clients " en sélectionnant le filtre "Développement de jeux". Nous améliorons donc progressivement le monde. Nous pouvons peut-être améliorer Amazon Lumberyard :).

Un collègue a récemment écrit un article sur le thème de la qualité des logiciels de jeux vidéo, je vous suggère de lire: " Analyse statique dans l'industrie du jeu vidéo: les 10 principales erreurs logicielles ".

Lien pour télécharger l'analyseur PVS-Studio , comment vous en passer ;-)

Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Svyatoslav Razmyslov. Amazon Lumberyard: un cri d'angoisse