Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't build perl with gcc4.9 due to UB in sv.c #13690

Open
p5pRT opened this issue Mar 25, 2014 · 36 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Mar 25, 2014

Migrated from rt.perl.org#121505 (status was 'open')

Searchable as RT121505$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 25, 2014

From polacek@redhat.com

Created by polacek@redhat.com

Perl Info

Flags:
    category=core
    severity=high

This perlbug was built using Perl 5.14.3 in the Fedora build system.
It is being executed now by Perl 5.14.3 - Fri Jan 11 13:09:43 UTC 2013.

Site configuration information for perl 5.14.3:

Configured by Red Hat, Inc. at Fri Jan 11 13:09:43 UTC 2013.

Summary of my perl5 (revision 5 version 14 subversion 3) configuration:
   
  Platform:
    osname=linux, osvers=2.6.32-279.9.1.el6.x86_64, archname=x86_64-linux-thread-multi
    uname='linux buildvm-25.phx2.fedoraproject.org 2.6.32-279.9.1.el6.x86_64 #1 smp fri aug 31 09:04:24 edt 2012 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Doptimize=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic -Dccdlflags=-Wl,--enable-new-dtags -DDEBUGGING=-g -Dversion=5.14.3 -Dmyhostname=localhost -Dperladmin=root@localhost -Dcc=gcc -Dcf_by=Red Hat, Inc. -Dprefix=/usr -Dvendorprefix=/usr -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl5 -Dsitearch=/usr/local/lib64/perl5 -Dprivlib=/usr/share/perl5 -Dvendorlib=/usr/share/perl5/vendor_perl -Darchlib=/usr/lib64/perl5 -Dvendorarch=/usr/lib64/perl5/vendor_perl -Darchname=x86_64-linux-thread-multi -Dlibpth=/usr/local/lib64 /lib64 /usr/lib64 -Duseshrplib -Dusethreads -Duseithreads -Dusedtrace=/usr/bin/dtrace -Duselargefiles -Dd_semctl_semun -Di_db -Ui_ndbm -Di_gdbm -Di_shadow -Di_syslog -Dman3ext=3pm -Duseperlio -Dinstallusrbinperl=n -Ubincompat5005 -Uversiononly -Dpager=/usr/bin/less -isr -Dd_gethostent_r_proto -Ud_endhostent_r_proto -Ud_sethostent_r_proto -Ud_endprotoent_r_proto -Ud_setprotoent_r_proto -Ud_endservent_r_proto -Ud_setservent_r_proto -Dscriptdir=/usr/bin'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.6.3 20120306 (Red Hat 4.6.3-2)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -fstack-protector'
    libpth=/usr/local/lib64 /lib64 /usr/lib64
    libs=-lresolv -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc -lgdbm_compat
    perllibs=-lresolv -lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.14.90'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,--enable-new-dtags -Wl,-rpath,/usr/lib64/perl5/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'

Locally applied patches:
    


@INC for perl 5.14.3:
    /home/marek/perl5/lib/perl5/x86_64-linux-thread-multi
    /home/marek/perl5/lib/perl5/x86_64-linux-thread-multi
    /home/marek/perl5/lib/perl5
    /usr/local/lib64/perl5
    /usr/local/share/perl5
    /usr/lib64/perl5/vendor_perl
    /usr/share/perl5/vendor_perl
    /usr/lib64/perl5
    /usr/share/perl5
    .


Environment for perl 5.14.3:
    HOME=/home/marek
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH=/home/marek/rh/x/trunk/gcc:/home/marek/rh/x/trunk/gcc/32:/home/marek/rh/x/trunk/x86_64-unknown-linux-gnu/32/libsanitizer/ubsan/.libs::/home/marek/rh/x/trunk/gcc:/home/marek/rh/x/trunk/gcc/32:/home/marek/rh/x/trunk/x86_64-unknown-linux-gnu/32/libsanitizer/ubsan/.libs:/usr/lib64/mpich2/lib
    LOGDIR (unset)
    PATH=/home/marek/perl5/bin:/home/marek/perl5/bin:/home/marek/perl5/bin:/usr/lib64/qt-3.3/bin:/usr/lib64/mpich2/bin:/usr/kerberos/sbin:/usr/kerberos/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/home/marek/.local/bin:/home/marek/bin
    PERL5LIB=/home/marek/perl5/lib/perl5/x86_64-linux-thread-multi:/home/marek/perl5/lib/perl5
    PERL_BADLANG (unset)
    PERL_LOCAL_LIB_ROOT=/home/marek/perl5
    PERL_MB_OPT=--install_base /home/marek/perl5
    PERL_MM_OPT=INSTALL_BASE=/home/marek/perl5
    SHELL=/bin/bash

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 2, 2014

From @tonycoz

On Tue, Apr 01, 2014 at 07​:24​:37AM +0100, Zefram wrote​:

Tony Cook wrote​:

-fwrapv does prevent the optimizations that cause the test failures.

Right. It might be an interesting exercise to check what other parts
of the code it affects. Diffing the assembler may yield a comprehensive
list of points where this `optimisation' breaks the semantics we expect.
This would be more complete than you'd get by just looking at test
failures. (Still not likely to be a complete list of places where we
rely on wrapping, though.)

Unfortunately there's a lot of code re-ordering differences between
the two that makes it difficult to compare.

Interestingly enough the code that the patch updates has no changes
between no -fwrapv and with -fwrapv.

Adding the patch produces a significant change in the code - closer to
a simple translation of the code rather than what appears to be some
fancy 2's complment processing​:

Without the patch​:

  ; the .loc lines are line number information
  .loc 1 2054 0
  cqto ; sign extend rax into rdx​:rax
  .loc 1 2053 0
  movabsq $9007199254740991, %rcx ; ( 1 << NV_PRESERVES_UV_BITS ) - 1
  .loc 1 2054 0
  xorq %rdx, %rax
  subq %rdx, %rax
  movq %rax, %rdx
  .loc 1 2285 0
  xorl %eax, %eax
  .loc 1 2053 0
  cmpq %rcx, %rdx
  ja .L4793

With the patch​:

  .loc 1 2053 0
  testq %rdx, %rdx
  jle .L4724
  movabsq $9007199254740991, %rdx
  cmpq %rdx, 32(%rax)
  setbe %al
.L4725​:
  .loc 1 2053 0 discriminator 4
  testb %al, %al
  je .L4787
  .loc 1 2060 0 is_stmt 1
  movl 12(%rbx), %edx
  .loc 1 2285 0
  xorl %eax, %eax
  .loc 1 2060 0
  testb $2, %dh
  je .L4787
  ...

.L4724​:
.LBE1932​:
  .loc 1 2053 0 discriminator 2
  movabsq $9007199254740991, %rax
  .loc 1 2054 0 discriminator 2
  negq %rdx
  .loc 1 2053 0 discriminator 2
  cmpq %rax, %rdx
  setbe %al
  jmp .L4725

If that works, there are multiple things we could do with the list.
An alternative to adding -fwrapv to the command line would be to edit
all the locations shown up to avoid signed overflow, along the lines of
the minimal patch that tackles just one location. This is weaker than
actually using -fwrapv, because it only fixes the locations that are
currently affected by the dodgy optimisation. We could also attempt to
write tests to exercise the overflow behaviour of each location.

I don't think that's practical.

To include -fwrapv where needed I think a change to Configure will be
needed, since gcc can be used on several platforms.

Yes, absolutely. We'd need to add it whenever we're using a gcc that
supports the flag. (The flag does exist prior to 4.9; don't know how
far back it goes.)

At least back to 4.1.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 2, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 8, 2014

From @tonycoz

Subject altered so it shows in the ticket too.

On Mon, Apr 07, 2014 at 11​:25​:14AM -0400, Ricardo Signes wrote​:

7. Can't build perl with gcc4.9 due to UB in sv.c
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121505

Discussion of this on list has made it seem that this can't be a blocker\,
and that we will need to carefully update our use of compiler flags to be
sure we get this right\.  Please inform me if I've misread\.

Alternately&#8203;: compiler\-savvy people&#8203;: is this a blocker for 5\.20\.0\, 5\.20\.1\,
or 5\.22?

GCC 4.9 hasn't been released yet, but will likely be released by the
time we release 5.20[1], so users will expect to be able to build and
test with it.

So I think it's a 5.20 issue.

The behaviour we're depending on technically violates the C standard
(at least C89 and C99), and GCC takes advantage of that. GCC 4.9
happens to take it further and breaks perl.

In the short term for 5.20.0 I think we (and I'll make a start on it)
update Configure to check if -fwrapv is supported and if so, enable
it (or cflags.SH?)

In the longer term I think we need to deal with the problem.

In some cases, like the handling of RExC_naughty in regcomp.c, I think
changing the type to an unsigned type will be enough of a change. I
suspect the current behaviour here is that some regexps aren't being
flagged "naughty" that should be[2].

Other cases, like the overflow at line 4661 in regcomp.c​:

  data->last_start_max += is_inf ? SSize_t_MAX
  : (maxcount - 1) * (minnext + data->pos_delta);

could be real problems leading to bugs down the road.

(the above 2 of many detected with -fsantize=undefined)

Tony

[1] 4.7.0 and 4.8.0 were released in March 2012 and 2013 respectively
(http​://www.gnu.org/software/gcc/releases.html) and the last GCC 4.9
status report hoped to be announcing a RC "soon"
(http​://gcc.gnu.org/ml/gcc/2014-03/msg00190.html)

[2] I don't know if that matters much, I only see one test for
PREGf_NAUGHTY in regexec.c

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 8, 2014

From @doughera88

On Tue, Apr 08, 2014 at 05​:00​:13PM +1000, Tony Cook wrote​:

Subject altered so it shows in the ticket too.

On Mon, Apr 07, 2014 at 11​:25​:14AM -0400, Ricardo Signes wrote​:

7. Can't build perl with gcc4.9 due to UB in sv.c
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121505

Discussion of this on list has made it seem that this can't be a blocker\,
and that we will need to carefully update our use of compiler flags to be
sure we get this right\.  Please inform me if I've misread\.

Alternately&#8203;: compiler\-savvy people&#8203;: is this a blocker for 5\.20\.0\, 5\.20\.1\,
or 5\.22?

GCC 4.9 hasn't been released yet, but will likely be released by the
time we release 5.20[1], so users will expect to be able to build and
test with it.

So I think it's a 5.20 issue.

The behaviour we're depending on technically violates the C standard
(at least C89 and C99), and GCC takes advantage of that. GCC 4.9
happens to take it further and breaks perl.

In the short term for 5.20.0 I think we (and I'll make a start on it)
update Configure to check if -fwrapv is supported and if so, enable
it (or cflags.SH?)

I think it would have to be Configure. cflags.SH only affects the
compilation of the perl core files. Don't we need the flag to be
propagated to config.sh (and hence Config.pm) and used in building
extensions as well?

(I'm afraid I haven't looked at the underlying issue at all, since it
looks like an issue that requires some sustained thought, and I haven't
had sufficient blocks of time for that in quite a while.)

--
  Andy Dougherty doughera@​lafayette.edu

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 8, 2014

From zefram@fysh.org

Andy Dougherty wrote​:

                                Don't we need the flag to be

propagated to config.sh (and hence Config.pm) and used in building
extensions as well?

Yes.

-zefram

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From @tonycoz

On Tue Apr 08 04​:46​:13 2014, zefram@​fysh.org wrote​:

Andy Dougherty wrote​:

                                Don't we need the flag to be

propagated to config.sh (and hence Config.pm) and used in building
extensions as well?

Patch to Configure attached for review.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From @tonycoz

0001-perl-121505-add-fwrapv-to-ccflags-for-gcc-4.9-and-la.patch
From 1d48451145d8351349ecdfa6a13ea422a4480cd3 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 9 Apr 2014 11:58:33 +1000
Subject: [PATCH] [perl #121505] add -fwrapv to ccflags for gcc 4.9 and later

But skip adding it if -fwrapv is already provided, or if -fno-wrapv or
-fsanitize=undefined is supplied in ccflags.
---
 Configure |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Configure b/Configure
index 15b3da1..56ce12d 100755
--- a/Configure
+++ b/Configure
@@ -4643,6 +4643,22 @@ case "$gccversion" in
     $rm -f try try.*
 esac
 
+# gcc 4.9 by default does some optimizations that break perl.
+# see ticket 121505.
+#
+# The -fwrapv disables those optimizations (and probably others,) so
+# for gcc 4.9 (and later, since the optimizations probably won't go
+# away), add -fwrapv unless the user requests -fno-wrapv, which
+# disables -fwrapv, or if the user requests -fsanitize=undefined,
+# which turns the overflows -fwrapv ignores into runtime errors.
+case "$gccversion" in
+4.9.*|4.1[0-9].*|[5-9].*)
+    case "$ccflags" in
+    *-fno-wrapv*|*-fsanitize=undefined*|*-fwrapv*) ;;
+    *) ccflags="$ccflags -fwrapv" ;;
+    esac
+esac
+
 : What should the include directory be ?
 : Use sysroot if set, so findhdr looks in the right place.
 echo " "
-- 
1.7.10.4

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From @Tux

On Tue, 8 Apr 2014 18​:59​:35 -0700, "Tony Cook via RT"
<perlbug-followup@​perl.org> wrote​:

On Tue Apr 08 04​:46​:13 2014, zefram@​fysh.org wrote​:

Andy Dougherty wrote​:

                                Don't we need the flag to be

propagated to config.sh (and hence Config.pm) and used in building
extensions as well?

Patch to Configure attached for review.

Why make it a new case, and not integrate into the switch just above?

And are there any plans at all to "fix" the cause for this patch in
perl-core? I saw some discussions pass, but I didn't pay too much
attention. I got the idea people thought it too much work to fix all
issues that are related to this optimization (other arch than intel)

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.19 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From @tonycoz

On Wed, Apr 09, 2014 at 08​:09​:33AM +0200, H.Merijn Brand wrote​:

On Tue, 8 Apr 2014 18​:59​:35 -0700, "Tony Cook via RT"
<perlbug-followup@​perl.org> wrote​:

On Tue Apr 08 04​:46​:13 2014, zefram@​fysh.org wrote​:

Andy Dougherty wrote​:

                                Don't we need the flag to be

propagated to config.sh (and hence Config.pm) and used in building
extensions as well?

Patch to Configure attached for review.

Why make it a new case, and not integrate into the switch just above?

To make it easier to remove later (or at least make it easier to to
what needs to be removed.)

And are there any plans at all to "fix" the cause for this patch in
perl-core? I saw some discussions pass, but I didn't pay too much
attention. I got the idea people thought it too much work to fix all
issues that are related to this optimization (other arch than intel)

I think it's too much work to fix them all and be sure the fixes
haven't introduced new issues before 5.20 is due for release.

I think they should be fixed for 5.22, and then the Configure change
removed.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From zefram@fysh.org

Tony Cook via RT wrote​:

+# for gcc 4.9 (and later, since the optimizations probably won't go
+# away), add -fwrapv

Should also add it for older gccs, as far back as the flag is supported
and working. We only notice the need on 4.9, but that doesn't mean
there aren't similar problems lurking.

+4.9.*|4.1[0-9].*|[5-9].*)

Misses a lot of possible version numbers. For 4.9+, you want

  4.9.*|4.[1-9][0-9]*|[5-9].*|[1-9][0-9]*)

A bit of googling says -fwrapv is broken prior to 4.3, so that would be
a better threshold​:

  4.[3-9].*|4.[1-9][0-9]*|[5-9].*|[1-9][0-9]*)

Some explanation​:

  http​://www.airs.com/blog/archives/120

That page also offers the options -fno-strict-overflow and
-Wstrict-overflow. I think we do want -fwrapv rather than
-fno-strict-overflow. We could usefully apply -Wstrict-overflow to
help audit (in the 5.21 cycle) for other places where we're relying on
overflow behaviour​: it won't give complete results, but will at least
tell us where the overflow would affect gcc.

-zefram

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From @Tux

On Wed, 9 Apr 2014 10​:15​:53 +0100, Zefram <zefram@​fysh.org> wrote​:

Tony Cook via RT wrote​:

+# for gcc 4.9 (and later, since the optimizations probably won't go
+# away), add -fwrapv

Should also add it for older gccs, as far back as the flag is supported
and working. We only notice the need on 4.9, but that doesn't mean
there aren't similar problems lurking.

+4.9.*|4.1[0-9].*|[5-9].*)

Misses a lot of possible version numbers. For 4.9+, you want

4\.9\.\*|4\.\[1\-9\]\[0\-9\]\*|\[5\-9\]\.\*|\[1\-9\]\[0\-9\]\*\)

A bit of googling says -fwrapv is broken prior to 4.3, so that would be
a better threshold​:

4\.\[3\-9\]\.\*|4\.\[1\-9\]\[0\-9\]\*|\[5\-9\]\.\*|\[1\-9\]\[0\-9\]\*\)

I already pushed Tony's patch. Shall I amend it?

Some explanation​:

http&#8203;://www\.airs\.com/blog/archives/120

That page also offers the options -fno-strict-overflow and
-Wstrict-overflow. I think we do want -fwrapv rather than
-fno-strict-overflow. We could usefully apply -Wstrict-overflow to
help audit (in the 5.21 cycle) for other places where we're relying on
overflow behaviour​: it won't give complete results, but will at least
tell us where the overflow would affect gcc.

-zefram

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.19 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From zefram@fysh.org

H.Merijn Brand wrote​:

I already pushed Tony's patch. Shall I amend it?

+1 to amend.

-zefram

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From @Tux

On Wed, 9 Apr 2014 10​:33​:23 +0100, Zefram <zefram@​fysh.org> wrote​:

H.Merijn Brand wrote​:

I already pushed Tony's patch. Shall I amend it?

+1 to amend.

For those not following git, both the original patch and the amendment
done in

commit 00051dd
Author​: H.Merijn Brand <h.m.brand@​xs4all.nl>
Date​: Wed Apr 9 12​:31​:23 2014 +0200

  -fwrapv is broken prior to gcc-4.3 (googled and patched by Zefram)

commit 8697475
Author​: H.Merijn Brand <h.m.brand@​xs4all.nl>
Date​: Wed Apr 9 11​:16​:55 2014 +0200

  gcc 4.9 by default does some optimizations that break perl (#121505)

  Patch by Tony Cook

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.19 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From @tonycoz

On Wed Apr 09 02​:16​:26 2014, zefram@​fysh.org wrote​:

Tony Cook via RT wrote​:

+# for gcc 4.9 (and later, since the optimizations probably won't go
+# away), add -fwrapv

Should also add it for older gccs, as far back as the flag is supported
and working. We only notice the need on 4.9, but that doesn't mean
there aren't similar problems lurking.

+4.9.*|4.1[0-9].*|[5-9].*)

Misses a lot of possible version numbers. For 4.9+, you want

4\.9\.\*|4\.\[1\-9\]\[0\-9\]\*|\[5\-9\]\.\*|\[1\-9\]\[0\-9\]\*\)

A bit of googling says -fwrapv is broken prior to 4.3, so that would be
a better threshold​:

4\.\[3\-9\]\.\*|4\.\[1\-9\]\[0\-9\]\*|\[5\-9\]\.\*|\[1\-9\]\[0\-9\]\*\)

Some explanation​:

http&#8203;://www\.airs\.com/blog/archives/120

That page also offers the options -fno-strict-overflow and
-Wstrict-overflow. I think we do want -fwrapv rather than
-fno-strict-overflow. We could usefully apply -Wstrict-overflow to
help audit (in the 5.21 cycle) for other places where we're relying on
overflow behaviour​: it won't give complete results, but will at least
tell us where the overflow would affect gcc.

The problem with including 4.3 - 4.8 is -fwrapv disables optimizations, and
since we've had no reports that those optimizations cause problems in gcc 4.3
through 4.8, we're slowing perl down for no particular reason.

I still need to produce an update for win32/makefile.mk.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 9, 2014

From @tonycoz

On Wed Apr 09 14​:55​:54 2014, tonyc wrote​:

I still need to produce an update for win32/makefile.mk.

Though op/numconvert.t doesn't fail, nor does any other relevant test, with 4.9.0 20140218.

I don't really see a way to pick out a version in makefile.mk either, the only real solution I see is to always add it.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 10, 2014

From @steve-m-hay

On 10 April 2014 00​:57, Tony Cook via RT <perlbug-followup@​perl.org> wrote​:

On Wed Apr 09 14​:55​:54 2014, tonyc wrote​:

I still need to produce an update for win32/makefile.mk.

Though op/numconvert.t doesn't fail, nor does any other relevant test, with 4.9.0 20140218.

I don't really see a way to pick out a version in makefile.mk either, the only real solution I see is to always add it.

Add a new CCTYPE, as is done for different MSVC++ versions? GCC49 or
GCC490 if you're using gcc-4.9.0 or higher?

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 10, 2014

From zefram@fysh.org

Tony Cook via RT wrote​:

The problem with including 4.3 - 4.8 is -fwrapv disables optimizations,

It disables specifically the optimisations that break our code.

since we've had no reports that those optimizations cause problems

The problems wouldn't necessarily have been noticed.

-zefram

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 14, 2014

From @tonycoz

On Thu Apr 10 00​:03​:05 2014, shay wrote​:

On 10 April 2014 00​:57, Tony Cook via RT <perlbug-followup@​perl.org>
wrote​:

On Wed Apr 09 14​:55​:54 2014, tonyc wrote​:

I still need to produce an update for win32/makefile.mk.

Though op/numconvert.t doesn't fail, nor does any other relevant
test, with 4.9.0 20140218.

I don't really see a way to pick out a version in makefile.mk either,
the only real solution I see is to always add it.

Add a new CCTYPE, as is done for different MSVC++ versions? GCC49 or
GCC490 if you're using gcc-4.9.0 or higher?

Adding extra CCTYPE values means touching every place that looks at CCTYPE.

I tried to add GCCVERSION and then parse that using findstr, but it was unwieldly[1] and didn't work, since dmake treats the non-zero exit from findstr when it doesn't find the string as a build-time error.

I ended up adding a GCCWRAPV macro that can be overridden from the dmake command-line which enables -fwrapv by default.

This builds and tests ok with the gcc 4.7.3 included with the strawberry perl I have installed.

It builds perl successfully with a mingw gcc-4.9 snapshot, but I suspect testing would fail due to some strangeness in the built Errno.pm (the errno codes have l (lowercase L) suffixes.

For review/comment.

Tony

[1] findstr supports regexps - but the regexps don't support alternation

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 18, 2014

From @steve-m-hay

On Sun Apr 13 20​:35​:51 2014, tonyc wrote​:

On Thu Apr 10 00​:03​:05 2014, shay wrote​:

On 10 April 2014 00​:57, Tony Cook via RT <perlbug-followup@​perl.org>
wrote​:

On Wed Apr 09 14​:55​:54 2014, tonyc wrote​:

I still need to produce an update for win32/makefile.mk.

Though op/numconvert.t doesn't fail, nor does any other relevant
test, with 4.9.0 20140218.

I don't really see a way to pick out a version in makefile.mk
either,
the only real solution I see is to always add it.

Add a new CCTYPE, as is done for different MSVC++ versions? GCC49 or
GCC490 if you're using gcc-4.9.0 or higher?

Adding extra CCTYPE values means touching every place that looks at
CCTYPE.

I tried to add GCCVERSION and then parse that using findstr, but it
was unwieldly[1] and didn't work, since dmake treats the non-zero exit
from findstr when it doesn't find the string as a build-time error.

I ended up adding a GCCWRAPV macro that can be overridden from the
dmake command-line which enables -fwrapv by default.

This builds and tests ok with the gcc 4.7.3 included with the
strawberry perl I have installed.

It builds perl successfully with a mingw gcc-4.9 snapshot, but I
suspect testing would fail due to some strangeness in the built
Errno.pm (the errno codes have l (lowercase L) suffixes.

For review/comment.

I don't see any new attachment or smoke-me branch for review/comment. Am I missing something?

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 18, 2014

From @rurban

I'm following this thread with more and more disgust.
Are your really advocating keeping wrong and UB? This will turn into
security issues, I'll promise you.
You really don't want to fix the signed integers to unsigned when you
wrap it, and you rather want to hurt all of integer arithmetic
optimizations by forcing -fwrapv onto us?

Seriously, fix the problems as patched by Mark Polacek who knows what
he is doing, and don't cover it up with -fwrapv.
Optimizers rely on proper signed vs unsigned types, and if you treat
it as unsigned declare it as such, so that the optimizer knows about
it.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2014

From @tonycoz

On Fri Apr 18 08​:25​:02 2014, shay wrote​:

I don't see any new attachment or smoke-me branch for review/comment.
Am I missing something?

Oops, here it is.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2014

From @tonycoz

0001-perl-121505-include-fwrapv-by-default-for-GCC.patch
From 738956387b9513375e745028974e58b211bd4573 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 14 Apr 2014 13:28:26 +1000
Subject: [perl #121505] include -fwrapv by default for GCC

Unfortunately dmake/the Win32 command-line make it difficult to parse
a GCC version (even if we pre-extract it), set -fwrapv by default and
make it easy to disable, eg.

  dmake GCCWRAPV=undef
---
 win32/makefile.mk |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/win32/makefile.mk b/win32/makefile.mk
index c11a7a9..ee25f11 100644
--- a/win32/makefile.mk
+++ b/win32/makefile.mk
@@ -141,6 +141,12 @@ USE_LARGE_FILES	*= define
 CCTYPE		*= GCC
 
 #
+# If you are using GCC, by default we add the -fwrapv option.
+# See https://rt.perl.org/Ticket/Display.html?id=121505
+#
+GCCWRAPV       *= define
+
+#
 # If you are using Intel C++ Compiler uncomment this
 #
 #__ICC		*= define
@@ -408,12 +414,19 @@ INST_HTML	= $(INST_TOP)$(INST_VER)\html
 
 .USESHELL :
 
+MINIBUILDOPT    *=
+
 .IF "$(CCTYPE)" == "GCC"
 
 .IF "$(GCCCROSS)" == "define"
 ARCHPREFIX      = x86_64-w64-mingw32-
 .ENDIF
 
+.IF "$(GCCWRAPV)" == "define"
+BUILDOPT        += -fwrapv
+MINIBUILDOPT    += -fwrapv
+.ENDIF
+
 CC		= $(ARCHPREFIX)gcc
 LINK32		= $(ARCHPREFIX)g++
 LIB32		= $(ARCHPREFIX)ar rc
@@ -1127,7 +1140,7 @@ $(MINIDIR) :
 	if not exist "$(MINIDIR)" mkdir "$(MINIDIR)"
 
 $(MINICORE_OBJ) : $(CORE_NOCFG_H)
-	$(CC) -c $(CFLAGS) -DPERL_EXTERNAL_GLOB -DPERL_IS_MINIPERL $(OBJOUT_FLAG)$@ ..\$(*B).c
+	$(CC) -c $(CFLAGS) $(MINIBUILDOPT) -DPERL_EXTERNAL_GLOB -DPERL_IS_MINIPERL $(OBJOUT_FLAG)$@ ..\$(*B).c
 
 $(MINIWIN32_OBJ) : $(CORE_NOCFG_H)
 	$(CC) -c $(CFLAGS) -DPERL_IS_MINIPERL $(OBJOUT_FLAG)$@ $(*B).c
-- 
1.7.4.msysgit.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2014

From @tonycoz

On Fri Apr 18 12​:30​:19 2014, rurban wrote​:

I'm following this thread with more and more disgust.
Are your really advocating keeping wrong and UB? This will turn into
security issues, I'll promise you.
You really don't want to fix the signed integers to unsigned when you
wrap it, and you rather want to hurt all of integer arithmetic
optimizations by forcing -fwrapv onto us?

That patch fixes only a few of several issues detected by -fsanitize=undefined.

Fixing all of the integer overflow undefined behaviour in perl would either mean releasing a perl with possibly tricky changes without extensive testing, or a several month delay while we wait for the changes to settle down.

Note that most of these issues are in 5.18, 5.16, 5.14 etc, we're not releasing a perl with new bugs. (well, not these new bugs)

Seriously, fix the problems as patched by Mark Polacek who knows what
he is doing, and don't cover it up with -fwrapv.
Optimizers rely on proper signed vs unsigned types, and if you treat
it as unsigned declare it as such, so that the optimizer knows about
it.

The -fwrapv change is only a workaround, which you know if you're read the thread.

I don't plan to close this ticket once these workarounds are in, I'll open new tickets that this ticket depends on for each of the issues -fsanitize=undefined.

No-one is happy with the -fwrapv "solution".

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 22, 2014

From @steve-m-hay

On Mon Apr 21 16​:05​:34 2014, tonyc wrote​:

On Fri Apr 18 08​:25​:02 2014, shay wrote​:

I don't see any new attachment or smoke-me branch for review/comment.
Am I missing something?

Oops, here it is.

Testing with a 32-bit snapshot (http​://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win32/Personal%20Builds/Experimental_Builds/4.9.0/threads-win32/sjlj/) I see no problem to start with, but with a 64-bit snapshot (http​://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/Experimental_Builds/4.9.0/threads-win32/seh/) I get failures in op/range.t and op/numconvert.t without the patch.

Those failures are fixed by the patch.

Why is the -fwrapv left out from the MINIWIN32_OBJ compilation? My test results suggest that we don't currently have a problem with those files (win32.c, win32sck.c, win32thread.c, fcrypt.c and win32io.c) but surely it wouldn't hurt to compile them with it anyway to protect against any future trouble? (Likewise the x2p files, although they hardly matter since they're moving out of core in due course anyway.)

Also, since the failure only happens on a 64-bit build, is it worth only including -fwrapv when doing a 64-bit build? That way, users of earlier versions of gcc that don't support -fwrapv aren't bothered by this if they're only doing a 32-bit build anyway. (Or have I just been lucky in not seeing any failure in a 32-bit build?)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 22, 2014

From @steve-m-hay

On Tue Apr 22 05​:10​:18 2014, shay wrote​:

On Mon Apr 21 16​:05​:34 2014, tonyc wrote​:

On Fri Apr 18 08​:25​:02 2014, shay wrote​:

I don't see any new attachment or smoke-me branch for
review/comment.
Am I missing something?

Oops, here it is.

Testing with a 32-bit snapshot (http​://sourceforge.net/projects/mingw-
w64/files/Toolchains%20targetting%20Win32/Personal%20Builds/Experimental_Builds/4.9.0/threads-
win32/sjlj/) I see no problem to start with, but with a 64-bit
snapshot (http​://sourceforge.net/projects/mingw-
w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/Experimental_Builds/4.9.0/threads-
win32/seh/) I get failures in op/range.t and op/numconvert.t without
the patch.

Those failures are fixed by the patch.

Why is the -fwrapv left out from the MINIWIN32_OBJ compilation? My
test results suggest that we don't currently have a problem with those
files (win32.c, win32sck.c, win32thread.c, fcrypt.c and win32io.c) but
surely it wouldn't hurt to compile them with it anyway to protect
against any future trouble? (Likewise the x2p files, although they
hardly matter since they're moving out of core in due course anyway.)

Also, since the failure only happens on a 64-bit build, is it worth
only including -fwrapv when doing a 64-bit build? That way, users of
earlier versions of gcc that don't support -fwrapv aren't bothered by
this if they're only doing a 32-bit build anyway. (Or have I just been
lucky in not seeing any failure in a 32-bit build?)

The following also initializes GCCWRAPV better, so that it's only set to "define" by default for 4.3.0 or higer​:

GCCWRAPV *= $(shell for /f "delims=. tokens=1,2,3" %i in ('gcc -dumpversion') do @​if "%i"=="4" (if "%j" geq "3" echo define) else if "%i" geq "5" (echo define))

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 22, 2014

From @steve-m-hay

On 22 April 2014 18​:12, Steve Hay via RT <perlbug-followup@​perl.org> wrote​:

On Tue Apr 22 05​:10​:18 2014, shay wrote​:

On Mon Apr 21 16​:05​:34 2014, tonyc wrote​:

On Fri Apr 18 08​:25​:02 2014, shay wrote​:

I don't see any new attachment or smoke-me branch for
review/comment.
Am I missing something?

Oops, here it is.

Testing with a 32-bit snapshot (http​://sourceforge.net/projects/mingw-
w64/files/Toolchains%20targetting%20Win32/Personal%20Builds/Experimental_Builds/4.9.0/threads-
win32/sjlj/) I see no problem to start with, but with a 64-bit
snapshot (http​://sourceforge.net/projects/mingw-
w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/Experimental_Builds/4.9.0/threads-
win32/seh/) I get failures in op/range.t and op/numconvert.t without
the patch.

Those failures are fixed by the patch.

Why is the -fwrapv left out from the MINIWIN32_OBJ compilation? My
test results suggest that we don't currently have a problem with those
files (win32.c, win32sck.c, win32thread.c, fcrypt.c and win32io.c) but
surely it wouldn't hurt to compile them with it anyway to protect
against any future trouble? (Likewise the x2p files, although they
hardly matter since they're moving out of core in due course anyway.)

Also, since the failure only happens on a 64-bit build, is it worth
only including -fwrapv when doing a 64-bit build? That way, users of
earlier versions of gcc that don't support -fwrapv aren't bothered by
this if they're only doing a 32-bit build anyway. (Or have I just been
lucky in not seeing any failure in a 32-bit build?)

The following also initializes GCCWRAPV better, so that it's only set to "define" by default for 4.3.0 or higer​:

GCCWRAPV *= $(shell for /f "delims=. tokens=1,2,3" %i in ('gcc -dumpversion') do @​if "%i"=="4" (if "%j" geq "3" echo define) else if "%i" geq "5" (echo define))

(I forgot​: that probably needs putting in a GCC-specific section to
avoid trouble for anyone using dmake with VC++. And the
cross-compiler's gcc isn't called gcc -- it's x86_64-w64-mingw32-gcc.)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 5, 2014

From @tonycoz

On Tue Apr 22 12​:37​:46 2014, shay wrote​:

On 22 April 2014 18​:12, Steve Hay via RT <perlbug-followup@​perl.org>
wrote​:

On Tue Apr 22 05​:10​:18 2014, shay wrote​:

On Mon Apr 21 16​:05​:34 2014, tonyc wrote​:

On Fri Apr 18 08​:25​:02 2014, shay wrote​:

I don't see any new attachment or smoke-me branch for
review/comment.
Am I missing something?

Oops, here it is.

Testing with a 32-bit snapshot
(http​://sourceforge.net/projects/mingw-
w64/files/Toolchains%20targetting%20Win32/Personal%20Builds/Experimental_Builds/4.9.0/threads-
win32/sjlj/) I see no problem to start with, but with a 64-bit
snapshot (http​://sourceforge.net/projects/mingw-
w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/Experimental_Builds/4.9.0/threads-
win32/seh/) I get failures in op/range.t and op/numconvert.t without
the patch.

Those failures are fixed by the patch.

Why is the -fwrapv left out from the MINIWIN32_OBJ compilation? My
test results suggest that we don't currently have a problem with
those
files (win32.c, win32sck.c, win32thread.c, fcrypt.c and win32io.c)
but
surely it wouldn't hurt to compile them with it anyway to protect
against any future trouble? (Likewise the x2p files, although they
hardly matter since they're moving out of core in due course
anyway.)

Also, since the failure only happens on a 64-bit build, is it worth
only including -fwrapv when doing a 64-bit build? That way, users of
earlier versions of gcc that don't support -fwrapv aren't bothered
by
this if they're only doing a 32-bit build anyway. (Or have I just
been
lucky in not seeing any failure in a 32-bit build?)

The following also initializes GCCWRAPV better, so that it's only set
to "define" by default for 4.3.0 or higer​:

GCCWRAPV *= $(shell for /f "delims=. tokens=1,2,3" %i in ('gcc
-dumpversion') do @​if "%i"=="4" (if "%j" geq "3" echo define) else if
"%i" geq "5" (echo define))

(I forgot​: that probably needs putting in a GCC-specific section to
avoid trouble for anyone using dmake with VC++. And the
cross-compiler's gcc isn't called gcc -- it's x86_64-w64-mingw32-gcc.)

Oops, this ticket slipped my (alleged) mind.

I've attached an updated patch​:

- uses the shell macro you provided to set a default GCCWRAPV, replacing gcc with $(CC) *after* CC has been set based on the GCCCROSS macro

- compiles MINIWIN32_OBJ with $(MINIBUILDOPT)

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 5, 2014

From @tonycoz

0001-perl-121505-include-fwrapv-by-default-for-GCC-4.3-an.patch
From e0c717880fdaf205d43f66cd54ba672119129d22 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 5 May 2014 10:22:25 +1000
Subject: [perl #121505] include -fwrapv by default for GCC 4.3 and later

---
 win32/makefile.mk |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/win32/makefile.mk b/win32/makefile.mk
index 99193a2..e005ee5 100644
--- a/win32/makefile.mk
+++ b/win32/makefile.mk
@@ -141,6 +141,12 @@ USE_LARGE_FILES	*= define
 CCTYPE		*= GCC
 
 #
+# If you are using GCC, 4.3 or later by default we add the -fwrapv option.
+# See https://rt.perl.org/Ticket/Display.html?id=121505
+#
+#GCCWRAPV       *= define
+
+#
 # If you are using Intel C++ Compiler uncomment this
 #
 #__ICC		*= define
@@ -408,6 +414,8 @@ INST_HTML	= $(INST_TOP)$(INST_VER)\html
 
 .USESHELL :
 
+MINIBUILDOPT    *=
+
 .IF "$(CCTYPE)" == "GCC"
 
 .IF "$(GCCCROSS)" == "define"
@@ -420,6 +428,13 @@ LIB32		= $(ARCHPREFIX)ar rc
 IMPLIB		= $(ARCHPREFIX)dlltool
 RSC		= $(ARCHPREFIX)windres
 
+GCCWRAPV *= $(shell for /f "delims=. tokens=1,2,3" %i in ('$(CC) -dumpversion') do @if "%i"=="4" (if "%j" geq "3" echo define) else if "%i" geq "5" (echo define))
+
+.IF "$(GCCWRAPV)" == "define"
+BUILDOPT        += -fwrapv
+MINIBUILDOPT    += -fwrapv
+.ENDIF
+
 i = .i
 o = .o
 a = .a
@@ -1139,10 +1154,10 @@ $(MINIDIR) :
 	if not exist "$(MINIDIR)" mkdir "$(MINIDIR)"
 
 $(MINICORE_OBJ) : $(CORE_NOCFG_H)
-	$(CC) -c $(CFLAGS) -DPERL_EXTERNAL_GLOB -DPERL_IS_MINIPERL $(OBJOUT_FLAG)$@ ..\$(*B).c
+	$(CC) -c $(CFLAGS) $(MINIBUILDOPT) -DPERL_EXTERNAL_GLOB -DPERL_IS_MINIPERL $(OBJOUT_FLAG)$@ ..\$(*B).c
 
 $(MINIWIN32_OBJ) : $(CORE_NOCFG_H)
-	$(CC) -c $(CFLAGS) -DPERL_IS_MINIPERL $(OBJOUT_FLAG)$@ $(*B).c
+	$(CC) -c $(CFLAGS) $(MINIBUILDOPT) -DPERL_IS_MINIPERL $(OBJOUT_FLAG)$@ $(*B).c
 
 # -DPERL_IMPLICIT_SYS needs C++ for perllib.c
 # rules wrapped in .IFs break Win9X build (we end up with unbalanced []s unless
-- 
1.7.4.msysgit.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 6, 2014

From @steve-m-hay

On 5 May 2014 01​:27, Tony Cook via RT <perlbug-followup@​perl.org> wrote​:

On Tue Apr 22 12​:37​:46 2014, shay wrote​:

On 22 April 2014 18​:12, Steve Hay via RT <perlbug-followup@​perl.org>
wrote​:

On Tue Apr 22 05​:10​:18 2014, shay wrote​:

On Mon Apr 21 16​:05​:34 2014, tonyc wrote​:

On Fri Apr 18 08​:25​:02 2014, shay wrote​:

I don't see any new attachment or smoke-me branch for
review/comment.
Am I missing something?

Oops, here it is.

Testing with a 32-bit snapshot
(http​://sourceforge.net/projects/mingw-
w64/files/Toolchains%20targetting%20Win32/Personal%20Builds/Experimental_Builds/4.9.0/threads-
win32/sjlj/) I see no problem to start with, but with a 64-bit
snapshot (http​://sourceforge.net/projects/mingw-
w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/Experimental_Builds/4.9.0/threads-
win32/seh/) I get failures in op/range.t and op/numconvert.t without
the patch.

Those failures are fixed by the patch.

Why is the -fwrapv left out from the MINIWIN32_OBJ compilation? My
test results suggest that we don't currently have a problem with
those
files (win32.c, win32sck.c, win32thread.c, fcrypt.c and win32io.c)
but
surely it wouldn't hurt to compile them with it anyway to protect
against any future trouble? (Likewise the x2p files, although they
hardly matter since they're moving out of core in due course
anyway.)

Also, since the failure only happens on a 64-bit build, is it worth
only including -fwrapv when doing a 64-bit build? That way, users of
earlier versions of gcc that don't support -fwrapv aren't bothered
by
this if they're only doing a 32-bit build anyway. (Or have I just
been
lucky in not seeing any failure in a 32-bit build?)

The following also initializes GCCWRAPV better, so that it's only set
to "define" by default for 4.3.0 or higer​:

GCCWRAPV *= $(shell for /f "delims=. tokens=1,2,3" %i in ('gcc
-dumpversion') do @​if "%i"=="4" (if "%j" geq "3" echo define) else if
"%i" geq "5" (echo define))

(I forgot​: that probably needs putting in a GCC-specific section to
avoid trouble for anyone using dmake with VC++. And the
cross-compiler's gcc isn't called gcc -- it's x86_64-w64-mingw32-gcc.)

Oops, this ticket slipped my (alleged) mind.

I've attached an updated patch​:

- uses the shell macro you provided to set a default GCCWRAPV, replacing gcc with $(CC) *after* CC has been set based on the GCCCROSS macro

- compiles MINIWIN32_OBJ with $(MINIBUILDOPT)

I'm not sure I'd have left the commented-out GCCWRAPV option at the
top of the file. Is it really something users would want to fiddle
with? Other gcc-specific options are added to BUILDOPT later on with
no mention in the "Build configuration" part of the file
(-fno-strict-aliasing -mms-bitfields), so I'd be inclined to just put
GCCWRAPV there.

Would it also make sense to only do this ".IF "$(WIN64)" == "define""?

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 8, 2014

From @tonycoz

On Tue May 06 10​:14​:50 2014, shay wrote​:

I'm not sure I'd have left the commented-out GCCWRAPV option at the
top of the file. Is it really something users would want to fiddle
with? Other gcc-specific options are added to BUILDOPT later on with
no mention in the "Build configuration" part of the file
(-fno-strict-aliasing -mms-bitfields), so I'd be inclined to just put
GCCWRAPV there.

A developer might want to disable it to track down the problems -fwrapv hides.

In the Configure change we look for the -fno-wrapv option and disable -fwrapv if that (or -fsanitize=undefined) is present, so you can investigate undefined value issues that -fwrapv hides with​:

  ./Configure -Accflags=-fsanitize=undefined ...

Including the commented out entry documents it as something the developer can change, or supply on the command-line, like​:

  dmake GCCWRAPV=undef BUILDOPTEXTRA=-fsanitize=undefined

Would it also make sense to only do this ".IF "$(WIN64)" == "define""?

No, these optimizations aren't limited to 64-bit builds.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 9, 2014

From @steve-m-hay

On 8 May 2014 05​:42, Tony Cook via RT <perlbug-followup@​perl.org> wrote​:

On Tue May 06 10​:14​:50 2014, shay wrote​:

I'm not sure I'd have left the commented-out GCCWRAPV option at the
top of the file. Is it really something users would want to fiddle
with? Other gcc-specific options are added to BUILDOPT later on with
no mention in the "Build configuration" part of the file
(-fno-strict-aliasing -mms-bitfields), so I'd be inclined to just put
GCCWRAPV there.

A developer might want to disable it to track down the problems -fwrapv hides.

In the Configure change we look for the -fno-wrapv option and disable -fwrapv if that (or -fsanitize=undefined) is present, so you can investigate undefined value issues that -fwrapv hides with​:

./Configure -Accflags=-fsanitize=undefined ...

Including the commented out entry documents it as something the developer can change, or supply on the command-line, like​:

dmake GCCWRAPV=undef BUILDOPTEXTRA=-fsanitize=undefined

Would it also make sense to only do this ".IF "$(WIN64)" == "define""?

No, these optimizations aren't limited to 64-bit builds.

Ok, fair enough. This patch looks fine to me then.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 12, 2014

From @tonycoz

On Fri May 09 06​:38​:26 2014, shay wrote​:

Ok, fair enough. This patch looks fine to me then.

Applied to blead as c8180b0.

I'll remove this ticket as a blocker rather than resolving it, since we need to go back and look for all the UB and fix them.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 6, 2015

From @jkeenan

On Sun May 11 17​:07​:01 2014, tonyc wrote​:

On Fri May 09 06​:38​:26 2014, shay wrote​:

Ok, fair enough. This patch looks fine to me then.

Applied to blead as c8180b0.

I'll remove this ticket as a blocker rather than resolving it, since
we need to go back and look for all the UB and fix them.

Tony

This ticket has been referenced by another ticket (126820), so this seems to be an appropriate point to ask​: Have we addressed the longer-term issues mentioned above?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 28, 2016

From @iabyn

On Sat, Dec 05, 2015 at 04​:27​:11PM -0800, James E Keenan via RT wrote​:

I'll remove this ticket as a blocker rather than resolving it, since
we need to go back and look for all the UB and fix them.

Tony

This ticket has been referenced by another ticket (126820), so this
seems to be an appropriate point to ask​: Have we addressed the
longer-term issues mentioned above?

No, so it needs to be kept open for now.

--
Art is anything that has a label (especially if the label is "untitled 1")

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 1, 2018

From @khwilliamson

On Mon, 28 Mar 2016 07​:49​:49 -0700, davem wrote​:

On Sat, Dec 05, 2015 at 04​:27​:11PM -0800, James E Keenan via RT wrote​:

I'll remove this ticket as a blocker rather than resolving it, since
we need to go back and look for all the UB and fix them.

Tony

This ticket has been referenced by another ticket (126820), so this
seems to be an appropriate point to ask​: Have we addressed the
longer-term issues mentioned above?

No, so it needs to be kept open for now.

This appears to have been dropped. Shouldn't we revisit this in early 5.29?
--
Karl Williamson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.