I'd guess that most of the time people are using this, they're going to be hitting some form of g_strdup_printv or similar pretty soon.  I'm in favour of changing this to be the gnu-format attribute for that reason (ie: damaging as little code in-the-wild as possible).

Meanwhile, the separate question of if we should have a tag for system printf is an interesting one... we would still call that G_GNUC_something, though -- since it's a GNU C attribute.

Yes, glib has internal GNU printf implementation (from gnulib), which is used if configure detects that current libc printf is not good. If software packages use G_GNUC_PRINTF on a function and implement that function using glib's printf, they should be safe with gnu_printf format.

Did some additional testing.

Apparently, gnu_format, as far as gcc is concerned, does NOT support MS %I64 size prefix (%I64d, %I64u and such). When compling with mingw printf implementation that is, obviously, not true - mingw printf implementation fully supports MS prefixes.

Moreover, MS started to support "ll" size prefix in MSVCRT 2005. It's not supported on XP SP3, but is supported on 7 (and probably Vista too).

I discovered this when i tried to patch glib to use gnu_printf format - i suddenly got warnings about G_INT64_FORMAT, because G_INT64_FORMAT is I64d.

Created attachment 270670 [details]
Testcase for format attribute (v2)

$ gcc _formattest.c -D__USE_MINGW_ANSI_STDIO=0 -Wformat -o _formattest0.exe && _formattest0.exe
_formattest.c: In function 'main':
_formattest.c:61:3: warning: unknown conversion type character 'z' in format [-Wformat=]
   myprint ("on line %d sizeof x is %zu (format printf, implementation ms)\n", __LINE__, sizeof (x));
   ^
_formattest.c:61:3: warning: too many arguments for format [-Wformat-extra-args]
_formattest.c:62:3: warning: unknown conversion type character 'z' in format [-Wformat=]
   mymingwprint ("on line %d sizeof x is %zu (format printf, implementation mingw)\n", __LINE__, sizeof (x));
   ^
_formattest.c:62:3: warning: too many arguments for format [-Wformat-extra-args]
_formattest.c:63:3: warning: unknown conversion type character 'l' in format [-Wformat=]
   myprint ("on line %d y is %lld (format printf, implementation ms)\n", __LINE__, y);
   ^
_formattest.c:63:3: warning: too many arguments for format [-Wformat-extra-args]
_formattest.c:64:3: warning: unknown conversion type character 'l' in format [-Wformat=]
   myprint ("on line %d y is %lli (format printf, implementation ms)\n", __LINE__, y);
   ^
_formattest.c:64:3: warning: too many arguments for format [-Wformat-extra-args]
_formattest.c:69:3: warning: format '%d' expects argument of type 'int', but argument 3 has type 'int64_t' [-Wformat=]
   mygnuprint ("on line %d y is %I64d (format gnu_printf, implementation mingw)\n", __LINE__, y);
   ^
_formattest.c:70:3: warning: format '%i' expects argument of type 'int', but argument 3 has type 'int64_t' [-Wformat=]
   mygnuprint ("on line %d y is %I64i (format gnu_printf, implementation mingw)\n", __LINE__, y);
   ^
on line 59 sizeof x is zu (format gnu_printf, implementation ms)
on line 60 sizeof x is 4 (format gnu_printf, implementation mingw)
on line 61 sizeof x is zu (format printf, implementation ms)
on line 62 sizeof x is 4 (format printf, implementation mingw)
on line 63 y is 9223372036854775807 (format printf, implementation ms)
on line 64 y is 9223372036854775807 (format printf, implementation ms)
on line 65 y is 9223372036854775807 (format printf, implementation ms)
on line 66 y is 9223372036854775807 (format printf, implementation ms)
on line 67 y is 9223372036854775807 (format gnu_printf, implementation mingw)
on line 68 y is 9223372036854775807 (format gnu_printf, implementation mingw)
on line 69 y is 9223372036854775807 (format gnu_printf, implementation mingw)
on line 70 y is 9223372036854775807 (format gnu_printf, implementation mingw)


$ gcc _formattest.c -D__USE_MINGW_ANSI_STDIO=1 -Wformat -o _formattest1.exe && _formattest1.exe
_formattest.c: In function 'main':
_formattest.c:61:3: warning: unknown conversion type character 'z' in format [-Wformat=]
   myprint ("on line %d sizeof x is %zu (format printf, implementation ms)\n", __LINE__, sizeof (x));
   ^
_formattest.c:61:3: warning: too many arguments for format [-Wformat-extra-args]
_formattest.c:62:3: warning: unknown conversion type character 'z' in format [-Wformat=]
   mymingwprint ("on line %d sizeof x is %zu (format printf, implementation mingw)\n", __LINE__, sizeof (x));
   ^
_formattest.c:62:3: warning: too many arguments for format [-Wformat-extra-args]
_formattest.c:63:3: warning: unknown conversion type character 'l' in format [-Wformat=]
   myprint ("on line %d y is %lld (format printf, implementation ms)\n", __LINE__, y);
   ^
_formattest.c:63:3: warning: too many arguments for format [-Wformat-extra-args]
_formattest.c:64:3: warning: unknown conversion type character 'l' in format [-Wformat=]
   myprint ("on line %d y is %lli (format printf, implementation ms)\n", __LINE__, y);
   ^
_formattest.c:64:3: warning: too many arguments for format [-Wformat-extra-args]
_formattest.c:69:3: warning: format '%d' expects argument of type 'int', but argument 3 has type 'int64_t' [-Wformat=]
   mygnuprint ("on line %d y is %I64d (format gnu_printf, implementation mingw)\n", __LINE__, y);
   ^
_formattest.c:70:3: warning: format '%i' expects argument of type 'int', but argument 3 has type 'int64_t' [-Wformat=]
   mygnuprint ("on line %d y is %I64i (format gnu_printf, implementation mingw)\n", __LINE__, y);
   ^
on line 59 sizeof x is 4 (format gnu_printf, implementation ms)
on line 60 sizeof x is 4 (format gnu_printf, implementation mingw)
on line 61 sizeof x is 4 (format printf, implementation ms)
on line 62 sizeof x is 4 (format printf, implementation mingw)
on line 63 y is 9223372036854775807 (format printf, implementation ms)
on line 64 y is 9223372036854775807 (format printf, implementation ms)
on line 65 y is 9223372036854775807 (format printf, implementation ms)
on line 66 y is 9223372036854775807 (format printf, implementation ms)
on line 67 y is 9223372036854775807 (format gnu_printf, implementation mingw)
on line 68 y is 9223372036854775807 (format gnu_printf, implementation mingw)
on line 69 y is 9223372036854775807 (format gnu_printf, implementation mingw)
on line 70 y is 9223372036854775807 (format gnu_printf, implementation mingw)

On XP the version compiled with -D__USE_MINGW_ANSI_STDIO=0 prints -1 on lines 63 and 64, because ms printf implementation does not support ll there. As for %z, it's not supported by MS implementation even in 7.

Created attachment 270712 [details] [review]
Switch G_GNUC_(PRINT|SCAN)F to gnu_(print|scan)f format explicitly

* Change G_GNUC_PRINTF and G_GNUC_SCANF.
* To accommodate new, GNUish format checks, enable 64 bit int format check
in configure for mingw again (previously it was skipped in favour of
hard-coding I64)
* Replace hard-coded I64i with G_GUINT64_FORMAT (how did it even get there?)

Created attachment 271032 [details] [review]
Updated patch to properly handle 64-bit builds

I update patch to fix win64 builds with MinGW.

Created attachment 274822 [details] [review]
Switch G_GNUC_(PRINT|SCAN)F to gnu_(print|scan)f format explicitly

* Change G_GNUC_PRINTF and G_GNUC_SCANF.
* To accommodate new, GNUish format checks, enable 64 bit int format check
in configure for mingw again (previously it was skipped in favour of
hard-coding I64)
* Replace hard-coded I64i with G_GUINT64_FORMAT (how did it even get there?)

v2: Added alexey's change for printrf on 64-bit

Review of attachment 274822 [details] [review]:

Hi,

On first look, in regards to the Visual Studio side, one more thing that I would like to do (or have done) is to update glibconfig.h.win32.in accordingly, where we keep the things we have there for the gsize_modifier and gssize_modifier when we are on MSVC, and use the z modifier when we are not on MSVC.

I need to check later for the gpoll.h part though to make sure things are ok on Visual Studio builds though.

With blessings.

Created attachment 302134 [details] [review]
Update the pre-configured glibconfig.h.win32.in

Hi,

This is the update that I have for glibconfig.h.win32.in, so that MinGW (x64) builds can make use directly, using the "z" format modifier for 64-bit int's.

For MSVC builds, things remain unchanged for this.

With blessings.

Review of attachment 274822 [details] [review]:

Hi LRN,

Unfortunately this line in gwinhttpfile.c broke the build on Visual Studio, due to wchar_t* and char* inconsistencies (and this could mean problems when the code is run on your side):

if (swscanf (content_length, L"%"G_GINT64_FORMAT"%n", &cl, &n) == 1 &&

You need to declare a const gchar* for that format string and convert to gunichar2* (which is wchar_t*), and free it.

With blessings, thank you!

Yeah, this is more difficult than i anticipated. glib does have gnulib *printf implementation, but that does not include a *scanf implementation, nor does it include wide-character versions of either.

This does not matter for MinGW, as we just compile with -D__USE_MINGW_ANSI_STDIO=1 and use either the compliant gnulib implementation or the compliant MinGW implementation, whichever the code ends up using.

For MSVC this is more difficult, as the only available compliant implementation is *printf from gnulib. So anytime wide-character function or any of the *scanf functions is used, it has to use MSVC format specifiers.

I have difficulty finding all the instances of non-gnu-printf-backed printf/scanf calls. swscanf you've pointed out is one of them, but there should be more.
I've tried hacking stdio.h and adding ms_printf/ms_scanf format attributes to ms functions there, and compiling glib against that without -D__USE_MINGW_ANSI_STDIO=1, but it still mostly compiles and does not warn about worng format specifiers much.

As for swscanf specifically, it should probably stay the way it was. I have no idea why alexey changed it to use G_GINT64_FORMAT in attachment 271032 [details] [review] ... maybe it breaks for 64-bit builds...I'll ask him.

What's the status of this? The warnings triggered by this bug for user code are rather annoying -- or even dangerous, since heeding these wrong warnings would actually break the code.

One can use the crude hack

  CPPFLAGS="-D__printf__=__gnu_printf__"

to work around the issue locally, but that isn't something I'd be willing to define in source code or a configuration header.

I can't fix this for MSVC, because i don't build anything with MSVC these days. Without MSVC side being taken care of i can't get fanc's agreement to go forward with this.

The lack of wide-character version of this function in gnulib-provided *printf implementation (and lack of any *scanf implementation) is a major obstacle. I don't have enough data to formulate a correct solution for this. Option (C) seems like the best course right now:
* use 'gnu_printf' for functions that are known to be backed by gnulib implementation
* use 'ms_printf' or 'gnu_printf' (depending on whether it's MSVC or MinGW) for others
* hardcode -D__USE_MINGW_ANSI_STDIO=1 CPPFLAG for glib when built with MinGW).

I think MinGW and MSVC should be treated separately. E.g. the format warning issue applies to GCC only. Likewise, I'm not sure we absolutely need to deal with scanf() -- it already has a separate GLib macro for the format attribute, so using a different standard here as for printf is not necessarily an obstacle.

If we hardcode __USE_MINGW_ANSI_STDIO=1, we need to put it into the Cflags for pkg-config, too. That's doable, but I'm not 100% percent sure we should force this upon all GLib users. (Although -std=c99 already seems to enable it, and so does using g++, apparently.)

In any case, I'd be in favor of GLib's bundled printf implementation to accept both the ISO and the MS syntax, as far as that is possible. Looking at the code, this appears to be already the case as long as the appropriate defines are enabled. Supporting both will make sure that it still works even when user code is built with different flags than GLib was.

Format warning issue applies to gcc only, format *use* applies to glib itself and any applications that use glib.

It's not necessary to force 3rd-parties to use __USE_MINGW_ANSI_STDIO=1. This macro makes the program that is compiled with it use mingw implementation. 3rd-party software that uses glib will already be using glib (which would already be using mingw implementation internally in addition to gnulib implementation), __USE_MINGW_ANSI_STDIO=1 will only make that 3rd-party application use mingw print/scan implementation for when it uses C print/scan functions, not g_print() or anything related.

However, 3rd-party developers tend to mix things up somewhat, so it's not unreasonable to expect to find them using G_FORMAT_* macros for calls to functions that are not backed by glib. These will have to be fixed.

Although you're right, thinking back to the original problem fanc found, it does seem to be rather MSVC-specific (and the original problem that this bug is about is MinGW-specific).
I kind of got sidetracked. comment 10 points at the real issue - the use of G_* format specifier macro in a call to a swscanf(), which is not backed by gnulib. This is obviously an error.

However, i have difficulty finding errors like this. Mixing L"..." and "..." is not an error or even a warning for gcc, AFAICS, and i don't use MSVC.

Hi,

For records, the patch to gio/win32/gwinhttpfile.c (part of attachment 274822 [details] [review]) was committed as 305a9b12 and fixed for Visual Studio as d20e88f.  I didn't get to check the other parts, but do take a look at Nacho's recent commits (within the past day or two) to see whether the other parts of this patch series overlap with his recent commits.

With blessings.

Review of attachment 274822 [details] [review]:

A lot of this patch is no longer necessary, due to incrementally being applied in other patches and bugs. LRN, can you rebase this and see if anything is still needed here please? I suspect this was all fixed with bug #795569. Thanks!

At first glance i'd say that this patch is completely unneeded now...*except* for the autotools changes. Recent improvements to this situation only affected meson.build, but not configure.ac and Makefile.am.

In this regard a proposition was already mooted to just drop autotools support for Windows builds.

Also, i would like to note that the new code does not use the 'z' format specifier (i.e. instead of defining gsize as size_t it defines gsize as unsigned int, long or long long, depending on the size needed, and thus 'z' format loses its meaning). I don't know whether this is a good or a bad thing.

(In reply to LRN from comment #18)
> At first glance i'd say that this patch is completely unneeded
> now...*except* for the autotools changes. Recent improvements to this
> situation only affected meson.build, but not configure.ac and Makefile.am.
> 
> In this regard a proposition was already mooted to just drop autotools
> support for Windows builds.

Indeed. Since it’s effectively you and Christoph maintaining the MinGW/MSYS port at the moment, I’ll leave that up to you. I’m fairly in favour of dropping autotools for Windows, though. It would ease the transition. However, let’s discuss that on the other bug report.

Let’s close this bug now then. Thanks for checking.

> Also, i would like to note that the new code does not use the 'z' format
> specifier (i.e. instead of defining gsize as size_t it defines gsize as
> unsigned int, long or long long, depending on the size needed, and thus 'z'
> format loses its meaning). I don't know whether this is a good or a bad
> thing.

As long as it remains interoperable with size_t variables from native code, I don’t see a problem either way.