Static Code Analysis With PVS-Studio (Part 2)

UPDATE 3rd June 2016: the fixes below were added to the code base in UE4.12

In part 2 of this series, I’m just going to jump right in and give you some of the findings that PVS-Studio was able to highlight.

Since the developers of the software already ran passes over UE4 and fixed up issues found, there’s really not that many left to find.

Anyway, here we go…

A simple copy/paste bug was found in UHTMakeFileHeaderDescriptor.h with the warning:-

V524 It is odd that the body of ‘StopExporting’ function is fully equivalent to the body of ‘StartExporting’ function.

Checking the code, it looks like this:-

void StartExporting()
{
  LoadingPhase = EUHTMakefileLoadingPhase::Export;
}

void StopExporting()
{
  LoadingPhase = EUHTMakefileLoadingPhase::Export;
}

Should actually be:-

void StartExporting()
{
  LoadingPhase = EUHTMakefileLoadingPhase::Export;
}

void StopExporting()
{
  LoadingPhase = EUHTMakefileLoadingPhase::Max;
}

From Internationalization/Text.cpp, TextBiDi::IsControlCharacter.cpp:-

V501 There are identical sub-expressions ‘InChar == L’\u2068” to the left and to the right of the ‘||’ operator.

which is pretty easy to spot – note the final 2 tests in this code:-

bool TextBiDi::IsControlCharacter(const TCHAR InChar)
{
  return InChar == TEXT('\u061C')  // ARABIC LETTER MARK
    || InChar == TEXT('\u200E')  // LEFT-TO-RIGHT MARK
    || InChar == TEXT('\u200F')  // RIGHT-TO-LEFT MARK
...
    || InChar == TEXT('\u2068')  // FIRST STRONG ISOLATE
    || InChar == TEXT('\u2068'); // POP DIRECTIONAL ISOLATE
}

So it should, of course, be:-

bool TextBiDi::IsControlCharacter(const TCHAR InChar)
{
  return InChar == TEXT('\u061C')  // ARABIC LETTER MARK
    || InChar == TEXT('\u200E')  // LEFT-TO-RIGHT MARK
    || InChar == TEXT('\u200F')  // RIGHT-TO-LEFT MARK
...
    || InChar == TEXT('\u2068')  // FIRST STRONG ISOLATE
    || InChar == TEXT('\u2069'); // POP DIRECTIONAL ISOLATE
}

Within PVS-Studio, there are also options to find optimizations that can be made, it’s not just about finding bugs. So here’s a small change it suggests with the following warning:-

V808 ‘RTVs’ array of objects of ‘FRHIRenderTargetView’ type was created but was not utilized.

As you can see from the code below, it’s absolutely right:-

inline void TransitionSetRenderTargetsHelper(FRHICommandList& RHICmdList, uint32 NumRenderTargets, const FTextureRHIParamRef* NewRenderTargetsRHI, const FTextureRHIParamRef NewDepthStencilTargetRHI, FExclusiveDepthStencil DepthStencilAccess)
{
  FRHIRenderTargetView RTVs[MaxSimultaneousRenderTargets];
  FTextureRHIParamRef Transitions[MaxSimultaneousRenderTargets + 1];
...
  RHICmdList.TransitionResources(EResourceTransitionAccess::EWritable, Transitions, TransitionIndex);
}

The array at the top is never actually used… so we just delete it:-

inline void TransitionSetRenderTargetsHelper(FRHICommandList& RHICmdList, uint32 NumRenderTargets, const FTextureRHIParamRef* NewRenderTargetsRHI, const FTextureRHIParamRef NewDepthStencilTargetRHI, FExclusiveDepthStencil DepthStencilAccess)
{
  FTextureRHIParamRef Transitions[MaxSimultaneousRenderTargets + 1];
...
  RHICmdList.TransitionResources(EResourceTransitionAccess::EWritable, Transitions, TransitionIndex);
}

In DrawElements.h, FSlateDrawElement::SetPosition() isn’t going to do what’s expected of it:-

V570 The ‘Position’ variable is assigned to itself.

The bug is very clear from the code:-

FORCEINLINE void SetPosition(const FVector2D& InPosition) { Position = Position; }

Which should of course be:-

FORCEINLINE void SetPosition(const FVector2D& InPosition) { Position = InPosition; }

Finally, to wrap this up, I just want to show that bugs can’t only be fixed by static code analyzers… here’s a similar copy/paste style bug that I found myself (yeah, through hard graft!)…

This is in ShaderCache.cpp:-

int32 FShaderCache::GetPredrawBatchTime() const
{
  return OverridePrecompileTime == 0 ? PredrawBatchTime : OverridePrecompileTime;
}

int32 FShaderCache::GetTargetPrecompileFrameTime() const
{
  return OverridePredrawBatchTime == 0 ? TargetPrecompileFrameTime : OverridePredrawBatchTime;
}

Looking closely at the operations here, we see that this should actually be:-

int32 FShaderCache::GetPredrawBatchTime() const
{
  return OverridePredrawBatchTime == 0 ? PredrawBatchTime : OverridePredrawBatchTime;
}

int32 FShaderCache::GetTargetPrecompileFrameTime() const
{
  return OverridePrecompileTime == 0 ? TargetPrecompileFrameTime : OverridePrecompileTime;
}

And that’s it for Part 2!

Credit(s): Robert Troughton (Coconut Lizard),
Evgeniy Ryzhkov (Viva64)
Status: Fixed in 4.12