-
Notifications
You must be signed in to change notification settings - Fork 37.8k
Add printf-style warnings to strprintf() and OutputDebugStringF() #1807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This finds about ~150 potential problems with format characters on a 64 bit build.
Encapsulate _snprintf/sprintf difference in implementation not header
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 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 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:
Compilation
Result
So, no. Trying the same with %Iu (or the PRIszu macro that I defined) works. |
Also const correctness for lookup tables in hex functions throughout the code.
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 |
Add printf-style warnings to strprintf() and OutputDebugStringF()
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 |
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 1381ad2 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 |
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
forsize_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
: hexPRIszu
: unsigned (size_t)PRIszd
: signed (ssize_t)