GCC Bugzilla Bug 33088

Since RTH's patch <http://gcc.gnu.org/ml/gcc-patches/2005-06/msg00860.html>,
stores to the real or imaginary part of a complex variable are represented
using a load of the other part and a COMPLEX_EXPR.

If the variable is not yet initialized, and the load of the other part does not
get optimized away, this can yield spurious floating-point exceptions (in
particular, loading a signaling NaN on the x87 raises the "invalid" exception).
 This can happen with -ffloat-store (I don't have any proof that this is
restricted to -ffloat-store, but haven't observed the problem without this
option).

I observed the problem originally as a failure of glibc's test-double, which is
built with -ffloat-store and forms complex values by setting the real and
imaginary parts separately.  The following testcase reproduces it on
i686-pc-linux-gnu, with trunk (-ffloat-store together with any of -O1 -O2 -O3
-Os), 4.2 branch (same options), 4.1 branch (-float-store -O1), but not for 4.0
(which predates the above mentioned patch) with any of those options.  An
equivalent x86-specific test could of course be written without use of
<fenv.h>.

#include <fenv.h>
#include <stdlib.h>

volatile int x[1024];

void __attribute__((noinline))
fill_stack (void)
{
  volatile int y[1024];
  int i;
  for (i = 0; i < 1024; i++)
    y[i] = 0x7ff00000;
  for (i = 0; i < 1024; i++)
    x[i] = y[i];
}

volatile _Complex double vc;

void __attribute__((noinline))
use_complex (_Complex double c)
{
  vc = c;
}

double t0, t1, t2, t3;

#define USE_COMPLEX(X, R, C) \
  do { __real__ X = R; __imag__ X = C; use_complex (X); } while (0)

void __attribute__((noinline))
use_stack (void)
{
  _Complex double a, b, c, d;
  USE_COMPLEX (a, t0, t1);
  USE_COMPLEX (b, t1, t2);
  USE_COMPLEX (c, t2, t3);
  USE_COMPLEX (d, t3, t0);
}

int
main (void)
{
  fill_stack ();
  feclearexcept (FE_INVALID);
  use_stack ();
  if (fetestexcept (FE_INVALID))
    abort ();
  exit (0);
}

For one, I don't think " __real__ X = R; __imag__ X = C; " is really nice
looking.  Now if the person did:
 *(double*) &X = R; ((double*) & X)[1] = C; 

it might be a different story but then again X is still defined piece wise so
you will still get NaN.

I really don't think this is a bug as we have an uninitialized variables here
in the same way doing:
short a;
a = b&0xff|a;
a = (b<<8)&0xFF00|a;

Would be defined code.

Subject: Re:  [4.1/4.2/4.3 Regression] spurious exceptions
 with -ffloat-store

On Fri, 17 Aug 2007, pinskia at gcc dot gnu dot org wrote:

> For one, I don't think " __real__ X = R; __imag__ X = C; " is really nice
> looking.  Now if the person did:
>  *(double*) &X = R; ((double*) & X)[1] = C; 
> 
> it might be a different story but then again X is still defined piece wise so
> you will still get NaN.

I think setting parts is cleaner than using casts like that, and safer 
(because of not involving arithmetic that GCC's liable to get wrong in 
cases involving -0) than R + C * I.

> I really don't think this is a bug as we have an uninitialized variables here
> in the same way doing:
> short a;
> a = b&0xff|a;
> a = (b<<8)&0xFF00|a;
> 
> Would be defined code.

I think setting the parts is valid GNU C (ISO C doesn't have the __real__ 
and __imag__ operators), and should be considered much like initializing a 
struct by parts, where it would definitely be incorrect to generate an 
exception from an uninitialized part.

I think Joseph's code is valid.  All he's doing is initializing various parts
of complex numbers and then using them.  If using a fully initialized value
results in a floating-point exception, that's a bug.

I cannot reproduce this problem with any of 4.1, 4.2 or 4.3.  But the issues
raised look related the CCP problem in PR34099.

/usr/src/gcc/obj35/gcc/xgcc -B /usr/src/gcc/obj35/gcc/ -v 2>&1 | tail -n 1;
/usr/src/gcc/obj35/gcc/xgcc -B /usr/src/gcc/obj35/gcc/ -m32 -O2 -ffloat-store
-o pr33088{,.c} -lm; ./pr33088; echo $?
gcc version 4.3.0 20071115 (experimental) (GCC) 
Aborted
134

And I doubt any CCP changes would have anything to do with this.  The problem
is that the hole complex number is one gimple register and if either __real__
and
__imag__ setting isn't close enough to each other, or if -ffloat-store is used,
then this means GCC reads the unitialized value from memory using floating
point load and if there is a signalling NaN on the stack, already the load
signals.
And fill_stack fills the stack with 0x7ff000007ff00000, which is IEEE double
signalling NaN.

At -O0 this testcase doesn't trigger the abort, because the moves are done
using integer registers rather than FPU.

Subject: Re:  [4.1/4.2/4.3 Regression] spurious exceptions
 with -ffloat-store

The failure still appears with a compiler from revision 130227, after the 
patch for PR 34099 was committed.

By Jakub.

Investigating.

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

This breaks glibc 2.7 regression tests.

We could probably get away with a kludge for -ffloat-store and optimization,
but currently the flag comes into play only very late (in TER) and I think
it's better to keep this.

So I think the approach to solving this is two-pronged:
- at -O0, do not promote the partial stores to total stores in the gimplifier,
- at -O1 or above, insert dummy initializations for "uninitialized" SSA names,
  this should be cleaned up at the RTL level (if -ffloat-store is not passed).


Andrew, IIRC you extended DECL_COMPLEX_GIMPLE_REG_P to DECL_GIMPLE_REG_P, can
vectors be affected by the same issue?

(In reply to comment #11)
> Andrew, IIRC you extended DECL_COMPLEX_GIMPLE_REG_P to DECL_GIMPLE_REG_P, can
> vectors be affected by the same issue?

No because vector types are not effected by -ffloat-store and there is no magic
in initializing vector piece wise as far as I know unless the vector type is
memory (as only BIT_FIELD_REF is optimized for the first element so far).

Thanks,
Andrew Pinski

Subject: Bug 33088

Author: ebotcazou
Date: Thu Dec 13 21:49:09 2007
New Revision: 130917

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=130917
Log:
        PR middle-end/33088
        * gimplify.c (gimplify_modify_expr_complex_part): Add note to comment.
        * tree-complex.c (init_dont_simulate_again): Return true if there are
        uninitialized loads generated by gimplify_modify_expr_complex_part.
        * tree-gimple.c (is_gimple_reg_type): Return false for complex types
        if not optimizing.
        * tree-ssa.c (ssa_undefined_value_p): New predicate extracted from...
        (warn_uninit): ...here.  Use ssa_undefined_value_p.
        * tree-ssa-pre.c (is_undefined_value): Delete.
        (phi_translate_1): Use ssa_undefined_value_p.
        (add_to_exp_gen): Likewise.
        (make_values_for_stmt): Likewise.
        * tree-flow.h (ssa_undefined_value_p): Declare.


Added:
    trunk/gcc/testsuite/gcc.dg/complex-5.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/gimplify.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/uninit-13.c
    trunk/gcc/tree-complex.c
    trunk/gcc/tree-flow.h
    trunk/gcc/tree-gimple.c
    trunk/gcc/tree-ssa-pre.c
    trunk/gcc/tree-ssa.c

Fixed on the mainline.  Not sure what to do for the branches, the fix is not
totally trivial.

I think this patch causes invalid gimple to show up at -O0 .  With my patches
to detect invalid gimple shows that there are a lot of failures with complex
and -O0.  Did the definition of what is valid gimple for complex at -O0 change?

Subject: Re:  [4.1/4.2 regression] spurious exceptions
 with -ffloat-store

On Thu, 13 Mar 2008, pinskia at gcc dot gnu dot org wrote:

> ------- Comment #15 from pinskia at gcc dot gnu dot org  2008-03-13 17:00 -------
> I think this patch causes invalid gimple to show up at -O0 .  With my patches
> to detect invalid gimple shows that there are a lot of failures with complex
> and -O0.  Did the definition of what is valid gimple for complex at -O0 change?

No.  I also recognized that we still set DECL_IS_GIMPLE_REG but
is_gimple_reg says it is not.

We should go into SSA for -O0 as well and fix this for real.

Richard.

WONTFIX on the 4.1 branch.  Downgrading severity for the 4.2 regression.

Update milestone after 4.3.0 release.

4.2.4 is being released, changing milestones to 4.2.5.

Closing 4.2 branch, fixed in 4.3.