GCC Bugzilla Bug 35634

/* The next program is aborted with avr-gcc 4.1.2, 4.2.3, 4.3.0:
      result of char promotion comes out of CHAR_MIN/MAX.
   Options: -W -Wall -Os
   Know to work:
      3.3.6, 3.4.6 - good code
      4.0.4 - correct, but not the best
      4.1.2, 4.2.3, 4.3.0 - without optimization only
 */

#include <limits.h>

void abort (void);
void exit (int);

void foo (int i)
{
    static int n;
    if (i < CHAR_MIN || i > CHAR_MAX)
        abort ();
    if (++n > 1000)
        exit (0);
}

int main ()
{
    char c;
    for (c = 0; ; c++) foo (c);
}

Subject: Re:   New: [avr] result of char promotion comes out of CHAR_MIN/MAX

This code is only defined if char is unsigned which it is not on avr.  
(It is unsigned on some targets like powerpc-Linux-gnu.

Sent from my iPhone

On Mar 18, 2008, at 22:42, "dmixm at marine dot febras dot ru"
<gcc-bugzilla@gcc.gnu.org 
 > wrote:

> /* The next program is aborted with avr-gcc 4.1.2, 4.2.3, 4.3.0:
>      result of char promotion comes out of CHAR_MIN/MAX.
>   Options: -W -Wall -Os
>   Know to work:
>      3.3.6, 3.4.6 - good code
>      4.0.4 - correct, but not the best
>      4.1.2, 4.2.3, 4.3.0 - without optimization only
> */
>
> #include <limits.h>
>
> void abort (void);
> void exit (int);
>
> void foo (int i)
> {
>    static int n;
>    if (i < CHAR_MIN || i > CHAR_MAX)
>        abort ();
>    if (++n > 1000)
>        exit (0);
> }
>
> int main ()
> {
>    char c;
>    for (c = 0; ; c++) foo (c);
> }
>
>
> -- 
>           Summary: [avr] result of char promotion comes out of  
> CHAR_MIN/MAX
>           Product: gcc
>           Version: 4.3.0
>            Status: UNCONFIRMED
>          Severity: normal
>          Priority: P3
>         Component: target
>        AssignedTo: unassigned at gcc dot gnu dot org
>        ReportedBy: dmixm at marine dot febras dot ru
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35634
>

As I already mentioned this is undefined code, overflow for signed integeral
types is undefined.  char is a weird type as it is considered one of the
character types but it is still an integeral type.  Also it is weird that it
defualt to either signed or unsigned (implementation defined, in GCC's case it
is ABI defined).  For AVR, it defaults to signed.

Actually, there is no undefined behaviour here, as long as CHAR_MAX < INT_MAX
no overflow occurs.  c++ is the same as c = (int)c + 1 (when ignoring the
result), and the conversion from int to char is implementation defined.  For
gcc the result of such a conversion is always in the range of the target type.

This is a bug in the C frontend which does the increment on type char, not
on the promoted type (I noticed that while fixing bitfield issues as well),
code in question is in build_unary_op() and this way since forever.

Original dump as from the FE:

;; Function main (main)
;; enabled by -tree-original

{ 
  char c;

    char c;
  c = 0;
  <D.1559>:; 
  foo ((int) c);
  c++ ;
  goto <D.1559>;
}

I tried to fix this once but failed.  Joseph - can you give this a shot?
The FE should for all pre-/postincrements just emit the proper
{( int res = x; x = (typeof x)((int)x + 1); res; )}
with using TARGET_EXPR/COMPOUND_EXPRs as required.

Note the C++ frontend has the same problem here, so transition that bug
there once the C FE is fixed.

Thanks.

To quote the standard (6.5.4.1/2):

"The expression ++E is equivalent to (E+=1).  See the discussions of additive
operators and compound assignment for information on constraints, types,
side effects, and CONVERSIONS and the effects of operations on pointers"

emphasise mine, 6.5.6/4 then of course says

"If both operands have arithmetic type, the usual arithmetic conversions are
performed on them."

Created an attachment (id=15349) [edit]
Gimplification-time patch

Changing at build_unary_op time runs into OpenMP problems - the OpenMP code
needs the trees to correspond more directly to the increments and decrements in
the source code.

Changing at gimplification time, as in the attached patch, avoids that problem,
but a number of gcc.dg/vect tests regress because of the changes to the code
for increment/decrement of types that get promoted.

FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/pr30771.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/vect-multitypes-11.c scan-tree-dump-times vect "vectorized 1
loops" 2
FAIL: gcc.dg/vect/vect-reduc-dot-u16a.c scan-tree-dump-times vect "vectorized 1
loops" 2
FAIL: gcc.dg/vect/slp-21.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/no-scevccp-outer-13.c scan-tree-dump-times vect "OUTER LOOP
VECTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-14.c scan-tree-dump-times vect "OUTER LOOP
VECTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-16.c scan-tree-dump-times vect "OUTER LOOP
VECTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-17.c scan-tree-dump-times vect "OUTER LOOP
VECTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-19.c scan-tree-dump-times vect "OUTER LOOP
VECTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-21.c scan-tree-dump-times vect "OUTER LOOP
VECTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-7.c scan-tree-dump-times vect "OUTER LOOP
VECTORIZED." 1

Thanks.  I guess the vect fallout needs to be dealt with separately.  Now, I
think gimplification time is not the best here, can we maybe move this to
general gimplification code if we change the {PRE,POST}{IN,DE}CREMENT_EXPR
to have the type the increment is done in (and the expression result) be
TREE_TYPE of that expression?  This way the generic gimplification code
would need to make sure to lower it properly.

Diego, I suppose this lowering is before tuples come into play and we loose
this extra type, right?

Of course this may need auditing of the FEs wrt correctness of the type
in this expression but feels like a more general fix?

"Now, I think gimplification time is not the best here"

Now, if we think ... is the best here

obviously ;)

I did a quick scan and Ada, C++ and C ever build these operations.  Also a few
backends do (mips, rs6000 and s390).  So IMHO changing the semantics of
these to

/* Nodes for ++ and -- in C.
   The second arg is how much to increment or decrement by.
   For a pointer, it would be the size of the object pointed to.
   The type of the expression specifies the type the increment
   is performed on and the type of the result.  This type does not
   need to match the type of the first argument, instead that is
   properly size-/zero-extended before the arithmetic operation.  */
DEFTREECODE (PREDECREMENT_EXPR, "predecrement_expr", tcc_expression, 2)
DEFTREECODE (PREINCREMENT_EXPR, "preincrement_expr", tcc_expression, 2)
DEFTREECODE (POSTDECREMENT_EXPR, "postdecrement_expr", tcc_expression, 2)
DEFTREECODE (POSTINCREMENT_EXPR, "postincrement_expr", tcc_expression, 2)

is reasonable.  Note that expansion no longer handles these tree codes,
they are expected to only survive until gimplification.

Joseph, do you have that build_unary_op patch still around?
If that patch didn't cause any regressions but OpenMP, I could look at tweaking
OpenMP...

Created an attachment (id=15382) [edit]
build_unary_op patch

There may well be other regressions with this patch (in particular the vector
ones may appear with this patch as well); I stopped testing when the OpenMP
failures appeared.

Created an attachment (id=15455) [edit]
gcc43-pr35634.patch

Here is the updated FE only patch.  One change is that it avoids
P{RE,OST}{IN,DE}CREMENT_EXPR only for the promoting types, and has some
(admittedly very ugly) OpenMP parsing changes to counter that.  Unfortunately
unlike #pragma omp for increment, #pragma omp atomic can have some_lvalue++
, not necessarily a variable_name++, so I have no idea how to handle that.

On x86_64 with this patch I get 3 omp failures (2 in libgomp atomic-10.c, one
in gcc/testsuite atomic-1.c) and:
FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/pr30771.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/vect-multitypes-11.c scan-tree-dump-times vect "vectorized 1
loops" 2
so (depending on where the other patch was tested) doing this in the FE doesn't
help much or at all.

Subject: Re:  [4.1/4.2/4.3/4.4 Regression] operand of
 pre-/postin-/decrement not promoted

On Wed, 9 Apr 2008, jakub at gcc dot gnu dot org wrote:

> ------- Comment #12 from jakub at gcc dot gnu dot org  2008-04-09 14:32 -------
> Created an attachment (id=15455) [edit]
>  --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=15455&action=view)
> gcc43-pr35634.patch
> 
> Here is the updated FE only patch.  One change is that it avoids
> P{RE,OST}{IN,DE}CREMENT_EXPR only for the promoting types, and has some
> (admittedly very ugly) OpenMP parsing changes to counter that.  Unfortunately
> unlike #pragma omp for increment, #pragma omp atomic can have some_lvalue++
> , not necessarily a variable_name++, so I have no idea how to handle that.
> 
> On x86_64 with this patch I get 3 omp failures (2 in libgomp atomic-10.c, one
> in gcc/testsuite atomic-1.c) and:
> FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1
> FAIL: gcc.dg/vect/pr30771.c scan-tree-dump-times vect "vectorized 1 loops" 1
> FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1
> FAIL: gcc.dg/vect/vect-multitypes-11.c scan-tree-dump-times vect "vectorized 1
> loops" 2
> so (depending on where the other patch was tested) doing this in the FE doesn't
> help much or at all.

Thanks!

I will try doing the P{RE,OST}{IN,DE}CREMENT_EXPR semantic change and
handling it in the gimplifier.  Just because I am curious how much
I break the frontends...

After the summit paper deadline is over ;)

Richard.

Created an attachment (id=15491) [edit]
gimple semantics change patch

This is the variant I thought about with changing the way types are interpreted
for the *CREMENT_EXPRs.  The usual problem with vectorizer tests appear as SCEV
doesn't handle for example

<bb 3>:
  # i_14 = PHI <i_7(5), 0(2)>
  D.1560_4 = (int) i_14;
  a[D.1560_4] = D.1560_4;
  D.1561_6 = D.1560_4 + 1;
  i_7 = (short int) D.1561_6;
  if (i_7 <= 63)
    goto <bb 5>;
  else
    goto <bb 4>;

but for correctness reasons we cannot do the increment in signed short int
due to the undefined overflow issue.  We can avoid the promotion if the
result is truncated to an unsigned type (but this is an optimization that
I didn't want to put into this patch addressing correctness only).

I will re-test this patch, a slightly oder version tested ok apart from
the vectorizer fallout.

With the patch in comment #14 we have

                === g++ tests ===


Running target unix
FAIL: g++.dg/init/bitfield1.C (test for excess errors)
FAIL: g++.dg/gomp/atomic-1.C (test for excess errors)
FAIL: g++.dg/torture/pr33887-1.C  -O0  execution test
FAIL: g++.dg/torture/pr33887-1.C  -O1  execution test
FAIL: g++.dg/torture/pr33887-1.C  -O2  execution test
FAIL: g++.dg/torture/pr33887-1.C  -O3 -fomit-frame-pointer  execution test
FAIL: g++.dg/torture/pr33887-1.C  -O3 -g  execution test
FAIL: g++.dg/torture/pr33887-1.C  -Os  execution test

                === gcc tests ===

FAIL: gcc.dg/gomp/atomic-1.c (test for excess errors)
FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/pr30771.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/vect-multitypes-11.c scan-tree-dump-times vect "vectorized 1
l
oops" 2
FAIL: gcc.dg/vect/vect-reduc-dot-u16a.c scan-tree-dump-times vect "vectorized 1 
loops" 2
FAIL: gcc.dg/vect/slp-21.c scan-tree-dump-times vect "vectorized 1 loops" 1
FAIL: gcc.dg/vect/no-scevccp-outer-13.c scan-tree-dump-times vect "OUTER LOOP
VE
CTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-14.c scan-tree-dump-times vect "OUTER LOOP
VE
CTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-16.c scan-tree-dump-times vect "OUTER LOOP
VE
CTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-17.c scan-tree-dump-times vect "OUTER LOOP
VE
CTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-19.c scan-tree-dump-times vect "OUTER LOOP
VE
CTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-21.c scan-tree-dump-times vect "OUTER LOOP
VE
CTORIZED." 1
FAIL: gcc.dg/vect/no-scevccp-outer-7.c scan-tree-dump-times vect "OUTER LOOP
VEC
TORIZED." 1

                === libgomp tests ===


Running target unix
FAIL: libgomp.c/atomic-10.c (test for excess errors)
WARNING: libgomp.c/atomic-10.c compilation failed to produce executable

Downgrading to P2, the patches so far all seem to be quite risky for the
branches, the wrong-code is on a corner case and isn't a recent regression.

Regarding the comment #14 patch, I'd say the complete_type should be different
from argtype only when !TYPE_UNSIGNED (argtype), for unsigned char or unsigned
short the overflow behavior is well defined.

Closing 4.1 branch.

*** Bug 38929 has been marked as a duplicate of this bug. ***

I am going to make this a P1 for 4.5, but it's too late for 4.4.

How much of the fallout (especially the scev-related failures) goes away with
-funsafe-loop-optimizations?  I'm thinking that it is unavoidable. :-(

Closing 4.2 branch.

On no-undefined-overflow branch the FE can do the increment/decrement on the
target type safely (well, there are no NV variants of the
{PRE,POST}{IN,DEC}REMENT
expressions on the branch, so they at the moment all get lowered to
possibly wrapping variants during gimplification).

Unfortunately that branch is way from "ready".

Subject: Re:  [4.3/4.4/4.5 Regression] operand of pre-/postin-/decrement
 not promoted

On Sat, 11 Apr 2009, rguenth at gcc dot gnu dot org wrote:

> On no-undefined-overflow branch the FE can do the increment/decrement on the
> target type safely (well, there are no NV variants of the
> {PRE,POST}{IN,DEC}REMENT
> expressions on the branch, so they at the moment all get lowered to
> possibly wrapping variants during gimplification).

Of course increment/decrement of signed integer types at least as wide as 
int should get lowered to the no-overflow variants unless -fwrapv; 
likewise increment/decrement of pointer types.  Whether through a 
gimplification-time hook or through creating NV variants of 
increment/decrement and having the front end create those when 
appropriate.

Subject: Re:  [4.3/4.4/4.5 Regression] operand of pre-/postin-/decrement
 not promoted

On Sat, 11 Apr 2009, joseph at codesourcery dot com wrote:

> ------- Comment #23 from joseph at codesourcery dot com  2009-04-11 16:30 -------
> Subject: Re:  [4.3/4.4/4.5 Regression] operand of pre-/postin-/decrement
>  not promoted
> 
> On Sat, 11 Apr 2009, rguenth at gcc dot gnu dot org wrote:
> 
> > On no-undefined-overflow branch the FE can do the increment/decrement on the
> > target type safely (well, there are no NV variants of the
> > {PRE,POST}{IN,DEC}REMENT
> > expressions on the branch, so they at the moment all get lowered to
> > possibly wrapping variants during gimplification).
> 
> Of course increment/decrement of signed integer types at least as wide as 
> int should get lowered to the no-overflow variants unless -fwrapv; 
> likewise increment/decrement of pointer types.  Whether through a 
> gimplification-time hook or through creating NV variants of 
> increment/decrement and having the front end create those when 
> appropriate.

Indeed.  As they are not valid gimple but only in generic I lean
to a gimplification-time solution here.

Richard.

*** Bug 39736 has been marked as a duplicate of this bug. ***

GCC 4.3.4 is being released, adjusting target milestone.