| 编辑推荐: |
| 本文来自于csdn，本文章主要通过一个新项目对使用虚幻引擎的开发，PVS-Studio在改进虚幻引擎源码方面所发挥的作用，希望对您的学习有所帮助。 |
|
序言：Epic Games对使用PVS-Studio不断地改进引擎表示非常感兴趣。我们分析并修复虚幻引擎的源码，以便可以完全清除缺陷，使得工具最终不会生成任何误报的缺陷。之后，Epic将在基础代码上自己应用PVS-Studio，从而使得将该工具集成到开发过程中变得更简单、更顺利。
将PVS-STUDIO集成到虚幻引擎的编译过程中
为了管理编译过程，虚幻引擎使用了其自己的编译系统 – 虚幻编译工具。还有一些脚本，用于针对多个不同的平台和编译器生成项目文件。因为PVS-Studio
设计时最初是为了同Microsoft Visual C++编译器协同工作，所以我们使用了相应的脚本来为Microsoft
Visual Studio集成开发环境生成项目文件(*.vcxproj) 。
PVS-Studio带有一个插件，该插件可以集成到Visual Studio集成开发环境中，启动“一键”分析。但是，为虚幻引擎生成的项目不是Visual
Studio使用的“正常的”MSBuild 项目。
当从Visual Studio编译虚幻引擎时，该集成开发环境在启动编译过程时调用MSBuild，但是MSBuild本身只是作为运行虚幻编译工具程序的“封装器”。
要想在PVS-Studio中解析源码，该工具需要预处理器的输出 – 一个*.i 文件，该文件内具有了所有包含的头文件及展开的宏。
快速提示。如果您使用像虚幻引擎这样的自定义编译过程，并且如果您考虑在其编译过程具有复杂的独特性的项目上尝试PVS-Studio，那么这部分才有意义，我推荐您一直阅读该部分到结束处。或许，这会对您的情况有所帮助。但是，如果您的项目是正常的
Visual Studio项目或者没有耐心阅读我们找到的这些缺陷，那么您可以跳过这部分。
为了正确地启动预处理器，该工具需要关于编译参数的信息。在“正常的”MSBuild 项目中，这个信息是固有的；PVS-Studio插件可以“看到”它并自动为稍后将要调用的分析器处理所有必要的源文件。然而对于虚幻引擎项目，处理却有所不同。
正如我上面已经说的，当虚幻编译工具真正地调用编译器时它们的项目仅是一个“封装器”。这也是为什么在这种情况下Visual
Studio的PVS-Studio插件无法获得编译参数的原因。您只是无法“一键式”地运行分析，但仍然可以使用插件来查看分析结果。
分析器(PVS-Studio.exe)本身是一个命令行应用程序，其使用方式类似于C++编译器。就像编译器一样，必须为每个源文件单独地启动该程序，通过命令行或相应文件传入该文件的编译参数。分析器将自动地选择及调用适当的预处理器并执行分析。
注意。还有另一种方法。您可以为每个提前准备好的预处理文件启动该分析器。
因此，集成 PVS-Studio分析器到编译过程中的通用解决方案是在调用编译器的地方调用它的exe文件，在我们这个示例中，也就是在编译系统
– 虚幻编译工具。当然，它将要求修改当前的编译系统，这可能是不理想的，正如我们现在所处的情形。由于这个原因，我们为像这样的情况创建了一个编译器，称为“拦截”系统
- Compiler Monitoring(编译器监视)。
Compiler Monitoring(编译器监视)系统可以“拦截”编译过程启动(对于Visual
C++的情形来说，这是cl.exe进程)，收集成功进行预处理所需的所有必要参数，然后为所有这些正在编译的文件再次启动预处理过程，以进行进一步分析。这便是我们所做的。
图片 1. 虚幻引擎项目的分析过程方案
虚幻引擎分析集成恰好是在编译进程之前调用的，监测进程(CLMonitor.exe)将执行所有必要步骤来进行预处理并在编译进程结束时启动分析器。要想运行监测进程，我们需要运行一个简单的命令：
CLMonitor.exe将会以“跟踪模式”调用其自身并终止。同时，另一个CLMonitor.exe进程将仍然在后台运行，用于“拦截”编译器调用。当编译进程完成时，我们需要运行另一个简单的命令：
| CLMonitor.exe
analyze "UE.plog" |
请注意：在PVS-Studio 5.26及其后续版本中，您需要写为：
| CLMonitor.exe
analyze –l "UE.plog" |
现在CLMonitor.exe将会启动对预先收集的源码文件的分析，将结果保存到UE.plog文件中，该文件可以轻松地在我们的集成开发环境插件中进行处理。
我们针对这个最有趣的虚幻引擎配置进行夜间编译，并在我们的连续集成服务器上进行分析。这种方法对于我们来说，首先可以确保我们的修改不会破坏版本；第二，在早晨可以获得关于虚幻引擎分析的一份新日志，并且该日志涵盖了前天所进行的全部修改。所以，在我们发送Pull
Request（拉入请求）来向GitHub上的虚幻引擎库提交我们的对虚幻引擎的修改之前，我们可以轻松地通过在服务器上重新编译它来确保当前版本在我们的库中是稳定的。
非线性的缺陷修复速度
我们已经解决了项目编译过程及解析相关的问题。现在让我们谈谈我们基于分析器输出的诊断日志进行缺陷修复相关的内容。
乍一看，分析器输出警告的数量逐日平稳下降似乎是自然而然的事情：随着代码中完成的修复的数量变化，某些PVS-Studio机制也会禁止大约同样数量的信息。
也就是，理论上讲，您可能期望看到类似这样的情形：
图片2. 完美图形。缺陷数量每天均匀下降。
然而，事实上，在缺陷修复初始阶段消息消除的速度要比在后期阶段快。首先是因为初始阶段，我们禁止了宏触发的警告，这帮助快速降低了问题的总体数量。第二，是因为我们最初修复了最明显的问题，而将比较复杂的问题推迟到后期修复。我可以解释这一点。我们想向Epic
Games展示我们已经开始处理，并且有一定进展。刚开始就处理难的问题然后卡在那里，让人感觉很奇怪，不是吗？
分析虚幻引擎代码并修复缺陷总共花了我们17个工作日的时间。我们的目标是消除所生成的所有一级和二级安全级别的分析信息。这里是我们工作的进展：
表格1. 每天剩余的警告的数量。
请注意红色的数字。在前两天，我们开始习惯该项目，然后禁止了一些宏中的警告，因此大大地降低了假阳性缺陷的数量。
17天的时间是很长的，我想解释下为什么要这么多时间。首先，不是整个团队都在处理该项目，仅是其中的两名成员。当然，在这期间，他们也要忙于其他任务。第二，我们对虚幻引擎的代码完全不熟悉，所以进行修复是非常困难的工作。我们必须时不时地停下来判断我们是否能够修复某个问题及如何修复。
现在，这里是同样的数据在平滑图表中的样子：
图片3. 警告数量随着时间变化的平滑表
实用结论 – 我们自己要记住并想告诉他人的经验：仅根据前几天的工作就预测修复所有警告所需的时间并不是个好主意。最初是缺陷修复速度可能非常快，所以所预测的时间可能过于乐观。
但是，我们仍然需要进行某种程度的预估。我认为一定有针对这样问题的魔法公式，希望我们日后能发现他并将它公布于众。但是目前，我们缺少统计数据来提供非常可靠的答案。
关于该项目中发现的缺陷
我们修复了很多代码段。这些修复从理论上讲可以分为3类：
1.真正的缺陷。我将向您展示几个作为示例。
2.实际上不是错误的代码片段，但是这些代码迷惑了分析器，所以这些代码也会令日后学习该代码的程序员困惑。换句话说，这也是需要修复的"粗略的"
代码。
3.进行编辑仅是因为需要“满足”在这些代码片段处生成假阳性缺陷的分析器的要求。我们正在尝试在一个特殊的单独文件中隔离这些错误的警告禁止或者在任何可能的时候改进分析器自身的工作。但是，在某些地方我们仍然需要进行一些重构来辅助分析器判断问题。
正如我之前所承诺的，以下是一些缺陷示例。我们选择了几个理解透彻的最有趣的缺陷。
PVS-Studio提供的第一个有趣的信息：V506 局部变量'NewBitmap'的指针存储在了该变量范围之外。这个指针将是无效的。fontcache.cpp
466
void GetRenderData(....)
{
....
FT_Bitmap* Bitmap = nullptr;
if( Slot->bitmap.pixel_mode == FT_PIXEL_MODE_MONO
)
{
FT_Bitmap NewBitmap;
....
Bitmap = &NewBitmap;
}
....
OutRenderData.RawPixels.AddUninitialized(
Bitmap->rows * Bitmap->width );
....
} |
NewBitmap对象的地址被保存到了Bitmap 指针中。问题出在就在此之后，NewBitmap的生命周期过期，然后它被销毁。所以，这变成了Bitmap指向一个已经销毁的对象。
当尝试使用指针来指向一个已销毁的对象的地址时，就会发生不明确的行为的问题。它所呈现的形式是未知的。如果您足够幸运，只要那个已销毁对象的数据（存储在堆栈中）不会被其他东西覆盖，程序可能运行几年都没问题，
修复该代码的正确方法是将NewBitmap的声明移动到‘if’操作符之外：
void GetRenderData(....)
{
....
FT_Bitmap* Bitmap = nullptr;
FT_Bitmap NewBitmap;
if( Slot->bitmap.pixel_mode == FT_PIXEL_MODE_MONO
)
{
FT_Bitmap_New( &NewBitmap );
// Convert the mono font to 8bbp from 1bpp
FT_Bitmap_Convert( FTLibrary, &Slot->bitmap,
&NewBitmap, 4 );
Bitmap = &NewBitmap;
}
else
{
Bitmap = &Slot->bitmap;
}
....
OutRenderData.RawPixels.AddUninitialized(
Bitmap->rows * Bitmap->width );
....
} |
PVS-Studio生成的下一个警告：V522 可能发生解引用空指针
'GEngine'。请检查逻辑条件。
void UGameplayStatics::DeactivateReverbEffect(....)
{
if (GEngine || !GEngine->UseSound())
{
return;
}
UWorld* ThisWorld = GEngine->GetWorldFromContextObject(....);
....
} |
如果Gengine指针不为空，则该函数返回，且一切顺利。如果该指针为空，那么将会对其解引用。
我们通过以下方式修复了该代码:
void UGameplayStatics::DeactivateReverbEffect(....)
{
if (GEngine == nullptr || !GEngine->UseSound())
{
return;
}
UWorld* ThisWorld = GEngine->GetWorldFromContextObject(....);
....
} |
在下一个代码片段中出现的是打印错误。分析器检测到了一个无意义的函数调用：V530 要求使用函数'Memcmp'的返回值。pathfollowingcomponent.cpp
715
int32 UPathFollowingComponent::OptimizeSegmentVisibility(
int32 StartIndex)
{
....
if (Path.IsValid())
{
Path->ShortcutNodeRefs.Reserve(....);
Path->ShortcutNodeRefs.SetNumUninitialized(....);
}
FPlatformMemory::Memcmp(Path->ShortcutNodeRefs.GetData(),
RaycastResult.CorridorPolys,
RaycastResult.CorridorPolysCount *
sizeof(NavNodeRef));
....
} |
Memcmp函数的返回结果没有被使用。这也是分析器所不喜欢的。
该程序员实际上是想通过Memcpy()函数复制一块内存，但是拼写错误了。这是修复后的版本：
int32 UPathFollowingComponent::OptimizeSegmentVisibility(
int32 StartIndex)
{
....
if (Path.IsValid())
{
Path->ShortcutNodeRefs.Reserve(....);
Path->ShortcutNodeRefs.SetNumUninitialized(....);
FPlatformMemory::Memcpy(Path->ShortcutNodeRefs.GetData(),
RaycastResult.CorridorPolys,
RaycastResult.CorridorPolysCount *
sizeof(NavNodeRef));
}
....
} |
现在，我们探讨一个您可能几乎在每个项目中都一定会遇到的诊断信息 – 它指向的缺陷也很常见。我们说的就是V595诊断。在我们的缺陷数据库中，它处于项目中频繁出现的缺陷列表的首位(请参照示例)。乍一看，那个列表还不如V501诊断大，但是实际上是因为V595
诊断从某种程度上讲是很无聊的，所以每个项目中很多这样的缺陷我们都没有写出来。我们通常仅显示一个示例并加一个注释，比如：及161条额外的诊断信息。这些缺陷中，有一半都是真实的错误。以下是它的看上去的样子：
图片4，恐怖的V595 诊断。
设计诊断规则 V595的目的是检测在判断指针是否为空之前解引用指针的代码片段。我们在我们分析的项目中总是能找到大量这样的缺陷。一个函数中的指针判断和解引用操作可能设置的非常远—间隔甚至几百行代码，这使得很难修复缺陷。但是也有些非常小的、非常有代表性的示例，比如，这个函数：
float SGammaUIPanel::OnGetGamma()
const
{
float DisplayGamma = GEngine->DisplayGamma;
return GEngine ? DisplayGamma : 2.2f;
} |
PVS-Studio的诊断信息：V595 在验证'GEngine'指针是否为空之前使用了该指针。请检查检查代码行：47,
48。gammauipanel.cpp 47
我们将通过以下方式修复该缺陷：
float SGammaUIPanel::OnGetGamma()
const
{
return GEngine ? GEngine->DisplayGamma : 2.2f;
} |
接着移动到下一个代码片段:
V517 发现使用了'if (A) {...} else if (A) {...}'形式。可能存在逻辑错误。请检查代码行：289,
299。 automationreport.cpp 289
void FAutomationReport::ClustersUpdated(const
int32 NumClusters)
{
...
//Fixup Results array
if( NumClusters > Results.Num() ) //<==
{
for( int32 ClusterIndex = Results.Num();
ClusterIndex < NumClusters; ++ClusterIndex
)
{
....
Results.Add( AutomationTestResult );
}
}
else if( NumClusters > Results.Num() ) //<==
{
Results.RemoveAt(NumClusters, Results.Num() -
NumClusters);
}
....
} |
在它的当前形式中，第二个条件将永远不为true。推断这个错误和其中所使用的符号有关系是合理的，因为它最初是想从'Result'数组中删除不必要的项。
void FAutomationReport::ClustersUpdated(const
int32 NumClusters)
{
....
//Fixup Results array
if( NumClusters > Results.Num() )
{
for( int32 ClusterIndex = Results.Num();
ClusterIndex < NumClusters; ++ClusterIndex
)
{
....
Results.Add( AutomationTestResult );
}
}
else if( NumClusters < Results.Num() )
{
Results.RemoveAt(NumClusters, Results.Num() -
NumClusters);
}
....
} |
这里是测试您注意力的代码示例。分析器警告：V616 以'DT_POLYTYPE_GROUND'命名值为0的常量应用在了逐位运算中。pimplrecastnavmesh.cpp
2006
/// Flags representing
the type of a navigation mesh polygon.
enum dtPolyTypes
{
DT_POLYTYPE_GROUND = 0,
DT_POLYTYPE_OFFMESH_POINT = 1,
DT_POLYTYPE_OFFMESH_SEGMENT = 2,
};
uint8 GetValidEnds(...., const dtPoly&
Poly)
{
....
if ((Poly.getType() & DT_POLYTYPE_GROUND)
!= 0)
{
return false;
}
....
} |
乍一看，一切都很好。您可能认为掩码分配了一些位，且已经判断了它的值。但是，实际上它仅是一个已命名的常量，定义在
'dtPolyTypes'枚举值中，它们并不意味着分配任何位。
在这个条件中， DT_POLYTYPE_GROUND常量等于0，这意味着该条件将永远为true。
修复后的代码:
uint8 GetValidEnds(....,
const dtPoly& Poly)
{
....
if (Poly.getType() == DT_POLYTYPE_GROUND)
{
return false;
}
....
} |
监测到的拼写错误:V501 '||'操作符的左右两侧有相同的子表达式：!bc.lclusters ||!bc.lclusters
detourtilecache.cpp 687
dtStatus dtTileCache::buildNavMeshTile(....)
{
....
bc.lcset = dtAllocTileCacheContourSet(m_talloc);
bc.lclusters = dtAllocTileCacheClusterSet(m_talloc);
if (!bc.lclusters || !bc.lclusters) //<==
return status;
status = dtBuildTileCacheContours(....);
....
} |
当复制粘帖一个变量时，程序员忘了将'bc.lclusters'重命名为'bc.lcset'。
定期分析结果
上面的示例并不是到目前为止在该项目中找到的所有缺陷，仅是其中的一小部分。我们引用这些示例来向您展示PVS-Studio可以找到何种缺陷，即使在顶级的、经过彻底测试的代码中也不例外。
然而，我们想提醒您，仅运行一次基础代码分析并不是使用静态分析器的好方法。该分析应该定期进行 – 这样您就能在代码初期阶段发现大量缺陷和拼写错误，而不是在测试或维护阶段才发现。
虚幻引擎项目是通过真实示例验证我们所说内容的好机会。
起初，我们在代码中修复缺陷时并没有跟踪它们是新增变更还是旧的内容。因为当有这么多缺陷要解决时，在初期修复阶段对判断这些并没有兴趣。但是，当我们将警告数量降低为0后，我们确实能够注意到PVS-Studio分析器是如何在新编代码或已修改代码中检测缺陷的。
事实上，完成这个代码花费了不止17天的时间。当我们停止编辑并在分析器中获得“零缺陷”信息时，我们必须等待两天来让虚幻引擎团队集成我们的最终Pull
Request（拉入请求）。在这期间，我们仍然从Epic的库中不断更新基础代码版本，并分析新代码。
我们可以看到分析器在这两天发现了新缺陷。这些缺陷，我们也进行了修复。这是验证定期进行静态分析检查是多么有用的好例子。
事实上，“警告数量”图表现在如下所示：
图片5.警告数量减到0后其增长速度的图解
现在，让我们看下在最后两天当我们分析项目代码的更新时，我们发现了哪些问题。
第一天
消息1：V560 条件表达式的一部分总为true：FBasicToken::TOKEN_Guid.
k2node_mathexpression.cpp 235
virtual FString
ToString() const override
{
if (Token.TokenType == FBasicToken::TOKEN_Identifier
||
FBasicToken::TOKEN_Guid) //<==
{
....
}
else if (Token.TokenType == FBasicToken::TOKEN_Const)
{
....
} |
程序员忘了写 "Token.TokenType =="。因为命名的常量'FBasicToken::TOKEN_Guid'不等于0，所以导致条件总为true。
消息2：V611 使用 'new T[]'操作符分配了内存，但是确实使用 'delete'操作符来释放该内存。请考虑检查该代码，最好使用
'delete [] CompressedDataRaw;'。crashupload.cpp 222
void FCrashUpload::CompressAndSendData()
{
....
uint8* CompressedDataRaw = new uint8[BufferSize];
//<==
int32 CompressedSize = BufferSize;
int32 UncompressedSize = UncompressedData.Num();
....
// Copy compressed data into the array.
TArray<uint8> CompressedData;
CompressedData.Append( CompressedDataRaw, CompressedSize
);
delete CompressedDataRaw; //<==
CompressedDataRaw = nullptr;
....
} |
这个缺陷在实际操作中并不是总会出现，因为我们正在处理的是char型数组的分配。但是它仍然是一个缺陷，会导致不明确的行为，必须进行修复。
第二天
消息1：V521 使用 ','操作符的表达式很危险。请确保该表达式是正确的。unrealaudiodevicewasapi.cpp
128
static void GetArrayOfSpeakers(....)
{
Speakers.Reset();
uint32 ChanCount = 0;
// Build a flag field of the speaker outputs of
this device
for (uint32 SpeakerTypeIndex = 0;
SpeakerTypeIndex < ESpeaker::SPEAKER_TYPE_COUNT,
//<==
ChanCount < NumChannels; ++SpeakerTypeIndex)
{
....
}
check(ChanCount == NumChannels);
} |
这是一个非常好的大缺陷。
逗号操作符 ','用于按照从左向右的顺序执行其两侧的两个表达式，并获得右侧操作数的值。
结果，循环终止条件仅通过以下判断进行表示：ChanCount < NumChannels
已修复的条件:
static void GetArrayOfSpeakers(....)
{
Speakers.Reset();
uint32 ChanCount = 0;
// Build a flag field of the speaker outputs of
this device
for (uint32 SpeakerTypeIndex = 0;
SpeakerTypeIndex < ESpeaker::SPEAKER_TYPE_COUNT
&&
ChanCount < NumChannels; ++SpeakerTypeIndex)
{
....
}
check(ChanCount == NumChannels);
} |
消息2。 V543 将值'-1'赋予HRESULT类型的变量 'Result'感觉很奇怪。unrealaudiodevicewasapi.cpp
568
#define S_OK
((HRESULT)0L)
#define S_FALSE ((HRESULT)1L)
bool
FUnrealAudioWasapi::OpenDevice(uint32 DeviceIndex,
EStreamType::Type StreamType)
{
check(WasapiInfo.DeviceEnumerator);
IMMDevice* Device = nullptr;
IMMDeviceCollection* DeviceList = nullptr;
WAVEFORMATEX* DeviceFormat = nullptr;
FDeviceInfo DeviceInfo;
HRESULT Result = S_OK; //<==
....
if (!GetDeviceInfo(DataFlow, DeviceIndex, DeviceInfo))
{
Result = -1; //<==
goto Cleanup;
}
....
} |
HRESULT 是一个分离到三个不同域的32-位的值：错误严重程度代码、设备码、错误代码。要想应用HRESULT，就要使用像S_OK、E_FAIL、E_ABORT等这样的特殊常量。要想判断HRESULT
值，应该使用类似SUCCEEDED和FAILED这样的宏。
警告V543仅当程序员尝试向HRESULT 型的变量写入-1、true或fals时才会出现。
写入值"-1"是错误的。如果您想报告一些未知错误，您应该使用0x80004005L
(未知错误)。这个常量及其他类似常量都定义在"WinError.h"文件中。
花费两个多星期来将静态分析集成到他们的项目中，肯能会令一些程序员及项目经理感到难过。但您不是必须这么做。您仅需要明白Epic
Games开发者选择了一条理想路径，而不是最简单、最快的路径。
是的，理想的情形就是立即去除所有缺陷，然后立即解决由新代码触发的新缺陷信息。但是，您也可以从花费时间来修复旧代码之前开始受益于静态分析。
出于这个考虑，PVS-Studio实际上提供了一种特殊的"消息标记"机制。以下是该功能的总体介绍：
在一个特殊的数据库中将分析器输出的所有消息标记为不活动状态。在那之后，用户便只能看到关于新编写的代码或者已修改的代码相关的信息了。也就是，您立刻可以应用静态分析。然后，当您有时间和心情时，您可以逐渐地处理旧代码相关的缺陷信息。
关于这个主题的详细内容，请查看以下源内容：文档, 如何快速地将静态分析集成到您的项目中。
"您把缺陷汇报给项目制作者了吗?"
每次检查一些项目并发布一片新文章后，大家就会问：“您把缺陷汇报给项目制作者了吗？”当然，我们一直这样做！但是这次，我们不仅“向项目制作者汇报了缺陷”，我们自己还修复了所有这些缺陷。任何感兴趣的人都可以在Github上的虚幻引擎库中受益（在您创建一个Epic
Games账户并链接到您的GitHub 账户之后）
总结
我们希望使用虚幻引擎的开发者将感谢PVS-Studio在改进虚幻引擎源码方面所发挥的作用，我们期望看到更多基于虚幻引擎的新项目！
以下是从我们工作结果中提取出来的一些最终总结：
1.虚幻引擎项目代码的质量非常高。不要太在意初始阶段的那些大量警告：这很正常。其中大部分警告都可以通过各种技术及设置消除。相对于一个如此庞大的项目来说，代码中所发现的真正缺陷的数量非常少。
2.修复您不熟悉的别人的代码通常是很难的。大部分程序员可能对此都有直观感受。我们也只是老生常谈。
3.解决分析器警告的速度并不是线性的。它的速度是逐渐下降的，并且当您在估算完成此项工作的时间时尤其要记住这一点。
4.您只有定期地应用静态分析，才能从中获得最大好处。 |
