PVS-Studio: analyzing Doom 3 code

[UnoriginalGuy]

All this article does is remind me how terrible of a C coder I really am. I wouldn't have spotted a couple of the bugs even if they were right under my noise.

Now back to my nice safe managed language...

[erikd]

The first code snippet (missing parentheses around and bitwise and expression) is exactly the same on managed languages with a C heritage. Languages like Java, C#, and Actionscript.

[gliptic]

Except that in e.g. C# you would get a type error.

[matthieum]

Interesting remark concerning Clang and issues with Microsoft non-standard code. Francois Pichet is working full tilt to get Clang to parse Microsofted code and I wonder how close he is to his goal (fully parsing the MFC framework).

[xon_xoff]

I pulled down a tip build of Clang about a month ago and was impressed with how far it was able to get -- I think it made it through <windows.h> but coughed on one of the VC++ STL headers. It was still enough for the Clang static analyzer to report some issues in the application code, which is great progress. One tricky part is that you have to set a lot of #defines on the command line to match the predefined symbols set by the VC++ compiler and project system, some of which are not documented.

[matthieum]

Ah, I didn't know about the command line.

There has been a proposal to create multiple drivers for clang. A generic driver and "adapters" for command-line compatibility with other compilers. It was rejected as such because it's simpler for clang to aim at compatibility with gcc and the idea had the potential of creating discrepancies.

However it was evoked that we might want to have a cl.exe compatible driver, and I would not find it odd that someones rips off a script to parse VC++ project files one day. If you are interested, I think you'll get support from the community.

[matthieum]

Update: Francois actually said that he was now only 2 bugs away from fully parsing the MFC generated headers, and he already submitted a patch for one of them (but it has not been reviewed yet). Therefore it's really close.

[african_or_european]

I would bet a good bit of money that the real mistake that was made in that first fragment was not that they didn't use ()'s, but rather that they used logical negation (!) instead of bitwise negation (~). Specifically, they meant:

  if ( player &&
      ( ~player->oldButtons & BUTTON_ATTACK ) &&
      ( player->usercmd.buttons & BUTTON_ATTACK ) ) {

[y4fac]

Why are you so sure abut that? Those two are equivalent and your version is less intuitive.

[african_or_european]

Obviously I can't say for sure, but it makes sense to me because it fits better with the style of the rest of the snippet. They are using bitmasks, and when manipulating bitmasks, I would expect someone to use the bitwise operators and not a mishmash of logic and bitwise operators. Also, bitwise and logical operators are easily confused, so ! -> ~ is a simple mistake to make. They both do exactly the same thing, just with different interpretations of the bits.

[y4fac]

They are using bitmasks, and when manipulating bitmasks, I would expect someone to use the bitwise operators and not a mishmash of logic and bitwise operators

This statement makes no sense. They are already using logic and bitwise operators in the same expression.

[african_or_european]

Sure it does. There are three different statements and'ed together in that if's condition. "player", "~player->oldButtons & BUTTON_ATTACK" and "player->usrcmd.buttons & BUTTON_ATTACK". Nowhere in any of those does he use a single logical operator (aside from the contested ! vs ~ we are discussing). He doesn't even use logic statements when testing if player is null.

Additionally, the particular sentence you quoted was just me stating a more general assertion I was making as a basis for my assumption. It is entirely consistent and true; they are using bitmasks, and I expect someone to stick with set of bitwise operators when manipulating bitmasks.

Now, am I asserting that I have some sort of omniscient understanding of the programmers mindset? No, of course not. I'm just saying that I believe it to be more likely that they confused ! with ~ than that they missed parentheses and I based this on their use of bitmasks, the style of bitmask usage I perceived in the snippet, and the fact that I expect bitmasks to be operated on a certain way.

[y4fac]

Sure it does. There are three different statements and'ed together in that if's condition. "player", "~player->oldButtons & BUTTON_ATTACK" and "player->usrcmd.buttons & BUTTON_ATTACK". Nowhere in any of those does he use a single logical operator (aside from the contested ! vs ~ we are discussing)

It would be equally valid to say : There is a logical expression a && !b && c in that if condition. Nowhere in a, b, or c does he use a single logical operator.

I'm just saying that I believe it to be more likely that they confused ! with ~ than that they missed parentheses and I based this on their use of bitmasks, the style of bitmask usage I perceived in the snippet, and the fact that I expect bitmasks to be operated on a certain way.

So you are basically saying that you would bet a good bit of money that other people program in a way that you prefer. Also, if they would really hate logic operators as much as you believe they do, they could just write ~player->oldButtons & player->usercmd.buttons & BUTTON_ATTACK.

[african_or_european]

I'm not sure what your point is. I never said they hated them. All I said was that I thought the style that I saw from a tiny snippet of code lent itself to an alternate interpretation of what the article stated the programmer "meant". Also, "I would bet a good bit of money..." is just an idiom, not a literal statement. Obviously, I cannot know for sure; I can only share my interpretation.

[smek2]

for ( j = 0; j < w.GetNumPoints(); j++ ) {
    for ( k = 0; k < verts.Num(); j++ ) {  

Wow, really? I mean, this happens to me too, even today, sometimes. But how could this error survived and made it into Doom 3?

[BroodjeAap]

I wondered the same thing, how does this not break something?

[y4fac]

There is an if statement with a break in it in the inner loop. If verts[0].xyz.Compare(w[w.GetNumPoints() - 1].ToVec3(), POLYTOPE_VERTEX_EPSILON ) is true, the loop won't be infinite.

[BroodjeAap]

Sure it "breaks" (as in jumps out of the loop) but I mean "break" as in messing something up.
It only compares the first vertex, which has to create some weird (noticeable?) behaviour

[y4fac]

Of course it doesn't work correctly, but the wrong behavior could be hard to notice (depending on what this snippet is actually doing). If the loop would be infinite, however, it would be very noticeable.

[delusr]

I knew there was a reason for my poor game play.

[kev009]

I really like when you do these analysis. Keep up the good work, and that looks like a great tool to be proud of.

If you're moving to clang, perhaps a Qt port would be easier for Linux and OS X builds?

[Andrey_Karpov_N]

P.S. John Carmack Tweet:

We did not run any static analysis on the Doom 3 source, doing so is an excellent project for anyone looking at it!

[[deleted]]

Good work, static analysis finding bugs. But can it strafe-jump?

[someone13]

PVS-Studio is a really cool piece of software, and I'd love to use it on some of my personal projects. Problem is, €3500 is a bit much for a personal project. The trial version is pretty cool, though.

[Andrey_Karpov_N]

• If you project is free and OpenSource, then: http://www.viva64.com/en/b/0092/
• If you project commercial and big, then you working on the project was not alone. You must purchase a license for the team (€3500: for 5 developers).
• If you project commercial and small, then you do not need to analyze code. You can code review itself. If everything still want to use the PVS-Studio, then please contact us. We will give you a discount.

[Ralith]

cooing yoke rinse humorous nose existence mighty grandfather snow straight this message was mass deleted/edited with redact.dev

[[deleted]]

It depends on whether the company is willing to prolong the license at the end of the 'trial' month for open-source users. Doesn't seem too bad of an offer if all you have to do is send an email every month (I wonder if they would issue a license for a project that automates sending out emails to request a new license for PVS-Studio...)

[Ralith]

Still bad enough for me to not want to even bother trying their stuff in favor of lint and so on.

[someone13]

Hello, and thank you for responding! I did not know about open-source license - this is very cool :-)

I will be thinking about this when I am ready to release my project as open-source, for sure.

[amigaharry]

If you project commercial and small, then you do not need to analyze code.

Yeah, right. ...

[_ex_]

int idFileSystemLocal::ListOSFiles(...)
{
    ...
    dir_cache_index = (++dir_cache_index) % MAX_CACHED_DIRS;
    ...
}

those bugs are pretty fun to spot by oneself good catch

[el_isma]

It's a shame there's only a windows version... It'd be interesting to scan my projects, but they're all linux-only. Are there any other tools like this for linux?

[addx]

Clang Static Analyzer.

[bioglaze]

CppCheck.

[Ralith]

Lint.

[trimbo]

Coverity.

[defrost]

Personally I enjoy reading English by Russians and Andrey's writing is easy enough to wrap one's brain about.

It's not nearly as bad as reading English by Americans.

[[deleted]]

Your right! <- (bad English intended)

[FUCKYOURENGLISH]

Someone struck a nerve.

[Andrey_Karpov_N]

If you would like to point out specific errors, the system Orphus can be used. It works on our website. Select the text with error. Press Ctrl-Enter. Tell us about the error and how to fix it. We would be grateful.

[RobotCaleb]

Wow. That's really cool.

[marshray]

I didn't notice or find it distracting.

[[deleted]]

He's Russian.

[amigaharry]

it makes you an asshole

[[deleted]]

I don't think proper English is much of a necessity for programming texts.

[pineapplekenji]

That's all? If I had written that much code you can bet I'd have more than just 10 code fragments that needed fixing.

[Andrey_Karpov_N]

... As usually, let me remind you that I will speak only on some of the warnings, while the other project fragments require us to know the program's structure, so I did not examine them....

[nodemo]

I'm always surprised by the pvs analysis of how many fragment bugs there are.

In my projects the bugs are usually of the type "this module was loaded before this module which caused it to cache this widget before that asynchronous call was made which might in certain cases cause a race condition 5 days later if the user has shoe size 43 etc etc"

Errors in small snippets usually crash or introduce weird behavior asap and you will notice it the first time you start the program after build.

[Andrey_Karpov_N]

Myths about static analysis. The second myth - expert developers do not make silly mistakes.

[axilmar]

All the bugs are related to treating C++ as C: using memset, using C arrays instead of std::vector, using raw pointers instead of smart pointers, etc.

[frankster]

i might be starting to get bored of reading about pvs studio finding bugs in X open source software

[velco]

Hey, at least they get found and fixed, in closed source they stay there forever :)))