Re: [PATCH] Remove -fstrict-overflow, default to undefined signed integer and pointer overflow

[Christophe Lyon]

On 27 April 2017 at 17:32, Jeff Law  wrote:
> On 04/26/2017 05:31 AM, Richard Biener wrote:
>>
>>
>> The following removes the third state we had apart from signed integer
>> overflow wrapping and being undefined.  It makes signed integer overflow
>> undefined, consistently at all optimization levels.  -fno-strict-overflow
>> stays as a backward compatible way to avoid optimizations that rely on
>> signed integer overflow being undefined by making it wrapping
>> (this is also the reason of using !flag_wrapv in
>> POINTER_TYPE_OVERFLOW_UNDEFINED rather than a new option, for now).
>>
>> Surprisingly there's no UBSAN integer overflow testsuite fallout,
>> foldings that happen before instrumentation (which is done after
>> into-SSA) and rely on signed integer overflow being undefined will
>> cause false negatives.  If that turns out to be a problem the
>> flag_strict_overflow flag can be re-introduced (not that this would
>> be my preference) and it can be unset after UBSAN instrumentation
>> is finished.
>>
>> The main motivation for aliasing -fstrict-overflow to -f[no-]wrapv
>> is that with -fno-strict-overflow (and thus -O1 at the moment) you get
>> the worst of both worlds, you can't optimize based on the undefinedness
>> but you also cannot rely on wrapping behavior (to know that
>> re-association will not introduce undefined behavior).  Using -fwrapv
>> for -fno-strict-overflow makes it clear what the semantics are.
>>
>> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>>
>> I opened PR80525 for the appearant mishandling of (a + 1) && (a + 1)
>> with -Wlogical-op when overflow is undefined.
>>
>> If there are no further comments I plan to install this after 7.1
>> is released.  I consider the Ada FE change obvious.
>>
>> The next step is to get rid of all that ugly -Wstrict-overflow code
>> in VRP.  strict-overflow warnings from folding were already
>> detoriating with moving stuff to match.pd where it isn't easy to
>> preserve those.  Ripping those out can be done later, it's not
>> blocking other stuff, and eventually somebody picks up -Wstrict-overflow
>> to warn for some cases from the FEs.
>>
>> changes.html/porting_to.html will need to have instructions how to
>> use ubsan to get at the real problems in code.
>
> This all sounds good to me.
>
> jeff

Hi,

This patch (r247495) causes regressions in fortran on aarch64/arm:
  - PASS now FAIL [PASS => FAIL]:

  Executed from: gfortran.dg/dg.exp
gfortran.dg/coarray_lock_7.f90   -O   scan-tree-dump-times
original "_gfortran_caf_lock \\(caf_token.., \\(3 -
\\(integer\\(kind=4\\)\\) parm...dim\\[0\\].lbound\\) \\+
\\(integer\\(kind=4\\)\\) MAX_EXPR <\\(parm...dim\\[0\\].ubound -
parm...dim\\[0\\].lbound\\) \\+ 1, 0> \\* \\(3 -
\\(integer\\(kind=4\\)\\) parm...dim\\[1\\].lbound\\), 0, 0B, , 0B,
0\\);|_gfortran_caf_lock \\(caf_token.1, \\(3 -
parm...dim\\[0\\].lbound\\) \\+ MAX_EXPR <\\(parm...dim\\[0\\].ubound
- parm...dim\\[0\\].lbound\\) \\+ 1, 0> \\* \\(3 -
parm...dim\\[1\\].lbound\\), 0, 0B, , 0B, 0\\);" 1
gfortran.dg/coarray_lock_7.f90   -O   scan-tree-dump-times
original "_gfortran_caf_unlock \\(caf_token.., \\(2 -
\\(integer\\(kind=4\\)\\) parm...dim\\[0\\].lbound\\) \\+
\\(integer\\(kind=4\\)\\) MAX_EXPR <\\(parm...dim\\[0\\].ubound -
parm...dim\\[0\\].lbound\\) \\+ 1, 0> \\* \\(3 -
\\(integer\\(kind=4\\)\\) parm...dim\\[1\\].lbound\\), 0, , 0B,
0\\);|_gfortran_caf_unlock \\(caf_token.., \\(2 -
parm...dim\\[0\\].lbound\\) \\+ MAX_EXPR <\\(parm...dim\\[0\\].ubound
- parm...dim\\[0\\].lbound\\) \\+ 1, 0> \\* \\(3 -
parm...dim\\[1\\].lbound\\), 0, , 0B, 0\\);" 1

Thanks,

Christophe

Re: [PATCH] Remove -fstrict-overflow, default to undefined signed integer and pointer overflow

[Jeff Law]

On 04/26/2017 05:31 AM, Richard Biener wrote:


The following removes the third state we had apart from signed integer
overflow wrapping and being undefined.  It makes signed integer overflow
undefined, consistently at all optimization levels.  -fno-strict-overflow
stays as a backward compatible way to avoid optimizations that rely on
signed integer overflow being undefined by making it wrapping
(this is also the reason of using !flag_wrapv in
POINTER_TYPE_OVERFLOW_UNDEFINED rather than a new option, for now).

Surprisingly there's no UBSAN integer overflow testsuite fallout,
foldings that happen before instrumentation (which is done after
into-SSA) and rely on signed integer overflow being undefined will
cause false negatives.  If that turns out to be a problem the
flag_strict_overflow flag can be re-introduced (not that this would
be my preference) and it can be unset after UBSAN instrumentation
is finished.

The main motivation for aliasing -fstrict-overflow to -f[no-]wrapv
is that with -fno-strict-overflow (and thus -O1 at the moment) you get
the worst of both worlds, you can't optimize based on the undefinedness
but you also cannot rely on wrapping behavior (to know that
re-association will not introduce undefined behavior).  Using -fwrapv
for -fno-strict-overflow makes it clear what the semantics are.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

I opened PR80525 for the appearant mishandling of (a + 1) && (a + 1)
with -Wlogical-op when overflow is undefined.

If there are no further comments I plan to install this after 7.1
is released.  I consider the Ada FE change obvious.

The next step is to get rid of all that ugly -Wstrict-overflow code
in VRP.  strict-overflow warnings from folding were already
detoriating with moving stuff to match.pd where it isn't easy to
preserve those.  Ripping those out can be done later, it's not
blocking other stuff, and eventually somebody picks up -Wstrict-overflow
to warn for some cases from the FEs.

changes.html/porting_to.html will need to have instructions how to
use ubsan to get at the real problems in code.

This all sounds good to me.

jeff

Re: [PATCH] Remove -fstrict-overflow, default to undefined signed integer and pointer overflow

[Eric Botcazou]

> If there are no further comments I plan to install this after 7.1
> is released.  I consider the Ada FE change obvious.

For the record: yes, it is, since the whole patch is a no-op for Ada.

-- 
Eric Botcazou

[PATCH] Remove -fstrict-overflow, default to undefined signed integer and pointer overflow

[Richard Biener]

The following removes the third state we had apart from signed integer
overflow wrapping and being undefined.  It makes signed integer overflow
undefined, consistently at all optimization levels.  -fno-strict-overflow
stays as a backward compatible way to avoid optimizations that rely on
signed integer overflow being undefined by making it wrapping
(this is also the reason of using !flag_wrapv in 
POINTER_TYPE_OVERFLOW_UNDEFINED rather than a new option, for now).

Surprisingly there's no UBSAN integer overflow testsuite fallout,
foldings that happen before instrumentation (which is done after
into-SSA) and rely on signed integer overflow being undefined will
cause false negatives.  If that turns out to be a problem the
flag_strict_overflow flag can be re-introduced (not that this would
be my preference) and it can be unset after UBSAN instrumentation
is finished.

The main motivation for aliasing -fstrict-overflow to -f[no-]wrapv
is that with -fno-strict-overflow (and thus -O1 at the moment) you get
the worst of both worlds, you can't optimize based on the undefinedness
but you also cannot rely on wrapping behavior (to know that 
re-association will not introduce undefined behavior).  Using -fwrapv
for -fno-strict-overflow makes it clear what the semantics are.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

I opened PR80525 for the appearant mishandling of (a + 1) && (a + 1)
with -Wlogical-op when overflow is undefined.

If there are no further comments I plan to install this after 7.1
is released.  I consider the Ada FE change obvious.

The next step is to get rid of all that ugly -Wstrict-overflow code
in VRP.  strict-overflow warnings from folding were already
detoriating with moving stuff to match.pd where it isn't easy to
preserve those.  Ripping those out can be done later, it's not
blocking other stuff, and eventually somebody picks up -Wstrict-overflow
to warn for some cases from the FEs.

changes.html/porting_to.html will need to have instructions how to
use ubsan to get at the real problems in code.

Thanks,
Richard.

2017-04-26  Richard Biener  

* common.opt (fstrict-overflow): Alias negative to fwrapv.
* doc/invoke.texi (fstrict-overflow): Remove all traces of
-fstrict-overflow documentation.
* tree.h (TYPE_OVERFLOW_UNDEFINED): Do not test flag_strict_overflow.
(POINTER_TYPE_OVERFLOW_UNDEFINED): Test !flag_wrapv instead of
flag_strict_overflow.
* ipa-inline.c (can_inline_edge_p): Do not test flag_strict_overflow.
* lto-opts.c (lto_write_options): Do not stream it.
* lto-wrapper.c (merge_and_complain): Do not handle it.
* opts.c (default_options_table): Do not set -fstrict-overflow.
(finish_options): Likewise do not clear it when sanitizing.
* simplify-rtx.c (simplify_const_relational_operation): Do not
test flag_strict_overflow.

ada/
* gcc-interface/misc.c (gnat_post_options): Do not set
-fstrict-overflow.

* c-c++-common/Wlogical-op-1.c: Add -fwrapv to restore previous
behavior.
* gcc.target/i386/pr46253.c: Make i unsigned to avoid warning.

Index: gcc/ada/gcc-interface/misc.c
===
--- gcc/ada/gcc-interface/misc.c(revision 247273)
+++ gcc/ada/gcc-interface/misc.c(working copy)
@@ -266,10 +266,6 @@ gnat_post_options (const char **pfilenam
   if (!global_options_set.x_flag_diagnostics_show_caret)
 global_dc->show_caret = false;
 
-  /* Set strict overflow by default for Ada.  */
-  if (!global_options_set.x_flag_strict_overflow)
-global_options.x_flag_strict_overflow = true;
-
   /* Warn only if STABS is not the default: we don't want to emit a warning if
  the user did not use a -gstabs option.  */
   if (PREFERRED_DEBUGGING_TYPE != DBX_DEBUG && write_symbols == DBX_DEBUG)
Index: gcc/common.opt
===
--- gcc/common.opt  (revision 247273)
+++ gcc/common.opt  (working copy)
@@ -2342,8 +2342,8 @@ Common Report Var(flag_strict_aliasing)
 Assume strict aliasing rules apply.
 
 fstrict-overflow
-Common Report Var(flag_strict_overflow) Optimization
-Treat signed overflow as undefined.
+Common NegativeAlias Alias(fwrapv)
+Treat signed overflow as undefined.  Negated as -fwrapv.
 
 fsync-libcalls
 Common Report Var(flag_sync_libcalls) Init(1)
Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 247273)
+++ gcc/doc/invoke.texi (working copy)
@@ -420,7 +420,7 @@ Objective-C and Objective-C++ Dialects}.
 -fsplit-paths @gol
 -fsplit-wide-types  -fssa-backprop  -fssa-phiopt @gol
 -fstdarg-opt  -fstore-merging  -fstrict-aliasing @gol
--fstrict-overflow  -fthread-jumps  -ftracer  -ftree-bit-ccp @gol
+-fthread-jumps  -ftracer  -ftree-bit-ccp @gol
 -ftree-builtin-call-dce  -ftree-ccp  -ftree-ch @gol