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: ‘__t’ is used uninitialized in this function Which is bogus. This hapens only when optimizing with -O[123] AND -g.
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.
This is a bug in libstdc++ headers. Since complex has been come a "scalar" and cannot be loaded piece wise.
(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.
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
Agree with Gaby.
(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.
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}
Note in the mathematical sense complex numbers are scalars, I know in the compiler world this is different.
Certainly, the test-case in Comment #1 does depend on libstdc++ at all. Let's fix this.
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.
(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.
(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.
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.
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.
(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.
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.
(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.
(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?
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.
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.
(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.
(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.
(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;
We tried that. You get suckier optimization that way.
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.
(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.
(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<>.
(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.
(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).
Clearly the only way to stop getting mail in this thread is to fix it.
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.
(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.
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.
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
__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.
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
Fixed.