Долгожданная проверка CryEngine V

    В мае 2016 года немецкая компания Crytek решила опубликовать на Github исходный код игрового движка CryEngine V. Игровой движок написан на языке C++ и сразу привлёк внимание как сообщества open-source разработчиков, так и команду разработчиков статического анализатора PVS-Studio, выполняющую проверку качества кода открытых проектов. На CryEngine разных версий сделано много отличных игр от разных игровых студий, и теперь движок стал доступен ещё большему числу разработчиков. Статья содержит обзор ошибок, выявленных с помощью статического анализатора кода.

    Введение


    CryEngine — игровой движок, созданный немецкой компанией Crytek в 2002 году и первоначально используемый в шутере от первого лица Far Cry. На CryEngine разных версий сделано много отличных игр от разных игровых студий, которые лицензировали движок: Far Cry, Crysis, Entropia Universe, Blue Mars, Warface, Homefront: The Revolution, Sniper: Ghost Warrior, Armored Warfare, Evolve и многие другие. В марте 2016 года компания Crytek анонсировала выход своего нового движка CryEngine V и вскоре опубликовала исходный код на Github.

    Для проверки открытого исходного кода использовался статический анализатор PVS-Studio версии 6.05. Это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Единственный верный способ использования методологии статического анализа — регулярная проверка кода на компьютерах разработчиков и build-сервере. Но для демонстрации возможностей анализатора PVS-Studio мы выполняем разовые проверки open-source проектов и пишем обзорные статьи с описанием выявленных ошибок. Впрочем, если проект показался нам интересным, по пришествию одного-двух лет мы можем вновь проверить его. По сути такие повторные проверки ничем не отличаются от разовых, так как в коде накапливается много изменений.

    Для проверки выбираются как просто известные и популярные проекты, так и предложенные читателями по электронной почте. Поэтому среди игровых движков CryEngine V далеко не первый. Были проверены следующие движки:
    Также однажды был проверен CryEngine 3 SDK.

    Особое внимание хочу уделить проверке игрового движка Unreal Engine 4. На примере этого проекта мы смогли в подробностях рассказать о правильном применении методологии статического анализа на реальном проекте: от внедрения анализатора в проект до сведения количества предупреждений до нуля с последующим контролем за появлением ошибок на новом коде. Наша работа с проектом Unreal Engine 4 вылилась в сотрудничество с компанией Epic Games, в рамках которого команда PVS-Studio исправила все предупреждения анализатора в исходном коде движка, и была написана совместная статья о проделанной работе, которая опубликована в Unreal Engine Blog (ссылка на русский перевод). Также компания Epic Games приобрела лицензию PVS-Studio для самостоятельного контроля качества кода. К аналогичному сотрудничеству мы готовы и с компанией Crytek.

    Структура отчёта анализатора


    В этой статье я хочу ответить на несколько часто задаваемых вопросов, связанных с количеством предупреждений и ложных срабатываний. Например, — «Сколько процентов составляют ложные срабатывания?» — или — «Почему в таком большом проекте так мало ошибок?».

    Для начала, все предупреждения анализатора PVS-Studio делятся на три уровня важности: High, Medium и Low. Так High уровень содержит высоко критичные предупреждения, с наибольшей вероятностью являющиеся реальными ошибками, а Low уровень — предупреждения с низкой критичностью, либо предупреждения с очень высокой вероятностью ложно-позитивного срабатывания. Стоит помнить, что конкретный код ошибки не обязательно привязывает её к определённому уровню важности, а распределение сообщений по группам сильно зависит от контекста, в котором они были сгенерированы.

    В проекте CryEngine V предупреждения анализатора общего назначения (General Analysis) распределены по уровням важности следующим образом:
    • High: 576 предупреждений;
    • Medium: 814 предупреждений,
    • Low: 2942 предупреждения.

    На рисунке 1 изображено распределение предупреждений по уровням важности на круговой диаграмме.



    Рисунок 1 — Распределение предупреждений по уровням важности в процентном отношении

    В статью невозможно уместить описание всех предупреждений и показать соответствующие фрагменты кода. Обычно статья содержит 10-40 примеров ошибок с описанием. Некоторые подозрительные места кода приводятся просто списком. Большинство предупреждений остаются нерассмотренными. В лучшем случае, после уведомления разработчиков, они спрашивают полный отчёт анализатора для детального изучения. Горькая правда заключается в том, что в большинстве проверяемых нами проектов, материала для статьи более чем достаточно после просмотра только предупреждений уровня High. И игровой движок CryEngine V — не исключение. На рисунке 2 представлена структура предупреждений уровня High.



    Рисунок 2 — Описание предупреждений High уровня

    Теперь более подробно о секторах представленной круговой диаграммы:
    • Described in the article (6%) — сообщения анализатора, приведённые в статье с фрагментами кода и описанием.
    • Presented list (46%) — сообщения анализатора, приведённые в статье списком. Такие предупреждения очень похожи на какой-нибудь случай, который уже подробно описан в статье, поэтому просто указывается информация о предупреждении.
    • False Positive (8%) — некоторый процент ложных срабатываний был выписан для доработки анализатора в будущем.
    • Skipped (40%) — все остальные сообщения анализатора. В них входят предупреждения, которые не удалось уместить в статье, не являющиеся критичными, не очень интересные, либо встретившиеся на участках кода, в которых сложно разобраться человеку не из команды разработчиков проверяемого проекта. Как показала работа с Unreal Engine 4, на практике такой код «пахнет» и все предупреждения всё равно исправляются.

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


    Обидный copy-paste




    V501 There are identical sub-expressions to the left and to the right of the '-' operator: q2.v.z — q2.v.z entitynode.cpp 93
    bool
    CompareRotation(const Quat& q1, const Quat& q2, float epsilon)
    {
      return (fabs_tpl(q1.v.x - q2.v.x) <= epsilon)
          && (fabs_tpl(q1.v.y - q2.v.y) <= epsilon)
          && (fabs_tpl(q2.v.z - q2.v.z) <= epsilon) // <=
          && (fabs_tpl(q1.w - q2.w) <= epsilon);
    }

    Опечатка в одной цифре, пожалуй, одна из самых обидных опечаток, которую может допустить программист. В этой функции анализатор обнаружил подозрительное выражение (q2.v.z — q2.v.z), в котором с большой вероятностью перепутали переменные q1 и q2.

    V501 There are identical sub-expressions '(m_eTFSrc == eTF_BC6UH)' to the left and to the right of the '||' operator. texturestreaming.cpp 919
    //! Texture formats.
    enum ETEX_Format : uint8
    {
      ....
      eTF_BC4U,     //!< 3Dc+.
      eTF_BC4S,
      eTF_BC5U,     //!< 3Dc.
      eTF_BC5S,
      eTF_BC6UH,
      eTF_BC6SH,
      eTF_BC7,
      eTF_R9G9B9E5,
      ....
    };
    
    bool CTexture::StreamPrepare(CImageFile* pIM)
    {
      ....
      if ((m_eTFSrc == eTF_R9G9B9E5) ||
          (m_eTFSrc == eTF_BC6UH) ||     // <=
          (m_eTFSrc == eTF_BC6UH))       // <=
      {
        m_cMinColor /= m_cMaxColor.a;
        m_cMaxColor /= m_cMaxColor.a;
      }
      ....
    }

    Другой тип опечаток связан с копированием констант. В данном случае переменная m_eTFSrc два раза сравнивается с константой eTF_BC6UH, на месте которой должна быть любая другая, например, отличающая всего одной буквой — константа eTF_BC6SH.

    Ещё два похожих места в проекте:
    • V501 There are identical sub-expressions '(td.m_eTF == eTF_BC6UH)' to the left and to the right of the '||' operator. texture.cpp 1214
    • V501 There are identical sub-expressions 'geom_colltype_solid' to the left and to the right of the '|' operator. attachmentmanager.cpp 1004

    V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 266, 268. d3dhwshader.cpp 266
    int SD3DShader::Release(EHWShaderClass eSHClass, int nSize)
    {
      ....
      if (eSHClass == eHWSC_Pixel)
        return ((ID3D11PixelShader*)pHandle)->Release();
      else if (eSHClass == eHWSC_Vertex)
        return ((ID3D11VertexShader*)pHandle)->Release();
      else if (eSHClass == eHWSC_Geometry)                   // <=
        return ((ID3D11GeometryShader*)pHandle)->Release();  // <=
      else if (eSHClass == eHWSC_Geometry)                   // <=
        return ((ID3D11GeometryShader*)pHandle)->Release();  // <=
      else if (eSHClass == eHWSC_Hull)
        return ((ID3D11HullShader*)pHandle)->Release();
      else if (eSHClass == eHWSC_Compute)
        return ((ID3D11ComputeShader*)pHandle)->Release();
      else if (eSHClass == eHWSC_Domain)
        return ((ID3D11DomainShader*)pHandle)->Release()
      ....
    }

    Вот пример ленивого копирования каскада условных операторов, в одном из случаев которого забыли внести изменения в код.

    V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 970, 974. environmentalweapon.cpp 970
    void CEnvironmentalWeapon::UpdateDebugOutput() const
    {
      ....
      const char* attackStateName = "None";
      if(m_currentAttackState &                       // <=
         EAttackStateType_EnactingPrimaryAttack)      // <=
      {
        attackStateName = "Primary Attack";
      }
      else if(m_currentAttackState &                  // <=
              EAttackStateType_EnactingPrimaryAttack) // <=
      {
        attackStateName = "Charged Throw";
      }
      ....
    }

    В предыдущем коде была хоть какая-то вероятность того, что лишнее условие — это результат слишком большого количества копий кода и одна проверка осталась просто лишней. В этом фрагменте кода из-за идентичных условных выражений переменная attackStateName никогда не примет значение «Charged Throw».

    V519 The 'BlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1265, 1266. ccrydxgldevicecontext.cpp 1266
    void CCryDXGLDeviceContext::
    OMGetBlendState(...., FLOAT BlendFactor[4], ....)
    {
      CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState);
      if ((*ppBlendState) != NULL)
        (*ppBlendState)->AddRef();
      BlendFactor[0] = m_auBlendFactor[0];
      BlendFactor[1] = m_auBlendFactor[1];
      BlendFactor[2] = m_auBlendFactor[2]; // <=
      BlendFactor[2] = m_auBlendFactor[3]; // <=
      *pSampleMask = m_uSampleMask;
    }

    В коде проекта обнаружилась такая вот функция, в которой из-за опечатки в индексе пропущено заполнение элемента с индексом три: BlendFactor[3]. Это место было бы просто одним из интересных мест с опечаткой, если бы анализатор не обнаружил ещё 2 фрагмента, куда скопировали код с допущенной опечаткой:

    V519 The 'm_auBlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 904, 905. ccrydxgldevicecontext.cpp 905
    void CCryDXGLDeviceContext::
      OMSetBlendState(....const FLOAT BlendFactor[4], ....)
    {
      ....
      m_uSampleMask = SampleMask;
      if (BlendFactor == NULL)
      {
        m_auBlendFactor[0] = 1.0f;
        m_auBlendFactor[1] = 1.0f;
        m_auBlendFactor[2] = 1.0f;                   // <=
        m_auBlendFactor[2] = 1.0f;                   // <=
      }
      else
      {
        m_auBlendFactor[0] = BlendFactor[0];
        m_auBlendFactor[1] = BlendFactor[1];
        m_auBlendFactor[2] = BlendFactor[2];         // <=
        m_auBlendFactor[2] = BlendFactor[3];         // <=
      }
    
      m_pContext->SetBlendColor(m_auBlendFactor[0],
                                m_auBlendFactor[1],
                                m_auBlendFactor[2],
                                m_auBlendFactor[3]);
      m_pContext->SetSampleMask(m_uSampleMask);
      ....
    }

    Вот то самое место, где продолжают пропускать заполнение элемента с индексом '3'. На мгновение я даже подумал, что в этом был какой-то смысл, но эта мысль меня быстро покинула, когда в конце функции я увидел обращение ко всем четырём элементам массива m_auBlendFactor. Похоже в файле ccrydxgldevicecontext.cpp просто сделали несколько копий кода с опечаткой.

    V523 The 'then' statement is equivalent to the 'else' statement. d3dshadows.cpp 1410
    void CD3D9Renderer::ConfigShadowTexgen(....)
    {
      ....
      if ((pFr->m_Flags & DLF_DIRECTIONAL) ||
        (!(pFr->bUseHWShadowMap) && !(pFr->bHWPCFCompare)))
      {
        //linearized shadows are used for any kind of directional
        //lights and for non-hw point lights
        m_cEF.m_TempVecs[2][Num] = 1.f / (pFr->fFarDist);
      }
      else
      {
        //hw point lights sources have non-linear depth for now
        m_cEF.m_TempVecs[2][Num] = 1.f / (pFr->fFarDist);
      }
      ....
    }

    В заключение раздела про copy-paste привожу описание ещё одной интересной ошибки. Независимо от результата условного выражения, значение m_cEF.m_TempVecs[2][Num] всегда вычисляется по одной формуле. Судя по со соседнему коду этого фрагмента, здесь нет опечатки в индексе, в этом условии хотят заполнить именно элемент с индексом '2'. А вот формулу расчёта, скорее всего, хотели использовать разную, но после копирования строки забыли изменить код.

    Проблемы с инициализацией




    V546 Member of a class is initialized by itself: 'eConfigMax(eConfigMax)'. particleparams.h 1013
    ParticleParams() :
      ....
      fSphericalApproximation(1.f),
      fVolumeThickness(1.0f),
      fSoundFXParam(1.f),
      eConfigMax(eConfigMax.VeryHigh), // <=
      fFadeAtViewCosAngle(0.f)
    {}

    Анализатор обнаружил возможную опечатку, когда поле класса инициализируется с использованием собственного значения.

    V603 The object was created but it is not being used. If you wish to call constructor, 'this->SRenderingPassInfo::SRenderingPassInfo(....)' should be used. i3dengine.h 2589
    SRenderingPassInfo()
      : pShadowGenMask(NULL)
      , nShadowSide(0)
      , nShadowLod(0)
      , nShadowFrustumId(0)
      , m_bAuxWindow(0)
      , m_nRenderStackLevel(0)
      , m_eShadowMapRendering(static_cast<uint8>(SHADOW_MAP_NONE))
      , m_bCameraUnderWater(0)
      , m_nRenderingFlags(0)
      , m_fZoomFactor(0.0f)
      , m_pCamera(NULL)
      , m_nZoomInProgress(0)
      , m_nZoomMode(0)
      , m_pJobState(nullptr)
    {
      threadID nThreadID = 0;
      gEnv->pRenderer->EF_Query(EFQ_MainThreadList, nThreadID);
      m_nThreadID = static_cast<uint8>(nThreadID);
      m_nRenderFrameID = gEnv->pRenderer->GetFrameID();
      m_nRenderMainFrameID = gEnv->pRenderer->GetFrameID(false);
    }
      
    SRenderingPassInfo(threadID id)
    {
      SRenderingPassInfo(); // <=
      SetThreadID(id);
    }

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

    В этом коде произойдёт следующее: будет создан новый неименованный объект типа SRenderingPassInfo и тут же разрушен. В результате поля класса остаются неинициализированными. Одним из способов исправления ошибки будет написание отдельной функции инициализации и вызов её из разных конструкторов.

    V688 The 'm_cNewGeomMML' local variable possesses the same name as one of the class members, which can result in a confusion. terrain_node.cpp 344
    void CTerrainNode::Init(....)
    {
      ....
      m_nOriginX = m_nOriginY = 0; // sector origin
      m_nLastTimeUsed = 0;         // basically last time rendered
    
      uint8 m_cNewGeomMML = m_cCurrGeomMML = m_cNewGeomMML_Min ....
    
      m_pLeafData = 0;
    
      m_nTreeLevel = 0;
      ....
    }

    Анализатор обнаружил совпадения имени локальной переменной cNewGeomMML с полем класса. Часто такой код не является ошибкой, но здесь это выглядит очень подозрительно на фоне инициализации других полей класса.

    V575 The 'memset' function processes '0' elements. Inspect the third argument. crythreadutil_win32.h 294
    void EnableFloatExceptions(....)
    {
      ....
      CONTEXT ctx;
      memset(&ctx, sizeof(ctx), 0);  // <=
      ....
    }

    С помощью анализатора была найдена очень интересная ошибка. При вызове функции memset() перепутали переданные аргументы, в результате чего функцию зовут для заполнения 0 байт памяти. Вот как выглядит прототип функции:
    void * memset ( void * ptr, int value, size_t num );

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

    Исправленный вариант:
    void EnableFloatExceptions(....)
    {
      ....
      CONTEXT ctx;
      memset(&ctx, 0, sizeof(ctx));
      ....
    }

    V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 62
    void CBuffer::Execute()
    {
      ....
      QuatT * pJointsTemp = static_cast<QuatT*>(
        alloca(m_state.m_jointCount * sizeof(QuatT)));
      ....
    }

    В коде проекта встречаются места, где с помощью функции alloca() выделяют память для массива объектов. В приведённом коде, при таком способе выделения памяти у объектов класса QuatT не будет вызывать ни конструктор, ни деструктор. Подобный код может привести к работе с неинициализированными переменными и другим ошибкам.

    Весь список подозрительных мест:
    • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 67
    • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. posematching.cpp 144
    • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 280
    • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 282
    • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. scriptbind_entity.cpp 6252
    • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. jobmanager.cpp 1016
    • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. driverd3d.cpp 5859

    V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 330
    ILINE bool InitializePoseAlignerPinger(....)
    {
      ....
      chainDesc.offsetMin = Vec3(0.0f, 0.0f, bIsMP ? -1.8f : -1.8f);
      chainDesc.offsetMax = Vec3(0.0f, 0.0f, bIsMP ? +0.75f : +1.f);
      ....
    }

    В коде проекта встречаются случаи, когда тернарный оператор ?: возвращает одно и то же значение. Возможно, в предыдущем случае так написали для красоты кода, но зачем так сделали в этом фрагменте?
    float predictDelta = inputSpeed < 0.0f ? 0.1f : 0.1f; // <=
    float dict = angle + predictDelta * ( angle - m_prevAngle) / dt ;

    Все такие места, найденные в проекте:
    • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 313
    • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -2.f. posealignerc3.cpp 347
    • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: D3D11_RTV_DIMENSION_TEXTURE2DARRAY. d3dtexture.cpp 2277
    • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 255U. renderer.cpp 3389
    • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: D3D12_RESOURCE_STATE_GENERIC_READ. dx12device.cpp 151
    • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0.1f. vehiclemovementstdboat.cpp 720

    V570 The 'runtimeData.entityId' variable is assigned to itself. behaviortreenodes_ai.cpp 1771
    void ExecuteEnterScript(RuntimeData& runtimeData)
    {
      ExecuteScript(m_enterScriptFunction, runtimeData.entityId);
    
      runtimeData.entityId = runtimeData.entityId; // <=
      runtimeData.executeExitScriptIfDestructed = true;
    }

    Подозрительное присваивание переменной самой себе. Разработчикам стоит проверить это место.

    Приоритеты операций




    V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. gpuparticlefeaturespawn.cpp 79
    bool HasDuration() { return m_useDuration; }
    
    void CFeatureSpawnRate::SpawnParticles(....)
    {
      ....
      SSpawnData& spawn = pRuntime->GetSpawnData(i);
      const float amount = spawn.amount;
      const int spawnedBefore = int(spawn.spawned);
      const float endTime = spawn.delay +
                            HasDuration() ? spawn.duration : fHUGE;
      ....
    }

    Похоже, в этой функции неверно выполняется замер времени. Приоритет оператора сложения выше, чему тернарного оператора ?:, поэтому к spawn.delay сначала прибавляется значение 0 или 1, а в переменную endTime всегда записывается значение spawn.duration или fHUGE. Это довольно распространённая ошибка. Об интересных паттернах ошибок в приоритетах операций, найденных в базе ошибок PVS-Studio, я рассказал в статье: Логические выражения в C/C++. Как ошибаются профессионалы.

    V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. model.cpp 336
    enum joint_flags
    {
      angle0_locked = 1,
      ....
    };
    
    bool CDefaultSkeleton::SetupPhysicalProxies(....)
    {
      ....
      for (int j = 0; .... ; j++)
      {
        // lock axes with 0 limits range
        m_arrModelJoints[i]....flags |= (....) * angle0_locked << j;
      }
      ....
    }

    Ещё одна очень интересная ошибка, связанная с приоритетом операций умножения и побитового сдвига. У последнего приоритет выполнения ниже, поэтому всё выражение на каждой итерации умножается на единицу (константа angle0_locked равна единице), что выглядит очень странно.

    Скорее всего хотели написать так:
    m_arrModelJoints[i]....flags |= (....) * (angle0_locked << j);

    Список из 35 подозрительных мест с приоритетом операторов сдвига приведён в файле CryEngine5_V634.txt.

    Неопределённое поведение


    Неопределённое поведение — свойство некоторых языков программирования в определённых ситуациях выдавать результат, зависящий от случайных факторов наподобие состояния памяти или сработавшего прерывания. Другими словами, спецификация не определяет поведение языка в любых возможных ситуациях. Допускать такую ситуацию в программе считается ошибкой, даже если на некотором компиляторе программа успешно выполняется, она не будет кроссплатформенной и может отказать на другой машине, в другой ОС и даже на других настройках компилятора.



    V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. physicalplaceholder.h 25
    #ifndef physicalplaceholder_h
    #define physicalplaceholder_h
    #pragma once
    ....
    const int NO_GRID_REG = -1<<14;
    const int GRID_REG_PENDING = NO_GRID_REG+1;
    ....

    Согласно современному стандарту C++, сдвиг влево отрицательного числа приводит к неопределённому поведению. Кроме этого, в коде CryEngine V есть ещё несколько таких мест:
    • V610 Undefined behavior. Check the shift operator '<<'. The left operand '~(TFragSeqStorage(0))' is negative. udpdatagramsocket.cpp 757
    • V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 324
    • V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 350
    • V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 617
    • V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 622
    • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(0xF))' is negative. d3ddeferredrender.cpp 876
    • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(0xF))' is negative. d3ddeferredshading.cpp 791
    • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(1 << 0))' is negative. d3dsprites.cpp 1038

    V567 Undefined behavior. The 'm_current' variable is modified while being used twice between sequence points. operatorqueue.cpp 105
    bool COperatorQueue::Prepare(....)
    {
      ++m_current &= 1;
      m_ops[m_current].clear();
      return true;
    }

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

    Ещё подозрительные места:
    • V567 Undefined behavior. The 'itail' variable is modified while being used twice between sequence points. trimesh.cpp 3101
    • V567 Undefined behavior. The 'ihead' variable is modified while being used twice between sequence points. trimesh.cpp 3108
    • V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1194
    • V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1202
    • V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1220
    • V567 Undefined behavior. The 'm_commandBufferIndex' variable is modified while being used twice between sequence points. xconsole.cpp 180
    • V567 Undefined behavior. The 'm_FrameFenceCursor' variable is modified while being used twice between sequence points. ccrydx12devicecontext.cpp 952
    • V567 Undefined behavior. The 'm_iNextAnimIndex' variable is modified while being used twice between sequence points. hitdeathreactionsdefs.cpp 192

    Разные ошибки в условиях




    V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 58
    bool
    operator==(const SComputePipelineStateDescription& other) const
    {
      return 0 == memcmp(this, &other, sizeof(this)); // <=
    }

    В операторе равенства допустили ошибку в вызове функции memcmp(), передав в неё размер указателя, а не размер объекта. Теперь объекты сравниваются по первым нескольким байтам.

    Исправленный вариант:
    memcmp(this, &other, sizeof(*this));

    К сожалению, в проекте есть ещё три таких места, которые стоит проверить:
    • V579 The memcpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. geomcacherendernode.cpp 286
    • V579 The AddObject function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. clipvolumemanager.cpp 145
    • V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 34

    V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. livingentity.cpp 181
    CLivingEntity::~CLivingEntity()
    {
      for(int i=0;i<m_nParts;i++) {
        if (!m_parts[i].pPhysGeom || ....)
          delete[] m_parts[i].pMatMapping; m_parts[i].pMatMapping=0;
      }
      ....
    }

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



    Рисунок 3 — Плохое форматирование кода

    С таким стилем программирования избежать ошибок на большом объёме практически невозможно. Так, в рассматриваемом случае планировали при определённом условии освобождать память из-под массива объектов и обнулять указатель. Но из-за неправильного форматирования указатель m_parts[i].pMatMapping обнуляется на каждой итерации цикла. Какие негативные последствия это может иметь, я предсказать не могу, но код однозначно настораживает.

    Ещё несколько мест с подозрительным форматированием:
    • V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. physicalworld.cpp 2449
    • V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. articulatedentity.cpp 1723
    • V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. articulatedentity.cpp 1726

    V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 538, 540. statobjrend.cpp 540
    bool CStatObj::RenderDebugInfo(....)
    {
      ....
      ColorB clr(0, 0, 0, 0);
      if (nRenderMats == 1)
        clr = ColorB(0, 0, 255, 255);
      else if (nRenderMats == 2)
        clr = ColorB(0, 255, 255, 255);
      else if (nRenderMats == 3)
        clr = ColorB(0, 255, 0, 255);
      else if (nRenderMats == 4)
        clr = ColorB(255, 0, 255, 255);
      else if (nRenderMats == 5)
        clr = ColorB(255, 255, 0, 255);
      else if (nRenderMats >= 6)          // <=
        clr = ColorB(255, 0, 0, 255);
      else if (nRenderMats >= 11)         // <=
        clr = ColorB(255, 255, 255, 255);
      ....
    }

    В этом коде программист допустил ошибку, из-за которой цвет ColorB(255, 255, 255, 255) никогда не будет выбран. Сначала значения nRenderMats сравниваются по порядку с числами от 1 до 5, но когда разработчик перешёл к работе с диапазоном значений, то не учёл, что значения больше 11 уже входят в диапазон больше 6, следовательно, последнее условие никогда не выполняется.

    Этот каскад условий был полностью скопирован ещё в одно место:
    • V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 338, 340. modelmesh_debugpc.cpp 340

    V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 393, 399. xmlcpb_nodelivewriter.cpp 399
    enum eNodeConstants
    {
      ....
      CHILDBLOCKS_MAX_DIST_FOR_8BITS = BIT(7) - 1,    // 127
      CHILDBLOCKS_MAX_DIST_FOR_16BITS   = BIT(6) - 1, // 63
      ....
    };
    
    void CNodeLiveWriter::Compact()
    {
      ....
      if (dist <= CHILDBLOCKS_MAX_DIST_FOR_8BITS) // dist <= 127
      {
        uint8 byteDist = dist;
        writeBuffer.AddData(&byteDist, sizeof(byteDist));
        isChildBlockSaved = true;
      }
      else if (dist <= CHILDBLOCKS_MAX_DIST_FOR_16BITS) // dist <= 63
      {
        uint8 byteHigh = CHILDBLOCKS_USING_MORE_THAN_8BITS | ....);
        uint8 byteLow = dist & 255;
        writeBuffer.AddData(&byteHigh, sizeof(byteHigh));
        writeBuffer.AddData(&byteLow, sizeof(byteLow));
        isChildBlockSaved = true;
      }
      ....
    }

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

    V547 Expression 'pszScript[iSrcBufPos] != '=='' is always true. The value range of char type: [-128, 127]. luadbg.cpp 716
    bool CLUADbg::LoadFile(const char* pszFile, bool bForceReload)
    {
      FILE* hFile = NULL;
      char* pszScript = NULL, * pszFormattedScript = NULL;
      ....
      while (pszScript[iSrcBufPos] != ' ' &&
        ....
        pszScript[iSrcBufPos] != '=' &&
        pszScript[iSrcBufPos] != '==' &&  // <=
        pszScript[iSrcBufPos] != '*' &&
        pszScript[iSrcBufPos] != '+' &&
        pszScript[iSrcBufPos] != '/' &&
        pszScript[iSrcBufPos] != '~' &&
        pszScript[iSrcBufPos] != '"')
      {}
      ....
    }

    В большом условном выражении есть подвыражение, которое всегда истинно. Литерал '==' будет иметь тип int и будет равен 15677. Массив pszScript состоит из элементов типа char. Значение типа char не может быть равно 15677. Именно поэтому выражение pszScript[iSrcBufPos] != '==' всегда истинно.

    V734 An excessive expression. Examine the substrings "_ddn" and "_ddna". texture.cpp 4212
    void CTexture::PrepareLowResSystemCopy(byte* pTexData, ....)
    {
      ....
      // make sure we skip non diffuse textures
      if (strstr(GetName(), "_ddn")              // <=
          || strstr(GetName(), "_ddna")          // <=
          || strstr(GetName(), "_mask")
          || strstr(GetName(), "_spec.")
          || strstr(GetName(), "_gloss")
          || strstr(GetName(), "_displ")
          || strstr(GetName(), "characters")
          || strstr(GetName(), "$")
          )
        return;
      ....
    }

    Функция strstr() ищет первое вхождение указанной подстроки в другой строке и возвращает указатель на первое вхождение подстроки или пустой указатель. Первой ищется подстрока "_ddn", а потом "_ddna", это означает, что условие выполнится, если будет найдена более короткая подстрока. Возможно, код работает не так, как планировал программист. Ну или по крайней мере выражение является избыточным и его можно упростить, удалив лишнюю проверку.

    V590 Consider inspecting this expression. The expression is excessive or contains a misprint. goalop_crysis2.cpp 3779
    void COPCrysis2FlightFireWeapons::ParseParam(....)
    {
      ....
      else if (!paused &&
              (m_State == eFP_PAUSED) &&        // <=
              (m_State != eFP_PAUSED_OVERRIDE)) // <=
      ....
    }

    Условное выражение в функции ParseParam() написано таким образом, что результат не зависит от подвыражения (m_State != eFP_PAUSED_OVERRIDE).

    Рассмотрим упрощённый пример:
    if ( err == code1 && err != code2)
    {
      ....
    }

    Результат всего условного выражения не зависит от результата подвыражения (err != code2), что хорошо видно при построении таблицы истинности для данного примера (рисунок 4)



    Рисунок 4 — Таблица истинности для логического выражения

    Сравнение беззнаковых чисел с нулём




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

    V547 Expression 'm_socket < 0' is always false. Unsigned type value is never < 0. servicenetwork.cpp 585
    typedef SOCKET CRYSOCKET;
    // Internal socket data
    CRYSOCKET m_socket;
    
    bool CServiceNetworkConnection::TryReconnect()
    {
      ....
      // Create new socket if needed
      if (m_socket == 0)
      {
        m_socket = CrySock::socketinet();
        if (m_socket < 0)
        {
          ....
          return false;
        }
      }
      ....
    }

    Подробно хочу остановиться на типе SOCKET, который может быть знаковым и беззнаковым на разных платформах, поэтому для работы с этим типом настоятельно рекомендуется использовать специальные макросы и константы, определённые в стандартных заголовочных файлах.

    В кроссплатформенных проектах часто встречаются сравнения со значениями 0 или -1, которые приводят к некорректной интерпретации кода ошибки. Проект CryEngine V не является исключением, хотя кое-где присутствуют правильные проверки, например:
    if (m_socket == CRY_INVALID_SOCKET)

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

    47 мест, где беззнаковые переменные подозрительно сравниваются с нулём, вынесены в файл CryEngine5_V547.txt. Разработчикам желательно проверить указанные места.

    Опасные указатели




    Диагностическое правило V595 находит в коде разыменование указателей, проверка валидности которых выполняется ниже по коду, т.е. уже после использования указателя. На практике эта диагностика находит очень крутые ошибки. Небольшое количество ложных срабатываний возникает из-за того, что проверка указателя выполняется косвенно, т.е. через одну или несколько других переменных, но согласитесь, что и для человека разобраться в таком коде будет весьма непросто. Я приведу три примера срабатывания этой диагностики, которые вызывают особое удивление, как работает такой код, остальные можно посмотреть в файле CryEngine5_V595.txt.

    Пример 1


    V595 The 'm_pPartManager' pointer was utilized before it was verified against nullptr. Check lines: 1441, 1442. 3denginerender.cpp 1441
    void C3DEngine::RenderInternal(....)
    {
      ....
      m_pPartManager->GetLightProfileCounts().ResetFrameTicks();
      if (passInfo.IsGeneralPass() && m_pPartManager)
        m_pPartManager->Update();
      ....
    }

    Разыменование и проверка указателя m_pPartManager.

    Пример 2


    V595 The 'gEnv->p3DEngine' pointer was utilized before it was verified against nullptr. Check lines: 1477, 1480. gameserialize.cpp 1477
    bool CGameSerialize::LoadLevel(....)
    {
      ....
      // can quick-load
      if (!gEnv->p3DEngine->RestoreTerrainFromDisk())
        return false;
    
      if (gEnv->p3DEngine)
      {
        gEnv->p3DEngine->ResetPostEffects();
      }
      ....
    }

    Разыменование и проверка указателя gEnv->p3DEngine.

    Пример 3


    V595 The 'pSpline' pointer was utilized before it was verified against nullptr. Check lines: 158, 161. facechannelkeycleanup.cpp 158
    void FaceChannel::CleanupKeys(....)
    {
    
      CFacialAnimChannelInterpolator backupSpline(*pSpline);
    
      // Create the key entries array.
      int numKeys = (pSpline ? pSpline->num_keys() : 0);
      ....
    }

    Разыменование и проверка указателя pSpline.

    Разные предупреждения




    V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. mergedmeshrendernode.cpp 999
    static inline void ExtractSphereSet(....)
    {
      ....
      switch (statusPos.pGeom->GetType())
      {
        if (false)
        {
        case GEOM_CAPSULE:
          statusPos.pGeom->GetPrimitive(0, &cylinder);
        }
        if (false)
        {
        case GEOM_CYLINDER:
          statusPos.pGeom->GetPrimitive(0, &cylinder);
        }
        for (int i = 0; i < 2 && ....; ++i)
        {
          ....
        }
        break;
      ....
    }

    Пожалуй, этот фрагмент кода самый странный из всех, что встречались в проекте CryEngine V. Выбор метки case не зависит от условного оператора if, даже если там if (false). В операторе switch выполняется безусловный переход к метке, если она удовлетворяет выражению switch. Без использования оператора break, с помощью такого кода можно «обходить» выполнение ненужных операторов, но опять же, сопровождать такой неочевидный код будет легко не всем разработчикам. И почему при переходе к меткам GEOM_CAPSULE и GEOM_CYLINDER выполняется один и тот же код?

    V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_action.cpp 143
    typedef CryStringT<char> string;
    // The actual fragment name.
    string m_fragName;
    //! cast to C string.
    const value_type* c_str() const { return m_str; }
    const value_type* data() const  { return m_str; };
      
    void LogError(const char* format, ...) const
    { .... }
      
    void QueueAction(const UpdateContext& context)
    {
      ....
      ErrorReporter(*this, context).LogError("....'%s'", m_fragName);
      ....
    }

    Если в описании функции невозможно указать число и типы всех допустимых параметров, то список формальных параметров завершается эллипсисом (...), что означает: «и, возможно, еще несколько аргументов». В качестве фактического параметра для эллипсиса могут выступать только POD (Plain Old Data) типы. Если эллипсис функции в качестве параметра передается объект класса, то практически всегда свидетельствует о наличии ошибки в программе. Вместо указателя на строку в стек попадает содержимое объекта. Такой код приведет к формированию в буфере «абракадабры» или к аварийному завершению программы. В коде CryEngine V используется свой класс строки, и у него уже есть подходящий метод c_str().

    Исправленный вариант:
    LogError("....'%s'", m_fragName.c_str();

    Ещё несколько подозрительных мест:
    • V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 1339
    • V510 The 'Format' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 2648
    • V510 The 'CryWarning' function is not expected to receive class-type variable as sixth actual argument. crypak.cpp 3324
    • V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. crypak.cpp 3333
    • V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4864
    • V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4931
    • V510 The 'Format' function is not expected to receive class-type variable as third actual argument. featuretester.cpp 1727

    V529 Odd semicolon ';' after 'for' operator. boolean3d.cpp 1314
    int CTriMesh::Slice(....)
    {
      ....
      bop_meshupdate *pmd = new bop_meshupdate, *pmd0;
      pmd->pMesh[0]=pmd->pMesh[1] = this;  AddRef();AddRef();
      for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // <=
        pmd0->next = pmd;
      ....
    }

    Очень подозрительный фрагмент кода. После цикла for поставили точку с запятой, хотя форматирование кода подразумевает о наличии тела у цикла.

    V535 The variable 'j' is being used for this loop and for the outer loop. Check lines: 3447, 3490. physicalworld.cpp 3490
    void CPhysicalWorld::SimulateExplosion(....)
    {
      ....
      for(j=0;j<pmd->nIslands;j++)                 // <= line 3447
      {
        ....
        for(j=0;j<pcontacts[ncont].nborderpt;j++)  // <= line 3490
        {
      ....
    }

    Код проекта полон и другим опасным кодом, например, использованием одного счётчика во вложенном и внешнем циклах. Большие файлы с исходным кодом содержат запутанное форматирование и изменение одних переменных в разных местах — без статического анализа здесь не обойтись!

    Ещё несколько подозрительных циклов:
    • V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1630, 1683. entity.cpp 1683
    • V535 The variable 'i1' is being used for this loop and for the outer loop. Check lines: 1521, 1576. softentity.cpp 1576
    • V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2315, 2316. physicalentity.cpp 2316
    • V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1288, 1303. shadercache.cpp 1303

    V539 Consider inspecting iterators which are being passed as arguments to function 'erase'. frameprofilerender.cpp 1090
    float CFrameProfileSystem::RenderPeaks()
    {
      ....
      std::vector<SPeakRecord>& rPeaks = m_peaks;
      
      // Go through all peaks.
      for (int i = 0; i < (int)rPeaks.size(); i++)
      {
        ....
        if (age > fHotToColdTime)
        {
          rPeaks.erase(m_peaks.begin() + i); // <=
          i--;
        }
      ....
    }

    Анализатор посчитал, что в функцию, выполняющую действие с контейнером, передаётся итератор от другого контейнера. В этом фрагменте кода это не так и ошибки нет. Переменная rPeaks является ссылкой на m_peaks. Однако такой код может вводить в заблуждение не только анализатор кода, но и людей, которые будут сопровождать код. Не стоит так писать.

    V713 The pointer pCollision was utilized in the logical expression before it was verified against nullptr in the same logical expression. actiongame.cpp 4235
    int CActionGame::OnCollisionImmediate(const EventPhys* pEvent)
    {
      ....
      else if (pMat->GetBreakability() == 2 &&
       pCollision->idmat[0] != pCollision->idmat[1] &&
       (energy = pMat->GetBreakEnergy()) > 0 &&
       pCollision->mass[0] * 2 > energy &&
       ....
       pMat->GetHitpoints() <= FtoI(min(1E6f, hitenergy / energy)) &&
       pCollision) // <=
        return 0;
      ....
    }

    Оператор if содержит довольно большое условное выражение, в котором везде выполняется обращение к указателю pCollision. Ошибка заключается в том, что на равенство нулю указатель pCollision проверяется самым последним, т.е. уже после многократного разыменовывания.

    V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 274
    typedef std::shared_ptr<....> CDeviceGraphicsCommandListPtr;
    
    CDeviceGraphicsCommandListPtr
    CDeviceObjectFactory::GetCoreGraphicsCommandList() const
    {
      return m_pCoreCommandList;
    }
    
    void CRenderItemDrawer::DrawCompiledRenderItems(....) const
    {
      ....
      {
        auto& RESTRICT_REFERENCE commandList = *CCryDeviceWrapper::
          GetObjectFactory().GetCoreGraphicsCommandList();
    
        passContext....->PrepareRenderPassForUse(commandList);
      }
      ....
    }

    В переменную commandList сохраняется ссылка на значение, которое хранит умный указатель. При разрушении объекта умным указателем ссылка становится недействительной.

    Ещё несколько таких мест:
    • V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 384
    • V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 368
    • V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 485
    • V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 553

    Заключение


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

    Сотрудничество с Epic Games продемонстрировало пользу от внедрения статического анализатора в Unreal Engine 4. Мы помогли разработчикам во всех вопросах интеграции анализатора и даже исправили найденные ошибки, чтобы они могли регулярно проверять новый код проекта. К аналогичному сотрудничеству мы готовы и с компанией Crytek.

    Предлагаю всем желающим попробовать PVS-Studio на своих C/C++/C# проектах.

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


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Long-Awaited Check of CryEngine V .

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

    Комментарии 65

      +1
      >Согласно современному стандарту C++, сдвиг влево отрицательного числа приводит к неопределённому поведению.

      Не являюсь C++ программистом, но интересно, почему так?
        0
        Статья на эту тему.
          0
          Статья неплохая, но полного ответа на вопрос она не дает, просто ссылается на стандарт.
            0
            Быть может потому, что старший бит часто является знаком и при сдвиге влево (любом: арифметическом или циклическим) он уничтожится или завернется в младший разряд. При арифметическом сдвиге вправо старший разряд копируется и поэтому такой ситуации нет.
          +6
          Стандарт не читал, но уверен, что UB ради перформанса. Сдвигая биты не ожидаешь изменения знака, т.е. при сдвиге вправо нужно дублировать старший бит, а при сдвиге влево нужно сдвигать все биты, кроме старшего. Это сложнее делать, поэтому биты просто сдвигаются и число становится то знаковым, то беззнаковым, в зависимости от того, какой бит попадётся. При этом оно из дополнительной формы не преобразовывается и числа просто «скачут». В общем, UB он и есть UB.
            0
            >> при сдвиге вправо нужно дублировать старший бит
            CPU, в массе своей, имеют команды логического и арифметического сдвига. Репликация старшего бита осуществляется аппаратно.
            Так что сдвиг int вправо работает на любом известном мне компиляторе, несмотря на то, что он отнесён к UB.

            >> а при сдвиге влево нужно сдвигать все биты, кроме старшего.
            Вы точно понимаете основы представления чисел в дополнительном коде?
            -1 это 0xffffffff (для 32-битного числа). Ничего со старшим битом делать не надо.
            Если же у вас происходит переполнение из 30 в 31 бит, то по-любому будет неверное число.
              0
              Если разбудить среди ночи, то про дополнительный код вряд ли отвечу. Но! Помимо -1 есть и куча других отрицательных чисел. Например, если взять целое "-9651325" и 31 сдвинуть его по одному биту влево, то получаются вот такие числа:
              1824228096
              -646511104
              -1293022208
              1708922880
              -877121536
              -1754243072
              786481152
              1572962304
              -1149042688
              1996881920
              -301203456
              -602406912
              -1204813824
              1885339648
              -524288000
              -1048576000
              -2097152000
              100663296
              201326592
              402653184
              805306368
              1610612736
              -1073741824
              -2147483648

              Как видите, проблема не в том, что происходит «переполнение в старший бит». Ка краз наоборот! Происходит «недополнение». Т.е. 1 заменяется на 0. Если говорить о переполнении, то при сдвиге влево любого отрицательного числа старший выпадает. При сдвиге вправо реплицируется, как вы заметили, а при сдвиге влево замещается младшим битом.

              Это вывод в MSVC2015-debug. Т.к. UB, то цифры могут быть другими в других условиях.
                0
                я не знаю что вы выводите, но -9651325 это FF6CBB83
                Его вы можете сдвинуть влево 7 раз без переполнений

                >> «переполнение в старший бит». Ка краз наоборот! Происходит «недополнение».
                Не нужно изобретать «недотермины».
                  0
                  Предлагаете для каждого отрицательного числа выяснять, сколько раз его можно сдвинуть влево без изменения знака? Я не знаю, как underflow по русски будет, поэтому «недополнение».
                    0

                    Я тоже не знаю, но Яндекс утверждает, что это будет «потеря значимости». Google сильно неадекватнее: «изчезновение».


                    В любом случае, непонятно, при чём тут «underflow»: underflow будет когда вы (целое)3 разделите на (целое)2 и получите (целое)1, а не (с плавающей точкой)1,5.

          • НЛО прилетело и опубликовало эту надпись здесь
              +1
              Существует несколько способов представления отрицательных чисел: прямое, сдвиг, дополнение до единицы, дополнение до двух…
              Как представляет их микроконтроллер, это его дело, но (-1)<<1 (сдвиг без всякой логики) может превратиться в:
              прямое) 1001 -> 0010 = 2,
              сдвиг) 0111 -> 1110 = 6 (в 8-битной логике 126),
              до 1) 1110 -> 1100 = -3,
              до 2) 1111 -> 1110 = -2.
              0

              Скорей всего, дело в том, что крайний левый бит отвечает за знак. А побитовый сдвиг сдвигает все биты (в том числе и знаковый), и непонятно, что будет в этом крайнем бите после сдвига.


              P.S. Я тоже не C++ программист, поэтому это лишь мои догадки

                0
                Если бы было так, то поведение было вполне предсказуемым. Ну сдвигается знак, возможно, кто-то именно этого добиться и хочет. Вспоминая, что сам творил с этими сдвигами, у меня такая возможность не вызывает никакого удивления

                Так что даже так, не совсем понятно, почему поведение зовут «неопределенным».
                  +1
                  Разные процессоры реализуют сдвиг по-разному, поэтому комитет поступил, как поступает обычно в таких случаях — объявил поведение неопределённым. Цитата под спойлером насчёт сдвига на ширину, большую размеру типа, но со сдвигом отрицательных чисел история та же.

                  Скрытый текст
                  Shifting a uint32_t by 32 or more bits is undefined. My guess is that this originated because the underlying shift operations on various CPUs do different things with this: for example, X86 truncates 32-bit shift amount to 5 bits (so a shift by 32-bits is the same as a shift by 0-bits), but PowerPC truncates 32-bit shift amounts to 6 bits (so a shift by 32 produces zero). Because of these hardware differences, the behavior is completely undefined by C (thus shifting by 32-bits on PowerPC could format your hard drive, it is *not* guaranteed to produce zero). The cost of eliminating this undefined behavior is that the compiler would have to emit an extra operation (like an 'and') for variable shifts, which would make them twice as expensive on common CPUs.

                  What Every C Programmer Should Know About Undefined Behavior #1/3
                    0
                    Спасибо вам и meduzik за ответы. Тогда возникает интересный вопрос — почему в стандарте 98 года, как раз, когда соотношение между архитектурами еще не так сильно перекашивало так сильно в сторону отдельных пунктов, поведение неопределенным объявлено еще не было?
                      0

                      Скорее всего, одно из двух: либо не догадались о потере производительности на этом моменте, либо не уделили достаточно внимания описанию в спеке. Человеческий фактор, стандарт большой и его пишет много людей.

                    +4
                    Стандарт C++ не гарантирует вам представление отрицательных чисел. На некоторых архитектурах это вполне может быть не привычный two's complement, а one's complement или еще какой-то экзотический вариант.

                    Далее, опять же, на некоторых патформах некоторый набор бит в числе может быть так называемым trap representation, то есть вызывать хардварные исключения при появлении в вычислениях. Один из примеров — Signaling NaN.

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

                    Поскольку стандарты C и C++ писались «в пользу компилятора», то есть чтобы позволить максимальное число оптимизаций, они разрешают компилятору считать, что trap не произойдет и, значит, сдвига отрицательных чисел в программе нет.
                  0
                  Ради интереса, покажите false-positive.
                    +1

                    В прошлой статье я нашёл одно ложное срабатывание, в этой


                    void CBuffer::Execute()
                    {
                      ....
                      QuatT * pJointsTemp = static_cast<QuatT*>(
                        alloca(m_state.m_jointCount * sizeof(QuatT)));
                      ....
                    }

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


                    Но я пока не знаю теорию статического анализа так хорошо, как авторы, поэтому не могу оценить объём работы, который требуется, чтобы проанализировать эти два случая правильно. Более того, формально второе предупреждение скорее всего верно, потому что использовать Си-инициализацию в С++ разрешено только для POD-типов, а QuantT не подходит под это определение. И я не помню стандарт так хорошо, чтобы точно сказать, может ли быть UB при таком определении и при такой инициализации класса.

                  • НЛО прилетело и опубликовало эту надпись здесь
                      +21
                      А мне стыдно за ваш комментарий. Странно да?
                      • НЛО прилетело и опубликовало эту надпись здесь
                          +4
                          Ладно, тогда 75% всего бюджета будем выделять дизайнерам.
                          • НЛО прилетело и опубликовало эту надпись здесь
                              +3
                              Таким софтом пользуются не для просмотра красивых кнопок и логотипов.
                              • НЛО прилетело и опубликовало эту надпись здесь
                                  +13
                                  Предлагаю Вам начать писать статьи на Хабре, чтобы продемонстрировать красивый дизайн в оформлении иллюстраций и тем самым повысить общий уровень представленного здесь материала. Сделаем вместе мир лучше!
                                    +1
                                    «Не заметил Ваш комментарий»…
                                    +8
                                    Вы из тех людей, которые говорят, что всё плохо в России, национальные беды какие-то упоминаете, но сами при этом не пытаетесь что-либо сделать для изменения ситуации?

                                    Я много таких людей встречал. Уезжая в другую страну, такие людей точно также будут сетовать на проблемы в этой стране.
                                      +2
                                      Что плохого в сетовании на проблему? Уж точно лучше, чем жевать кактус.
                                        –1
                                        Сразу картинка в голове: бабульки на скамейке говорят за жизнь. Толку сетовать и ничего не делать. Это ничем не отличается от жевания кактуса.
                        +6
                        > High: 576

                        ого!!! Вам не кажется, что это ОЧЕНЬ МНОГО ошибок для коммерческой разработки? (или для сипипей это норма?)
                        В тырнетах все такие умные — про тесты говорят, инверсии зависимостей, шаблоны, а тут проверили годами полируемый код — и такая лажа! Кого ж эти СлёзоТеки наняли, что на выходе имеют такое позорное качество?

                        Но коммент не ради этого, удивлят другое — как эта копипастная лапша вообще работала-то?? Вы все учили матрицы, 3D и всё такое — нельзя просто взять и вместо Z присвоить Y — получится такая белиберда, что её вообще на экране нельзя отображать! У них там что, на каждую индусофункцию ещё 10, которые исправляют баги первой? :)

                        PS
                        Для сипиписников с их ужасным инструментом, PVS — это must have за любые деньги!
                          +5
                          Там проверки на коректность, а не присвоения, две большие разницы. Часть этих проверок в реальности может вообще оказаться лишней. Да и не надо делать вид, будто такую ошибку можно допустить только на плюсах. Конечно, 500 — это чуть-чуть многовато, но в движке не килобайт кода. Даже не мегабайт. Даже не 100 мегабайт. Так что будьте аккуратнее, сравнивая огромный проект и ваш личный опыт.

                          Что до годами полируемого кода — очень смешно. Если бы это было так, у них бы была графика уровня 2003 года. А у них реальная гонка по скоростному освоению новых высот геймдева и, прежде всего, графена. И тут за право первым реализовать поддержку DX11 или VulkanAPI приносят в жертву не одного бычка, включая спагетти-код.

                          > сипиписников с их ужасным инструментом
                          А вот это про что было?
                            0
                            Скорее уж тогда DX12, ну так, позанудствовать :)
                              0
                              Тогда уж лучше VR и продвинутую поддержку окулуса с кардбоксом, это ж самый мейнстрим. Ну так, позанудствовать)
                            0
                            Игровык движки это адовый овер-инженеринг. Это всё может фикситься на других уровнях абстракции. Не думаю, что каждый будет лезть в middleware при каждой баге. Время тоже дорого.
                              +5
                              Перепись людей, не работавших на продакшене, объявляется открытой
                              +2
                              Для UB тоже надо единорога сделать.
                                +1
                                ХЗ какой должен быть единорог для UB :-).

                                image
                                  0
                                  Неопределённый Единорог.
                                    +1
                                    Неопределеннорог
                                      +1
                                      Единорог Шрёдингера?
                                        +8
                                          –1
                                          Розовый единорог! (в обнимку с чайником Рассела)
                                          — ты его не видишь, а он есть!
                                +2
                                Круто, так их, багов, давить!

                                [Далее небольшой сеанс конструктивной (надеюсь) критики]

                                Скажите, а у вас есть доступ к native speaker'ам или хотя бы к учителям английского?
                                Мне кажется, это слишком похоже на «рунглиш со словарём»:
                                — «There is a probability of logical error presence
                                — «The '...' variable is assigned values twice successively
                                — «Member of a class is initialized by itself»
                                — «always returns one and the same value»
                                — «The memcmp function receives the pointer and its size as arguments.»
                                (аналогично вашей статье, выбрал только несколько ошибок для демонстрации, обращайтесь за полным отчётом :))

                                Я сам не носитель, но вышеперечисленные примеры вызывают у меня подозрения на правильность с точки зрения английского. Мне кажется, вам стоит нанять кого-нибудь, отлично знающего английский, и разбирающегося в программировании, чтобы разок-другой пройтись по списку ошибок, чтобы тулза, явно ориентированная в том числе на англоязычных пользователей, не носила ушанку и не говорила с русским акцентом [линк].

                                Keep up the good work!
                                  0
                                  Тимур, привет! Пойдешь к нам на эту работу?
                                    0
                                    Евгений, привет, давно не виделись!
                                    Сколько предлагаете? :D
                                      0
                                      Пошли в личку. Но прежде чем написать свой ценник, вспомни, что мы маленькая провинциальная компания :-).
                                        +3
                                        Другим: учитесь на работу устраиваться!
                                          0
                                          Кстати, на полном серьёзе, примерно так я и устроился на свою первую работу: на презентации одной команды нашёл что они не подумали об одной проблеме, и прислал им прототип решения этой проблемы.
                                          0
                                          ;)
                                    0
                                    const float endTime = spawn.delay + HasDuration()? spawn.duration: fHUGE;

                                    Так писать вообще плохая практика, неужели переменной отдельной жалко выделить под spawnDuration.
                                      +1
                                      Вы на столько идеальный и стерильный, что у вас даже нет своего крайенжна.
                                        0
                                        Плохая практика?
                                        0
                                        Здравствуйте. Я очень давно работаю с движками серии «CryEngine», и у меня есть несколько вопросов касательно ошибок которые нашёл ваш анализатор, конкретно меня интересуют ошибки связанные с функциями и алгоритмами выделения памяти объектам в движке. Дело в том, что в движке есть очень серьёзные проблемы с выделением памяти, зачастую движок не в состоянии задействовать более 3.5 гигабайт системной памяти, иногда меньше, иногда чуть больше, зависит от того чем заполнять память, инстанциями каких объектов и т.д. Это наблюдается на любом железе с достаточным объёмом оперативной памяти и любой 64х разрядной ОС. Данная проблема, скорее всего, уже имела место быть с самой первой версии движка, однако я с ней столкнулся начиная со второй версии движка(CryEngine 2), первую не тестировал на предмет наличия.

                                        Собственно, при достижении указанного значения потребления памяти движок просто выдаёт ошибку выделения памяти(memory allocation for x bytes failed) по сей день на самой последней версии СЕ 5.1.1, прямо «из коробки», т.е без каких либо изменений кода и прочих элементов. На систему установлено 32 гигабайта оперативной памяти, в других движках, например «Юнити 5», подобных проблем с выделением памяти не наблюдалось. Память 100% остаётся свободна при возникновении ошибки, были протестированы конфигурации с 8ю и 16ю гигабайтами памяти, ошибка проявляется всегда при одинаковых условиях, появление ошибки так же не зависит от размера файла подкачки(при размере меньше 1го гб появляется похожая ошибка но уже с рекомендацией увеличить размер «heap» памяти).

                                        Что меня конкретно интересует по результатам анализа: ошибки и спорные места в файлах GeneralMemoryHeap.h и cpp, MemoryFragmentationProfiler.h, MemoryAddressRange.h и cpp, MemoryManager.h и cpp, CryMemoryManager.cpp, CustomMemoryHeap.h и cpp, MTSafeAllocator.h и срр, PageMappingHeap.h и срр, CryDLMalloc.c, CrySizerImpl.h и срр, LevelHeap.cpp, MemReplay.h и срр а так же SystemWin32.cpp. Крайтек игнорирует сообщения об этой ошибке на своих форумах уже довольно давно, или в некоторых случаях просто не может сказать ничего вразумительного. Учитывая тот факт что за 10 лет проблема как была так и осталась то просто очевидно что фиксить они её не собираются в ближайшее время.

                                        Данная проблема проявляет себя во всей красе при работе с большими уровнями, или уровнями с хорошей детализацией( в большом количестве мелкие объекты, инстанции растительности и пр.), всё это в совокупности может привести к заполнению движком 3.5 гб памяти с последующим возникновением ошибки выделения памяти. При тестировании такого уровня в дальнейшем в режиме игры проблем не возникает, т.к в режиме игры включаются определённые оптимизации и часть объектов просто не загружается в память полностью, тем не менее можно вызвать данную ошибку выделения даже во время игры заспавнив определённое количество объектов в любых местах уровня, при этом количество объектов не обязательно будет слишком большим, скажем 1700 объектов, что далеко от предела количества объектов на одном уровне обозначенного в офиц. документации.

                                        Буду также очень рад если вы поделитесь своими идеями по поводу корней данной проблемы а так же возможных теоретических способах её решения. Вообще я стараюсь работать только с 2мя модулями движка — СryGame и CryAction и не лезть в другие модули, но моя надежда на то что Крайтек решит эту проблему в каком нибудь обновлении движка давно утрачена, а учитывая что теперь исходный код почти всех модулей доступен — почему бы не попробовать решить её?
                                          0
                                          Из описания не понятно, речь идёт о 32-битной или 64-битной версии библиотеки. Если о 32-битной, то тогда собственно это нормальное поведение — просто не хватает памяти (3.5 гигабайт это и есть предел). И даже если в данный момент выделено меньше, все равно из-за фрагментации памяти может возникнуть ситуация, когда нельзя выделить память под очередной большой массив.

                                          Если речь о 64-битной версии, то скорее всего библиотека содержит классические 64-битные ошибки, о которых мы много писали. И тогда, когда память начинает выделяться за пределами младших 4 гигабайт, начинаются различные проблемы. Например, может разрушаться менеджер памяти и нехватка памяти это просто последствия его некорректной работы. На 64-битные ошибки мы код библиотеки не исследовали, так как для написания статьи нам хватило и предупреждений общего назначения. :)
                                            –1
                                            Из описания не понятно, речь идёт о 32-битной или 64-битной версии библиотеки.
                                            Там вроде нормально написано:
                                            Это наблюдается на любом железе с достаточным объёмом оперативной памяти и любой 64х разрядной ОС.

                                            Не думаю, что человек, тестирующий на 32ГБ ОЗУ в течении 10 лет споткнётся о такую простую ошибку как несоответствие разрядности ОС и библиотеки.
                                              +1
                                              А причём здесь разрядность ОС? А причём здесь несоответствие разрядности ОС и библиотеки? Всё смешалось, кони, люди…

                                              Программа 32-битная или 64-битная?!
                                              0
                                              Версия библиотек 64х битная, 32х битная версия не поддерживает редактор уровней,.ехе файл редактора давно не поставляется с ней в комплекте, потому то я и не посчитал нужным указать информацию о версии. Спасибо за ответ, значит проблема всё же в 64х битных ошибках.
                                              0
                                              почему бы не попробовать решить её?


                                              Потому что подобные ошибки с памятью очень тяжело отлаживать. Даже программисту, который помнит свой код наизусть. А уж человеку, впервые увидевшему эти исходники, тем более нужно огромное количество времени, чтобы хоть как-то начать ориентироваться. И вообще, проблема может заключаться не только в движке, но и в каких-то сторонних библиотеках или даже в операционной системе (мало ли, какая программа из-за ошибки вышла за пределы своей памяти).
                                                0
                                                мало ли, какая программа из-за ошибки вышла за пределы своей памяти
                                                На какой платформе, поддерживаемой CryEngine, это возможно, на приставках?
                                                  0
                                                  Думаю проблема всё же в коде движка, сторонние библиотеки здесь не играют роли, разные версии движка тестировались на разных ОС и железе(конечно количество установленной оперативной памяти позволяло диагностировать ошибку), ошибка везде одинаковая, количество свободной памяти при возникновении ошибки достаточно большое(в случае 32х гб свободными остаются более 22х), ос и библиотеки движка 64х битные. А так понятно что дело сложное, тем не менее при возникновении ошибки дебаггер первым делом скидывает на модуль CrySystem, впрочем я ещё толком не смотрел что там, нужно сначала его скомпилировать чтобы сгенерировалась информация для дебаггера. В общем движок сам по себе стал нестабилен со времён пятой версии, иногда зависает на начальном этапе загрузки без видимых причин, бывает что вылетает на загрузке уровня указав на atidxx64.dll. В предыдущих версиях (EAAS) подобные проблемы отсутствовали, за исключением конечно ошибки выделения памяти.
                                                0
                                                (del)
                                                  0

                                                  Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                                  Самое читаемое