mingw-w64 and __attribute__((format(printf))) issue

mingw-w64 and __attribute__((format(printf))) issue

[Liu Hao]

[-- Attachment #1.1: Type: text/plain, Size: 1159 bytes --]

Due to a recent change in mingw-w64 master [1], libgomp ceases to build:

```
../../../gcc-git/libgomp/target.c:936:21: error: unknown conversion type
character 'l' in format [-Werror=format=]
  936 |         gomp_fatal ("present clause: !acc_is_present (%p, "
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```


On line 29 of 'libgomp/libgomp.h' from GCC 9 branch I found this declaration

```
extern void gomp_fatal (const char *, ...)
        __attribute__ ((noreturn, format (printf, 1, 2)));
```

, which uses the `printf` attribute, but the `PRIu64` macro from
<inttypes.h> expands to `%llu` because now GCC has `-std=gnu11` by
default, which is only valid with `gnu_printf`.

AFAICS there are three solutions:

1. Revert bfd33f6c0ec5e652cc9911857dd1492ece8d8383 in mingw-w64, or
2. Make GCC treat `format(printf)` as `format(gnu_printf)` if C11 or
   C++11 is selected, or
3. Replace `format(printf)` with `format(gnu_printf)` in libgomp source.


What do you think?


[1]
https://github.com/mingw-w64/mingw-w64/commit/bfd33f6c0ec5e652cc9911857dd1492ece8d8383


-- 
Best regards,
LH_Mouse


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Mingw-w64-public] mingw-w64 and __attribute__((format(printf))) issue

[Martin Storsjö]

On Wed, 6 May 2020, Liu Hao wrote:

> Due to a recent change in mingw-w64 master [1], libgomp ceases to build:
>
> ```
> ../../../gcc-git/libgomp/target.c:936:21: error: unknown conversion type
> character 'l' in format [-Werror=format=]
>  936 |         gomp_fatal ("present clause: !acc_is_present (%p, "
>      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ```
>
>
> On line 29 of 'libgomp/libgomp.h' from GCC 9 branch I found this declaration
>
> ```
> extern void gomp_fatal (const char *, ...)
>        __attribute__ ((noreturn, format (printf, 1, 2)));
> ```
>
> , which uses the `printf` attribute, but the `PRIu64` macro from
> <inttypes.h> expands to `%llu` because now GCC has `-std=gnu11` by
> default, which is only valid with `gnu_printf`.
>
> AFAICS there are three solutions:
>
> 1. Revert bfd33f6c0ec5e652cc9911857dd1492ece8d8383 in mingw-w64, or

This is generally the risk of this kind of commit - regardless of how 
right/wrong the status quo is, there's _a lot_ of code that relies on it 
behaving in a specific way, and changing that will certainly run into 
minor issues in a lot of places.

> 2. Make GCC treat `format(printf)` as `format(gnu_printf)` if C11 or
>   C++11 is selected, or

I'm not very keen on that. The compiler should ideally not assume to much 
about what the platform headers do in detail, because it limits what we 
can change. (If we'd make that change in the compiler, and then for 
another reason end up reverting the commit, we'd have the same issue in 
reverse.)

> 3. Replace `format(printf)` with `format(gnu_printf)` in libgomp source.

This also kind of hardcodes too much; libgomp in general can't and 
shouldn't assume too much about which format kind it uses, unless libgomp 
itself hardcodes __USE_MINGW_ANSI_STDIO. Also, the whole gnu_printf format 
is something that only GCC supports, not Clang, but I guess libgomp is 
rather GCC specific anyway.

However we do have a define that should expand to the right thing - just 
like inttypes.h PRIu64 follows __USE_MINGW_ANSI_STDIO - we have 
__MINGW_PRINTF_FORMAT.

So something like this should work:

#ifdef __MINGW32__
#define PRINTF_FORMAT __MINGW_PRINTF_FORMAT
#else
#define PRINTF_FORMAT printf
#endif

__attribute__((format(PRINTF_FORMAT)))

Not very pretty, but should work without hardcoding any assumptions about 
which format actually is used.


// Martin

Re: [Mingw-w64-public] mingw-w64 and __attribute__((format(printf))) issue

[Liu Hao]

[-- Attachment #1.1: Type: text/plain, Size: 2811 bytes --]

在 2020/5/6 18:48, Martin Storsjö 写道:
> 
> This is generally the risk of this kind of commit - regardless of how
> right/wrong the status quo is, there's _a lot_ of code that relies on it
> behaving in a specific way, and changing that will certainly run into
> minor issues in a lot of places.
> 

As it was I that committed the patch - I thought it should be safe,
because 0) G++ has been hardcoding `__USE_MINGW_ANSI_STDIO` for years
(see 'bits/os_defines.h'), and 1) MSYS2 has it too (see
'makepkg_mingw64.conf' which affects all packages in C or C++).

It was however an oversight that during bootstrapping GCC doesn't use
the CPPFLAGS from the build environment, which I think is unusual, as
all MSYS2 packages have `__USE_MINGW_ANSI_STDIO`. The hard-coded (?)
`printf` attribute expects the MS behavior, which shouldn't be the case,
if we were building something that calls functions from libgomp.h
directly, despite the fact that it isn't a public header.


>> 2. Make GCC treat `format(printf)` as `format(gnu_printf)` if C11 or
>>   C++11 is selected, or
> 
> I'm not very keen on that. The compiler should ideally not assume to
> much about what the platform headers do in detail, because it limits
> what we can change. (If we'd make that change in the compiler, and then
> for another reason end up reverting the commit, we'd have the same issue
> in reverse.)
> 

Yes I agree. It's pretty bad that the C (but not C++) standard allows
users to declare standard functions without including appropriate
headers. If someone declare `printf` themselves then they will end up in
calling it from MSVCRT, even in C11 mode.


>> 3. Replace `format(printf)` with `format(gnu_printf)` in libgomp source.
> 
> This also kind of hardcodes too much; libgomp in general can't and
> shouldn't assume too much about which format kind it uses, unless
> libgomp itself hardcodes __USE_MINGW_ANSI_STDIO. Also, the whole
> gnu_printf format is something that only GCC supports, not Clang, but I
> guess libgomp is rather GCC specific anyway.
> 

As long as 'libgomp.h' is a private header, it might be fine to change
or remove this attribute.


> However we do have a define that should expand to the right thing - just
> like inttypes.h PRIu64 follows __USE_MINGW_ANSI_STDIO - we have
> __MINGW_PRINTF_FORMAT.
> 
> So something like this should work:
> 
> #ifdef __MINGW32__
> #define PRINTF_FORMAT __MINGW_PRINTF_FORMAT
> #else
> #define PRINTF_FORMAT printf
> #endif
> 
> __attribute__((format(PRINTF_FORMAT)))
> 
> Not very pretty, but should work without hardcoding any assumptions
> about which format actually is used.
> 

So we end up in patching libgomp source anyway. I will try this tomorrow.



-- 
Best regards,
LH_Mouse


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Mingw-w64-public] mingw-w64 and __attribute__((format(printf))) issue

[Liu Hao]

[-- Attachment #1.1: Type: text/plain, Size: 1298 bytes --]

在 2020/5/6 18:48, Martin Storsjö 写道:
> 
> So something like this should work:
> 
> #ifdef __MINGW32__
> #define PRINTF_FORMAT __MINGW_PRINTF_FORMAT
> #else
> #define PRINTF_FORMAT printf
> #endif
> 
> __attribute__((format(PRINTF_FORMAT)))
> 
> Not very pretty, but should work without hardcoding any assumptions
> about which format actually is used.
> 

This requires `#include <stdio.h>` first.

Also I don't think it is correct to check for `__MINGW32__` for this
purpose, as the original MinGW.org header doesn't seem to provide
`__MINGW_PRINTF_FORMAT` [1].

However a direct check for `__MINGW_PRINTF_FORMAT` should suffice [2].
It is building well now, albeit still with some minor warnings:

```
../../gcc-git/lto-plugin/lto-plugin.c:927:29: note: format string is
defined here
  927 |     sscanf (s, ".%" PRI_LL "x", &obj->out->id);
      |                  ~~~~~~~~~~~^
      |                             |
      |                             unsigned int *
      |                  %" PRI_LL "llx

```


[1]
https://osdn.net/projects/mingw/scm/git/mingw-org-wsl/blobs/5.3-trunk/mingwrt/include/stdio.h
[2]
https://github.com/msys2/MINGW-packages/pull/6453/files#diff-49b46b088a6393d449afcdbfbe4e710e



-- 
Best regards,
LH_Mouse


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: mingw-w64 and __attribute__((format(printf))) issue

[JonY]

[-- Attachment #1.1: Type: text/plain, Size: 1342 bytes --]

On 5/6/20 5:36 AM, Liu Hao via Gcc-help wrote:
> Due to a recent change in mingw-w64 master [1], libgomp ceases to build:
> 
> ```
> ../../../gcc-git/libgomp/target.c:936:21: error: unknown conversion type
> character 'l' in format [-Werror=format=]
>   936 |         gomp_fatal ("present clause: !acc_is_present (%p, "
>       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ```
> 
> 
> On line 29 of 'libgomp/libgomp.h' from GCC 9 branch I found this declaration
> 
> ```
> extern void gomp_fatal (const char *, ...)
>         __attribute__ ((noreturn, format (printf, 1, 2)));
> ```
> 
> , which uses the `printf` attribute, but the `PRIu64` macro from
> <inttypes.h> expands to `%llu` because now GCC has `-std=gnu11` by
> default, which is only valid with `gnu_printf`.
> 
> AFAICS there are three solutions:
> 
> 1. Revert bfd33f6c0ec5e652cc9911857dd1492ece8d8383 in mingw-w64, or
> 2. Make GCC treat `format(printf)` as `format(gnu_printf)` if C11 or
>    C++11 is selected, or
> 3. Replace `format(printf)` with `format(gnu_printf)` in libgomp source.
> 
> 
> What do you think?
> 
> 
> [1]
> https://github.com/mingw-w64/mingw-w64/commit/bfd33f6c0ec5e652cc9911857dd1492ece8d8383
> 
> 

I think option #3 is the simplest approach in the short term, though I
prefer option #2.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: mingw-w64 and __attribute__((format(printf))) issue

[Liu Hao]

[-- Attachment #1.1: Type: text/plain, Size: 874 bytes --]

在 2020/5/10 12:27, JonY via Gcc-help 写道:
> 
> I think option #3 is the simplest approach in the short term, though I
> prefer option #2.
> 

This patch has been accepted by MSYS2 [1], tested for bootstrapping on
both i686 and x86_64.

So here comes a 4th option: Disable `-Werror` if `--disable-werror` is
specified to the top-level configure.

EGREP'ing for `(format|__format__)\s*\(\s*(__printf__|printf)` in GCC
source reveals ~30 matches (excluding testsuites). Probably they should
all be fixed similarly, or we ignore such warnings for simplicity.
libgomp doesn't build despite `--disable-werror` at top level, which
seems a bug in this case.


[1]
https://github.com/msys2/MINGW-packages/blob/9501ee2afc8d01dc7d85383e4b22e91c30d93ca7/mingw-w64-gcc/0020-libgomp-Don-t-hard-code-MS-printf-attributes.patch


-- 
Best regards,
LH_Mouse


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: mingw-w64 and __attribute__((format(printf))) issue

[JonY]

[-- Attachment #1.1: Type: text/plain, Size: 977 bytes --]

On 5/10/20 9:03 AM, Liu Hao wrote:
> 在 2020/5/10 12:27, JonY via Gcc-help 写道:
>>
>> I think option #3 is the simplest approach in the short term, though I
>> prefer option #2.
>>
> 
> This patch has been accepted by MSYS2 [1], tested for bootstrapping on
> both i686 and x86_64.
> 
> So here comes a 4th option: Disable `-Werror` if `--disable-werror` is
> specified to the top-level configure.
> 
> EGREP'ing for `(format|__format__)\s*\(\s*(__printf__|printf)` in GCC
> source reveals ~30 matches (excluding testsuites). Probably they should
> all be fixed similarly, or we ignore such warnings for simplicity.
> libgomp doesn't build despite `--disable-werror` at top level, which
> seems a bug in this case.
> 
> 
> [1]
> https://github.com/msys2/MINGW-packages/blob/9501ee2afc8d01dc7d85383e4b22e91c30d93ca7/mingw-w64-gcc/0020-libgomp-Don-t-hard-code-MS-printf-attributes.patch
> 
> 

Any thoughts on the libgomp printf attribute changes?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: mingw-w64 and __attribute__((format(printf))) issue

[Jakub Jelinek]

On Tue, May 12, 2020 at 11:14:02AM +0000, JonY wrote:
> >> I think option #3 is the simplest approach in the short term, though I
> >> prefer option #2.
> >>
> > 
> > This patch has been accepted by MSYS2 [1], tested for bootstrapping on
> > both i686 and x86_64.
> > 
> > So here comes a 4th option: Disable `-Werror` if `--disable-werror` is
> > specified to the top-level configure.
> > 
> > EGREP'ing for `(format|__format__)\s*\(\s*(__printf__|printf)` in GCC
> > source reveals ~30 matches (excluding testsuites). Probably they should
> > all be fixed similarly, or we ignore such warnings for simplicity.
> > libgomp doesn't build despite `--disable-werror` at top level, which
> > seems a bug in this case.
> > 
> > 
> > [1]
> > https://github.com/msys2/MINGW-packages/blob/9501ee2afc8d01dc7d85383e4b22e91c30d93ca7/mingw-w64-gcc/0020-libgomp-Don-t-hard-code-MS-printf-attributes.patch
> > 
> > 
> 
> Any thoughts on the libgomp printf attribute changes?

gomp_fatal etc. call vfprintf under the hood though, does that one handle
%llu on mingw?  If not, using %llu for the PRI* macros looks wrong to me.

	Jakub

Re: mingw-w64 and __attribute__((format(printf))) issue

[Liu Hao]

[-- Attachment #1.1: Type: text/plain, Size: 1140 bytes --]

在 2020/5/12 20:02, Jakub Jelinek 写道:
> 
> gomp_fatal etc. call vfprintf under the hood though, does that one handle
> %llu on mingw?  If not, using %llu for the PRI* macros looks wrong to me.
> 

As long as libgomp doesn't enable `__USE_MINGW_ANSI_STDIO`, the answer
is a bit complex:

0) With all versions of mingw-w64 (probably MinGW.org too) that have
   been released, the MS `vfprintf()` is called; see below.

1) With current master of mingw-w64, and:

   a) -std={gnu,c}89, or
      -std={gnu,c}++03: Ditto; see below.

   b) -std={gnu,c}99, or
      -std={gnu,c}++11: The mingw-w64 one is called,
                        so it is always supported.

2) If stdio.h is not included (the user declares this function
   themselves, e.g. during configure checks), then the MS one is called;
   see below.


About the behavior of MS `vfprintf()`:

0) On Windows 7 (probably Vista and 2008, untested), *printf functions
   from MSVCRT do accept `%llu`, so it's valid despite the warning.

1) On Windows XP (probably also 2003, untested), `%lld` is not
   recognized.

-- 
Best regards,
LH_Mouse


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]