Skip to content

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Sep 9, 2012

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)

This finds about ~150 potential problems with format characters on a 64 bit build.
Encapsulate _snprintf/sprintf difference in implementation not header
@jgarzik
Copy link
Contributor

jgarzik commented Sep 9, 2012

ACK

@gavinandresen
Copy link
Contributor

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.

@laanwj
Copy link
Member Author

laanwj commented Sep 9, 2012

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...

@BitcoinPullTester
Copy link

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

@luke-jr
Copy link
Member

luke-jr commented Sep 9, 2012

%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.

@laanwj
Copy link
Member Author

laanwj commented Sep 9, 2012

@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...

@luke-jr
Copy link
Member

luke-jr commented Sep 9, 2012

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

@laanwj
Copy link
Member Author

laanwj commented Sep 9, 2012

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.

Also const correctness for lookup tables in hex functions throughout the code.
@BitcoinPullTester
Copy link

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

@BitcoinPullTester
Copy link

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

@Diapolo
Copy link

Diapolo commented Sep 20, 2012

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

@laanwj
Copy link
Member Author

laanwj commented Sep 21, 2012

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

@gavinandresen
Copy link
Contributor

ACK

laanwj added a commit that referenced this pull request Sep 26, 2012
Add printf-style warnings to strprintf() and OutputDebugStringF()
@laanwj laanwj merged commit 5a1a362 into bitcoin:master Sep 26, 2012
@jgarzik
Copy link
Contributor

jgarzik commented Sep 26, 2012

Gah!

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

@laanwj
Copy link
Member Author

laanwj commented Sep 27, 2012

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.

@jgarzik
Copy link
Contributor

jgarzik commented Sep 27, 2012

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.

@laanwj
Copy link
Member Author

laanwj commented Sep 27, 2012

Just remove -Wformat then for your build.

@jgarzik
Copy link
Contributor

jgarzik commented Sep 27, 2012

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.

@jgarzik
Copy link
Contributor

jgarzik commented Sep 27, 2012

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

@laanwj
Copy link
Member Author

laanwj commented Sep 27, 2012

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...

@laanwj
Copy link
Member Author

laanwj commented Sep 27, 2012

Voila 14ac0ad

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants