Use C99 printf statements in mingw #3244

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2013_11_mingw_c99_printf changing 4 files +36 −26

laanwj commented at 10:37 AM on November 13, 2013: member

Change mingw build to use C99 printf statements. Otherwise, format specifiers such as PRId64 will not work on XP or earlier (see http://sourceforge.net/apps/trac/mingw-w64/wiki/gnu%20printf)

This bug was introduced with 51ed9ec9.

Continued from #3237 (Silence inttypes.h compiler warnings on Windows) and #3236 (massive compiler warning spam).

This also adds strprintf() testcases so that hopefully this problem will be caught by the pulltester next time.
laanwj commented at 12:19 PM on November 13, 2013: member

Checked the test_bitcoin.exe produced by the pull tester on WinXP: no errors in sprintf_tests.
brandondahler commented at 12:37 PM on November 13, 2013: contributor

@laanwj continuing dicussion about using iostreams, would there be any known cross-compatibility issues with the switch?
laanwj commented at 12:53 PM on November 13, 2013: member

@brandondahler This should be safe, and is only a very small change. As it works with the ancient mingw that pulltester uses (and on windows XP, which is the minimum Windows version that we support), it certainly works with latter ones.

A later patch could switch to using boost::format and/or iostreams, but that would be a huge patch and touch every file and function in the core (breaking all other pull requests to boot!). So I'd prefer to leave it at this for now and focus on more important issues.
Diapolo commented at 3:45 PM on November 13, 2013: none

Tried it, still results in the compiler spaming warnings for me...
laanwj commented at 3:53 PM on November 13, 2013: member
@Diapolo but does the resulting bitcointest work?

I guess the reason that you get warnings is that the ATTR_WARN has to be changed to use C99 conventions too on WIN32:
```
#define ATTR_WARN_PRINTF(X,Y) __attribute__((format(printf,X,Y)))
```
Can you try changing it to?
```
#define ATTR_WARN_PRINTF(X,Y) __attribute__((format(gnu_printf,X,Y)))
```
Edit: already pushed this, works on other OSes too, yippie, no need for win32-specific exception
Diapolo commented at 3:54 PM on November 13, 2013: none

Let me check the executable...

Use C99 printf statements in mingw

Otherwise, format specifiers such as %llu will not work on XP or
earlier.
This bug was introduced with 51ed9ec9.
http://sourceforge.net/apps/trac/mingw-w64/wiki/gnu%20printf

d6f690f7da

tests: add testcases for strprintf c82788efff
Diapolo commented at 4:15 PM on November 13, 2013: none

<pre> Running 103 test cases... *** No errors detected </pre>

Win7 x64 with the test_bitcoin.exe from pulltester above. Still weird...
Diapolo commented at 4:18 PM on November 13, 2013: none

Your latest change leads to this warnings:

<pre> C:\Users\Diapolo\bitcoin.Qt\src\util.h: Warnung:format '%d' expects argument of type 'int', but argument 3 has type 'int64_t {aka long long int}' [-Wformat=] #define strprintf(format, ...) real_strprintf(format, 0, __VA_ARGS__) ^ </pre>

or

<pre> C:\Users\Diapolo\bitcoin.Qt\src\util.h: Warnung:format '%u' expects argument of type 'unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned int}' [-Wformat=] #define LogPrintf(...) LogPrint(NULL, __VA_ARGS__) ^ </pre>

Edit: Wait... this was with my former patch applied, trying a new compilation...
in src/util.h:None in c82788efff
```
 103 | @@ -117,7 +104,7 @@ inline void MilliSleep(int64_t n)
 104 |   * Parameters count from 1.
 105 |   */
 106 |  #ifdef __GNUC__
 107 | -#define ATTR_WARN_PRINTF(X,Y) __attribute__((format(printf,X,Y)))
 108 | +#define ATTR_WARN_PRINTF(X,Y) __attribute__((format(gnu_printf,X,Y)))
```
Diapolo commented at 4:23 PM on November 13, 2013:

It seems this did the final magic, I get no more warning with MinGW on Win7 x64. Drawback could be that we broke MSVC support? I really dunno, as I never tried that compiler and wont in the near future...

laanwj commented at 4:29 PM on November 13, 2013:

In theory, yes. But as I understand it, newer MSVCs (on Vista and newer) also support C99 syntax for printf format characters. Though I'm sure there are many small things that would have the be changed to successfully build in MSVC. No one is bothering with that anyway at the moment.

Diapolo commented at 4:36 PM on November 13, 2013:

@laanwj Perhaps we should remove MSVC compatibility changes that are all over the code then? No one is maintaining them, so what benefit have they?

laanwj commented at 5:04 PM on November 13, 2013:

Don't bother. I don't want to intentionally make it harder to build with MSVC.
BitcoinPullTester commented at 4:34 PM on November 13, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c82788effffda0758de20880b2925b42044ce08d for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
Diapolo commented at 4:36 PM on November 13, 2013: none

ACK
laanwj referenced this in commit 5ce4361077 on Nov 13, 2013
laanwj merged this on Nov 13, 2013
laanwj closed this on Nov 13, 2013
laanwj deleted the branch on Apr 9, 2014
Bushstar referenced this in commit db6ea1de8e on Apr 8, 2020
Bushstar referenced this in commit 630f792b5c on Apr 8, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#3236 massive compiler warning spam after #2767 #3237 Silence inttypes.h compiler warnings on Windows #3271 Use C++ streams instead of C-style printf functions