Долгожданная проверка Unreal Engine 4

    Unreal Engine 4 and PVS-Studio

    19 марта 2014 года Unreal Engine 4 стал доступен для всех желающих. Цена подписки всего 19$ в месяц. Исходные коды также выложены на github репозиторий. С этого момента нам поступила масса сообщений на почту, в твиттер и так далее, с просьбой проверить этот игровой движок. Мы удовлетворяем просьбу наших читателей. Давайте посмотрим, что интересного можно найти в исходном коде с помощью статического анализатора кода PVS-Studio.



    Unreal Engine


    Unreal Engine — игровой движок, разрабатываемый и поддерживаемый компанией Epic Games. Написан на языке C++. Позволяет создавать игры для большинства операционных систем и платформ: Microsoft Windows, Linux, Mac OS и Mac OS X, консолей Xbox, Xbox 360, PlayStation 2, PlayStation Portable, PlayStation 3, Wii, Dreamcast и Nintendo GameCube.

    Официальный сайт: https://www.unrealengine.com/

    Описание на сайте Wikipedia: Unreal Engine.

    Способ проверки nmake-based проекта


    С проверкой проекта Unreal Engine не всё так просто. Чтобы проверить этот проект, нам пришлось воспользоваться новой возможностью, которую мы недавно реализовали в PVS-Studio Standalone. Из-за этого нам пришлось немного задержать публикацию статьи. Мы решили опубликовать её после релиза PVS-Studio, где уже будет эта новая возможность. Возможно, многим захочется ее попробовать. Она существенно облегчает проверку проектов, где используются сложные или нестандартные системы сборки.

    Классический принцип работы PVS-Studio таков:
    • Вы открываете проект в Visual Studio.
    • Нажимаете кнопку «проверить».
    • Плагин, интегрированный в Visual Studio, собирает всю необходимую информацию: какие файлы надо проверять, какие макросы использовать, где лежат заголовочные файлы и так далее.
    • Плагин запускает собственно анализатор и отображает полученные результаты.
    Нюанс в том, что Unreal Engine 4 — это nmake-based проект, поэтому проверить его с помощью плагина PVS-Studio нельзя.

    Поясню. Есть проект для среды Visual Studio. Но сборка осуществляется с помощью nmake. Это значит, что плагин не знает, какие файлы и с какими ключами компилируются. Соответственно, анализ невозможен. Вернее, анализ возможен, но процесс трудоёмок (смотри раздел документации: "Прямая интеграция анализатора в системы автоматизации сборки").

    И здесь на выручку приходит PVS-Studio Standalone! Он умеет работать в двух вариантах:
    1. Вы каким-то образом генерируете препроцессированные файлы, и он проверят уже их.
    2. Он отслеживает запуски компилятора и «подсматривает» всю необходимую информацию.
    Сейчас нас интересует именно второй сценарий использования. Вот как происходила проверка Unreal Engine:
    1. Запустили PVS-Studio Standalone.
    2. Нажали кнопку «Compiler Monitoring».
    3. Нажали на кнопку «Start Monitoring». Увидели, что включился режим наблюдения за вызовами компилятора.
    4. Открыли проект Unreal Engine в Visual Studio. Запустили сборку проекта. При этом в окне мониторинга видно, что перехватываются вызовы компилятора.
    5. Когда сборка в Visual Studio закончилась, мы нажали кнопку Stop Monitoring. После этого начал работу анализатор PVS-Studio.
    В результате, диагностические сообщения появляются в окне утилиты PVS-Studio Standalone.

    Hint. Для удобства, лучше использовать Visual Studio, а не встроенный в PVS-Studio Standalone редактор. Достаточно сохранить результаты анализа в файл, а затем открыть его из среды Visual Studio (Menu->PVS-Studio->Open/Save->Open Analysis Report).

    Всё это и многое другое описано в статье "PVS-Studio теперь поддерживает любую сборочную систему на Windows и любой компилятор. Легко и «из коробки». Прошу обязательно прочитать эту статью, прежде чем начинать эксперименты с PVS-Studio Standalone!

    Результаты проверки


    Код проекта Unreal Engine показался мне очень качественным. Например, разработчики используют при разработке статический анализ кода. Об этом говорят такие фрагменты в коде, как:
    // Suppress static code analysis warning about a
    // potential comparison of two constants
    CA_SUPPRESS(6326);
    ....
    // Suppress static code analysis warnings about a
    // potentially ill-defined loop. BlendCount > 0 is valid.
    CA_SUPPRESS(6294)
    ....
    #if USING_CODE_ANALYSIS

    Судя по этим фрагментам кода, используется статический анализатор кода, встроенный в Visual Studio. Об этом анализаторе кода: Visual Studio 2013 Static Code Analysis in depth: What? When and How?

    Возможно, используются и другие анализаторы, но я про это ничего не знаю.

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

    Опечатки


    static bool PositionIsInside(....)
    {
      return
        Position.X >= Control.Center.X - BoxSize.X * 0.5f &&
        Position.X <= Control.Center.X + BoxSize.X * 0.5f &&
        Position.Y >= Control.Center.Y - BoxSize.Y * 0.5f &&
        Position.Y >= Control.Center.Y - BoxSize.Y * 0.5f;
    }

    Предупреждение PVS-Studio: V501 There are identical sub-expressions 'Position.Y >= Control.Center.Y — BoxSize.Y * 0.5f' to the left and to the right of the '&&' operator. svirtualjoystick.cpp 97

    Обратите внимание, что переменная «Position.Y» два раза сравнивается с выражением «Control.Center.Y — BoxSize.Y * 0.5f». Это явно опечатка. В последней строчке следует заменить оператор '-' на оператор '+'.

    Ещё одна схожая опечатка в условии:
    void FOculusRiftHMD::PreRenderView_RenderThread(
      FSceneView& View)
    {
      ....
      if (View.StereoPass == eSSP_LEFT_EYE ||
          View.StereoPass == eSSP_LEFT_EYE)
      ....
    }

    Предупреждение PVS-Studio: V501 There are identical sub-expressions 'View.StereoPass == eSSP_LEFT_EYE' to the left and to the right of the '||' operator. oculusrifthmd.cpp 1453

    Видимо, работа с Oculus Rift пока не очень оттестирована.

    Продолжим.
    struct FMemoryAllocationStats_DEPRECATED
    {
      ....
      SIZE_T  NotUsed5;
      SIZE_T  NotUsed6;
      SIZE_T  NotUsed7;
      SIZE_T  NotUsed8;
      ....
    };
    
    FMemoryAllocationStats_DEPRECATED()
    {
      ....
      NotUsed5 = 0;
      NotUsed6 = 0;
      NotUsed6 = 0;  
      NotUsed8 = 0;  
      ....
    }

    Предупреждение PVS-Studio: V519 The 'NotUsed6' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 86, 88. memorybase.h 88

    Инициализируются члены структуры. Из-за опечатки, член 'NotUsed6' инициализируется дважды. А член 'NotUsed7' остался неинициализированным. Впрочем, суффикс _DEPRECATED() в названии функции говорит, что это уже неинтересный код.

    Ещё пара мест, где одной переменной дважды присваивается значение:
    • V519 The 'HighlightText' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 204, 206. srichtextblock.cpp 206
    • V519 The 'TrackError.MaxErrorInScaleDueToScale' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1715, 1716. animationutils.cpp 1716

    Нулевые указатели


    Очень часто я встречаю разыменование нулевого указателя в обработчиках ошибок. Это не удивительно. Такие фрагменты сложно и не интересно тестировать. Встретить разыменование нулевого указателя в обработчике ошибок можно и в проекте Unreal Engine:
    bool UEngine::CommitMapChange( FWorldContext &Context )
    {
      ....
      LevelStreamingObject = Context.World()->StreamingLevels[j];
      if (LevelStreamingObject != NULL)
      {
        ....
      }
      else
      {
        check(LevelStreamingObject);
        UE_LOG(LogStreaming, Log,
               TEXT("Unable to handle streaming object %s"),
               *LevelStreamingObject->GetName());
      }
      ....
    }

    Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'LevelStreamingObject' might take place. unrealengine.cpp 10768

    Если произошла ошибка, то хочется распечатать имя объекта. Вот только не существует этого объекта.

    Рассмотрим другой фрагмент, где будет разыменован нулевой указатель. Здесь всё интереснее. Возможно, ошибка присутствует из-за неправильного merge. В любом случае, по комментарию видно, что код не доделан:
    void FStreamingPause::Init()
    {
      ....
      if( GStreamingPauseBackground == NULL && GUseStreamingPause )
      {
        // @todo UE4 merge andrew
        // GStreamingPauseBackground = new FFrontBufferTexture(....);
        GStreamingPauseBackground->InitRHI();
      }
    }

    Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'GStreamingPauseBackground' might take place. streamingpauserendering.cpp 197

    Ещё о нулевых указателях


    Почти в любой программе я встречаю массу предупреждений с кодом V595 (примеры). Эти предупреждения говорят о следующей ситуации:

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

    Диагностика V595 позволяет выявлять вот такие ляпы:
    /**
     * Global engine pointer.
     * Can be 0 so don't use without checking.
     */
    ENGINE_API UEngine* GEngine = NULL;
    
    bool UEngine::LoadMap( FWorldContext& WorldContext,
      FURL URL, class UPendingNetGame* Pending, FString& Error )
    {
      ....
      if (GEngine->GameViewport != NULL)
      {
        ClearDebugDisplayProperties();
      }
    
      if( GEngine )
      {
        GEngine->WorldDestroyed( WorldContext.World() );
      }
      ....
    }

    Предупреждение PVS-Studio: V595 The 'GEngine' pointer was utilized before it was verified against nullptr. Check lines: 9714, 9719. unrealengine.cpp 9714

    Обратите внимание на комментарий. Глобальная переменная GEngine может быть равна нулю. Прежде чем её использовать, её нужно проверить.

    В функции LoadMap() действительно есть такая проверка:
    if( GEngine )

    Только вот незадача. Проверка расположена уже после того, как указателем попользовались:
    if (GEngine->GameViewport != NULL)

    Предупреждений V595 довольно много (82 штуки). Думаю, многие из них окажутся ложными. Поэтому не буду захламлять статью сообщениями и приведу их отдельным списком: ue-v595.txt.

    Лишнее объявление переменной


    Рассмотрим красивую ошибку. Она связана с тем, что случайно создаётся новая переменная, а не используется старая.
    void FStreamableManager::AsyncLoadCallback(....)
    {
      ....
      FStreamable* Existing = StreamableItems.FindRef(TargetName);
      ....
      if (!Existing)
      {
        // hmm, maybe it was redirected by a consolidate
        TargetName = ResolveRedirects(TargetName);
        FStreamable* Existing = StreamableItems.FindRef(TargetName);
      }
      if (Existing && Existing->bAsyncLoadRequestOutstanding)
      ....
    }

    Предупреждение PVS-Studio: V561 It's probably better to assign value to 'Existing' variable than to declare it anew. Previous declaration: streamablemanager.cpp, line 325. streamablemanager.cpp 332

    Как я понимаю, на самом деле, должно быть написано так:
    // hmm, maybe it was redirected by a consolidate
    TargetName = ResolveRedirects(TargetName);
    Existing = StreamableItems.FindRef(TargetName);

    Ошибки при вызове функций


    bool FRecastQueryFilter::IsEqual(
      const INavigationQueryFilterInterface* Other) const
    {
      // @NOTE: not type safe, should be changed when
      // another filter type is introduced
      return FMemory::Memcmp(this, Other, sizeof(this)) == 0;
    }

    Предупреждение PVS-Studio: V579 The Memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. pimplrecastnavmesh.cpp 172

    Комментарий предупреждает, что использовать Memcmp() опасно. Но, на самом деле, всё ещё хуже, чем думает программист. Дело в том, что функция сравнивает только часть объекта.

    Оператор sizeof(this) возвращает размер указателя. То есть в 32-битной программе функция сравнит первые 4 байта. В 64-битной программе будут сравниваться 8 байт.

    Правильный код:
    return FMemory::Memcmp(this, Other, sizeof(*this)) == 0;

    На этом злоключения с функцией Memcmp() не заканчиваются. Рассмотрив следующий фрагмент кода:
    D3D11_STATE_CACHE_INLINE void GetBlendState(
      ID3D11BlendState** BlendState, float BlendFactor[4],
      uint32* SampleMask)
    {
      ....
      FMemory::Memcmp(BlendFactor, CurrentBlendFactor,
                      sizeof(CurrentBlendFactor));
      ....
    }

    Предупреждение PVS-Studio: V530 The return value of function 'Memcmp' is required to be utilized. d3d11statecacheprivate.h 547

    Анализатор удивился, увидев, что результат работы функции Memcmp() никак не используется. И действительно, это ошибка. Как я понимаю, здесь хотели копировать, а не сравнивать данные. Тогда надо использовать функцию Memcpy():
    FMemory::Memcpy(BlendFactor, CurrentBlendFactor,
                    sizeof(CurrentBlendFactor));

    Переменная присваивается сама себе


    enum ECubeFace;
    ECubeFace CubeFace;
    
    friend FArchive& operator<<(
      FArchive& Ar,FResolveParams& ResolveParams)
    {
      ....
      if(Ar.IsLoading())
      {
        ResolveParams.CubeFace = (ECubeFace)ResolveParams.CubeFace;
      }
      ....
    }

    Предупреждение PVS-Studio: V570 The 'ResolveParams.CubeFace' variable is assigned to itself. rhi.h 1279

    Переменная 'ResolveParams.CubeFace' имеет тип ECubeFace. Используется явное приведение переменной к типу ECubeFace. То есть ничего не происходит. После чего переменная присваивается сама себе. С этим кодом что-то не так.

    Самая красивая из найденных ошибок


    Like

    Больше всего, мне понравилась вот эта ошибка:
    bool VertInfluencedByActiveBone(
      FParticleEmitterInstance* Owner,
      USkeletalMeshComponent* InSkelMeshComponent,
      int32 InVertexIndex,
      int32* OutBoneIndex = NULL);
    
    void UParticleModuleLocationSkelVertSurface::Spawn(....)
    {
      ....
      int32 BoneIndex1, BoneIndex2, BoneIndex3;
      BoneIndex1 = BoneIndex2 = BoneIndex3 = INDEX_NONE;
    
      if(!VertInfluencedByActiveBone(
            Owner, SourceComponent, VertIndex[0], &BoneIndex1) &&
         !VertInfluencedByActiveBone(
            Owner, SourceComponent, VertIndex[1], &BoneIndex2) && 
         !VertInfluencedByActiveBone(
            Owner, SourceComponent, VertIndex[2]) &BoneIndex3)
      {
      ....
    }

    Предупреждение PVS-Studio: V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. particlemodules_location.cpp 2120

    Заметить ошибку не просто. Я уверен, что вы бегло просмотрели код и не увидели в нём ничего подозрительного. Предупреждение анализатора, к сожалению, тоже странное и кажется ложным срабатыванием. А между тем, мы имеем дело с очень интересной ошибкой.

    Давайте разберёмся. Обратите внимание, что последний аргумент функции VertInfluencedByActiveBone() является необязательным.

    В коде функция VertInfluencedByActiveBone() вызывается 3 раза. Два раза ей передаётся 4 аргумента. В последнем случае аргументов всего 3. В этом и есть ошибка.

    Благодаря везению, код компилируется, и ошибка незаметна. Вот что здесь получается:
    1. Вызывается функция с 3 аргументами: «VertInfluencedByActiveBone(Owner, SourceComponent, VertIndex[2])»;
    2. К результату функции применяется оператор '!';
    3. Результат выражения "!VertInfluencedByActiveBone(...)" имеет тип bool;
    4. К результату применяется оператор '&' (bitwise AND);
    5. Всё успешно компилируется, так как слева от оператора '&' находится выражение типа bool. Справа от '&' находится целочисленная переменная BoneIndex3.
    Анализатор заподозрил неладное, когда увидел, что один из аргументов оператора '&' является тип 'bool'. Об этом он сообщил. И не зря.

    Чтобы исправить ошибку, нужно добавить запятую и разместить закрывающуюся скобку в другом месте:
    if(!VertInfluencedByActiveBone(
          Owner, SourceComponent, VertIndex[0], &BoneIndex1) &&
       !VertInfluencedByActiveBone(
          Owner, SourceComponent, VertIndex[1], &BoneIndex2) && 
       !VertInfluencedByActiveBone(
          Owner, SourceComponent, VertIndex[2], &BoneIndex3))

    Забыт оператор break


    static void VerifyUniformLayout(....)
    {
      ....
      switch(Member.GetBaseType())
      {
        case UBMT_STRUCT:  BaseTypeName = TEXT("struct"); 
        case UBMT_BOOL:    BaseTypeName = TEXT("bool"); break;
        case UBMT_INT32:   BaseTypeName = TEXT("int"); break;
        case UBMT_UINT32:  BaseTypeName = TEXT("uint"); break;
        case UBMT_FLOAT32: BaseTypeName = TEXT("float"); break;
        default:           
          UE_LOG(LogShaders, Fatal,
            TEXT("Unrecognized uniform ......"));
      };
      ....
    }

    Предупреждение PVS-Studio: V519 The 'BaseTypeName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 862, 863. openglshaders.cpp 863

    В самом начале забыт оператор «break;». Думаю, комментарии и пояснения излишни.

    Микрооптимизации


    В анализаторе PVS-Studio имеется небольшой набор диагностических правил, которые помогают провести небольшие оптимизации в коде. Впрочем, иногда они весьма полезны. Рассмотрим в качестве примера один из операторов присваивания:
    FVariant& operator=( const TArray<uint8> InArray )
    {
      Type = EVariantTypes::ByteArray;
      Value = InArray;
      return *this;
    }

    Предупреждение PVS-Studio: V801 Decreased performance. It is better to redefine the first function argument as a reference. Consider replacing 'const… InArray' with 'const… &InArray'. variant.h 198

    Не очень хорошая идея передавать массив по значению. Массив 'InArray' можно и нужно передавать, используя константную ссылку.

    Анализатор выдаёт достаточно много предупреждений, связанных с микрооптимизациями. Далеко не все из них окажутся полезны, но на всякий случай приведу их списком: ue-v801-V803.txt.

    Подозрительная сумма


    uint32 GetAllocatedSize() const
    {
      return UniformVectorExpressions.GetAllocatedSize()
        + UniformScalarExpressions.GetAllocatedSize()
        + Uniform2DTextureExpressions.GetAllocatedSize()
        + UniformCubeTextureExpressions.GetAllocatedSize()
        + ParameterCollections.GetAllocatedSize()
        + UniformBufferStruct
            ?
            (sizeof(FUniformBufferStruct) +
             UniformBufferStruct->GetMembers().GetAllocatedSize())
            :
            0;
    }

    Предупреждение PVS-Studio: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. materialshared.h 224

    Код весьма сложный. Чтобы пояснить, что не так, я составлю упрощенный искусственный пример:
    return A() + B() + C() + uniform ? UniformSize() : 0;

    Вычисляется некий размер. В зависимости от значения переменной 'uniform', следует прибавлять 'UniformSize()' или 0. На самом деле, этот код работает не так. Приоритет операторов сложения '+' выше, чем приоритет оператора '?:'.

    Вот, что получается:
    return (A() + B() + C() + uniform) ? UniformSize() : 0;

    Аналогичную ситуацию мы наблюдаем и в коде Unreal Engine. Мне кажется, вычисляется не то, что хотел программист.

    Путаница с enum


    Я вначале не хотел описывать этот случай, так как нужно привести достаточно большой фрагмент кода. Потом я всё-таки переборол лень. Прошу потерпеть и читателей.
    namespace EOnlineSharingReadCategory
    {
      enum Type
      {
        None          = 0x00,
        Posts         = 0x01,
        Friends       = 0x02,
        Mailbox       = 0x04,
        OnlineStatus  = 0x08,
        ProfileInfo   = 0x10,  
        LocationInfo  = 0x20,
        Default       = ProfileInfo|LocationInfo,
      };
    }
    
    namespace EOnlineSharingPublishingCategory
    {
      enum Type {
        None          = 0x00,
        Posts         = 0x01,
        Friends       = 0x02,
        AccountAdmin  = 0x04,
        Events        = 0x08,
        Default       = None,
      };
    
      inline const TCHAR* ToString
        (EOnlineSharingReadCategory::Type CategoryType)
      {
        switch (CategoryType)
        {
        case None:
        {
          return TEXT("Category undefined");
        }
        case Posts:
        {
          return TEXT("Posts");
        }
        case Friends:
        {
          return TEXT("Friends");
        }
        case AccountAdmin:
        {
          return TEXT("Account Admin");
        }
        ....
      }
    }

    Анализатор выдаёт здесь сразу несколько предупреждений V556. Дело в том, что аргументом оператора 'switch()' является переменная типа EOnlineSharingReadCategory::Type. При этом в операторах 'case' используются значения от другого типа EOnlineSharingPublishingCategory::Type.

    Логическая ошибка


    const TCHAR* UStructProperty::ImportText_Internal(....) const
    {
      ....
      if (*Buffer == TCHAR('\"'))
      {
        while (*Buffer && *Buffer != TCHAR('\"') &&
               *Buffer != TCHAR('\n') && *Buffer != TCHAR('\r'))
        {
          Buffer++;
        }
    
        if (*Buffer != TCHAR('\"'))
      ....
    }

    Предупреждение PVS-Studio: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 310, 312. propertystruct.cpp 310

    Здесь хотели пропустить всё, что содержится в двойных кавычках. Алгоритм должен был быть таким:
    • Если находим двойную кавычку, то запускаем цикл.
    • В цикле попускаем символы до тех пор, пока не встретим следующую двойную кавычку.
    Ошибка в том, что после нахождения первой двойной кавычки, мы не сдвигаем указатель на следующий символ. В результате, сразу находится вторая кавычка. Цикл не запускается.

    Поясню это на упрощенном коде:
    if (*p == '\"')
    {
      while (*p && *p != '\"')
          p++;
    }

    Чтобы исправить ошибку, надо сделать следующие изменения:
    if (*p == '\"')
    {
      p++;
      while (*p && *p != '\"')
          p++;
    }

    Подозрительный сдвиг


    class FMallocBinned : public FMalloc
    {
      ....
      /* Used to mask off the bits that have been used to
         lookup the indirect table */
      uint64 PoolMask;
      ....
      FMallocBinned(uint32 InPageSize, uint64 AddressLimit)
      {
        ....
        PoolMask = ( ( 1 << ( HashKeyShift - PoolBitShift ) ) - 1 );
        ....
      }
    }

    Предупреждение PVS-Studio: V629 Consider inspecting the '1 << (HashKeyShift — PoolBitShift)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. mallocbinned.h 800

    Есть здесь ошибка или нет, зависит от того, нужно ли сдвигать 1 более чем на 31 разряд. Так как результат помещается в 64-битную переменную PoolMask, видимо такая необходимость есть.

    Если я угадал, то в библиотеке имеется ошибка в подсистеме выделения памяти.

    Число 1 имеет тип int. Это значит, что сдвинуть 1, скажем на 35 разрядов, нельзя. Формально, возникнет неопределённое поведение (подробности). На практике, произойдёт переполнение и будет вычислено неправильное значение.

    Правильный код:
    PoolMask = ( ( 1ull << ( HashKeyShift - PoolBitShift ) ) - 1 );

    Устаревшие проверки


    void FOculusRiftHMD::Startup()
    {
      ....
      pSensorFusion = new SensorFusion();
      if (!pSensorFusion)
      {
        UE_LOG(LogHMD, Warning,
          TEXT("Error creating Oculus sensor fusion."));
        return;
      }
      ....
    }  

    Предупреждение PVS-Studio: V668 There is no sense in testing the 'pSensorFusion' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. oculusrifthmd.cpp 1594

    Уже давно, в случае ошибки выделения памяти, оператор 'new' генерирует исключение. Проверка «if (!pSensorFusion)» не нужна.

    В больших проектах я, как правило, встречаю очень много таких мест. На удивление, в Unreal Engine таких мест мало. Список: ue-V668.txt.

    Copy-Paste


    Следующие фрагменты кода, скорее всего, получились из-за использования методики Copy-Paste. Независимо от условия, выполняется одно и то же действие:
    FString FPaths::CreateTempFilename(....)
    {
      ....  
      const int32 PathLen = FCString::Strlen( Path );
      if( PathLen > 0 && Path[ PathLen - 1 ] != TEXT('/') )
      {
        UniqueFilename =
          FString::Printf( TEXT("%s/%s%s%s"), Path, Prefix,
                           *FGuid::NewGuid().ToString(), Extension );
      }
      else
      {
        UniqueFilename =
          FString::Printf( TEXT("%s/%s%s%s"), Path, Prefix,
                           *FGuid::NewGuid().ToString(), Extension );
      }
      ....
    }

    Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. paths.cpp 703

    Ещё один такой фрагмент кода:
    template< typename DefinitionType >            
    FORCENOINLINE void Set(....)
    {
      ....
      if ( DefinitionPtr == NULL )
      {
        WidgetStyleValues.Add( PropertyName,
          MakeShareable( new DefinitionType( InStyleDefintion ) ) );
      }
      else
      {
        WidgetStyleValues.Add( PropertyName,
          MakeShareable( new DefinitionType( InStyleDefintion ) ) );
      }
    }

    Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. slatestyle.h 289

    Разное


    Осталась разная мелочь. Описывать её не интересно. Просто приведу фрагменты кода и диагностические сообщения.
    void FNativeClassHeaderGenerator::ExportProperties(....)
    {
      ....
      int32 NumByteProperties = 0;
      ....
      if (bIsByteProperty)
      {
        NumByteProperties;
      }
      ....
    }

    Предупреждение PVS-Studio: V607 Ownerless expression 'NumByteProperties'. codegenerator.cpp 633
    static void GetModuleVersion( .... )
    {
      ....
      char* VersionInfo = new char[InfoSize];
      ....
      delete VersionInfo;
      ....
    }

    Предупреждение PVS-Studio: V611 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 [] VersionInfo;'. windowsplatformexceptionhandling.cpp 107
    const FSlateBrush* FSlateGameResources::GetBrush(
      const FName PropertyName, ....)
    {
      ....
      ensureMsgf(BrushAsset, TEXT("Could not find resource '%s'"),
                 PropertyName);
      ....
    }

    Предупреждение PVS-Studio: V510 The 'EnsureNotFalseFormatted' function is not expected to receive class-type variable as sixth actual argument. slategameresources.cpp 49

    Выводы


    Использовать статический анализатор, встроенный в Visual Studio, полезно, но не достаточно. Разумно использовать и специализированные инструменты, такие как наш анализатор PVS-Studio. Например, если сравнивать PVS-Studio и статический анализатора из VS2013, то PVS-Studio находит в 6 раз больше ошибок. Чтобы не быть голословным — «Методология сравнения». Приглашаю всех ценителей качественного кода попробовать наш анализатор кода.

    Эта статья на английском


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. A Long-Awaited Check of Unreal Engine 4.

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    1,158.87
    Static Code Analysis for C, C++, C# and Java
    Share post

    Comments 65

      +10
      Сегодня вышла PVS-Studio, где доступен описанный в статье мониторинг работы компилятора. Приглашаю скачивать и пробовать.
        –3
        А вы Epic рассказали о найденых у них ошибках?
        UPD:
        глупый вопрос — просто не проснулся, извиняюсь
          0
          В конце статьи есть ответ на ваш вопрос.
            +1
            Старый NDA не позволяет мне толком ответить на этот вопрос. Скажу только, что Epic знает о существовании анализатора PVS-Studio и что он умеет находить ошибки в программах.
              +17
              Да божечки, ну сколько можно? Да, они пишут разработчикам и прикладывают список ошибок. Да, они предлагают купить их аналазитор. Да, они делают скидки (а то и отдают нахаляву) для опенсорс проектов.
              • UFO just landed and posted this here
                  0
                  Комментарий в плюс, карма в минус… Простите-простите, не ною.
                +22
                В последней строчке следует заменить оператор '-' на оператор '+'.

                Там еще и знак неравенства надо сменить.
                Position.Y <= Control.Center.Y + BoxSize.Y * 0.5f
                
                  –1
                  Да, точно! Ну да ладно, не буду править статью.
                    0
                    «Акела промахнулся!»
                      +9
                      Странное решение
                        +4
                        Не странное. Если-бы написали в почту, я бы тихо поправил. А так получится, что комментарий будет не в тему. Вот я и оставил всё как есть. На родном сайте, конечно поправил. Не бойтесь, авторы UE прочитают уточнённый вариант статьи.
                          +2
                          Тот кто прочитает комментарий, прочитает и следующие.
                          Ну и тех, кто прочитает статью, но не будет читать комментарии — гораздо больше.
                            +1
                            Странная позиция. Комментарий ведь явно писался с целью улучшить статью. А вы тут — бац! — не буду ничего править, ибо автор останется в дураках. Да не останется он в дураках. У него и цитата приведена, что явно доказывает его комментарий. Плюс, люди же на этом сайте не дураки и если человек внимательно читал статью и комментарии, то он сразу поймёт, что тут творилась история! Ну, а тем, кто выхватил коммент из контекста, уже ничего не поможет, ИМХО.
                              +4
                              Ф. М. Достоевский в одном из романов написал
                              В центре комнаты стоял круглый стол овальной формы

                              Дотошные ученики указали мастеру на ошибку. Достоевский глянул, пожевал губами и сказал:
                              -Оставьте так…
                                –3
                                Вы знаете, я бы прошёл мимо, если бы не чёткий посыл во всех постах уважаемого Andrey2008. Правда, мне очень нравятся его посты и дело, что он и его команда делает, очень полезное и нужное, его всегда интересно читать. Тем нелепее выглядит ситуация, когда человек, ратующий за досрочное исправление ошибок и неточностей в чужом коде (путём использования статического анализатора), встаёт в позу и говорит — здесь в статье поправлять не будем потому что, дескать, комментарий станет «не в тему». Тем более на родном сайте, неточность, судя по всему поправили. Мне правда не понятна мотивация. Какие-то двойные стандарты получаются, господа.
                      –10
                      Это когда Linux стал поддерживаемой UE платформой?
                      Они то Mac со скрипом поддерживать стали, а под андроид с UDK нельзя было ничего не сделать.
                      +20
                      Спасибо за статью. Хочу обратить внимание на один момент:

                      Вначале указатель разыменовывается. Ниже этот указатель проверяется на равенство нулю. Это далеко не всегда ошибка. Но это очень подозрительный код, и его надо проверить!
                      Дело в том, что подобная конструкция может привести к совершенно неожиданным последствиям. Советую почитать статью из блога разработчиков LLVM где эта ситуация разбирается подробно.

                      Вкратце: оптимизатор компилятора может вырезать код проверки (а следовательно и целую ветвь программы) как «мертвый» код. Одно из соображений, которым может руководствоваться оптимизатор — проверка на NULL разадресованного ранее указателя является избыточной и может быть удалена.

                      Опасность этой ситуации в том, что она зависит от используемого компилятора (и даже его версии). В любом случае, опасность гораздо выше, чем может показаться.
                        +1
                        Ну так компилятор будет прав — если NULL, так значит уже все упало и сюда никак не дошло. Поведение не меняется.
                        +14
                        Почитаешь такое, и начинаешь себя вроде минера чувствовать во время написания кода: вроде все делаешь так, а потом посмотрят умные люди на твои труды, и увидят все эти & вместо && и прочее — как раз то, что ты искренне считал нормальным кодом!

                        Нужно включать еще в конце проверки психологическую помощь программисту: мол, «у Вас на 1000 строк кода всего 18 ошибок, из них 2 — нетривиальные — Вы молодец! Хотите твитнуть это?».
                          +1
                          Мы давно уже думаем над тем, как похвалить программиста. Идеи крутятся вокруг чего-то вроде как Вы пишите про 2 нетривиальные, но пока что-то окончательно мысль не сформулировалась.
                            +2
                            Главное чтобы пользователи не стали соревноваться в плане самой нетривиальной ошибки :)
                              0
                              Эмммм? Мне кажется, в эту статью народ как раз и приходит, чтобы посмотреть «Хммм, что же они там нетривиального навыдумывали?».
                                0
                                У меня пвс нашел нетривиальную ошибку
                                int i=k<<-2;
                                

                                Исход операции неясен.
                                  0
                                  Так по стандарту сказано, что сдвиг на отрицательные количество бит, равно как и сдвиг отрицательного числа — это UB. Причем именно undefined а не unspecified.

                                  Поэтому это имхо вполне тривиальная ошибка. Если подобное выражение получилось в результате разворачивания макросов (как обычно и бывает), это еще можно назвать таковой, но с натяжкой :)
                                0
                                Надо сделать, как в играх: результат отправляется на сервер, и в зависимости от него изменяется общий рейтинг программиста.
                                Ну, и, конечно, «достижения»:
                                «novice»: 1 ошибка на каждую строчку кода
                                «hindu»: 10 нетривиальных ошибок
                                «tester killer»: сделать одну и ту же ошибку 10 раз
                                etc.
                                  +2
                                  Серьезно, ребят, напишите кто-нибудь статью с продуманной полной идеей. Лучшие варианты реализуем! :-)
                            • UFO just landed and posted this here
                                +1
                                Скорее всего они проверили его первым делом. На официальном сайте, в списке выявленных ошибок, не нашел.
                                  +6
                                  Ответы на вопросы читателей.

                                  Огромный баннер в конце статьи что-ли вставлять… :)
                                    +4
                                    Баннер не надо. Я как-то не смог в бета-тестеры одной программы записаться — они вывесили огромный баннер наверху главной страницы (и без текстовой новости), а мозг его умело вырезал. Может попробовать подвал в шапку перенести «Не прочитали статью и уже есть вопросы»?
                                0
                                Предупреждений V595 довольно много (82 штуки).

                                Удивительно, как оно (не только UE, но и остальной софт) не падает на каждом шагу с ошибками в выделении памяти, sizeof, работе с нулевыми указателями… Особенно учитывая, что они попадаются не том коде, который вызывается редко, а в основополагающих вещах вроде рассматриваемого LoadMap().
                                  +2
                                  Всё дело в том, что обычно память выделяется, файлы читаются и так далее. А если что-то не то, то вместо адекватного поведения, программа аварийное завершается. Думаю, все наблюдали такую ситуацию: в какой-то программе вы что-то случайно сделали нестандартно, и она вместо «не могу открыть файл» тихо закрывается. Это и есть проявление таких ошибок.
                                    +3
                                    Ничего удивительного — это игровой движок. Нулевые указатели, это в 99% ошибки, когда, скажем система отказалась выделять память. Или отсутствует файл из ресурсов который там должен быть, или еще что-то такое. В таких случаях упасть для игры самое правильное решение. Когда она упадет, стек дамп отправится разработчику, они посмотрят, и поймут что ошибка совсем в другом месте (т.п. пользователь решил подмастерить ресурсы игры или памяти не хватает). А что тогда надо делать? Правильно, падать. Я отнюдь не говорю что такие ошибки не нужно править. Я только пытаюсь пояснить что поправить большинство nullptr ошибок ничего не даст. Если память не выделили, или ресурс не доступен, то проверять это мало как поможет.
                                    С другой стороны, есть куча других ошибок которые было бы очень круто починить, особенно с проверкой «точка в коробке» или что кости анимации в порядке. Такие опечатки действительно приводят к ошибкам вроде размазанных по пол карте персонажей или еще каким-то малоприятным багам.
                                    P.S. Сам разрабатываю уже давно игры на C++ в больших компаниях, с сорсами больших движков и своих собственных.
                                      +4
                                      Вот только тогда вопрос, а зачем ниже по коду проверка указателей? Тогда надо вообще никаких проверок не делать. Разыменовали, упали, profit. Раз есть проверка, значит хотели как-то этот случай обработать.
                                        +1
                                        Ну, не весь код пишется за один раз одним человеком. Я ведь не говорю «это не ошибка». Это конечно же ошибки. Я просто отвечаю на вопрос «как такой софт вообще работает». Кстате, в геймдеве такие ошибки почти всегда чинятся в редакторе уровней, где кто-то забыл присоединить объект к актору, или как-то так. Это как бы реальный баг, а то что упало на отсутствующем наллчеке, ну, это уже следсвие. Конечно, будет клево если перед смертью выдаст в лог чего конкретно не хватает, но, обычно код проверки ошибок это то о чем заботятся очень мало. Тут очень хорошо видна специфика разработки игр, здесь и приоритеты сильно другие. Но это совсем другая история.
                                        0
                                        С другой стороны, есть куча других ошибок которые было бы очень круто починить, особенно с проверкой «точка в коробке» или что кости анимации в порядке. Такие опечатки действительно приводят к ошибкам вроде размазанных по пол карте персонажей или еще каким-то малоприятным багам.

                                        Тоже хороший пример, кстати, это довольно часто используемая вещь, и с ошибкой, однако же движок как-то работает…
                                    • UFO just landed and posted this here
                                        +1
                                        За найденные баги авторам дадут пожизненную подписку на UE?
                                          +10
                                          Прочитал как: «За найденные баги» авторам дадут пожизненно".
                                            0
                                            — Госдума РФ
                                          +2
                                          Было бы интересно, если бы вам дали протестировать какой-нибудь… скажем, код ядерной электростанции :) Предвижу: «Так, а здесь из-за неправильной скобки выезжают графитовые стержни» или «А здесь из-за выхода за пределы массива разогревается охладитель». Кстати, неплохо было бы и отечественной космической промышленности так помочь — глядишь и спутники бы до орбиты добирались. К вам не обращались из подобных организаций, если не секрет?
                                            +8
                                            Нет. И не обратятся. Наш анализатор несертифицированный.
                                              +1
                                              Я, конечно, понимаю, что я полный ламер в таких вопросах, но зачем сертифицировать то, что явно не используется? Исправления вносит «сертифицированный» разработчик, а откуда он узнал о том, что исправлять: сам поставил 100 экспериментов, приснилось или подсказал анализатор — какая разница? Да и заказчик никаким образом не сможет достоверно узнал, что анализатор таки использовался (а интернет и так на всех машинах перекрыт, так что код спёрт не будет).
                                                0
                                                Всё равно нехорошо. Вдруг PSV код отошлёт разработчикам, а то и американцам? Ладно, тут такого нет, но этому «разработчику» может «присниться», что он фрагмент кода отослал на stackoverflow, а там явный баг с системой запуска электростанции, который теперь в паблике.
                                                  +4
                                                  Мир сертифицированных программ, это другой параллельный мир магии, живущий по иным физическим законом. У нас нет туда доступа. Чтобы посетить этот параллельный мир, нужно много денег и различных магических артефактов, таких как лицензия от ФСБ на секретность и так далее.
                                                  И кстати, даже если мы его посетим, то на Хабре всё равно про это не расскажешь и куски кода не опубликуешь. :)
                                                    0
                                                    это, конечно, да, но какой-нибудь Фобос-грунт независимый космический аппарат, может быть и получилось бы :)
                                                +1
                                                На АЭС всё до сих пор дублируется релейной защитой, так что всё ок. Ну и вообще для таких задач софт разрабатывается по совсем иным принципам — на Хабре была статья о разработке софта для Шаттла, кажется.

                                                Контрпример: embeddedgurus.com/barr-code/2013/10/an-update-on-toyota-and-unintended-acceleration/
                                                Кстати, один из факторов, из-за которых случилась ЧАЭС, был такой. Выбросы мощности при сработке АЗ-5 на низком ОЗР (что послужило начальным событием аварии) происходили и регистрировались и ранее на нескольких станциях. Но их посчитали глюками измерительной аппаратуры на переходных режимах, да и просто не хотели поднимать шум и начинать исследования. Много лет реакторы работали надежно, пока в одном случае физические параметры аппарата не сложились таким образом, что этот эффект привел к аварии… Сейчас (если вдруг) все эти недоработки устранены.
                                                  +1
                                                  Это все понятно, но, согласитесь, было бы интересно посмотреть код отдельного модуля, неужели там будет 0 ошибок? Не обязательно АЭС, какие-нибудь критически важные программы регулировки дорожного движения или систем самолета.
                                                    0
                                                    Соглашусь, интересно :) И, уверен, какое-то количество ошибок там есть.

                                                    Очень рекомендую погуглить на тему этого разбирательства с Тойотой. Там был целый отчет с примерами кода, на коирпый у меня, к сожалению, потерялась ссылка.
                                                      +2
                                                      Хм… Где-то я это видел тоже… www.viva64.com/ru/a/0083/
                                                        0
                                                        Вот там есть ссылка на большой PDF, а еще была краткая выжимка из него страниц в 30, презентация.
                                                    +2
                                                    > на Хабре была статья о разработке софта для Шаттла, кажется.

                                                    Ага, только что-то не находится… Можно обратиться к англоязычным источникам history.nasa.gov/computers/Ch4-5.html ntrs.nasa.gov/archive/nasa/casi.ntrs.nasa.gov/19930019745.pdf

                                                    > Ну и вообще для таких задач софт разрабатывается по совсем иным принципам

                                                    Например, согласно "EAL7: Formally Verified Design and Tested", "EAL6: Semiformally Verified Design and Tested", DO-178B/DO-178C. Ну или даже MISRA…
                                                  0
                                                  deleted
                                                    +3
                                                    Хочу такое для JS!
                                                      0
                                                      Теперь ждём проверки OpenSSL
                                                        0
                                                        О одной из многочисленных тем писали, что статический анализатор эту дырку не нашёл бы.
                                                          0
                                                          Не хотел писать на эту тему, так как в общем то писать не про что. Не смог PVS-Studio найти эту дырку, как и другие анализаторы. Слишком сложная ситуация. Однако, было столько писем, что пришлось краткую заметку написать. Хотя бы для того, чтобы просто ссылкой на письма и комментарии отвечать. На днях опубликую.
                                                        0
                                                        Уже давно, в случае ошибки выделения памяти, оператор 'new' генерирует исключение

                                                        Есть такой момент, что на игровых консолях исключения обычно выключены.
                                                          0
                                                          А зачем тогда операторы throw в коде?
                                                          Пример:
                                                          HRESULT hr = DataBaseConnection.CreateInstance(__uuidof(ADODB::Connection));
                                                          if (FAILED(hr))
                                                          {
                                                            FWindowsPlatformMisc::CoUninitialize();
                                                            throw _com_error(hr);
                                                          }
                                                          

                                                            0
                                                            Ну, видимо для других платформ…
                                                          0
                                                          Продолжение истории: How the PVS-Studio Team Improved Unreal Engine's Code.

                                                        Only users with full accounts can post comments. Log in, please.