Warnings in mingw64 builds on master

Warnings in mingw64 builds on master

[Andy Moreton]

Hi,

Building master for 64bit mingw64 gives the following warnings:

C:/emacs/git/emacs/master/src/w32reg.c:146:1: warning: function might be candidate for attribute 'malloc' [-Wsuggest-attribute=malloc]
  146 | w32_get_string_resource (void *v_rdb, const char *name, const char *class)
      | ^~~~~~~~~~~~~~~~~~~~~~~

C:/emacs/git/emacs/master/src/w32menu.c: In function 'set_frame_menubar':
C:/emacs/git/emacs/master/src/w32menu.c:326:2: warning: 'memcpy' offset [3, 10] from the object at '<unknown>' is out of the bounds of referenced subobject 'contents' with type 'union Lisp_X *[]' at offset 3 [-Warray-bounds]
  326 |  memcpy (previous_items, XVECTOR (f->menu_bar_vector)->contents,
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  327 |   previous_menu_items_used * word_size);
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from C:/emacs/git/emacs/master/src/w32menu.c:26:
C:/emacs/git/emacs/master/src/lisp.h:1631:17: note: subobject 'contents' declared here
 1631 |     Lisp_Object contents[FLEXIBLE_ARRAY_MEMBER];
      |                 ^~~~~~~~

C:/emacs/git/emacs/master/src/w32.c: In function '_sys_read_ahead':
C:/emacs/git/emacs/master/src/w32.c:8785:10: warning: 'rc' may be used uninitialized in this function [-Wmaybe-uninitialized]
 8785 |   if (rc == sizeof (char))
      |       ~~~^~~~~~~~~~~~~~~~
C:/emacs/git/emacs/master/src/image.c:116: warning: macro "PIX_MASK_DRAW" is not used [-Wunused-macros]
  116 | #define PIX_MASK_DRAW 1
      | 

These must have been introduced fairly recently, as the build was clean
earlier this year.

    AndyM

Re: Warnings in mingw64 builds on master

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Fri, 14 Aug 2020 23:37:27 +0100
> 
> Building master for 64bit mingw64 gives the following warnings:
> 
> C:/emacs/git/emacs/master/src/w32reg.c:146:1: warning: function might be candidate for attribute 'malloc' [-Wsuggest-attribute=malloc]
>   146 | w32_get_string_resource (void *v_rdb, const char *name, const char *class)
>       | ^~~~~~~~~~~~~~~~~~~~~~~

That's just noise.  There's nothing wrong with the code.

> C:/emacs/git/emacs/master/src/w32menu.c: In function 'set_frame_menubar':
> C:/emacs/git/emacs/master/src/w32menu.c:326:2: warning: 'memcpy' offset [3, 10] from the object at '<unknown>' is out of the bounds of referenced subobject 'contents' with type 'union Lisp_X *[]' at offset 3 [-Warray-bounds]
>   326 |  memcpy (previous_items, XVECTOR (f->menu_bar_vector)->contents,
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   327 |   previous_menu_items_used * word_size);
>       |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from C:/emacs/git/emacs/master/src/w32menu.c:26:
> C:/emacs/git/emacs/master/src/lisp.h:1631:17: note: subobject 'contents' declared here
>  1631 |     Lisp_Object contents[FLEXIBLE_ARRAY_MEMBER];
>       |                 ^~~~~~~~

Likewise.

> C:/emacs/git/emacs/master/src/w32.c: In function '_sys_read_ahead':
> C:/emacs/git/emacs/master/src/w32.c:8785:10: warning: 'rc' may be used uninitialized in this function [-Wmaybe-uninitialized]
>  8785 |   if (rc == sizeof (char))
>       |       ~~~^~~~~~~~~~~~~~~~

This is a bug in the compiler you are using: rc _is_ initialized.

> C:/emacs/git/emacs/master/src/image.c:116: warning: macro "PIX_MASK_DRAW" is not used [-Wunused-macros]
>   116 | #define PIX_MASK_DRAW 1
>       | 

This macro is used in the PNG code.  Are you building without PNG
support?

> These must have been introduced fairly recently, as the build was clean
> earlier this year.

Actually, this code was not touched in years, so I'm guessing you
upgraded to a later version of the compiler lately.

Thanks.

Re: Warnings in mingw64 builds on master

[Andy Moreton]

On Sat 15 Aug 2020, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Fri, 14 Aug 2020 23:37:27 +0100
>> 
>> Building master for 64bit mingw64 gives the following warnings:
>> 
>> C:/emacs/git/emacs/master/src/w32reg.c:146:1: warning: function might be
>> candidate for attribute 'malloc' [-Wsuggest-attribute=malloc]
>>   146 | w32_get_string_resource (void *v_rdb, const char *name, const char *class)
>>       | ^~~~~~~~~~~~~~~~~~~~~~~
>
> That's just noise.  There's nothing wrong with the code.

Agreed. However it is better to have a clean build than to have
pointless warnings, so the real errors stand out.

>> C:/emacs/git/emacs/master/src/w32menu.c: In function 'set_frame_menubar':
>> C:/emacs/git/emacs/master/src/w32menu.c:326:2: warning: 'memcpy' offset [3,
>> 10] from the object at '<unknown>' is out of the bounds of referenced
>> subobject 'contents' with type 'union Lisp_X *[]' at offset 3
>> [-Warray-bounds]
>>   326 |  memcpy (previous_items, XVECTOR (f->menu_bar_vector)->contents,
>>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   327 |   previous_menu_items_used * word_size);
>>       |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from C:/emacs/git/emacs/master/src/w32menu.c:26:
>> C:/emacs/git/emacs/master/src/lisp.h:1631:17: note: subobject 'contents' declared here
>>  1631 |     Lisp_Object contents[FLEXIBLE_ARRAY_MEMBER];
>>       |                 ^~~~~~~~
>
> Likewise.
>
>> C:/emacs/git/emacs/master/src/w32.c: In function '_sys_read_ahead':
>> C:/emacs/git/emacs/master/src/w32.c:8785:10: warning: 'rc' may be used uninitialized in this function [-Wmaybe-uninitialized]
>>  8785 |   if (rc == sizeof (char))
>>       |       ~~~^~~~~~~~~~~~~~~~
>
> This is a bug in the compiler you are using: rc _is_ initialized.

The is "gcc version 10.2.0 (Rev1, Built by MSYS2 project)" so not
exactly old. More silly warnings that it would be better to disable.


>> C:/emacs/git/emacs/master/src/image.c:116: warning: macro "PIX_MASK_DRAW" is not used [-Wunused-macros]
>>   116 | #define PIX_MASK_DRAW 1
>>       | 
>
> This macro is used in the PNG code.  Are you building without PNG
> support?

Yes, as the native image support handles PNG.

>> These must have been introduced fairly recently, as the build was clean
>> earlier this year.
>
> Actually, this code was not touched in years, so I'm guessing you
> upgraded to a later version of the compiler lately.

Yes - it is whatever is current in in the MSYS distro. 

    AndyM

Re: Warnings in mingw64 builds on master

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 15 Aug 2020 19:48:56 +0100
> 
> >> C:/emacs/git/emacs/master/src/w32reg.c:146:1: warning: function might be
> >> candidate for attribute 'malloc' [-Wsuggest-attribute=malloc]
> >>   146 | w32_get_string_resource (void *v_rdb, const char *name, const char *class)
> >>       | ^~~~~~~~~~~~~~~~~~~~~~~
> >
> > That's just noise.  There's nothing wrong with the code.
> 
> Agreed. However it is better to have a clean build than to have
> pointless warnings, so the real errors stand out.

I'd rather we removed -Wsuggest-attribute=malloc from the options we
use.
> >> C:/emacs/git/emacs/master/src/w32.c: In function '_sys_read_ahead':
> >> C:/emacs/git/emacs/master/src/w32.c:8785:10: warning: 'rc' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >>  8785 |   if (rc == sizeof (char))
> >>       |       ~~~^~~~~~~~~~~~~~~~
> >
> > This is a bug in the compiler you are using: rc _is_ initialized.
> 
> The is "gcc version 10.2.0 (Rev1, Built by MSYS2 project)" so not
> exactly old.

Let's hope GCC 10.3 will fix this.

Re: Warnings in mingw64 builds on master

[Andy Moreton]

On Sat 15 Aug 2020, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 15 Aug 2020 19:48:56 +0100
>> 
>> >> C:/emacs/git/emacs/master/src/w32reg.c:146:1: warning: function might be
>> >> candidate for attribute 'malloc' [-Wsuggest-attribute=malloc]
>> >>   146 | w32_get_string_resource (void *v_rdb, const char *name, const char *class)
>> >>       | ^~~~~~~~~~~~~~~~~~~~~~~
>> >
>> > That's just noise.  There's nothing wrong with the code.
>> 
>> Agreed. However it is better to have a clean build than to have
>> pointless warnings, so the real errors stand out.
>
> I'd rather we removed -Wsuggest-attribute=malloc from the options we
> use.

Yes - I meant that removing the warning switch is preferenable (even if
I did not manage to say so clearly).

    AndyM

Re: Warnings in mingw64 builds on master

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 15 Aug 2020 19:48:56 +0100
>> 
>> >> C:/emacs/git/emacs/master/src/w32reg.c:146:1: warning: function might be
>> >> candidate for attribute 'malloc' [-Wsuggest-attribute=malloc]
>> >>   146 | w32_get_string_resource (void *v_rdb, const char *name, const char *class)
>> >>       | ^~~~~~~~~~~~~~~~~~~~~~~
>> >
>> > That's just noise.  There's nothing wrong with the code.
>> 
>> Agreed. However it is better to have a clean build than to have
>> pointless warnings, so the real errors stand out.
>
> I'd rather we removed -Wsuggest-attribute=malloc from the options we
> use.
>> >> C:/emacs/git/emacs/master/src/w32.c: In function '_sys_read_ahead':
>> >> C:/emacs/git/emacs/master/src/w32.c:8785:10: warning: 'rc' may be used uninitialized in this function [-Wmaybe-uninitialized]
>> >>  8785 |   if (rc == sizeof (char))
>> >>       |       ~~~^~~~~~~~~~~~~~~~
>> >
>> > This is a bug in the compiler you are using: rc _is_ initialized.
>> 
>> The is "gcc version 10.2.0 (Rev1, Built by MSYS2 project)" so not
>> exactly old.
>
> Let's hope GCC 10.3 will fix this.

I'm half-asleep, so excuse me if I'm missing the obvious... but rc is
assigned or its address taken within conditional blocks, so if none of
those conditions are true, rc is not initialized at the warning site.

Re: Warnings in mingw64 builds on master

[Paul Eggert]

On 8/15/20 12:03 PM, Eli Zaretskii wrote:
> I'd rather we removed -Wsuggest-attribute=malloc from the options we
> use.

That option is useful for the non-w32 source code. If we want to disable it in 
MinGW we could add something like

#if defined __MINGW32__ && 8 <= __GNUC__
# pragma GCC diagnostic ignored "-Wsuggest-attribute=malloc"
#endif

to src/w32.h.

Re: Warnings in mingw64 builds on master

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Sat, 15 Aug 2020 22:36:12 +0200
> 
> if none of those conditions are true

That cannot happen.

Re: Warnings in mingw64 builds on master

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 15 Aug 2020 15:34:24 -0700
> 
> On 8/15/20 12:03 PM, Eli Zaretskii wrote:
> > I'd rather we removed -Wsuggest-attribute=malloc from the options we
> > use.
> 
> That option is useful for the non-w32 source code.

Useful how?

Re: Warnings in mingw64 builds on master

[Andy Moreton]

On Sun 16 Aug 2020, Eli Zaretskii wrote:

>> From: Óscar Fuentes <ofv@wanadoo.es>
>> Date: Sat, 15 Aug 2020 22:36:12 +0200
>> 
>> if none of those conditions are true
>
> That cannot happen.

True, but it requires deeper analysis for the compiler to prove it.

Why not just initialise rc ? I doubt any performance difference is
measurable.

    AndyM

Re: Warnings in mingw64 builds on master

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sun, 16 Aug 2020 12:21:43 +0100
> 
> >> if none of those conditions are true
> >
> > That cannot happen.
> 
> True, but it requires deeper analysis for the compiler to prove it.

You mean, pay attention to the test of the same flags a few lines
before?

> Why not just initialise rc ?

No objections from me.

Re: Warnings in mingw64 builds on master

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> if none of those conditions are true
>
> That cannot happen.

The compiler can't prove that. Such a proof is way too difficult. The
"fix" you are hoping for will not arrive in 10.3. Neither in 20.3,
unless some extraordinary advancements occur.

In any case, the idiom

if( cond1 ) {}
else if(cond2) {}

where one of cond1 or cond2 are true, is questionable. It not only
confuses de compiler, a human reader would reasonably think that those
conditions can be simultaneously false. Something like this would be
clearer:

if( cond1 ) {}
else { // if we reach this point, cond2 is true
assert(cond2); // Honest! And future-proof.
}

This way you are conveying knowledge to both the reader and the
compiler, and even making the code more robust.

Re: Warnings in mingw64 builds on master

[Paul Eggert]

>>> I'd rather we removed -Wsuggest-attribute=malloc from the options we use.
>>
>> That option is useful for the non-w32 source code.
> 
> Useful how?

It lets both the human reader and the compiler know that the storage addressed 
by the function's result cannot alias with any other storage. I find this 
helpful when reading the code because it lets me not worry about aliasing, and 
compilers can use this info to generate more-efficient code.

Re: Warnings in mingw64 builds on master

[Eli Zaretskii]

> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 16 Aug 2020 08:25:33 -0700
> 
> >>> I'd rather we removed -Wsuggest-attribute=malloc from the options we use.
> >>
> >> That option is useful for the non-w32 source code.
> > 
> > Useful how?
> 
> It lets both the human reader and the compiler know that the storage addressed 
> by the function's result cannot alias with any other storage. I find this 
> helpful when reading the code because it lets me not worry about aliasing, and 
> compilers can use this info to generate more-efficient code.

I think you described what __attribute__((malloc)) does.  But that's
not what I asked; I asked why do we need to put that warning option
into the list of options with which we compile.  It sounds like a
specialized option intended for maintainers when they want to find
ways to improve the code, because the warning it emits has nothing to
do with correctness of the code.

So I think we should by default disable it.  Those who find it useful
can always enable it locally.

Re: Warnings in mingw64 builds on master

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> >> if none of those conditions are true
>> >
>> > That cannot happen.
>> 
>> True, but it requires deeper analysis for the compiler to prove it.
>
> You mean, pay attention to the test of the same flags a few lines
> before?

The information gained about the contents of fd_info[fd] from that test
is discarded when the compiler finds a call to an external function
(DebPrint) a few lines below, since DebPrint (or one of the functions it
calls) might change the contents of the global fd_info array.

If the OP was not building with EMACSDEBUG defined, the warning would be
bogus indeed, since DebPrint is a no-op on that case. But please note
that the warning says "may be undefined", that is, the compiler is just
saying that it was unable to prove that `rc' is assigned before it is
used. It is not a bug, but a limitation, a QoI issue.

This piked my curiosity and wrote this sample:

#include <stdlib.h>

enum { FILE_PIPE, FILE_SERIAL, FILE_SOCKET, FILE_READ, FILE_CONNECT};

typedef struct
{
  unsigned         flags;
} filedesc;

extern filedesc fd_info [10];

extern void _DebPrint (const char *fmt, ...);
#define DebPrint(stuff) _DebPrint stuff
// #define DebPrint(stuff) ((void) 0)

int _sys_read_ahead (int fd) {
  int rc;

  if ((fd_info[fd].flags & (FILE_PIPE | FILE_SERIAL | FILE_SOCKET)) == 0
      || (fd_info[fd].flags & FILE_READ) == 0)
  {
    DebPrint (("_sys_read_ahead: internal error: fd %d is not a pipe, serial port, or socket!\n", fd));
    return(0);
  }

  if ((fd_info[fd].flags & FILE_CONNECT) != 0)
    DebPrint (("_sys_read_ahead: read requested from fd %d, which waits for async connect!\n", fd));

  if (fd_info[fd].flags & FILE_PIPE) {
    rc = 1;
  }
  else if (fd_info[fd].flags & FILE_SERIAL) {
    rc = 2;
  }
  else if (fd_info[fd].flags & FILE_SOCKET) {
    rc = 3;
  }

  return rc;
}


which compiled with `gcc -c -Wall -Wmaybe-uninitialized' (gcc 10.1)
shows no warnings, which indeed might indicate a bug. OTOH, `clang -c
-Wall' (clang 11) shows:


w32.c:35:12: warning: variable 'rc' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
  else if (fd_info[fd].flags & FILE_SOCKET) {
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
w32.c:39:10: note: uninitialized use occurs here
  return rc;
         ^~
w32.c:35:8: note: remove the 'if' if its condition is always true
  else if (fd_info[fd].flags & FILE_SOCKET) {
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
w32.c:17:9: note: initialize the variable 'rc' to silence this warning
  int rc;
        ^
         = 0
1 warning generated.


which is quite illustrative. But if you change the definition of
DebPrint to the no-op version, it keeps showing the same warning, which
demontrates the limitations of the compiler.

Right now the point of this type of warnings is that *sometimes* they
are helpful. The compiler guys try to do their best, but the result
never will be perfect, it can't be.

Re: Warnings in mingw64 builds on master

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Sun, 16 Aug 2020 18:45:31 +0200
> 
> The information gained about the contents of fd_info[fd] from that test
> is discarded when the compiler finds a call to an external function
> (DebPrint) a few lines below, since DebPrint (or one of the functions it
> calls) might change the contents of the global fd_info array.
> 
> If the OP was not building with EMACSDEBUG defined, the warning would be
> bogus indeed, since DebPrint is a no-op on that case.

I expect Andy was building without EMACSDEBUG, since he was almost
certainly building an optimized binary, see below.

> which compiled with `gcc -c -Wall -Wmaybe-uninitialized' (gcc 10.1)
> shows no warnings

I think you need to use -O2 (or some optimization switch) to have this
warning.  At least that's how it used to be in the past, I don't have
GCC 10 installed to check.

Re: Warnings in mingw64 builds on master

[Paul Eggert]

On 8/16/20 8:39 AM, Eli Zaretskii wrote:
> the warning it emits has nothing to
> do with correctness of the code.

I'm not sure I follow. Although it's true that adding __attribute__ ((malloc)) 
does not turn formerly-incorrect code into correct code, the same thing is true 
of other function attributes like _Noreturn. But that doesn't mean we shouldn't 
use function attributes.

The main reason I like __attribute__ ((malloc)) has little to do with improving 
performance (is that what you meant by "improving the code"?); it has to do with 
understanding the code and using that understanding to avoid and/or fix bugs. 
When I see code like this:

    char *p = xmalloc (s);
    memcpy (p, q, r);

I mentally have to answer questions like "is r <= s?" because otherwise the 
behavior is undefined. But I do not have to mentally answer questions like "are 
*p and *q aliases?" - even though the behavior of memcpy would be undefined if 
they were aliases - because malloc has __attribute__ ((malloc)) and this means 
*p cannot alias with anything else.

This is not simply a matter of memcpy vs memmove. In C one must constantly worry 
about aliasing. These worries are lessened with __attribute__ ((malloc)), so the 
attribute is typically a win.

Re: Warnings in mingw64 builds on master

[Eli Zaretskii]

> Cc: andrewjmoreton@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 16 Aug 2020 21:21:36 -0700
> 
> On 8/16/20 8:39 AM, Eli Zaretskii wrote:
> > the warning it emits has nothing to
> > do with correctness of the code.
> 
> I'm not sure I follow. Although it's true that adding __attribute__ ((malloc)) 
> does not turn formerly-incorrect code into correct code, the same thing is true 
> of other function attributes like _Noreturn. But that doesn't mean we shouldn't 
> use function attributes.

Agreed.  I'm not against function attributes in general or
__attribute__((malloc)) in particular.

> This is not simply a matter of memcpy vs memmove. In C one must constantly worry 
> about aliasing. These worries are lessened with __attribute__ ((malloc)), so the 
> attribute is typically a win.

Agreed.  I was talking about the compiler option
"-Wsuggest-attribute=malloc", not about using the function attributes
in our code.  I think including -Wsuggest-attribute=malloc in the
"normal" build just risks raising the noise level for no good reason.

So with that in mind, I went ahead and moved this warning into the set
used when --enable-gcc-warnings.