Pandemonium

August 13, 2008

Tools of the Trade – Part One: FxCop

Filed under: Games Development,Tools and Software Development — bittermanandy @ 9:04 pm

The established wisdom – and there’s undoubtedly a lot of truth in it – is that catching a bug late in the development cycle can cost as much as 1000 times as much to fix as the same bug would have cost, if you’d caught it earlier. Really bad bugs can even (depending on what you’re making) make you miss your Christmas launch date, fail to reach launch altogether, force a product recall after launch, or even open you up to legal action. Some bugs have cost their authors literally millions of pounds, and the hardware bug that causes the Xbox 360 “Red Ring of Death” cost Microsoft billions. Of course, “cost” could equally mean time instead of money – but in either case, I’d rather spend ten minutes fixing a bug just after I wrote it, than wait till just before releasing my game and realise I have to spend hours, days or weeks finding and fixing the same bug.

The first weapon in your armoury to find and fix bugs early is to turn the compiler warnings up to the most practical maximum, which in the case of Visual Studio means Warning Level 4, or /W4. (If you’re stuck using Visual C++ you might be surprised to learn that some amazingly useful, in fact, critical warnings are actually disabled by default at level 4). You should always ensure your code compiles without warnings at /W4. You may think some warnings are unnecessary; fix them anyway. It won’t hurt, and might even help without you realising.

Once you’ve got your warnings turned up and fixed, the next step is to use static analysis tools like FxCop. Static analysis tools find bugs in your code without you even needing to run it. In fact, the next version of FxCop isn’t even a separate tool, it’s a compiler option (/analyze). These tools parse your code (or in FxCop’s case, examine the MSIL assemblies) and compare what you’ve written against a set of rules that help to identify dangerous patterns in your code.

With FxCop installed, you simply need to open it up, start a new project, and point it at where to find the assemblies (.dll or .exe) for your code. It works equally well with either XNA game code or standard .NET non-game code. Hit “Analyze”, and a few seconds later it will come up with a list of problems that it has detected, along with an explanation about why they are problems and suggestions on how to fix them.

You will probably be shocked and surprised by how many problems it finds in your code. The good news is, it’s probably not as bad as it looks; the bad news is, that’s because FxCop is slightly on the noisy side, though you have complete custom control over which rules you enable (and you can even define your own). For example, FxCop regularly complains at me about the following:

IdentifiersShouldBeSpelledCorrectly: Correct the spelling of ‘Kensei’ in namespace name ‘Kensei’.

Well, first of all, Kensei is the correct spelling, it’s the codename of my engine. Secondly, I’m not completely sure that I care if the spelling is correct – nobody is going to play my game and think, “it’s a good game, but he misspelled one of his namespace names”. Thirdly, I’ve followed the instructions to create my own user dictionary for FxCop to add “Kensei” to it, and couldn’t get it to work; and I’m certainly not going to try and think up a whole new codename for my project just to keep FxCop quiet. So, I usually disable this rule, or at least exclude the specific occurrence of the warning.

There are some warnings that are a little bit trickier and need more thought. Like this one:

MarkAssembliesWithClsCompliant: Mark ‘Kensei.dll’ with CLSCompliant(true) because it exposes externally visible types.

Really…? ‘CLS compliant’ means, basically, that the assembly can be used by other languages in the Common Language Specification that aren’t C#, like Managed C++ or Visual Basic. I guess, on one level, that seems like a pretty noble thing to aim for, though I wouldn’t expect to ever use those other languages given that I love C# so much. But it’s possible I might make parts of my code available to other people, and they might want to use those other languages. So should I follow this advice or not?

In this case the answer comes when I do what it says to do. Marking the assembly as CLSCompliant results in a number of compiler warnings, because I’ve been using types that are not CLS compliant. That’s fair enough, except that these types include things like GameTime, which is a core XNA type. The XNA team chose not to make their assemblies CLS compliant; so there is absolutely no point in me trying to do so.

Anyway, by now you’re probably wondering why I’m talking about FxCop when so far I’ve just described some things it does less than amazingly well. As it happens, I think it’s a wonderful little tool, especially since it’s completely free. I run it on all my code – the Kensei engine, and my games, including Pandemonium – regularly; at minimum, after each “milestone” I’ve set myself. I’ll then fix the problems it highlights, either by changing the code as recommended or by marking a warning as Excluded, to say that the infraction was deliberate. So… why? Here’s why (a real-life example, though the names have been changed to protect the innocent):

    public class BaseClass

    {

        public BaseClass()

        {

            InitValues();

        }

 

        protected virtual void InitValues()

        {

            // …

        }

 

        // Rest of class implementation

    }

 

    public class DerivedClass : BaseClass

    {

        public DerivedClass()

        {

            // …

        }

 

        protected override void InitValues()

        {

            // …

            base.InitValues();

        }

 

        // Rest of class implementation

    }

Spotted the error yet? You probably should have done – I’ve certainly stripped it down far enough! When I wrote this, the BaseClass was in BaseClass.cs in a project in my Kensei solution, and the DerivedClass was in DerivedClass.cs in my Pandemonium solution, and both files were pushing 1000 lines of code. So the problem was a lot less obvious… but no less real for all that. FxCop spotted it straight away:

DoNotCallOverridableMethodsInConstructors: ‘BaseClass.BaseClass()’ contains a call chain that results in a call to a virtual method defined by the class. … Virtual methods defined on the class should not be called from constructors. If a derived class has overridden the method, the derived class version will be called (before the derived class constructor is called).

In other words, when I did something like this:

    DerivedClass derived = new DerivedClass();

… it would (conceptually; this isn’t completely accurate) first allocate memory on the heap, then call the BaseClass constructor, the BaseClass constructor would call the DerivedClass virtual InitValues method, which would call into the BaseClass InitValues method, and only then would it call the DerivedClass constructor before giving me back the reference to the object. That is to say – it would be calling a member function intended for a DerivedClass object, on an object that wasn’t a DerivedClass at all, only a BaseClass.

Most of you will be well aware that this is a Bad Thing. I certainly was already. Such behaviour is not polymorphism (a good and noble ideal), it’s just broken and wrong. It could rely on members being in a certain state that weren’t in that state yet. It could call out to other functions in other objects, and end up interacting with them despite being only partially constructed. It could, frankly, do just about anything, and almost none of the things it could do would be good. (For more on what is allowed in C#, see the C# Standard). However, despite the fact I’ve been programming for [mumblemumblemumble] years in total, and for six of those years I’ve been paid to do it, I still made this mistake. Well, I am only human after all – and we humans make mistakes.

It’s a mistake that FxCop spotted and I’ve since fixed it. It spotted it almost as soon as I’d written it, and it took five minutes to fix. Late in the dev cycle, I might only have spotted it after hours trying to work out why my DerivedClass objects were behaving strangely, and so much other code might have become dependant on that behaviour that it might have taken hours longer to fix. I could have lost days or weeks to one silly little bug – and believe me, I’ve seen exactly that happen before now.

There are many other genuinely code-breaking bugs that the compiler can’t spot but FxCop will pick up immediately. (Have you implemented the Dispose pattern correctly throughout your program? Are you really sure about that? How about private members and functions that are never used?) If you’re willing to spend money, there are other static analysers out there that check correctness and performance at an even lower level, therefore finding even more subtle (or critical!) bugs, and all without actually running a single cycle of your code. (Indeed, if you’re using C++ for some reason and not using Lint, BoundsChecker, PREFast or something similar, I bet real money your code is broken). But for C# and/or XNA, FxCop is free, easy to use, easy to customise, and if you’ve not used it yet I strongly recommend that you start right now.

“If debugging is the process of removing bugs, then programming must be the process of putting them in.”Edsger Dijkstra

Advertisements

10 Comments »

  1. Great blog, and great advice. Id never heard of this tool, so thanks for writing about it. Keep blogging, ive learnt loads about an ideal dev enviroment from you!

    Comment by Venatu — August 13, 2008 @ 9:23 pm | Reply

  2. While not free, I imagine ReSharper http://www.jetbrains.com/resharper/index.html would catch these issues on the fly (if those checks are enabled).

    Comment by John Bledsoe — August 13, 2008 @ 9:46 pm | Reply

  3. Hi there – you provide some solid advice, and while your examples are specific to game programming, the rules in question are valid in other situations.

    I’d like to point out though, that there is a built-in version of fxCop in Visual Studio (from VS 2005 onwards) called “Code Analysis”. This can be enabled so that it runs the analysis rules you choose on every single build.

    I guess that would be the ultimate in catching bugs as early as possible!

    Cheers

    Comment by Mark — August 14, 2008 @ 2:29 am | Reply

  4. Mark: isn’t that “Code Analysis” the “/analyze” switch – and isn’t that new in VS2008, not VS2005 (therefore not available to XNA 2.0)? Could be wrong though!

    Edit – it seems I am wrong. /analyze is available from the command line in VS2005, but only if you have Team Edition, which I don’t have. As Team Edition also comes with Product Studio… I really wish it was as affordable as VS Professional!

    Comment by bittermanandy — August 14, 2008 @ 9:09 am | Reply

  5. Just a heads up, after fooling around with FXCop, I discovered how to add your identifiers to the custom dictionary..

    Open CustomDictionary.xml from FXCop install directory, and add a new tag under Dictionary/Words/Recognized (The recognized namespace is located just after the second comment section in the xml file)..

    For instance, to add the word “xna” to the list, simply add xna to the Recognized namespace (is it called namespaces in XML?)..

    Comment by Caion — August 14, 2008 @ 4:38 pm | Reply

  6. Damn it, not edit functionality, and XML tags are not allowed.. “xna” should be wrapped in an Word XML tag like the rest of the words under the Recognized namespace..

    Comment by Caion — August 14, 2008 @ 4:39 pm | Reply

  7. Here’s a post that gives some options on integrating with 2005 non-Team Edition.

    http://geekswithblogs.net/sdorman/archive/2007/02/18/106630.aspx

    Comment by Graphics Runner — August 14, 2008 @ 6:23 pm | Reply

  8. Awesome. Cheers for the link.

    Next question – how can I get hold of Product Studio without spending £1500 on Team Edition? 😀

    Comment by bittermanandy — August 14, 2008 @ 6:59 pm | Reply

  9. FxCop complained that my assembly wasn’t marked CLSComplient-true so I added [assembly:CLSCompliant(true)] to my game file and now it complains that Microsoft.Xna.Framework.Game isn’t complient, do I need it or not?!? and why dose Game State Management Sample have so many fails!?!

    I’ll also as this in forums.xna.com and let you know if I get an answer.

    Comment by DNA8088 — August 23, 2008 @ 12:36 pm | Reply

  10. DNA8088, as mentioned above CLSCompliant isn’t really needed for XNA code, because it’s used to ensure cross-language compatibility and XNA is only written in C#.

    FxCop is intended for general-purpose code, so some things, like CLS compliance just aren’t necessary. The XNA team know this and, although they do use some FxCop rules, ignore others – like CLS compliance and presumably all the failures you’re seeing in the samples. FxCop is a very useful tool, but there are still some cases when you as a programmer know better. Use it, learn from it, but don’t always feel that you have to strictly obey it.

    Comment by bittermanandy — August 25, 2008 @ 2:41 pm | Reply


RSS feed for comments on this post. TrackBack URI

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Blog at WordPress.com.

%d bloggers like this: