User account creation filtered due to spam.

Bug 23497 - [4.1 regression] Bogus 'is used uninitialized...' warning about std::complex<T>
Summary: [4.1 regression] Bogus 'is used uninitialized...' warning about std::complex<T>
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.1.0
: P2 normal
Target Milestone: 4.1.0
Assignee: Richard Henderson
URL:
Keywords: diagnostic, missed-optimization, TREE
Depends on:
Blocks: Wuninitialized
  Show dependency treegraph
 
Reported: 2005-08-20 17:16 UTC by Jan van Dijk
Modified: 2005-11-16 23:47 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work: 3.4.5 4.0.2
Known to fail: 4.1.0
Last reconfirmed: 2005-11-16 18:56:45


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jan van Dijk 2005-08-20 17:16:55 UTC
With today's trunk, 
  
#include <complex>  
void f()  
{  
        std::complex<double> c1(.0),c2(.0);  
        c1*=c2;  
}  
 
g++ -c -O2 -Wall uninit.cpp  
  
Gives: 
 
/home/jan/local/gcc-head/lib/gcc/i686-pc-linux-gnu/4.1.0/../../../../include/c++/4.1.0/complex:1289: 
warning: &#8216;__t&#8217; is used uninitialized in this function 
 
Which is bogus. This hapens only when optimizing with -O[123] AND -g.
Comment 1 Falk Hueffner 2005-08-20 17:38:01 UTC
I can confirm this on alphaev68-linux, even without -g. C test case:

void f() {
    __complex__ double t;
    __imag__ t = 0;
}

Was introduced somewhere between 2005-05-17 and 2005-06-30.

Comment 2 Andrew Pinski 2005-08-20 18:27:14 UTC
This is a bug in libstdc++ headers.  Since complex has been come a "scalar" and cannot be loaded 
piece wise.
Comment 3 Jan van Dijk 2005-08-20 19:58:17 UTC
(In reply to comment #1)  
> I can confirm this on alphaev68-linux, even without -g. C test case:  
>   
 
-g should have been -Wall. Sorry about that. 
Comment 4 Gabriel Dos Reis 2005-08-22 19:49:58 UTC
Subject: Re:  [4.1 regression] Bogus 'is used uninitialized...' warning about std::complex<T>

"pinskia at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes:

| This is a bug in libstdc++ headers.  Since complex has been come a
| "scalar" and cannot be loaded  piece wise.

That does not make any sense to me.  complex is an aggregate, whether
it can be loaded separately or not is irrelevant from libstdc++
perspective.  This is truly a middle-end bug.

-- Gaby
Comment 5 Benjamin Kosnik 2005-09-12 19:18:04 UTC
Agree with Gaby.
Comment 6 Andrew Pinski 2005-09-12 19:21:50 UTC
(In reply to comment #5)
> Agree with Gaby.

I disagree but what do I know.
It would be like doing:
int f(void)
{
  int i;
  i = (i&0xFFFF0000) | 0x0000FFFF;
  i = (i&0x0000FFFF) | 0xFFFF0000;
  return i;
}

There is no different in this example or the example which Falk gave really.
Comment 7 Andrew Pinski 2005-10-12 00:32:03 UTC
Hmm, we also have a missed optimization on the tree level too.
For the following code:
__complex__ double t1;

void f() {
    __complex__ double t;
    __imag__ t = 0;
    __real__ t = 0;
    t1 = t;
}

we get:
  t1 = COMPLEX_EXPR <0.0, IMAGPART_EXPR <COMPLEX_EXPR <REALPART_EXPR <t>, 0.0>>>;
We should just get t1 = COMPLEX_CST {0.0, 0.0}
Comment 8 Andrew Pinski 2005-10-26 22:51:54 UTC
Note in the mathematical sense complex numbers are scalars, I know in the compiler world this is different.
Comment 9 Mark Mitchell 2005-10-31 05:28:33 UTC
Certainly, the test-case in Comment #1 does depend on libstdc++ at all.  Let's fix this.
Comment 10 Andrew Pinski 2005-11-15 16:31:28 UTC
Hold on, in C99, complex is a scaler type so libstdc++ is using a GCC extension over what C99 requires which causes this.

And my comment in #6 still holds for this bug.  I think libstdc++ should rethink about this.
Comment 11 Paolo Carlini 2005-11-15 16:39:20 UTC
(In reply to comment #10)
> And my comment in #6 still holds for this bug.  I think libstdc++ should
> rethink about this.

libstdc++ can rething anything, in principle, but if you change the component
to libstdc++, nobody will look into the example in #1, which, as Mark also
confirmed, has nothing to do with the library.
Comment 12 Andrew Pinski 2005-11-15 16:47:14 UTC
(In reply to comment #9)
> Certainly, the test-case in Comment #1 does depend on libstdc++ at all.  
> Let's fix this.

The testcase in comment #1 shows the issue with what libstdc++ is doing.  Since complex is scalers they cannot be (with defined behavior) constructed by parts.  So the testcase in comment #1 is in fact invalid with respect of the warning.
Comment 13 Francois-Xavier Coudert 2005-11-15 16:55:24 UTC
It's nice to see that PR bouncing between you all. Although I don't know anything about C++, I want to add my non-constructive comment to this discussion: I don't understand how a bug which has a C-only testcase (in comment #1) can be classified with libstdc++ as component. Please also note that this exact invalid warning happens during the libgfortran build process, preventing it to be done with -Werror.
Comment 14 Mark Mitchell 2005-11-15 19:23:12 UTC
There is nothing wrong with the code in Comment #1; __imag__ is an lvalue-yielding operator.  If, for some reason, we wanted to make __imag__ yield an rvalue, then this code would be rejected with an error; under no circumstances should it yield a warning about the complex value being uninitialized upon initialization of its imaginary component.  It would be reasonable to warn about a subsequent use of the complex value, as the real part remains uninitialized.

Andrew's comments about the fact that __complex__ is a scalar are irrelevant.
Comment 15 Andrew Pinski 2005-11-15 21:42:29 UTC
(In reply to comment #14)
> There is nothing wrong with the code in Comment #1; __imag__ is an
> lvalue-yielding operator.  If, for some reason, we wanted to make __imag__
> yield an rvalue, then this code would be rejected with an error; under no
> circumstances should it yield a warning about the complex value being
> uninitialized upon initialization of its imaginary component.  It would be
> reasonable to warn about a subsequent use of the complex value, as the real
> part remains uninitialized.
I think we should make this based on one part being uninitialized as we are currently because that is just crazy otherwise.  Again this is just like:

a=(a&0xFFFF0000)|(b&0x000FFFF)

which is what the gimplifier does currently where b is the real part of a is the full part.  There is no difference between that and doing __imag___ a = b;

I don't see why the warning here is bogus as it is loading by parts the a and is uninitialized when comming in.  Scaler types are defined this way so you can do optimizations like this.  __imag__ a = b is really defined as a = COMPLEX<REAL_PART<a>, b> which is the same as doing what I did for an integer type.  I don't understand why there is a reluntant to fixing libstdc++ so that you get better optimizable code.  I would not doubt this could cause wrong code due to how libstdc++ does the initialization of complex.
Comment 16 Andrew Pinski 2005-11-15 21:51:10 UTC
Here is the current patch so for libstdc++ (I did not test it yet):
Index: include/std/std_complex.h
===================================================================
--- include/std/std_complex.h   (revision 106892)
+++ include/std/std_complex.h   (working copy)
@@ -1061,29 +1061,27 @@ namespace std
   inline
   complex<float>::complex(float r, float i)
   {
-    __real__ _M_value = r;
-    __imag__ _M_value = i;
+    _M_value = r + i * 1.0fi;
   }
 
   inline complex<float>&
   complex<float>::operator=(float __f)
   {
-    __real__ _M_value = __f;
-    __imag__ _M_value = 0.0f;
+    _M_value = __f;
     return *this;
   }
 
   inline complex<float>&
   complex<float>::operator+=(float __f)
   {
-    __real__ _M_value += __f;
+    _M_value += __f;
     return *this;
   }
 
   inline complex<float>&
   complex<float>::operator-=(float __f)
   {
-    __real__ _M_value -= __f;
+    _M_value -= __f;
     return *this;
   }
 
@@ -1105,8 +1103,7 @@ namespace std
   inline complex<float>&
   complex<float>::operator=(const complex<_Tp>& __z)
   {
-    __real__ _M_value = __z.real();
-    __imag__ _M_value = __z.imag();
+    _M_value = __z.real() +  __z.imag() * 1.0fi;
     return *this;
   }
 
@@ -1114,8 +1111,7 @@ namespace std
   inline complex<float>&
   complex<float>::operator+=(const complex<_Tp>& __z)
   {
-    __real__ _M_value += __z.real();
-    __imag__ _M_value += __z.imag();
+    _M_value += __z.real() +  __z.imag() * 1.0fi;
     return *this;
   }
     
@@ -1123,8 +1119,7 @@ namespace std
     inline complex<float>&
     complex<float>::operator-=(const complex<_Tp>& __z)
     {
-     __real__ _M_value -= __z.real();
-     __imag__ _M_value -= __z.imag();
+     _M_value -= __z.real() +  __z.imag() * 1.0fi;
      return *this;
     } 
 
@@ -1132,9 +1127,7 @@ namespace std
     inline complex<float>&
     complex<float>::operator*=(const complex<_Tp>& __z)
     {
-      _ComplexT __t;
-      __real__ __t = __z.real();
-      __imag__ __t = __z.imag();
+      _ComplexT __t = __z.real() +  __z.imag() * 1.0fi;
       _M_value *= __t;
       return *this;
     }
@@ -1143,9 +1136,7 @@ namespace std
     inline complex<float>&
     complex<float>::operator/=(const complex<_Tp>& __z)
     {
-      _ComplexT __t;
-      __real__ __t = __z.real();
-      __imag__ __t = __z.imag();
+      _ComplexT __t = __z.real() +  __z.imag() * 1.0fi;
       _M_value /= __t;
       return *this;
     }


This also makes more and more optimization opertinutes for complex in libstdc++.  This also makes this code a little more readible.
Comment 17 Andrew Pinski 2005-11-15 21:57:10 UTC
(In reply to comment #13)
> It's nice to see that PR bouncing between you all. Although I don't know
> anything about C++, I want to add my non-constructive comment to this
> discussion: I don't understand how a bug which has a C-only testcase (in
> comment #1) can be classified with libstdc++ as component. Please also note
> that this exact invalid warning happens during the libgfortran build process,
> preventing it to be done with -Werror.

The C example is just as invalid as what libstdc++ does.  Now libgfortran must likely needs to be changed also.
Comment 18 Paolo Carlini 2005-11-15 22:00:35 UTC
(In reply to comment #16)
> Here is the current patch so for libstdc++ (I did not test it yet):

Before patching this and that in the runtime library, don't you believe that:
1- If, as Mark said, (__imag__ t) is an lvalue, doesn't make any sense to emit
   warnings at that line?
2- If the user can write that kind of code anyway, the middle end had better
   being able to optimize it?
Comment 19 Mark Mitchell 2005-11-15 22:10:03 UTC
There are only two choices: either __imag__ is an lvalue, and the code in Comment #1 is valid, or __imag__ is not an lvalue, and the compiler should issue an error.

Nobody wants to see a warning about an uninitialized variable use on:

  __complex__ double t;
  __imag__ t = 0.0;
  __real__ t = 0.0;

If Andrew doesn't like the fact that __imag__ is an lvalue, then Andrew should complain to the C front-end maintainers.
Comment 20 Paolo Carlini 2005-11-15 22:16:31 UTC
About the optimization issue, maybe we should file a separate enhancement PR,
if there isn't already one. Really, we should be able to optimize well any
variant of this kind of code.
Comment 21 Francois-Xavier Coudert 2005-11-16 13:19:06 UTC
(In reply to comment #19)
> There are only two choices: either __imag__ is an lvalue, and the code in
> Comment #1 is valid, or __imag__ is not an lvalue, and the compiler should
> issue an error.

For the libgfortran issue (libgfortran uses __imag__ as a lvalue) what should be done? Who can decide whether __imag__ is or isn't a lvalue? Sorry to ask for the obvious, but I'm lost.

Anyhow, if the change in behaviour between 4.0 and 4.1 is intended, it should surely be noted somewhere.
Comment 22 Andrew Pinski 2005-11-16 13:36:31 UTC
(In reply to comment #21)
> For the libgfortran issue (libgfortran uses __imag__ as a lvalue) what should
> be done? Who can decide whether __imag__ is or isn't a lvalue? Sorry to ask for
> the obvious, but I'm lost.
> Anyhow, if the change in behaviour between 4.0 and 4.1 is intended, it should
> surely be noted somewhere.

Note I never said __imag__ a should not act like an lvalue. I just said that
__imag__ a = b; acts like a = COMPLEX<REAL<a>, b> which is just like what
a = (a&0xFFFF0000)|(b&0x0000FFFF) does.
Comment 23 Andreas Schwab 2005-11-16 14:20:15 UTC
(In reply to comment #22)
> Note I never said __imag__ a should not act like an lvalue. I just said that
> __imag__ a = b; acts like a = COMPLEX<REAL<a>, b> which is just like what
> a = (a&0xFFFF0000)|(b&0x0000FFFF) does.

IMHO it should rather act like this:

struct complex { double real, imag; } a; a.imag = b;
Comment 24 Richard Henderson 2005-11-16 16:47:03 UTC
We tried that.  You get suckier optimization that way.
Comment 25 Mark Mitchell 2005-11-16 16:53:33 UTC
Subject: Re:  [4.1 regression] Bogus 'is used uninitialized...'
 warning about std::complex<T>

schwab at suse dot de wrote:
> ------- Comment #23 from schwab at suse dot de  2005-11-16 14:20 -------
> (In reply to comment #22)
> 
>>Note I never said __imag__ a should not act like an lvalue. I just said that
>>__imag__ a = b; acts like a = COMPLEX<REAL<a>, b> which is just like what
>>a = (a&0xFFFF0000)|(b&0x0000FFFF) does.

If the compiler wants to implement it that way, that's OK.

What's not OK is to warn about the fact that the real part is
uninitialized.  There is no user out there anywhere that wants to be
warned about this case; it is in fact valid code.

Comment 26 Gabriel Dos Reis 2005-11-16 18:40:23 UTC
(In reply to comment #17)
> (In reply to comment #13)
> > It's nice to see that PR bouncing between you all. Although I don't know
> > anything about C++, I want to add my non-constructive comment to this
> > discussion: I don't understand how a bug which has a C-only testcase (in
> > comment #1) can be classified with libstdc++ as component. Please also note
> > that this exact invalid warning happens during the libgfortran build process,
> > preventing it to be done with -Werror.
> 
> The C example is just as invalid as what libstdc++ does.  Now libgfortran must
> likely needs to be changed also.
> 

That is silly.  It is a bug elsewhere in the compiler, not in the libraries.
You can't properly fix this by requiring all libraries to paper over the issue.
Comment 27 Gabriel Dos Reis 2005-11-16 18:44:29 UTC
(In reply to comment #19)
> There are only two choices: either __imag__ is an lvalue, and the code in
> Comment #1 is valid, or __imag__ is not an lvalue, and the compiler should
> issue an error.
> 
> Nobody wants to see a warning about an uninitialized variable use on:
> 
>   __complex__ double t;
>   __imag__ t = 0.0;
>   __real__ t = 0.0;
> 
> If Andrew doesn't like the fact that __imag__ is an lvalue, then Andrew should
> complain to the C front-end maintainers.
> 

From libstdc++ perspective, __imag__ ought to be an lvalue. This is a recurent
issue for C++ complex<>. 
Comment 28 Gabriel Dos Reis 2005-11-16 18:47:17 UTC
(In reply to comment #25)
> Subject: Re:  [4.1 regression] Bogus 'is used uninitialized...'
>  warning about std::complex<T>
> 
> schwab at suse dot de wrote:
> > ------- Comment #23 from schwab at suse dot de  2005-11-16 14:20 -------
> > (In reply to comment #22)
> > 
> >>Note I never said __imag__ a should not act like an lvalue. I just said that
> >>__imag__ a = b; acts like a = COMPLEX<REAL<a>, b> which is just like what
> >>a = (a&0xFFFF0000)|(b&0x0000FFFF) does.
> 
> If the compiler wants to implement it that way, that's OK.
> 
> What's not OK is to warn about the fact that the real part is
> uninitialized.  There is no user out there anywhere that wants to be
> warned about this case; it is in fact valid code.

We're in violent agree on the diagnostic issue.

 
Comment 29 Andrew Pinski 2005-11-16 18:54:00 UTC
(In reply to comment #27)
> From libstdc++ perspective, __imag__ ought to be an lvalue. This is a recurent
> issue for C++ complex<>. 

Of course the lvalue is moot, I never said __imag__ should not be lvalue, I don't know where Mark gets that from (maybe it is because I was showing why the warning is correct or something I don't know).

From the C perspective the warning is correct as you are loading piece wise which is not really supported at all, even shown by my integer testcase.

As I said before __imag__ a should be an lvalue but __imag__ a = b is equivalent to a = COMPLEX < REAL<a>, b> there is nothing from any point of view doing it this way.

Also I should note that __imag__ being around is an extension, a not so well documented one at that.  So changing this to behavior this way is well defined.  And if you look and you will see that now we actually perform a lot more optimizations on complex unlike before when we split it up.  (this is also in reference to RTH's comments in comment #24).
Comment 30 Richard Henderson 2005-11-16 18:56:45 UTC
Clearly the only way to stop getting mail in this thread is to fix it.
Comment 31 Mark Mitchell 2005-11-16 18:58:04 UTC
Subject: Re:  [4.1 regression] Bogus 'is used uninitialized...'
 warning about std::complex<T>

pinskia at gcc dot gnu dot org wrote:

> From the C perspective the warning is correct as you are loading piece wise
> which is not really supported at all, even shown by my integer testcase.

This is not a valid argument.

The point of warnings is not to tell the user how the compiler works;
it's to tell the user that their code is in some way suspect.

Please explain why a user would want a warning on this code.

Comment 32 Andrew Pinski 2005-11-16 19:58:06 UTC
(In reply to comment #31)
> This is not a valid argument.
What would you consider a valid argument for an extension which is not that well documented?

> The point of warnings is not to tell the user how the compiler works;
> it's to tell the user that their code is in some way suspect.

Wait this is an extension to the language.

> Please explain why a user would want a warning on this code.

I should also note that:
http://gcc.gnu.org/onlinedocs/gcc/Complex.html#Complex
recomends against using the extensions anyways.

I should note that page does not talk about __imag__ b being a lvalue at all.
Comment 33 Mark Mitchell 2005-11-16 20:08:25 UTC
Subject: Re:  [4.1 regression] Bogus 'is used uninitialized...'
 warning about std::complex<T>

pinskia at gcc dot gnu dot org wrote:
> ------- Comment #32 from pinskia at gcc dot gnu dot org  2005-11-16 19:58 -------
> (In reply to comment #31)
> 
>>This is not a valid argument.
> 
> What would you consider a valid argument for an extension which is not that
> well documented?

We're not talking about the extension; we're talking about the warning.

Your job is to explain why a user wants to see that warning on that code.

Thankfully, RTH is going to eliminate the warning.  You can file a bug
about the "missing" warning, if you think it's important.

You can also lobby for removing the extension, if you like -- but please
don't do it in this PR.

Comment 34 Gabriel Dos Reis 2005-11-16 20:29:49 UTC
Subject: Re:  [4.1 regression] Bogus 'is used uninitialized...' warning about std::complex<T>

"pinskia at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes:

| I should also note that:
| http://gcc.gnu.org/onlinedocs/gcc/Complex.html#Complex
| recomends against using the extensions anyways.

Exactly which sentence says that?

It is an extension, useful to build libraries that cannot plug or
wait to have C99 <complex.h>.  You would have to explain with hard
arguments why it should go.

-- Gaby
Comment 35 Andreas Schwab 2005-11-16 20:42:23 UTC
__imag__ a = b looks like a simple assignment, thus it should act like a simple assignment.  You can pack a struct { short a,b; } in a single register, but that should not result in a warning when assigning a part of it, even though it would have to be implemented as RMW on many targets.
Comment 36 Richard Henderson 2005-11-16 23:43:43 UTC
Subject: Bug 23497

Author: rth
Date: Wed Nov 16 23:43:39 2005
New Revision: 107107

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=107107
Log:
        PR middle-end/23497
        * tree-ssa.c (warn_uninitialized_var): Skip real and imaginary
        parts of an SSA_NAME.

Added:
    trunk/gcc/testsuite/gcc.dg/uninit-12.c
    trunk/gcc/testsuite/gcc.dg/uninit-13.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-ssa.c

Comment 37 Richard Henderson 2005-11-16 23:47:44 UTC
Fixed.