Недавно появилась статья "Hackathon 2: Time lapse analysis of Unreal Engine 4", в которой рассказывается, как взяв инструмент Klocwork, можно найти множество ошибок в проекте Unreal Engine 4. Я не могу пройти мимо этой статьи. Дело в том, что в свое время мы исправили все ошибки, которые нашел в этом проекте анализатор PVS-Studio. Понятно, что были исправлены не вообще все ошибки, а только обнаруживаемые анализатором. Однако статья создаёт впечатление, что анализатор PVS-Studio пропустил слишком много. Что ж, теперь мой ход. Я тоже заново перепроверил Unreal Engine 4 и нашел массу ошибок. Таким образом я могу заявить, что PVS-Studio тоже может найти сейчас в Unreal Engine 4 новые ошибки. Ничья.
Всё началось полтора года назад, когда я написал статью "Долгожданная проверка Unreal Engine 4". Далее было сотрудничество с компанией Epic Games, результатом которого стало устранение всех предупреждений, которые выдавал PVS-Studio. В результате было поправлено множество ошибок и устранены все ложные срабатывания анализатора. Наша команда выдала компании Epic Games проект, свободный от предупреждений PVS-Studio. Как это происходило, можно узнать из статьи "Как команда PVS-Studio улучшила код Unreal Engine".
Недавно на просторах интернета я наткнулся на статью "Hackathon 2: Time lapse analysis of Unreal Engine 4". Статья хорошая и корректная. Компания Rogue Wave молодец, что проводит такие мероприятия и делает мощный статический анализатор кода Klocwork. Michail Greshishchev тоже молодец, что проделал работу по проверке Unreal Engine и нашёл время написать про это статью. Всё хорошо. Но я обеспокоен, что сторонний человек, мало знакомый со статическими анализаторами, может сделать неправильные выводы. Поэтому я обязан прокомментировать эту статью.
Непреднамеренно, но непосвященному читателю эта статья показывает нас в невыгодном свете по сравнению Klocwork. Кажется, что PVS-Studio находит меньше ошибок, чем Klocwork. На самом деле мир устроен сложнее. Оба анализатора имеют различные множества диагностик. Частично эти множества пересекаются. Однако каждый из анализаторов имеет уникальный набор диагностик. Таким образом, проверив большой проект одним анализатором, вы потом всегда что-то найдёте другим анализатором.
Ещё один нюанс. Мы не проверяли сторонние библиотеки (каталог ThirdParty), а Michail Greshishchev проверял (по крайней мере частично), о чем свидетельствует один из примеров кода (см. функцию HeadMountedDisplayCommon). В ThirdParty анализатор PVS-Studio может также легко найти множество интересных дефектов. Тем более, что размер ThirdParty-исходников в 3 раза больше самого UE4.
Однако всё это звучит как жалкие попытки оправдаться :). Поэтому мне не остаётся ничего другого, как сравнять счёт. С этой целью были взяты исходники Unreal Engine 4 и проверены последней версией анализатора PVS-Studio.
И сейчас я продемонстрирую, что всегда можно легко найти ошибки в большом быстро меняющемся проекте.
Я проверил исходные коды UE4 последней версией PVS-Studio. Проверялись исходные коды UE4 без учета каталога ThirdParty. Если я их проверю, у меня получится не статья, а справочник :).
Я получил 1792 предупреждения общего назначения первого и второго уровня. Не надо пугаться. Сейчас станет понятно, откуда взялось это число.
Большинство предупреждений (93%) связано с новым диагностическим правилом V730, предназначенным для выявления неинициализированных членов класса. Неинициализированный член класса - это не всегда ошибка, но тем не менее это то место в программе, которое стоит проверить. Вообще 1672 предупреждений V730 это много. На других проектах я не видел подобного количества этих предупреждений. Тем более, что анализатор по возможности пытается предугадать, когда неинициализированный член - это не страшно. Кстати, поиск неинициализированных членов - неблагодарная работа, и возможно читателям будет интересно познакомиться со статьёй "Поиск неинициализированных членов класса".
Однако вернемся к проекту UE4. В статье я не буду касаться предупреждения V730. Их слишком много, и я слишком плохо знаю проект UE4, чтобы судить, может-ли привести та или иная неинициализированная переменная к ошибке в работе программы. Однако я уверен, что среди этих 1672 предупреждения скрывается немало настоящих серьезных ошибок. Думаю, разработчикам из Epic Games стоит проанализировать их. Если же они сочтут, что это сплошные ложные срабатывания, то тогда эту диагностику можно просто отключить.
Итак, 1792 - 1672 = 120. Итого PVS-Studio выдал 120 предупреждений общего назначения (1 и 2 уровень) при проверке Unreal Engine. Множество из этих предупреждений выявили настоящие ошибки. Рассмотрим наиболее интересные фрагменты кода и соответствующие им предупреждения.
Ещё раз подчеркну, что в статье я приведу далеко не все участки кода, которые заслуживают внимания и правки. Во-первых, я смотрел отчет бегло и мог пропустить что-то интересное. Во-вторых, я не выписывал несерьезные ошибки или те, которые сложно объяснить (требуется привести много фрагментов кода).
Ошибка N1
FORCEINLINE
bool operator==(const FShapedGlyphEntryKey& Other) const
{
return FontFace == Other.FontFace
&& GlyphIndex == Other.GlyphIndex
&& FontSize == Other.FontSize
&& FontScale == Other.FontScale
&& GlyphIndex == Other.GlyphIndex;
}Предупреждение PVS-Studio: V501 There are identical sub-expressions 'GlyphIndex == Other.GlyphIndex' to the left and to the right of the '&&' operator. fontcache.h 139
Два раза повторяется проверка "GlyphIndex == Other.GlyphIndex". Эффект последней строки в действии. По всей видимости, последним сравнением должно быть: KeyHash == Other.KeyHash.
Ошибка N2
Ещё один эффект последней строки. Прямо-таки канонический.
bool
Compare(const FPooledRenderTargetDesc& rhs, bool bExact) const
{
....
return Extent == rhs.Extent
&& Depth == rhs.Depth
&& bIsArray == rhs.bIsArray
&& ArraySize == rhs.ArraySize
&& NumMips == rhs.NumMips
&& NumSamples == rhs.NumSamples
&& Format == rhs.Format
&& LhsFlags == RhsFlags
&& TargetableFlags == rhs.TargetableFlags
&& bForceSeparateTargetAndShaderResource ==
rhs.bForceSeparateTargetAndShaderResource
&& ClearValue == rhs.ClearValue
&& AutoWritable == AutoWritable;
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: AutoWritable == AutoWritable rendererinterface.h 180
В самом конце забыли дописать "rhs." и в результате переменная 'AutoWritable' сравнивается сама с собой.
Ошибка N3
void AEQSTestingPawn::PostLoad()
{
....
UWorld* World = GetWorld();
if (World && World->IsGameWorld() &&
bTickDuringGame == bTickDuringGame)
{
PrimaryActorTick.bCanEverTick = false;
}
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: bTickDuringGame == bTickDuringGame eqstestingpawn.cpp 157
Ошибка N4
int32 SRetainerWidget::OnPaint(....) const
{
....
if ( RenderTargetResource->GetWidth() != 0 &&
RenderTargetResource->GetWidth() != 0 )
....
}Предупреждение PVS-Studio: V501 There are identical sub-expressions 'RenderTargetResource->GetWidth() != 0' to the left and to the right of the '&&' operator. sretainerwidget.cpp 291
Ошибка N5, N6
Две одинаковых ошибки расположились по соседству. Макросы ZeroMemory, которые есть не что иное как вызовы функции memset(), обнуляют только часть выделенной памяти.
class FD3D12BufferedGPUTiming
{
....
FD3D12CLSyncPoint* StartTimestampListHandles;
FD3D12CLSyncPoint* EndTimestampListHandles;
....
};
void FD3D12BufferedGPUTiming::InitDynamicRHI()
{
....
StartTimestampListHandles = new FD3D12CLSyncPoint[BufferSize];
ZeroMemory(StartTimestampListHandles,
sizeof(StartTimestampListHandles));
EndTimestampListHandles = new FD3D12CLSyncPoint[BufferSize];
ZeroMemory(EndTimestampListHandles,
sizeof(EndTimestampListHandles));
....
}Предупреждения PVS-Studio:
Ошибка в том, что оператор sizeof() вычисляет размер указателя, а не массива. Одним из правильных вариантов, будет написать так:
ZeroMemory(StartTimestampListHandles,
sizeof(FD3D12CLSyncPoint) * BufferSize);
ZeroMemory(EndTimestampListHandles,
sizeof(FD3D12CLSyncPoint) * BufferSize);Ошибка N7
void FDeferredShadingSceneRenderer::RenderLight(....)
{
....
if (bClearCoatNeeded)
{
SetShaderTemplLighting<false, false, false, true>(
RHICmdList, View, *VertexShader, LightSceneInfo);
}
else
{
SetShaderTemplLighting<false, false, false, true>(
RHICmdList, View, *VertexShader, LightSceneInfo);
}
....
}Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. lightrendering.cpp 864
В независимости от условия, выполняются одинаковые действия.
Ошибка N8
bool FBuildDataCompactifier::Compactify(....) const
{
....
uint64 CurrentFileSize;
....
CurrentFileSize = IFileManager::Get().FileSize(*File);
if (CurrentFileSize >= 0)
{
....
}
else
{
GLog->Logf(TEXT("Warning. ......"), *File);
}
....
}Предупреждение PVS-Studio: V547 Expression 'CurrentFileSize >= 0' is always true. Unsigned type value is always >= 0. buildpatchcompactifier.cpp 135
Проверка "if (CurrentFileSize >= 0)" не имеет смысла. Переменная 'CurrentFileSize' имеет беззнаковый тип, а значит её значение всегда >= 0.
Ошибка N9
template<typename TParamRef>
void UnsetParameters(....)
{
....
int32 NumOutUAVs = 0;
FUnorderedAccessViewRHIParamRef OutUAVs[3];
OutUAVs[NumOutUAVs++] = ObjectBuffers......;
OutUAVs[NumOutUAVs++] = ObjectBuffers.Bounds.UAV;
OutUAVs[NumOutUAVs++] = ObjectBuffers.Data.UAV;
if (CulledObjectBoxBounds.IsBound())
{
OutUAVs[NumOutUAVs++] = ObjectBuffers.BoxBounds.UAV;
}
....
}V557 Array overrun is possible. The 'NumOutUAVs ++' index is pointing beyond array bound. distancefieldlightingshared.h 388
Если условие (CulledObjectBoxBounds.IsBound()) выполнится, то произойдёт выход за границы массива. Обратите внимание, что массив 'OutUAVs' состоит только из 3 элементов.
Ошибка N10
class FSlateDrawElement
{
....
FORCEINLINE void SetPosition(const FVector2D& InPosition)
{ Position = Position; }
....
FVector2D Position;
....
};Предупреждение PVS-Studio: V570 The 'Position' variable is assigned to itself. drawelements.h 435
Эту ошибку даже описывать не нужно. Опечатка. Должно быть: { Position = InPosition; }.
Ошибка N11
bool FOculusRiftHMD::DoesSupportPositionalTracking() const
{
const FGameFrame* frame = GetFrame();
const FSettings* OculusSettings = frame->GetSettings();
return (frame && OculusSettings->Flags.bHmdPosTracking &&
(OculusSettings->SupportedTrackingCaps &
ovrTrackingCap_Position) != 0);
}Предупреждение PVS-Studio: V595 The 'frame' pointer was utilized before it was verified against nullptr. Check lines: 301, 302. oculusrifthmd.cpp 301
Сначала переменная 'frame' используется, а затем проверяется на равенство nullptr.
Эта ошибка очень схожа с той, что описана в статье Klocwork:
bool FHeadMountedDisplay::IsInLowPersistenceMode() const
{
const auto frame = GetCurrentFrame();
const auto FrameSettings = frame->Settings;
return frame && FrameSettings->Flags.bLowPersistenceMode;
}Как видите, оба анализатора умеют выявлять данный вид дефекта.
Стоит отметить, что код приводимый в статье Klocwork, относится к каталогу ThirdParty, проекты из которого мы не проверяли.
Ошибка N12 - N21
FName UKismetNodeHelperLibrary::GetEnumeratorName(
const UEnum* Enum, uint8 EnumeratorValue)
{
int32 EnumeratorIndex = Enum->GetIndexByValue(EnumeratorValue);
return (NULL != Enum) ?
Enum->GetEnum(EnumeratorIndex) : NAME_None;
}Предупреждение PVS-Studio: V595 The 'Enum' pointer was utilized before it was verified against nullptr. Check lines: 146, 147. kismetnodehelperlibrary.cpp 146
Опять ситуация, когда в начале указатель разыменован, и только затем проверен. Рассматривать такие ошибки дальше скучно. Просто приведу списком места, на которые стоит обратить внимание:
Ошибка N22
class FD3D12Device
{
....
virtual void InitD3DDevice();
virtual void CleanupD3DDevice();
....
// Деструктор не объявлен
....
};V599 The virtual destructor is not present, although the 'FD3D12Device' class contains virtual functions. d3d12device.cpp 448
В классе FD3D12Device имеются виртуальные методы. Это значит, что предполагается, что у этого класса будут наследники. Однако в этом классе отсутствует виртуальный деструктор. Это очень опасно и скорее всего рано или поздно приведёт к ошибке.
Ошибка N23-N26
int SpawnTarget(WCHAR* CmdLine)
{
....
if(!CreateProcess(....))
{
DWORD ErrorCode = GetLastError();
WCHAR* Buffer = new WCHAR[wcslen(CmdLine) + 50];
wsprintf(Buffer,
L"Couldn't start:\n%s\nCreateProcess() returned %x.",
CmdLine, ErrorCode);
MessageBoxW(NULL, Buffer, NULL, MB_OK);
delete Buffer;
return 9005;
}
....
}Предупреждение 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 [] Buffer;'. bootstrappackagedgame.cpp 110
Неправильным способом освобождается выделенная память. Должно быть написано:
delete [] Buffer;Есть ещё несколько таких ошибок:
Ошибка N27
void FSlateTexture2DRHIRef::InitDynamicRHI()
{
....
checkf(GPixelFormats[PixelFormat].BlockSizeX ==
GPixelFormats[PixelFormat].BlockSizeY ==
GPixelFormats[PixelFormat].BlockSizeZ == 1,
TEXT("Tried to use compressed format?"));
....
}Предупреждение PVS-Studio: V709 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. slatetextures.cpp 67
Проверка работает совсем не так, как задумывал программист. По всей видимости следовало написать:
GPixelFormats[PixelFormat].BlockSizeX == 1 &&
GPixelFormats[PixelFormat].BlockSizeY == 1 &&
GPixelFormats[PixelFormat].BlockSizeZ == 1Ошибка N28
void UWidgetComponent::UpdateRenderTarget()
{
....
FLinearColor ActualBackgroundColor = BackgroundColor;
switch ( BlendMode )
{
case EWidgetBlendMode::Opaque:
ActualBackgroundColor.A = 1.0f;
case EWidgetBlendMode::Masked:
ActualBackgroundColor.A = 0.0f;
}
....
}V519 The 'ActualBackgroundColor.A' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 938, 940. widgetcomponent.cpp 940
Здесь косвенным образом выявлен пропущенный оператор 'break'. Переменной 'ActualBackgroundColor.A' два раза подряд могут быть присвоены разные значения. Это и настораживает анализатор PVS-Studio.
Ошибка N29
void FProfilerManager::TrackDefaultStats()
{
// Find StatId for the game thread.
for( auto It = GetProfilerInstancesIterator(); It; ++It )
{
FProfilerSessionRef ProfilerSession = It.Value();
if( ProfilerSession->GetMetaData()->IsReady() )
{
....;
}
break;
}
}Предупреждение PVS-Studio: V612 An unconditional 'break' within a loop. profilermanager.cpp 717
Очень подозрительный код. Кажется, оператор 'break' расположен не на своём месте. Я не уверен, но возможно планировалось написать так:
for( auto It = GetProfilerInstancesIterator(); It; ++It )
{
FProfilerSessionRef ProfilerSession = It.Value();
if( ProfilerSession->GetMetaData()->IsReady() )
{
....;
break;
}
}По крайней мере 29 предупреждений из 120, выданных PVS-Studio указали на настоящие ошибки (24%). Ещё около 50%, это код, который пахнет. Оставшееся - ложные срабатывания. Суммарное затраченное мной время на проверку проекта и написание этой статьи - около 10 часов.
Какие выводы можно сделать по результатам работы анализаторов PVS-Studio и Klocwork:
Спасибо всем за внимание.