Add printf-style warnings to strprintf() and OutputDebugStringF() #1807

This finds about ~150 potential problems with wrong format characters on a 64 bit build (-Wformat).

Most mistakes seem to have to do with using %d for size_t / ssize_t. When memory addresses are 32-bit this is no problem, however on 64-bit architectures this is wrong. See also http://stackoverflow.com/questions/1546789/clean-code-to-printf-size-t-in-c-or-nearest-equivalent-of-c99s-z-in-c . Other warnings have to do with pointers, where %x is used instead of %p.

I don't feel like fixing all the warnings right now, but it is a useful diagnostic nevertheless as it may reveal hidden bugs. This pull does add macros akin to PRI64x to help with this, and represent format characters for these types for MSVC and GCC:

• PRIszx: hex
• PRIszu: unsigned (size_t)
• PRIszd: signed (ssize_t)

ACK

Can we get rid of the PRI64x macros by just not supporting older compilers?

If I recall correctly, latest MSVC supports the standard %foo way of doing things.

You'd hope so... But

VC2010 size prefixes: http://msdn.microsoft.com/en-us/library/tcxf1dw6%28v=vs.100%29.aspx VC2012: http://msdn.microsoft.com/en-us/library/tcxf1dw6%28v=vs.110%29.aspx

Seems that they still have no support for "ll" and "z", but only "I64" and "I". I don't think there is really a standard for these types, so every libc vendor kind of does what they want.

Boost probably has some truly portable and type-safe solution for formatting...

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/76a57705eae7ebb1b47267f38c1bad0ccaf9d6e6 for binaries and test log.

%zu is AFAIK standard C++, so is it confirmed to not work with MingW? @gavinandresen PRI64x is the standard way of printing int64 types. %x, %lx, %llx all target implementation-dependent sizes for int, long, and long long.

@luke-jr Well as I said, I can't find it in the list in the official documentation on the MS site, so if it works at all I don't think it is safe to use.

Also I'm pretty sure that C++ standard says nothing about printf, that's from the (Ansi/C89/C99) C standard. C++ has the funny std::cout << formatting...

Edit: I do read that %z is official C99 and quite some C++ compilers implement it. However, it's still strange that it's not in the documentation for MSVC...

@laanwj Meh, it's part of C99 (page 289), so if it works then I don't see any reason to avoid it.

printftest.c:

#include <stdio.h>
int main()
{
    size_t size = 0x12345678;
    printf("%zu\n", size);
    return 0;
}

Compilation

$ i686-w64-mingw32-g++ printftest.c -Wformat -o printftest
printftest.c: In function ‘int main()’:
printftest.c:6:25: warning: unknown conversion type character ‘z’ in format [-Wformat]
printftest.c:6:25: warning: too many arguments for format [-Wformat-extra-args]

Result

$ ./printftest
zu

So, no. Trying the same with %Iu (or the PRIszu macro that I defined) works.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7506e2fa2020193e65621581835c08f62419a689 for binaries and test log.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3b3d9996181d9c93c81667f610e31212997fd184 for binaries and test log.

I would love to see this one getting in as afterwards we can start fixing all left signed/unsigned stuff.

AFAIK this does not help with signed/unsigned, just the width of data types to prevent segfaults and such.

ACK

Gah!

This adds well over 100 warnings to the build. Please clean up.

Yeah it sucks to be messenger of bad news, however it adds warnings for problems that were hidden before, not new problems. Having them visible is better IMO.

A few, sure.

Over a hundred, including several repeated each time main.h etc. was included? It is noise that obscures real warnings, and gets in the way of actual work.

I am basing my current work off 1381ad2b21733071acb2a43c251dbecef524f110 as that is the last usable commit here.

Just remove -Wformat then for your build.

That's not usually how software development works.

If you cause problems in the build, you fix it, or revert it.

It is not the responsibility of each individual developer to work around the build noise added here.

If you are not volunteering to fix it, let's revert the change until it is ready for prime time.

You will recall, with the previous swath of warnings, that the warnings were fixed before we adjusted the makefiles.

I have already volunteered to fix it, but I also have many other things to do.

I actually made -Wformat work. Before, it was enabled but not doing anything in the bitcoin build (because of our custom printf redefine). If we don't like the warnings it creates, it should not be enabled in the first place.

I think removing -Wformat for now is a better fix than reverting this commit, because people that are interested in the warnings can simply add it...

Voila 14ac0ad