Static Code Analysis - Part One

By RobertTroughton 25 October 2023 Coding 6 min read

What Is Static Code Analysis (SCA) #

Static Code Analysis is a crucial tool in software development. It identifies potential bugs, security vulnerabilities and performance inefficiences, all without needing to even run the program or game that you're developing.

By scanning every line of sourcecode it pinpoints issues such as:-

memory leaks
uninitialized variables
logic errors
common copy/paste mistakes
and much more

This preemptive approach can prevent critical bugs from reaching production - potentially saving both resources and money.

Available Tools #

Here's a quick list of some of the analysers that you could choose from:-

Visual Studio's in-built analyser
Clang's analyser
CPPCheck
PC-Lint
Coverity
PVS-Studio

It's this latter one, PVS-Studio, that I've spent many years utilising - so that's the one that I'll be focusing on here.

Setting Up Static Code Analysis In Unreal Engine #

There's a simple guide to getting started with SCA in Unreal's online docs, here.

So, in my case, I'll be using the following command to invoke analysis with PVS-Studio, which will perform full analysis of a development Win64 build of Unreal Editor.

Engine\Build\BatchFiles\RunUBT.bat UnrealEditor Win64 Development -StaticAnalyzer=PVSStudio

Once finished, there should be some files available in Engine/Saved/PVS-Studio. The .pvslog file can be opened in Visual Studio by selecting Extensions, PVS-Studio, Open/Save, Open Analysis Report:-

This should give you a window with some output along the lines of the below. As you can see from the numbers in the column tabs at the top, you can select to see various subsets of warnings - High, Medium and Low priorities, along with General and Optimization categories. It's common - especially with a codebase like Unreal Engine - to see many false-positives, warnings about things that are commonplace in the engine code.. it's something that you need to get used to and learn to move around.

Looking At An Actual Bug #

To jump straight in, let's take a look at one of the potential bugs that the analysis shows:-

V501 There are identical sub-expressions to the left and to the right of the '==' operator: Priority == Priority NaniteStreamingManager.h 204

And here's the code that this report's highlighting in NaniteStreamingManager.h:-

	struct FVirtualPage
	{
		uint32 Priority				= 0u;						// Priority != 0u means referenced this frame
		uint32 RegisteredPageIndex	= INDEX_NONE;

		FORCEINLINE bool operator==(const FVirtualPage& Other) const
		{
			return Priority == Priority && RegisteredPageIndex == Other.RegisteredPageIndex;
		}
	};

Hopefully you can see straightaway that the problem here is on the 6th line. We're checking to see whether Priority == Priority .. which, oddly enough, it always does ;p

Clearly the corrected line should be:-

			return Priority == Other.Priority && RegisteredPageIndex == Other.RegisteredPageIndex;

This is just an example, please note. At this point I don't actually know much about Nanite, how to use it, how to test it and so forth .. so I would need someone familiar with the system to look more deeply. Clearly, the fixed code above is what the programmer originally intended - but there could be unintended consequences for fixing the code.. with the line how it was originally, we were effectively doing:-

			return RegisteredPageIndex == Other.RegisteredPageIndex;

So, yeah, our "fix" adds an additional condition. It wouldn't be the first time if fixing the code unearths a bug further along that nobody's noticed.

A Copy/Paste Bug #

Take a look at this warning:-

V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 475, 480. MovieSceneTransformTrail.cpp 480

And the code that this is pointing us toward is in MovieSceneTransformTrail.cpp:-

bool FMovieSceneComponentTransformTrail::ApplyDelta(const FVector& Pos, const FRotator& Rot, const FVector& WidgetLocation)
{

//; some code omitted

	UMovieSceneSection* Section = GetSection();
	if (!Section)
	{
		return false;
	}
	UMovieScene3DTransformSection* TransformSection = Cast<UMovieScene3DTransformSection>(Section);
	if (!Section)
	{
		return false;
	}

Another clear fix - the code at the end should of course be checking TransformSection instead of Section (which had already been checked above):-

	UMovieScene3DTransformSection* TransformSection = Cast<UMovieScene3DTransformSection>(Section);
	if (!TransformSection)
	{
		return false;
	}

Finding Skeletons #

Another copy/paste bug here:-

V501 There are identical sub-expressions 'InOldBoneName == NAME_None' to the left and to the right of the '||' operator. SkeletonModifier.cpp 970

Stemming from the following code in SkeletonModifier.cpp:-

bool USkeletonModifier::RenameBone(const FName InOldBoneName, const FName InNewBoneName)
{
	if (InOldBoneName == NAME_None || InOldBoneName == NAME_None || InNewBoneName == InOldBoneName)
	{
		UE_LOG(LogAnimation, Error, TEXT("Skeleton Modifier - Rename: cannot rename %s with %s."), *InOldBoneName.ToString(), *InNewBoneName.ToString());
		return false;
	}
	
	return RenameBones({InOldBoneName}, {InNewBoneName});
}

Which should clearly begin (replacing the second check so that it verifies InNewBoneName instead of checking InOldBoneName for a 2nd time):-

	if (InOldBoneName == NAME_None || InNewBoneName == NAME_None || InNewBoneName == InOldBoneName)

A Dodgy String Compare? #

The following error points at some more code that, honestly, could never have worked as intended..?

V698 Expression 'Compare(....) == -1' is incorrect. This function can return not only the value '-1', but any negative value. Consider using 'Compare(....) < 0' instead. SNiagaraGraphActionMenu.cpp 149

The code (from SNiagaraGraphActionMenu.cpp):-

bool SNiagaraGraphActionMenu::OnCompareCategoriesForSorting(const FString& CategoryA,
	const FString& CategoryB)
{
	return CategoryA.Compare(CategoryB) == -1;
}

And here's that Compare() function (from UnrealString.h):-

	UE_NODISCARD FORCEINLINE int32 Compare( const FString& Other, ESearchCase::Type SearchCase = ESearchCase::CaseSensitive ) const
	{
		if( SearchCase == ESearchCase::CaseSensitive )
		{
			return FCString::Strcmp( **this, *Other ); 
		}
		else
		{
			return FCString::Stricmp( **this, *Other );
		}
	}

Strcmp() and Stricmp() won't just return 1, 0 and -1, of course, they will return non-zero values.. so the correct code would be:-

bool SNiagaraGraphActionMenu::OnCompareCategoriesForSorting(const FString& CategoryA,
	const FString& CategoryB)
{
	return CategoryA.Compare(CategoryB) < 0;
}

This same problem can also be seen in a few other files, including SNiagaraStackFunctionInputValue.cpp and SNiagaraStackItemGroupAddMenu.cpp ..

Code That Does Nothing #

There're quite a few instances of the V1078 warning:-

V1078 An empty container is iterated. The loop will not be executed. D:\Git\UnrealEngine\Engine\Plugins\Animation\AnimationData\Source\AnimationData\Private\AnimSequencerController.cpp 370

For example, in AnimSequencerController.cpp:-

bool UAnimSequencerController::RemoveBoneTracksMissingFromSkeleton(const USkeleton* Skeleton, bool bShouldTransact /*= true*/)
{

//; code truncated

		{
			TArray<FName> TracksToBeRemoved;
			TArray<FName> TracksUpdated;
			const FReferenceSkeleton& ReferenceSkeleton = Skeleton->GetReferenceSkeleton();


//; code truncated

						TracksToBeRemoved.Add(ExpectedBoneName);

//; code truncated

			if (TracksToBeRemoved.Num() || TracksUpdated.Num())
			{

//; code truncated

				for (const FName& TrackName : TracksUpdated)
				{
					FAnimationTrackChangedPayload Payload;
					Payload.Name = TrackName;
					Model->GetNotifier().Notify(EAnimDataModelNotifyType::TrackChanged, Payload);
				}

We have 2 arrays, TracksToBeRemoved and TracksUpdated. The second of these never changes, it's always empty... so there's either some missing code here (some lines to populate the TracksUpdated array) or TracksUpdated, and the code using it, can be removed. Given the function name, RemoveBoneTracksMissingFromSkeleton(), I'm thinking the former is the case... and that this is probably another case of sloppy cut and paste.

More Pointless Code #

Another of these can be seen here:-

V1078 An empty container is iterated. The loop will not be executed. D:\Git\UnrealEngine\Engine\Plugins\Experimental\ChaosNiagara\Source\ChaosNiagara\Private\NiagaraDataInterfaceChaosDestruction.cpp 1921

Which highlights the following code from NiagaraDataInterfaceChaosDestruction.cpp:-

bool UNiagaraDataInterfaceChaosDestruction::BreakingCallback(FNDIChaosDestruction_InstanceData* InstData)
{
	int32 IdxSolver = 0;
	for (FSolverData SolverData : Solvers)
	{
		if (SolverData.Solver->GetEventFilters()->IsBreakingEventEnabled() && BreakingEvents.Num() > 0 && SolverData.Solver->GetSolverTime() > 0.f && MaxNumberOfDataEntriesToSpawn > 0)
		{
			TArray<Chaos::FBreakingDataExt>& AllBreakingsArray = BreakingEvents;
			TMap<IPhysicsProxyBase*, TArray<int32>> AllBreakingsIndicesByPhysicsProxyMap;
			float TimeData_MapsCreated = SolverData.Solver->GetSolverTime();

			{
				size_t SizeOfAllBreakings = sizeof(Chaos::FBreakingData) * AllBreakingsArray.Num();
				size_t SizeOfAllBreakingsIndicesByPhysicsProxy = 0;
				for (auto& Elem : AllBreakingsIndicesByPhysicsProxyMap)
				{
					SizeOfAllBreakingsIndicesByPhysicsProxy += sizeof(int32) * (Elem.Value).Num();
				}
				SET_MEMORY_STAT(STAT_AllBreakingsDataMemory, SizeOfAllBreakings);
				SET_MEMORY_STAT(STAT_AllBreakingsIndicesByPhysicsProxyMemory, SizeOfAllBreakingsIndicesByPhysicsProxy);
			}

As you can see, AllBreakingsIndicesByPhysicsProxyMap will always be empty .. so there's no point iterating over it.

An Entire Pointless Function #

One final one of these to look at (by the way, there were 55 V1078 warnings in total):-

V1078 An empty container is iterated. The loop will not be executed. D:\Git\UnrealEngine\Engine\Source\Runtime\Engine\Private\Materials\HLSLMaterialTranslator.cpp 1945

The code, from HLSLMaterialTranslator.cpp clearly shows the problem on the first few lines:-

void FHLSLMaterialTranslator::ValidateShadingModelsForFeatureLevel(const FMaterialShadingModelField& ShadingModels)
{
	if (FeatureLevel <= ERHIFeatureLevel::ES3_1)
	{
		const TArray<EMaterialShadingModel>& InvalidShadingModels = {};
		for (EMaterialShadingModel InvalidShadingModel : InvalidShadingModels)
		{
			if (ShadingModels.HasShadingModel(InvalidShadingModel))
			{
				FString FeatureLevelName;
				GetFeatureLevelName(FeatureLevel, FeatureLevelName);

				FString ShadingModelName;
				const UEnum* EnumPtr = FindObject<UEnum>(nullptr, TEXT("/Script/Engine.EMaterialShadingModel"), true);
				if (EnumPtr)
				{
					ShadingModelName = EnumPtr->GetNameStringByValue(InvalidShadingModel);
				}

				Errorf(TEXT("ShadingModel %s not supported in feature level %s"), *ShadingModelName, *FeatureLevelName);
			}
		}
	}
}

InvalidShadingModels is never populated and the for() loop will never do any work. Maybe the programmer intended to come back to this code at a later date in order to finish the function? Who knows.. if that were the case, I'd highly recommend simply adding a comment along the lines of:-

//; TODO-RTroughton: Add the invalid shading models here!

But that's just me .. and, for all I know, the problem above could equally be down to code merging, code refactoring, or any other number of things. It's worth looking at, though.

Wrapping Up #

I doubt that any of these "fixes" would lead to anything noticeably improving .. but there's a good reason why there aren't so many "big hitting" bugs immediately apparent in the Unreal Engine codebase.. (1) community members, such as myself, already let Epic know about them; and (2) Epic started using PVS-Studio themselves some years ago, soon after I'd highlighted many of the bugs I'd found with it to them, and had integrated it into their build pipeline at the time.

Please note that my intention with all of the above isn't so much to fix the problems, which I appreciate could be useful, it's more to bring light to them. When using SCA in my day job, I would often just refer to the relevant programmer on the team to let them know about a potential problem. I don't like to traipse all over someone else's code - apart from the fact that I might unwittingly break something myself, I don't want to end up as "the guy who last changed the Niagare code"! .. which, when much of the Niagara team moves on, can leave you as "the Niagara expert!, even though you only changed one line of code that really didn't make much difference at all.

So, in summary, PVS-Studio, or another Static Code Analyser, should absolutely be part of your arsenal during development. It's especially useful if you're going to be changing, adapting and expanding the Unreal Engine codebase for your game. Coders don't like to admit it - but we do make mistakes.

Until next time!

Next: Using ChatGPT to Learn New Tricks