PVS-Studio static code analysis

Jan 21, 2012 at 4:44 PM

I have decided to give PVS-Studio static code analyzer a try and ran its most recent version 4.53 over the newest revision of Hieroglyph3 - 73210. There are some interesting code parts pointed out by PVS-Studio:

On 32-bit platform, structure size can be reduced from 20 to 16 bytes by rearranging the fields according to their sizes in decreasing order. Hieroglyph3 ms3dspec.h 56

struct MS3DVertex
	{
		unsigned char	flags;                                      // SELECTED | SELECTED2 | HIDDEN
		float			vertex[3];                                  //
		char			boneId;                                     // -1 = no bone
		unsigned char	referenceCount;
	};

On 32-bit platform, structure size can be reduced from 44 to 40 bytes by rearranging the fields according to their sizes in decreasing order. Hieroglyph3 ms3dspec.h 75

struct MS3DGroup
	{
		unsigned char	flags;                              // SELECTED | HIDDEN
		char            name[32];                           //
		unsigned short	numtriangles;                       //
		unsigned short  *triangleIndices;      // the groups group the triangles
		char            materialIndex;                      // -1 = no material
	};

The 'pParameter' pointer was utilized before it was verified against nullptr. Check lines: 59, 62. Hieroglyph3 parametermanagerdx11.cpp 59

void ParameterManagerDX11::SetParameter( RenderParameterDX11* pParameter )
{
	const std::wstring& name = pParameter->GetName();
	RenderParameterDX11* pCurrent = m_Parameters[name];

	if ( pParameter )
	{

A part of conditional expression is always true: true. Hieroglyph3 intrray3fbox3f.cpp 117

if ( bNotAllClipped && ( true || fT0 != fSaveT0 || fT1 != fSaveT1 ) )

The 'pRenderer' pointer was utilized before it was verified against nullptr. Check lines: 147, 148. MFCwithD3D11 mfcwithd3d11.cpp 147

int CMFCwithD3D11App::ExitInstance()
{
	pRenderer->Shutdown();
	if ( pRenderer ) {
		delete pRenderer;
	}

Expression 'box.left < 0' is always false. Unsigned type value is never < 0. ViewFromTheWindow app.cpp 253

if ( box.left < 0 ) box.left = 0;

Expression 'box.top < 0' is always false. Unsigned type value is never < 0. ViewFromTheWindow app.cpp 255

if ( box.top < 0 ) box.top = 0;

These are probably the only relevant ones, at least I think so. I hope this report is helpful. :)

 

Greetings.

Coordinator
Jan 21, 2012 at 6:49 PM

Hi Sephirothusi,

Thanks for the post - I got some similar results out of the Visual Studio static code analysis option too.  There were actually quite a few findings similar to these, where pointers are used without checking them first being the biggest culprit.  I will certainly add in the checks for this point.

The structure alignment in the MS3D specs may not be fixable, since it is a structure that is used for loading file contents - it may break things if I rearrange them...  I think this is ok to leave how it is, since the structures are only used in the duration of the loading method.  The extra memory may not be such a big deal, unless the model it is loading is super-huge.

In any case, thanks for the post.  Can you create an issue to implement the changes?  That will help me keep track of what needs to get updated!

- Jason

Jan 21, 2012 at 11:08 PM

Issue in the Issue Tracker created. :)

By the way I have a question: why is SSE2 support not enabled in the compiler options for the Hieroglyph3 and all its examples (C/C++ -> Code Generation -> Enable Enhanced Instruction Set -> Streaming SIMD Extensions 2 (/arch:SSE2))? As far as I know it has to be explicitly turned on (none of the other switches implies using SSE2), especially in the 32bit target (if I'm correct x86_64 always uses SSE2 as a required minimum).

Greetings.

Coordinator
Feb 27, 2012 at 12:02 PM

Hi Sephirothusi,

Sorry for the late response - I'm in the middle of an international move back to the U.S., so I've been disconnected for a while...  I'm not sure why the SSE2 flag isn't set, it is probably the same settings that I had enabled way back when I first started the project.  Do you know if this is supported by all processors - i.e. can I safely turn it on and have it still function on 99% of all the user's systems out there?

Thanks for the tip!

- Jason

Feb 27, 2012 at 6:44 PM

Yes, you can safely turn it on and 99,9+% of all the users' systems will handle it just fine. :)

If you are worried, here is the text from Wikipedia about SSE2:

"SSE2, Streaming SIMD Extensions 2, is one of the Intel SIMD (Single Instruction, Multiple Data) processor supplementary instruction sets first introduced by Intel with the initial version of the Pentium 4 in 2001. (...) Rival chip-maker AMD added support for SSE2 with the introduction of their Opteron and Athlon 64 ranges of AMD64 64-bit CPUs in 2003."

I can't imagine anybody having a PC with CPU equal to or older than Pentium 4/Athlon64 AND a modern DX11 video card trying to develop and/or run DX11 code. :) SSE2 is a standart now for a long time and as far as I know all the engines/games are built with it for years. Anyway, it can be safely turned on without any problems at all. :)