Warnings in mingw64 build on emacs-28 branch

Warnings in mingw64 build on emacs-28 branch

[Andy Moreton]

Hi,

There are currently some warnings on the emacs-28 branch when building
with 64bit mingw64 on Windows (gcc 11.2.0).


1) In w32.h, "prepare_standard_handles" and "reset_standard_handles"
have "HANDLE handles[4]" argument, but the handle array has 3 elements
in the definitions and callers.

In function 'child_setup',
    inlined from 'emacs_spawn' at C:/emacs/git/emacs/emacs-28/src/callproc.c:1408:13:
C:/emacs/git/emacs/emacs-28/src/callproc.c:1213:3: warning: 'prepare_standard_handles' accessing 32 bytes in a region of size 24 [-Wstringop-overflow=]
 1213 |   prepare_standard_handles (in, out, err, handles);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/emacs/git/emacs/emacs-28/src/callproc.c: In function 'emacs_spawn':
C:/emacs/git/emacs/emacs-28/src/callproc.c:1213:3: note: referencing argument 4 of type 'void **'
In file included from C:/emacs/git/emacs/emacs-28/nt/inc/sys/socket.h:82,
                 from C:/emacs/git/emacs/emacs-28/src/thread.h:25,
                 from C:/emacs/git/emacs/emacs-28/src/lisp.h:2162,
                 from C:/emacs/git/emacs/emacs-28/src/callproc.c:31:
C:/emacs/git/emacs/emacs-28/src/w32.h:157:13: note: in a call to function 'prepare_standard_handles'
  157 | extern void prepare_standard_handles (int in, int out,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~
In function 'child_setup',
    inlined from 'emacs_spawn' at C:/emacs/git/emacs/emacs-28/src/callproc.c:1408:13:
C:/emacs/git/emacs/emacs-28/src/callproc.c:1217:3: warning: 'reset_standard_handles' accessing 32 bytes in a region of size 24 [-Wstringop-overflow=]
 1217 |   reset_standard_handles (in, out, err, handles);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/emacs/git/emacs/emacs-28/src/callproc.c: In function 'emacs_spawn':
C:/emacs/git/emacs/emacs-28/src/callproc.c:1217:3: note: referencing argument 4 of type 'void **'
In file included from C:/emacs/git/emacs/emacs-28/nt/inc/sys/socket.h:82,
                 from C:/emacs/git/emacs/emacs-28/src/thread.h:25,
                 from C:/emacs/git/emacs/emacs-28/src/lisp.h:2162,
                 from C:/emacs/git/emacs/emacs-28/src/callproc.c:31:
C:/emacs/git/emacs/emacs-28/src/w32.h:161:13: note: in a call to function 'reset_standard_handles'
  161 | extern void reset_standard_handles (int in, int out,
      |             ^~~~~~~~~~~~~~~~~~~~~~


2) This warning has been present for a long time, and seems to be
confusion over the flexible array handling.

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

3) This warning is new with gcc 11.

C:/emacs/git/emacs/emacs-28/src/w32heap.c: In function 'getrlimit':
C:/emacs/git/emacs/emacs-28/src/w32heap.c:853:14: warning: 'm' may be used uninitialized [-Wmaybe-uninitialized]
  853 |         if (!VirtualQuery ((LPCVOID) &m, &m, sizeof m))
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/winbase.h:25,
                 from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:70,
                 from C:/emacs/git/emacs/emacs-28/src/w32common.h:24,
                 from C:/emacs/git/emacs/emacs-28/src/w32heap.c:54:
C:/msys64/mingw64/x86_64-w64-mingw32/include/memoryapi.h:45:28: note: by argument 1 of type 'LPCVOID' {aka 'const void *'} to 'VirtualQuery' declared here
   45 |   WINBASEAPI SIZE_T WINAPI VirtualQuery (LPCVOID lpAddress, PMEMORY_BASIC_INFORMATION lpBuffer, SIZE_T dwLength);
      |                            ^~~~~~~~~~~~
C:/emacs/git/emacs/emacs-28/src/w32heap.c:844:34: note: 'm' declared here
  844 |         MEMORY_BASIC_INFORMATION m;
      |                                  ^

Re: Warnings in mingw64 build on emacs-28 branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sun, 07 Nov 2021 14:46:51 +0000
> 
> 1) In w32.h, "prepare_standard_handles" and "reset_standard_handles"
> have "HANDLE handles[4]" argument, but the handle array has 3 elements
> in the definitions and callers.

I hope I fixed this now.

> 2) This warning has been present for a long time, and seems to be
> confusion over the flexible array handling.

I'm not sure what this is about.  We use memcpy to copy from a Lisp
vector's contents in gazillion other places, and I understand they
don't cause any warnings?  How is this place different?

> 3) This warning is new with gcc 11.
> 
> C:/emacs/git/emacs/emacs-28/src/w32heap.c: In function 'getrlimit':
> C:/emacs/git/emacs/emacs-28/src/w32heap.c:853:14: warning: 'm' may be used uninitialized [-Wmaybe-uninitialized]
>   853 |         if (!VirtualQuery ((LPCVOID) &m, &m, sizeof m))
>       |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/winbase.h:25,
>                  from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:70,
>                  from C:/emacs/git/emacs/emacs-28/src/w32common.h:24,
>                  from C:/emacs/git/emacs/emacs-28/src/w32heap.c:54:
> C:/msys64/mingw64/x86_64-w64-mingw32/include/memoryapi.h:45:28: note: by argument 1 of type 'LPCVOID' {aka 'const void *'} to 'VirtualQuery' declared here
>    45 |   WINBASEAPI SIZE_T WINAPI VirtualQuery (LPCVOID lpAddress, PMEMORY_BASIC_INFORMATION lpBuffer, SIZE_T dwLength);
>       |                            ^~~~~~~~~~~~
> C:/emacs/git/emacs/emacs-28/src/w32heap.c:844:34: note: 'm' declared here
>   844 |         MEMORY_BASIC_INFORMATION m;
>       |                                  ^

That's a compiler bug, I think.  I see nothing wrong in the call.

Re: Warnings in mingw64 build on emacs-28 branch

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> 3) This warning is new with gcc 11.
>> 
>> C:/emacs/git/emacs/emacs-28/src/w32heap.c: In function 'getrlimit':
>> C:/emacs/git/emacs/emacs-28/src/w32heap.c:853:14: warning: 'm' may be used uninitialized [-Wmaybe-uninitialized]
>>   853 |         if (!VirtualQuery ((LPCVOID) &m, &m, sizeof m))
>>       |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/winbase.h:25,
>>                  from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:70,
>>                  from C:/emacs/git/emacs/emacs-28/src/w32common.h:24,
>>                  from C:/emacs/git/emacs/emacs-28/src/w32heap.c:54:
>> C:/msys64/mingw64/x86_64-w64-mingw32/include/memoryapi.h:45:28: note: by argument 1 of type 'LPCVOID' {aka 'const void *'} to 'VirtualQuery' declared here
>>    45 |   WINBASEAPI SIZE_T WINAPI VirtualQuery (LPCVOID lpAddress, PMEMORY_BASIC_INFORMATION lpBuffer, SIZE_T dwLength);
>>       |                            ^~~~~~~~~~~~
>> C:/emacs/git/emacs/emacs-28/src/w32heap.c:844:34: note: 'm' declared here
>>   844 |         MEMORY_BASIC_INFORMATION m;
>>       |                                  ^
>
> That's a compiler bug, I think.  I see nothing wrong in the call.

AFAIU that call to VirtualQuery does not do what we want. When lpAddress
is provided the function rounds down the address to a page boundary and
starts scanning pages *up*. Since we want information about the capacity
of the stack, the scan ignores all the space below the current page
boundary.

There is GetCurrentThreadStackLimits, but requires _WIN32_WINNT >=
0x0602 and it works on allocated pages.

OTOH, we could call CreateThread with dwStackSize set to a fixed value
and return that value from getrlimit(RLIMIT_STACK...). However, we need
the main thread's stack size as well...

There is a hack that uses NtCurrentTeb to obtain the lower address of
the stack and pass it to VirtualQuery. I don't know how portable it is
across Windows versions.

Re: Warnings in mingw64 build on emacs-28 branch

[Andy Moreton]

On Sun 07 Nov 2021, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sun, 07 Nov 2021 14:46:51 +0000
>> 
>> 1) In w32.h, "prepare_standard_handles" and "reset_standard_handles"
>> have "HANDLE handles[4]" argument, but the handle array has 3 elements
>> in the definitions and callers.
>
> I hope I fixed this now.

Yes, thanks.

>> 2) This warning has been present for a long time, and seems to be
>> confusion over the flexible array handling.
>
> I'm not sure what this is about.  We use memcpy to copy from a Lisp
> vector's contents in gazillion other places, and I understand they
> don't cause any warnings?  How is this place different?

No idea. I think this may have been discussed previously, but I cannot
find it in the mail archives. xvector_contents_addr in lisp.h mentions
gcc bug 95072 (filed by Paul Eggert with a similar code example).

Should this be using xvector_contents or vcopy to placate this warning ?

>> 3) This warning is new with gcc 11.
>> 
>> C:/emacs/git/emacs/emacs-28/src/w32heap.c: In function 'getrlimit':
>> C:/emacs/git/emacs/emacs-28/src/w32heap.c:853:14: warning: 'm' may be used uninitialized [-Wmaybe-uninitialized]
>>   853 |         if (!VirtualQuery ((LPCVOID) &m, &m, sizeof m))
>>       |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/winbase.h:25,
>>                  from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:70,
>>                  from C:/emacs/git/emacs/emacs-28/src/w32common.h:24,
>>                  from C:/emacs/git/emacs/emacs-28/src/w32heap.c:54:
>> C:/msys64/mingw64/x86_64-w64-mingw32/include/memoryapi.h:45:28: note: by argument 1 of type 'LPCVOID' {aka 'const void *'} to 'VirtualQuery' declared here
>>    45 |   WINBASEAPI SIZE_T WINAPI VirtualQuery (LPCVOID lpAddress, PMEMORY_BASIC_INFORMATION lpBuffer, SIZE_T dwLength);
>>       |                            ^~~~~~~~~~~~
>> C:/emacs/git/emacs/emacs-28/src/w32heap.c:844:34: note: 'm' declared here
>>   844 |         MEMORY_BASIC_INFORMATION m;
>>       |                                  ^
>
> That's a compiler bug, I think.  I see nothing wrong in the call.

Indeed, although passing "&m" as PMEMORY_BASIC_INFORMATION and as LPCVOID
introduces aliasing.

    AndyM

Re: Warnings in mingw64 build on emacs-28 branch

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Sun, 07 Nov 2021 16:51:56 +0100
> 
> AFAIU that call to VirtualQuery does not do what we want. When lpAddress
> is provided the function rounds down the address to a page boundary and
> starts scanning pages *up*. Since we want information about the capacity
> of the stack, the scan ignores all the space below the current page
> boundary.

How do you see that it _ignores_ that space?  The code does this:

	rlp->rlim_cur = (DWORD_PTR) &m - (DWORD_PTR) m.AllocationBase;

m.AllocationBase is not the page of 'm', it's the base address of a
range of pages to which 'm' belongs.  And since Emacs calls getrlimit
very early during its startup, directly from 'main', 'm' is not far
from the beginning of the stack, and m.AllocationBase is very likely
to be the base address of the memory initially allocated for the
stack.

It is a matter of fact that the result of this code produces the 8MB
stack size that Emacs on Windows is compiled to use (see the link
command in src/Makefile.in).  So it isn't just the theory, that code
actually works.

So I think we are okay in that department.

Re: Warnings in mingw64 build on emacs-28 branch

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Óscar Fuentes <ofv@wanadoo.es>
>> Date: Sun, 07 Nov 2021 16:51:56 +0100
>> 
>> AFAIU that call to VirtualQuery does not do what we want. When lpAddress
>> is provided the function rounds down the address to a page boundary and
>> starts scanning pages *up*. Since we want information about the capacity
>> of the stack, the scan ignores all the space below the current page
>> boundary.
>
> How do you see that it _ignores_ that space?  The code does this:
>
> 	rlp->rlim_cur = (DWORD_PTR) &m - (DWORD_PTR) m.AllocationBase;
>
> m.AllocationBase is not the page of 'm', it's the base address of a
> range of pages to which 'm' belongs.  And since Emacs calls getrlimit
> very early during its startup, directly from 'main', 'm' is not far
> from the beginning of the stack, and m.AllocationBase is very likely
> to be the base address of the memory initially allocated for the
> stack.
>
> It is a matter of fact that the result of this code produces the 8MB
> stack size that Emacs on Windows is compiled to use (see the link
> command in src/Makefile.in).  So it isn't just the theory, that code
> actually works.
>
> So I think we are okay in that department.

Yes, I concentrated on the description of VirtualQuery, which looks like
it ignores everything below lpAddress, and didn't pay attention to
MEMORY_BASIC_INFORMATION contents.

Sorry for the noise.

As for the warning, I think it is because a const void* is passed to
VirtualQuery pointing to an uninitialized object.

Re: Warnings in mingw64 build on emacs-28 branch

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Sun, 07 Nov 2021 20:25:21 +0100
> 
> As for the warning, I think it is because a const void* is passed to
> VirtualQuery pointing to an uninitialized object.

I don't understand why the compiler thinks the object must be
initialized for its pointer to be valid.  Does the compiler assume
something about what VirtualQuery does?  Why does it think the
function will dereference the pointer?

Re: Warnings in mingw64 build on emacs-28 branch

[Andy Moreton]

On Sun 07 Nov 2021, Andy Moreton wrote:

> On Sun 07 Nov 2021, Eli Zaretskii wrote:
>
>>> From: Andy Moreton <andrewjmoreton@gmail.com>
>>> Date: Sun, 07 Nov 2021 14:46:51 +0000
>>> 
>>> 1) In w32.h, "prepare_standard_handles" and "reset_standard_handles"
>>> have "HANDLE handles[4]" argument, but the handle array has 3 elements
>>> in the definitions and callers.
>>
>> I hope I fixed this now.
>
> Yes, thanks.
>
>>> 2) This warning has been present for a long time, and seems to be
>>> confusion over the flexible array handling.
>>
>> I'm not sure what this is about.  We use memcpy to copy from a Lisp
>> vector's contents in gazillion other places, and I understand they
>> don't cause any warnings?  How is this place different?
>
> No idea. I think this may have been discussed previously, but I cannot
> find it in the mail archives. xvector_contents_addr in lisp.h mentions
> gcc bug 95072 (filed by Paul Eggert with a similar code example).
>
> Should this be using xvector_contents or vcopy to placate this warning ?

In set_frame_menubar from w32menu.c, this shows the warning:

	memcpy (previous_items, XVECTOR (f->menu_bar_vector)->contents,
 		previous_menu_items_used * word_size);

...and this placates the compiler:

	memcpy (previous_items, xvector_contents (f->menu_bar_vector),
 		previous_menu_items_used * word_size);

Re: Warnings in mingw64 build on emacs-28 branch

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> As for the warning, I think it is because a const void* is passed to
>> VirtualQuery pointing to an uninitialized object.
>
> I don't understand why the compiler thinks the object must be
> initialized for its pointer to be valid.  Does the compiler assume
> something about what VirtualQuery does?  Why does it think the
> function will dereference the pointer?

The warning says "may be used uninitialized". The compiler doesn't know,
it sees something suspicious and speaks up. That's with -Wall, without
it, the compiler remains silent.

I guess the MinGW headers should be annotated with whatever decoration
gcc uses to convey that the pointer is not dereferenced.

Or we can initialize `m' to zero, which will silence the warning and is
a good practice in general when working with the Windows API.

Re: Warnings in mingw64 build on emacs-28 branch

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sun, 07 Nov 2021 19:41:51 +0000
> 
> In set_frame_menubar from w32menu.c, this shows the warning:
> 
> 	memcpy (previous_items, XVECTOR (f->menu_bar_vector)->contents,
>  		previous_menu_items_used * word_size);
> 
> ...and this placates the compiler:
> 
> 	memcpy (previous_items, xvector_contents (f->menu_bar_vector),
>  		previous_menu_items_used * word_size);

That doesn't explain why we don't need the same in the other places
where memcpy and XVECTOR are used exactly like in w32menu.c, at least
AFAICT.

What your trick does is hide XVECTOR behind enough indirections for
the compiler to lose sight of what is going on.  That is fine, but as
compilers look deeper and deeper, this trick will one day cease to be
enough.  Thus, I'd still prefer to understand why the other calls of
memcpy don't trigger similar warnings.

Thanks.

Re: Warnings in mingw64 build on emacs-28 branch

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Sun, 07 Nov 2021 20:57:38 +0100
> 
> > I don't understand why the compiler thinks the object must be
> > initialized for its pointer to be valid.  Does the compiler assume
> > something about what VirtualQuery does?  Why does it think the
> > function will dereference the pointer?
> 
> The warning says "may be used uninitialized". The compiler doesn't know,
> it sees something suspicious and speaks up. That's with -Wall, without
> it, the compiler remains silent.
> 
> I guess the MinGW headers should be annotated with whatever decoration
> gcc uses to convey that the pointer is not dereferenced.
> 
> Or we can initialize `m' to zero, which will silence the warning and is
> a good practice in general when working with the Windows API.

I actually tend to think that this is a compiler bug that should be
reported to the GCC folks.

Re: Warnings in mingw64 build on emacs-28 branch

[Andy Moreton]

On Sun 07 Nov 2021, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sun, 07 Nov 2021 19:41:51 +0000
>> 
>> In set_frame_menubar from w32menu.c, this shows the warning:
>> 
>> 	memcpy (previous_items, XVECTOR (f->menu_bar_vector)->contents,
>>  		previous_menu_items_used * word_size);
>> 
>> ...and this placates the compiler:
>> 
>> 	memcpy (previous_items, xvector_contents (f->menu_bar_vector),
>>  		previous_menu_items_used * word_size);
>
> That doesn't explain why we don't need the same in the other places
> where memcpy and XVECTOR are used exactly like in w32menu.c, at least
> AFAICT.

I suspect that understanding that will require the expertise of someone
with deep knowledge of gcc internals.

> What your trick does is hide XVECTOR behind enough indirections for
> the compiler to lose sight of what is going on.  That is fine, but as
> compilers look deeper and deeper, this trick will one day cease to be
> enough.  Thus, I'd still prefer to understand why the other calls of
> memcpy don't trigger similar warnings.

True, but hopefully by the time compilers can see past the indirections,
they can also perform a correct analysis of shallower paths.

Such accessor helpers are surely useful anyway, as they make the
callsites clearer (and if used consistently, allow the
implementation to be changed without changing the callers).

    AndyM