For Developers Working With Unreal Engine

Static Code Analysis With PVS-Studio (Part 3)

by

It’s been a while since our last blog entry – so we’re back with another batch of fixes, helped along by Viva64’s PVS-Studio again!

Let’s jump right in…


I was making some super-cool change to UnrealBuildTool that, sadly, I can’t tell you about just yet … but while I was doing that, PVS jumped in with a couple of neat observations:-

  • V3001 There are identical sub-expressions to the left and to the right of the ‘||’ operator.
  • V3063 A part of conditional expression is always false.

The code in question is found in ProjectFileGenerator.cs, here:-

// Not expecting to have both a game and a program in the same project. These would alias because we share the project and solution configuration names (just because it makes sense to)
if (ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Game && ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Program ||
 ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Program && ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Game)
{
   throw new BuildException("Not expecting project {0} to already have a Game/Program target ({1}) associated with it while trying to add: {2}", ProjectFilePath, ExistingProjectTarget.TargetRules.ToString(), TargetRulesObject.ToString());
}

So… we’re saying something along the lines of “if the animal is a cat and a dog, or the animal is a dog and a cat”, warn the user. Two minor problems with this of course:-

  1. If the animal is a dog, it’s a dog. It’s not a cat. Full stop;
  2. If it were possible to be a dog AND a cat, it would obviously also be a cat AND a dog… (!?!)

The correct code, of course, should be:-

// Not expecting to have both a game and a program in the same project. These would alias because we share the project and solution configuration names (just because it makes sense to)
if ((ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Game && TargetRulesObject.Type == TargetRules.TargetType.Program)
   || (ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Program && TargetRulesObject.Type == TargetRules.TargetType.Game))
{
   throw new BuildException("Not expecting project {0} to already have a Game/Program target ({1}) associated with it while trying to add: {2}", ProjectFilePath, ExistingProjectTarget.TargetRules.ToString(), TargetRulesObject.ToString());
}

This made PVS much happier – note:-

  • Congratulations! PVS-Studio has not found any issues in your source code!

While we’re looking at this, I wonder what the following line, just above the code quoted above, is trying to tell the user…?!?

throw new BuildException("Not expecting project {0} to already have a target rules of with configuration name {1} ...

The programmer clearly gained a 100% score in the “Baffling Warning Messages 101” class.


Another copy-paste bug next … I’ll just give you the code and the fix in this case – I doubt this one would actually affect anyone … but, then, if it doesn’t, it begs the question of what the code’s there for at all I guess … so it might as well be patched up.

In MovieScene3DAttachSection.cpp, note the following lines:-

if (!bConstrainRx)
{
   OutRotation.Roll = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Roll;
}

if (!bConstrainRx)
{
   OutRotation.Pitch = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Pitch;
}

if (!bConstrainRx)
{
   OutRotation.Yaw = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Yaw;
}

You can just go ahead and change these to the following:-

if (!bConstrainRx)
{
   OutRotation.Roll = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Roll;
}

if (!bConstrainRy)
{
   OutRotation.Pitch = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Pitch;
}

if (!bConstrainRz)
{
   OutRotation.Yaw = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Yaw;
}

That being said, a better fix to avoid confusion might be to use better naming – so bConstrainRz, for example, would become bConstrainYaw (that’s just good coding practice IMO).


There’s a possible array overrun in SteamVRController.cpp (from the code, I guess the overrun would only happen if you had more than 4 SteamVR controllers in use?)

  • V557 Array overrun is possible. The value of ‘DeviceIndex’ index could reach 15.

on the 3rd line of this block of code:-

DeviceToControllerMap[DeviceIndex] = FMath::FloorToInt(NumControllersMapped / CONTROLLERS_PER_PLAYER);
ControllerToDeviceMap[NumControllersMapped] = DeviceIndex;
ControllerStates[DeviceIndex].Hand = (EControllerHand)(NumControllersMapped % CONTROLLERS_PER_PLAYER);
++NumControllersMapped;

DeviceIndex is within the range of [0, vr::k_unMaxTrackedDeviceCount) with k_unMaxTrackedDeviceCount defined to be 16.

NumControllersMapped, on the other hand, is within the range [0, MaxControllers) with MaxControllers set to be 8 by way of MaxUnrealControllers and MAX_STEAMVR_CONTROLLER_PAIRS.

I don’t have any SteamVR controllers to test with, and certainly don’t have 5+, so can’t easily test a fix for this… but an educated guess tells me that the correct code should be:-

ControllerStates[NumControllersMapped].Hand = (EControllerHand)(NumControllersMapped % CONTROLLERS_PER_PLAYER);

We can also go ahead and fix a bug later in the sourcefile before it happens by changing the following:-

 int32 ControllerIndex = DeviceToControllerMap[DeviceIndex];
 FControllerState& ControllerState = ControllerStates[ DeviceIndex ];
 EControllerHand HandToUse = ControllerState.Hand;

to:-

int32 ControllerIndex = DeviceToControllerMap[DeviceIndex];
FControllerState& ControllerState = ControllerStates[ControllerIndex];
EControllerHand HandToUse = ControllerState.Hand;

As I say, I can’t test this … but, regardless, the code as it was would never have worked.


There’s a small problem in RHIDefinitions.h, identified by the warning:-

  • V501 There are identical sub-expressions ‘Platform == SP_OPENGL_ES2_ANDROID’ to the left and to the right of the ‘||’ operator.

The code as it currently stands:-

inline bool RHIHasTiledGPU(const EShaderPlatform Platform)
{
   return (Platform == SP_METAL_MRT) || Platform == SP_METAL || Platform == SP_OPENGL_ES2_IOS || Platform == SP_OPENGL_ES2_ANDROID || Platform == SP_OPENGL_ES2_ANDROID;
}

This problem came about with the following change:-

  • Rename shader platform SP_OPENGL_ES2 to SP_OPENGL_ES2_ANDROID
  • [CL 2705741 by Jack Porter in Main branch]

So it looks like the correct code would be just:-

inline bool RHIHasTiledGPU(const EShaderPlatform Platform)
{
   return (Platform == SP_METAL_MRT) || Platform == SP_METAL || Platform == SP_OPENGL_ES2_IOS || Platform == SP_OPENGL_ES2_ANDROID;
}

UE4’s CrossCompiler has a small snafu found by PVS with the warning:-

  • V547 Expression is always false.

Note the offending code in ShaderCompilerCommon.cpp:-

CCTCmdLine += ((CCFlags & HLSLCC_PackUniformsIntoUniformBuffers) == HLSLCC_PackUniformsIntoUniformBuffers) ? TEXT(" -packintoubs") : TEXT("");
CCTCmdLine += ((CCFlags & HLSLCC_FixAtomicReferences) == HLSLCC_PackUniformsIntoUniformBuffers) ? TEXT(" -fixatomics") : TEXT("");

The correct code being, of course:-

CCTCmdLine += ((CCFlags & HLSLCC_PackUniformsIntoUniformBuffers) == HLSLCC_PackUniformsIntoUniformBuffers) ? TEXT(" -packintoubs") : TEXT("");
CCTCmdLine += ((CCFlags & HLSLCC_FixAtomicReferences) == HLSLCC_FixAtomicReferences) ? TEXT(" -fixatomics") : TEXT("");

An interesting warning shows up for ScriptCore.cpp:-

  • V523 The ‘then’ statement is equivalent to the ‘else’ statement. scriptcore.cpp 1210

Here’s the code (from the current UE4 codebase):-

if (Function->FunctionFlags & FUNC_Native)
{
   Function->Invoke(this, NewStack, ReturnValueAdress);
}
else
{
   Function->Invoke(this, NewStack, ReturnValueAdress);
}

I can’t say for sure what’s intended here – though I can see through looking back at the file’s history that it’s been operating this way for a while… so perhaps this should just be:-

Function->Invoke(this, NewStack, ReturnValueAdress);

I doubt this change would make any difference – but it’s always best to try to keep the code clean…

  • you never know for sure that the compiler will optimize the code
  • if another programmer sees this code, they might waste time looking into whether or not this is actually a bug

Several of these warnings came up in BoostrapPackagedGame.cpp:-

  • 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;’.
  • 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 [] ChildCmdLine;’.

The problem here being that Buffer and ChildCmdLine are allocated like this:-

WCHAR* Buffer = new WCHAR[wcslen(CmdLine) + 50];
WCHAR* ChildCmdLine = new WCHAR[wcslen(BaseDirectory) + wcslen(ExecFile) + wcslen(BaseArgs) + wcslen(CmdLine) + 20];

But are then cleaned up with:-

delete Buffer;
delete ChildCmdLine;

These can just be changed to:-

delete [] Buffer;
delete [] ChildCmdLine;

(note that there are 3 occurrences of the latter)


And that wraps up another analysis of UE4 sourcecode. Hope you enjoyed it!


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


3 Comments

Leave a Reply

Your email address will not be published.

*

Latest from ALL

Placating The Natives

In this article we delve into Blueprint Nativization, a relatively new feature
Go to Top
%d bloggers like this: