What’s wrong with this code–a real world example

re: What’s wrong with this code–a real world example

alegr1 — Fri, 04 Jan 2013 04:58:38 GMT

And, by the way, using reinterpret_cast is a bad habit. It's even worse than using a C-style cast indiscriminately.

re: What’s wrong with this code–a real world example

alegr1 — Fri, 04 Jan 2013 04:55:33 GMT

Prefast would find that a declaration is hidden by a nested declaration of the same name.

re: What’s wrong with this code–a real world example

Larry Osterman [MSFT] — Wed, 12 Dec 2012 18:06:05 GMT

@Jon: Yes it does make debugging crashes harder. But it helps make the produce more reliable.

@Richard: We have a different mechanism other than /analyze - it runs the same tool chain but slightly differently

re: What’s wrong with this code–a real world example

Richard — Mon, 10 Dec 2012 14:26:00 GMT

Do you not compile with /analyze? (implied by Ramesh's post)

re: What’s wrong with this code–a real world example

Jon — Thu, 06 Dec 2012 23:23:52 GMT

"our debug builds are built with full optimization enabled"

@Larry,

Doesn't this make it harder to debug crashes?

re: What’s wrong with this code–a real world example

Ramesh Dharan — Fri, 30 Nov 2012 20:51:12 GMT

The GCC compiler has -Wshadow for catching this kind of thing. This StackOverflow article has a couple of interesting tidbits on the topic:

stackoverflow.com/.../is-there-an-equivalent-of-gccs-wshadow-in-visual-c

"Check out warnings C6244 and C6246

But you will need to enable automatic code analysis to get them."

re: What’s wrong with this code–a real world example

Karellen — Fri, 30 Nov 2012 13:09:03 GMT

This is also an argument for turning on warnings for "shadow"d variables/functions. That's caught a number of bugs in some of my code before.

re: What’s wrong with this code–a real world example

Larry Osterman [MSFT] — Thu, 29 Nov 2012 21:01:01 GMT

@JM: Yes, but this was built from the NT build system - our debug builds are built with full optimization enabled - this ensures that our debug and release builds are as close to identical as possible - the only difference between the two is the asserts in the debug build.

And yes, I'm working with the compiler folks to figure out why a warning wasn't generated - there are other uninitialized local variable errors that are generated.

re: What’s wrong with this code–a real world example

JM — Thu, 29 Nov 2012 09:53:13 GMT

Aside from bitness, another way that would hopefully expose this is building and testing the code as a debug build first, with optimizations turned off. That should encourage the compiler to not do storage folding. The 32-bit vs. 64-bit is really just you getting lucky (or unlucky, depending on your perspective) -- any change in compiler setting or minor tweak to the code could have exposed the bug.

And I agree with Jon, and I would put effort into tracking down why you didn't get a warning. "Uninitialized local variable" is *not* a warning you want suppressed for any reason other than locally, as a workaround for a compiler limitation where it can't see the variable really is initialized (and you can't add explicit initialization yourself, for some reason). If people protest a change in build settings, tell them ignoring these warnings is a potential security risk. Because it is.

If it's a compiler bug, the static_cast<> is not the reason, if my little test program is any indication (apologies if the blog formatting turns this into mush):

unsigned int foo(unsigned int x) {

return x;

}

int main() {

unsigned int x = 3.0;

{

unsigned int x = foo(static_cast<int>(x));

}

This produces a warning when compiled with CL 16.0.

re: What’s wrong with this code–a real world example

Jonathan — Thu, 29 Nov 2012 08:33:02 GMT

In C#, you'd also get:

error CS0136: A local variable named 'cchString' cannot be declared in this scope because it would give a different meaning to 'cchString', which is already used in a 'parent or current' scope to denote something else

Of course, in C# you'd seldom need to manually calculate storage...