Always set -fwrapv in the gcc compiler flags on Win32. #18781
Conversation
|
If this builds and passes tests on Win32 (and my wording is good fail), please could someone fast-forward blead with this. Do we actually have anything smoking gcc on win32? |
We need this because we are unfortunately relying on the behaviour of signed integer overflow. This is undefined behaviour in C (which gcc's optimiser likes to take advantage of) unless we use this flag. Previously we had used it conditionally based on gcc version, but that code failed to consider gcc versions greater than 9. We realise that the flag is actually present from the minimum gcc that we support, so we should simply add it always. Fixes #18739.
nwc10
force-pushed
the
smoke-me/win32-fwrapv-always
branch
from
9fefecc
to
3ca197b
Compare
|
Sigh, I shouldn't do this on a fast machine that doesn't have a spell checker locally. |
|
It's fine for me. I haven't tested to see that -fwrapv doesn't get added when MSVC is used, but it's quite apparent from the code that -fwrapv is not added when CC is 'cl'. Cheers, |
There was a problem hiding this comment.
I worry a little about the comment that Zefram added on #13690 that -fwrapv is broken prior to 4.3: #13690 (comment)
I will try to test this with earlier (www.mingw.org) gccs, but I just found that it doesn't currently build anyway (SE_PRIVILEGE_REMOVED, RRF_RT_REG_DWORD and KEY_WOW64_64KEY are undeclared... - presumably happening since commit edfcb93).
Also, there is a typo in the comments: "addeed".
Pants! Thanks. I guess I should drag that commit down onto a machine with a spell checker and see what else I goofed. In 9 days we will celebrate the 13th anniversary of the gcc 4.2.4 release - the question @rjbs asked on the PSC call was whether we are really concerned with supporting a compiler that old. Does 4.2 even compile "hello world" on post NT 4 systems? I'm not really familiar with how compatible Win32 manages to be, but is supporting this about as relevant as trying to support running on Unix on mixed endian hardware? |
The build of 5.34.0 RC1 is also broken. I've just posted to p5p to ask the question should we drop support for www.mingw.org compilers, given how flakey they seem to be: We only had 3.4.5-5.3.0 working anyway; all the later ones already had other issues. (Yes, gcc-3.4.5 does build a perfectly runnable "hello world" C program on Windows 10.) |
|
I do not have HP-UX 10.20 anymore. That system died in a poof of orange smoke, but it had gcc-3.4.6 and no way to build anything newer. Which means that anyone still using HP-UX 10.20, the most recent available gcc compiler is 3.4.6 (as I was the only one making gcc available for 10.20) The AIX box that p5p has access to runs gcc-4.2.4 and I have exactly NULL tuits to even try to build a more recent gcc on it |
|
This was only relevant for Win32. For *nix, it seems that Configure handles all this nicely already: so I don't think we need to worry about old gcc for anything that builds with Configure - for *nix "it ain't broken, so don't fix it". |
|
On Mon, May 10, 2021 at 10:15 PM Nicholas Clark ***@***.***>
wrote
In 9 days we will celebrate the 13th anniversary of the gcc 4.2.4 release
<https://gcc.gnu.org/gcc-4.2/> - the question @rjbs
<https://github.com/rjbs> asked on the PSC call was whether we are really
concerned with supporting a compiler that old.
Personally, I'm not.
Does 4.2 even compile "hello world" on post NT 4 systems?
It probably does. The oldest gcc-4.x.x version that I have on Windows 7 is
4.7.0.
But gcc-3.4.5 still compiles basic C programs on that Windows box - though
I very rarely use that compiler.
I did try the following with it:
____________________
C:\_32\C>gcc --version
gcc (GCC) 3.4.5 (mingw special)
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
C:\_32\C>type try.c
/* try.c */
#include <stdio.h>
int main(void) {
unsigned long bar = 1073741823;
printf("%u\n", bar << 10);
return 0;
}
C:\_32\C>gcc -o try.exe try.c -fwrapv -fno-strict-aliasing
C:\_32\C>try
4294966272
___________________
It's the same results as I get with gcc-11.1.0 on the same Windows 7 box.
I'm not really familiar with how compatible Win32 manages to be, but is
supporting this about as relevant as trying to support running on Unix on
mixed endian hardware?
The challenges might be different, but the level of relevance sounds about
the same to me. (However, I don't really feel qualified to be making such
assessments.)
Cheers,
Rob
|
I tried to find anything about this in google, but to no avail. Anyway, I'm not aware of any valid reasons to use such an old gcc version on Windows. We really should revise our supported compiler/OS version list. |
| # If you are using GCC, 4.3 or later by default we add the -fwrapv option. | ||
| # See https://github.com/Perl/perl5/issues/13690 | ||
| # | ||
| GCCWRAPV := $(shell if "$(GCCVER1)"=="4" (if "$(GCCVER2)" geq "3" echo define) else if "$(GCCVER1)" geq "5" (echo define)) | ||
| # Previously, if you were using GCC, 4.3 or later we addeed the -fwrapv option. | ||
| # See https://github.com/Perl/perl5/issues/13690 and 18739 | ||
| # However, as documented in README.win32 our minimum Win32 gcc version is 3.4.5. | ||
| # As that supports -fwrapv it's both simpler and more correct to unconditionally | ||
| # add it to the flags: | ||
|
|
||
| ifeq ($(GCCWRAPV),define) | ||
| BUILDOPT += -fwrapv | ||
| MINIBUILDOPT += -fwrapv | ||
| endif |
There was a problem hiding this comment.
I think the switch should remain, so you can do gmake GCCWRAPV=undef to test building without -fwrapv.
The ideal is that we eventually remove the -fwrapv entirely, being able to test without it easily is desirable.
There was a problem hiding this comment.
Good points, and I'd not thought of this. Agree, this is better than what I did.
Given that I don't have access to a Win32 system to test this, and I think that you do, and I think that testing the non-default case requires doing something interactively
- Could you implement this and test that it works
- Push it to a branch to replace mine, so that (at least one of) @steve-m-hay @xenu or @sisyphus can check it (better than me)
- If someone else says yes and no-one says no, it goes to blead.
I feel like I'm turning into a project manager and it's not an enjoyable thought. :-/
|
On Tue, May 11, 2021 at 4:16 PM Nicholas Clark ***@***.***> wrote:
Good points, and I'd not thought of this. Agree, this is better than what
I did.
Yes, I think that providing a ready option to disable -fwrapv is an
improvement - especially given our uncertainty regarding supposed bugs re
this switch on older gcc versions.
Then we can just leave it to the builder to override the default behaviour
for whatever reason, whenever convenient.
I can't imagine how this patch of Tony's could possibly do anything other
than what is intended, but I've tested with the following compilers:
mingw.org gcc-9.2.0
32 bit mingw-w64 gcc-10.3.0 and gcc-8.3.0
64 bit mingw-w64 gcc-11.1.0 and gcc-8.3.0.
By "tested", I mean that I verified that -fwrapv was being set. This only
takes a few seconds to ascertain, then I'd hit Ctrl-C, run 'make
distclean', and move on to the next compiler.
I also did the same quick test using those same compilers, having specified
"GCCWRAPV=undef'"- just to be sure that it did remove the -fwrapv switch.
LGTM.
Cheers,
Rob
|
|
Oooh, this is actually my PR. So I don't feel bad in rejecting it and saying #18782 is better. |
We need this because we are unfortunately relying on the behaviour of signed
integer overflow. This is undefined behaviour in C (which gcc's optimiser
likes to take advantage of) unless we use this flag.
Previously we had used it conditionally based on gcc version, but that code
failed to consider gcc versions greater than 9. We realise that the flag is
actually present from the minimum gcc that we support, so we should simply
add it always. Fixes #18739.