For Developers Working With Unreal Engine

Static Code Analysis With PVS-Studio (Part 1)

by

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

Static Code Analyzers are a fantastic tool to have in your arsenal during development… often, they find and enable you to fix bugs before you even know about them.

PVS-Studio (www.viva64.com/en/pvs-studio/) is one of the best that I’ve used. Highly recommended, particularly to AAA development studios.

In part 1 of this series, I’ll give you just two out of the many things that PVS brought to our attention.


First up, let’s take a look at a “duplicate code” warning that it gave us in ShowFlags.cpp:-

EViewModeIndex FindViewMode(const FEngineShowFlags& EngineShowFlags)
{
  if(EngineShowFlags.VisualizeBuffer)
  {
    return VMI_VisualizeBuffer;
  }
  else if(EngineShowFlags.StationaryLightOverlap)
  {
    return VMI_StationaryLightOverlap;
  }
  else if(EngineShowFlags.ShaderComplexity)
  {
    return VMI_ShaderComplexity;
  }
  // Test QuadComplexity before ShaderComplexity because QuadComplexity also use ShaderComplexity
  else if(EngineShowFlags.QuadComplexity)
  {
    return VMI_QuadComplexity;
  }
  else if(EngineShowFlags.WantedMipsAccuracy)
  {
    return VMI_WantedMipsAccuracy;
  }
  else if(EngineShowFlags.TexelFactorAccuracy)
  {
    return VMI_TexelFactorAccuracy;
  }
  else if(EngineShowFlags.ShaderComplexity)
  {
    return VMI_ShaderComplexity;
  }
  else if(EngineShowFlags.VisualizeLightCulling)
  {
    return VMI_LightComplexity;
  }
<-- LOTS MORE CODE -->
  return EngineShowFlags.Lighting ? VMI_Lit : VMI_Unlit;
}

See the comment in the middle about testing QuadComplexity before ShaderComplexity? Well, right before it – a ShaderComplexity test. And, not too far after, a second ShaderComplexity test. Doh! The correct code should have the first test removed, of course.


The second problem that I’m going to show you is within UnrealHeaderTool. Note the last 2 lines of the following function:-

bool FManifestModule::IsCompatibleWith(const FManifestModule& ManifestModule)
{
  return Name == ManifestModule.Name
    && ModuleType == ManifestModule.ModuleType
    && LongPackageName == ManifestModule.LongPackageName
    && BaseDirectory == ManifestModule.BaseDirectory
    && IncludeBase == ManifestModule.IncludeBase
    && GeneratedIncludeDirectory == ManifestModule.GeneratedIncludeDirectory
    && BaseDirectory == ManifestModule.BaseDirectory
    && PublicUObjectClassesHeaders == ManifestModule.PublicUObjectClassesHeaders
    && PublicUObjectHeaders == ManifestModule.PublicUObjectHeaders
    && PrivateUObjectHeaders == ManifestModule.PrivateUObjectHeaders
    && PCH == ManifestModule.PCH
    && GeneratedCPPFilenameBase == ManifestModule.GeneratedCPPFilenameBase
    && SaveExportedHeaders == SaveExportedHeaders
    && GeneratedCodeVersion == GeneratedCodeVersion;
}

which, of course, should be changed to:-

    && SaveExportedHeaders == ManifestModule.SaveExportedHeaders
    && GeneratedCodeVersion == ManifestModule.GeneratedCodeVersion;

 


So, as you can see, PVS-Studio is pretty cool. The errors above are both pretty obvious, when they’re staring you in the face, but when you’re dealing with codebases as large as UE4, these problems can often stay hidden for quite some time.


Credit(s): Robert Troughton (Coconut Lizard)
Status: Fixed in 4.12


3 Comments

  1. Hello! I am one of PVS-Studio developers. Thanks a lot for the article, it’s great to see that you like the way the tool works.

    It should be said, that you won’t probably find a lot of bugs in the code. The thing is, that once we have already delved in code of this project and fixed those bugs that were found (at that moment of course)

    It all started with an article http://www.viva64.com/en/b/0249/

    Later we worked cooperatively with Epic Games company and this work had the following results: http://www.viva64.com/en/b/0330/

    Which means that at this moment you will be finding bugs that appeared after our fixes. But it’s not a big deal, we’ll be still very interested to see the articles you write. I just wanted to let you know about the previous articles.

    Thank you once more, we are looking forward to reading Part II.

    Best regards,
    Andrey Karpov

  2. Hi Robert, thanks for sharing your findings with others! What’s up with the latest “Patch content early – not at runtime” post, it’s protected and requires password to access. 🙁

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: