Geekly Articles each Day
📜 ⬆️ ⬇️

The Empire Strikes Back

Darth Vader - The Unicorn
A recent article, “Hackathon 2: Time lapse analysis of Unreal Engine 4,” describes how you can find a lot of errors in the Unreal Engine 4 project using the Klocwork tool. I can't ignore this article. The fact is that in due time we corrected all the errors that the PVS-Studio analyzer found in this project. It is clear that not all errors were corrected at all, but only detected by the analyzer. However, the article gives the impression that the PVS-Studio analyzer missed too much. Well, now my move. I also rechecked the Unreal Engine 4 again and found a lot of errors. Thus, I can say that PVS-Studio can also find new errors in Unreal Engine 4 now. Draw.

History reference


It all started a year and a half ago, when I wrote the article "The long-awaited check of Unreal Engine 4 ". Further, there was a cooperation with the Epic Games company, the result of which was the elimination of all warnings issued by PVS-Studio. As a result, many errors were corrected and all false alarms of the analyzer were eliminated. Our team issued a project to Epic Games, free from PVS-Studio warnings. You can find out how this happened in the article " How the PVS-Studio team improved the Unreal Engine code ."

Recently, on the Internet, I came across an article " Hackathon 2: Time lapse analysis of Unreal Engine 4 ". The article is good and correct. The company Rogue Wave is great that it holds such events and makes a powerful static code analyzer Klocwork. Michail Greshishchev is also great that he did the work on testing the Unreal Engine and found time to write about this article. All is well. But I’m concerned that an outsider who is not familiar with static analyzers can make wrong conclusions. Therefore, I am obliged to comment on this article.

Unintentionally, but to the uninitiated reader, this article shows us in an unfavorable light compared to Klocwork. It seems that PVS-Studio finds fewer errors than Klocwork. In fact, the world is more complicated. Both analyzers have different sets of diagnostics. Partially these sets intersect. However, each analyzer has a unique set of diagnostics. Thus, having checked a large project with one analyzer, you will then always find something with another analyzer.
')
Another nuance. We did not check third-party libraries (ThirdParty catalog), and Michail Greshishchev checked (at least partially), as evidenced by one of the code examples (see the HeadMountedDisplayCommon function). In ThirdParty, the PVS-Studio analyzer can also easily find many interesting defects. Moreover, the size of the ThirdParty source is 3 times the size of UE4 itself.

However, all this sounds like a miserable attempt to justify himself :). Therefore, I have no choice but to even the score. For this purpose, the Unreal Engine 4 sources were taken and checked with the latest version of the PVS-Studio analyzer.

And now I will demonstrate that you can always easily find errors in a large, rapidly changing project.

Results of project verification using PVS-Studio


I checked the source code of UE4 with the latest version of PVS-Studio. The source codes of UE4 were checked without considering the ThirdParty catalog. If I check them, I will not get an article, but a reference book :).

I received 1,792 first and second level general purpose warnings. Do not be afraid. Now it becomes clear where this number came from.

Most warnings (93%) are associated with the new V730 diagnostic rule, designed to detect uninitialized class members. An uninitialized member of a class is not always a mistake, but nevertheless it is a place in the program that is worth checking out. Generally 1672 warnings V730 is a lot. On other projects, I have not seen a similar number of these warnings. Moreover, the analyzer, if possible, tries to predict when an uninitialized member is not a problem. By the way, the search for uninitialized members is a thankless job, and it may be interesting for readers to get acquainted with the article " Search for uninitialized members of a class ."

But back to the UE4 project. In the article, I will not touch the warning V730 . There are too many of them, and I know the project UE4 too poorly to judge whether this or that uninitialized variable can lead to an error in the program's work. However, I am sure that among these 1672 warnings there are quite a few real serious mistakes. I think the developers at Epic Games should analyze them. If they find that these are continuous false positives, then this diagnostics can be simply turned off.

So, 1792 - 1672 = 120. Total PVS-Studio issued 120 general-purpose warnings (level 1 and 2) when checking the Unreal Engine. Many of these warnings revealed real errors. Consider the most interesting code snippets and their corresponding warnings.

Interesting errors found with PVS-Studio


Once again I will emphasize that in the article I will give far from all parts of the code that deserve attention and editing. First, I watched the report fluently and could miss something interesting. Secondly, I did not write out frivolous errors or those that are difficult to explain (it takes a lot of code fragments).

Error n1
FORCEINLINE bool operator==(const FShapedGlyphEntryKey& Other) const { return FontFace == Other.FontFace && GlyphIndex == Other.GlyphIndex && FontSize == Other.FontSize && FontScale == Other.FontScale && GlyphIndex == Other.GlyphIndex; }

Warning PVS-Studio: V501 There are identical sub-expressions 'GlyphIndex == Other.GlyphIndex' operator. fontcache.h 139

The “GlyphIndex == Other.GlyphIndex” check is repeated twice. The effect of the last line in action. Apparently, the last comparison should be: KeyHash == Other.KeyHash.

N2 error

Another effect of the last line. Right canonical.
 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 warning: V501 operator: AutoWritable == AutoWritable rendererinterface.h 180

At the very end, they forgot to add “rhs.” And as a result the variable 'AutoWritable' is compared with itself.

Error n3
 void AEQSTestingPawn::PostLoad() { .... UWorld* World = GetWorld(); if (World && World->IsGameWorld() && bTickDuringGame == bTickDuringGame) { PrimaryActorTick.bCanEverTick = false; } }

PVS-Studio warning: V501 operator: bTickDuringGame == bTickDuringGame eqstestingpawn.cpp 157

Error n4
 int32 SRetainerWidget::OnPaint(....) const { .... if ( RenderTargetResource->GetWidth() != 0 && RenderTargetResource->GetWidth() != 0 ) .... }

PVS-Studio warning: V501 There are identical sub-expressions 'RenderTargetResource-> GetWidth ()! = 0' the operator. sretainerwidget.cpp 291

Error N5, N6

Two identical errors are located next door. ZeroMemory macros, which are nothing more than memset () function calls, reset only part of the allocated memory.
 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 warnings:
The error is that the sizeof () operator computes the size of a pointer, not an array. One of the correct options would be to write this:
 ZeroMemory(StartTimestampListHandles, sizeof(FD3D12CLSyncPoint) * BufferSize); ZeroMemory(EndTimestampListHandles, sizeof(FD3D12CLSyncPoint) * BufferSize);

Error 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 warning: V523 The 'then' statement is equivalent to the 'else' statement. lightrendering.cpp 864

Regardless of the condition, the same actions are performed.

Error n8
 bool FBuildDataCompactifier::Compactify(....) const { .... uint64 CurrentFileSize; .... CurrentFileSize = IFileManager::Get().FileSize(*File); if (CurrentFileSize >= 0) { .... } else { GLog->Logf(TEXT("Warning. ......"), *File); } .... }

PVS-Studio warning: V547 Expression 'CurrentFileSize> = 0' is always true. Unsigned type value is always> = 0. buildpatchcompactifier.cpp 135

The “if (CurrentFileSize> = 0)” check does not make sense. The variable 'CurrentFileSize' has an unsigned type, which means its value is always> = 0.

Error 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

If the condition (CulledObjectBoxBounds.IsBound ()) is met, then the array will be exceeded. Note that the 'OutUAVs' array consists of only 3 elements.

Error N10
 class FSlateDrawElement { .... FORCEINLINE void SetPosition(const FVector2D& InPosition) { Position = Position; } .... FVector2D Position; .... };

PVS-Studio warning: V570 The 'Position' variable is assigned to itself. drawelements.h 435

This error is not even necessary to describe. A typo. It should be: {Position = InPosition; }.

Error 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 warning: V595 The 'frame' pointer was used before it was verified against nullptr. Check lines: 301, 302. oculusrifthmd.cpp 301

First, the 'frame' variable is used, and then it is checked for nullptr equality.

This error is very similar to the one described in the Klocwork article :
 bool FHeadMountedDisplay::IsInLowPersistenceMode() const { const auto frame = GetCurrentFrame(); const auto FrameSettings = frame->Settings; return frame && FrameSettings->Flags.bLowPersistenceMode; }

As you can see, both analyzers are able to detect this type of defect.

It should be noted that the code given in the Klocwork article refers to the ThirdParty catalog, the projects from which we did not check.

Error 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 warning: V595 The 'Enum' pointer was used before it was verified against nullptr. Check lines: 146, 147. kismetnodehelperlibrary.cpp 146

Again the situation is when the pointer is dereferenced at the beginning, and only then checked. To consider such errors further boring. Just give a list of places you should pay attention to:
Error 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

The FD3D12Device class has virtual methods. This means that it is assumed that this class will have heirs. However, there is no virtual destructor in this class. This is very dangerous and is likely to lead to an error sooner or later.

Error 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 warning: V611, it was allocated using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] Buffer;'. bootstrappackagedgame.cpp 110

Invalid way, allocated memory is freed. It should be written:
 delete [] Buffer;

There are a few more such errors:
Error n27
 void FSlateTexture2DRHIRef::InitDynamicRHI() { .... checkf(GPixelFormats[PixelFormat].BlockSizeX == GPixelFormats[PixelFormat].BlockSizeY == GPixelFormats[PixelFormat].BlockSizeZ == 1, TEXT("Tried to use compressed format?")); .... }

PVS-Studio warning: V709 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. slatetextures.cpp 67

The check does not work as the programmer intended. Most likely it was necessary to write:
 GPixelFormats[PixelFormat].BlockSizeX == 1 && GPixelFormats[PixelFormat].BlockSizeY == 1 && GPixelFormats[PixelFormat].BlockSizeZ == 1

Error 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

Here indirectly identified the missing 'break' operator. The variable 'ActualBackgroundColor.A' may be assigned different values ​​two times in a row. This is alarming the PVS-Studio analyzer.

Error 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 warning: V612 An unconditional 'break' within a loop. profilermanager.cpp 717

Very suspicious code. It seems that the operator 'break' is not located in its place. I'm not sure, but maybe it was planned to write this:
 for( auto It = GetProfilerInstancesIterator(); It; ++It ) { FProfilerSessionRef ProfilerSession = It.Value(); if( ProfilerSession->GetMetaData()->IsReady() ) { ....; break; } }

Results


At least 29 warnings out of 120 issued by PVS-Studio indicated real errors (24%). About 50% more, this is a code that smells. The remaining - false positives. The total time I spent on checking the project and writing this article is about 10 hours.

What conclusions can be drawn from the results of the work of the PVS-Studio and Klocwork analyzers:
  1. In a large and rapidly developing project you can always find a few more mistakes :)
  2. The PVS-Studio and Klocwork diagnostic kits are different, although there is an intersection.
  3. Perhaps Klocwork tested Unreal Engine 4, including third-party libraries (ThirdParty). We did not check these libraries either then or now.
  4. Both analyzers are great, and using them can bring a lot of value to the software development process.

Thank you all for your attention.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. The Empire Strikes Back .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/270783/

More articles:


All Articles