[Bug c++/19199] New: Wrong warning about returning a reference to a temporary

[Bug c++/19199] New: Wrong warning about returning a reference to a temporary

[lars at trolltech dot com]

Hi, 
 
The code below produces a warning about returning a reference to a temporary 
when using enum types for the qMin method. It works fine when using primitive 
types or classes instead of enums. 
 
Fortunately it's rather easy to work around the warning :) 
 
Best regards, 
Lars 
 
 
enum Foo { A, B }; 
 
template<typename T> const T &qMin(const T &a, const T &b)  
{ return a < b ? a : b; } 
 
int main(int,  char **) 
{ 
    Foo f = A; 
    Foo g = B; 
    Foo h = qMin(f, g); 
    return 0; 
}

-- 
           Summary: Wrong warning about returning a reference to a temporary
           Product: gcc
           Version: 4.0.0
            Status: UNCONFIRMED
          Severity: minor
          Priority: P3
         Component: c++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: lars at trolltech dot com
                CC: gcc-bugs at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-12-30 14:48 -------
Confirmed:
<<cleanup_point return <retval> = (const Foo &) (const Foo *) &TARGET_EXPR <D.1580, (const Foo) 
MIN_EXPR <(int) *b, (int) *a>>>>
why is there a cast to int in the MIN_EXPR.

This causes wrong code.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|minor                       |normal
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|                            |1
           Keywords|                            |diagnostic, wrong-code
      Known to fail|                            |3.4.0 4.0.0 3.3.2
      Known to work|                            |3.1
           Priority|P3                          |P2
   Last reconfirmed|0000-00-00 00:00:00         |2004-12-30 14:48:58
               date|                            |
            Summary|Wrong warning about         |[3.3/3.4/4.0 Regression]
                   |returning a reference to a  |Wrong warning about
                   |temporary                   |returning a reference to a
                   |                            |temporary
   Target Milestone|---                         |3.4.4


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[steven at gcc dot gnu dot org]

------- Additional Comments From steven at gcc dot gnu dot org  2004-12-31 12:45 -------
Why would this cause wrong code?  It's just a cast from an enum to an int, 
I don't see what's wrong with that except that it's ugly. 
 
You have to actually show that this causes wrong code, not just make your 
usual exaggerated claims ;-) 
 
 

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|wrong-code                  |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-12-31 14:51 -------
(In reply to comment #2)
> Why would this cause wrong code?  It's just a cast from an enum to an int, 
> I don't see what's wrong with that except that it's ugly. 
>  
> You have to actually show that this causes wrong code, not just make your 
> usual exaggerated claims ;-) 

Ok, the reason why it is wrong code is because we return an address to a temporary which we can 
overwrite.  Yes this is wrong code.  The diagnostic is just an indication that it will be wrong code.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[rth at gcc dot gnu dot org]

------- Additional Comments From rth at gcc dot gnu dot org  2005-01-01 00:21 -------
When references are involved, like this, reduction to MAX_EXPR is wrong to 
begin with.  The cast is only an additional symptom.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[gdr at integrable-solutions dot net]

------- Additional Comments From gdr at integrable-solutions dot net  2005-01-02 00:43 -------
Subject: Re:  [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

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

| (In reply to comment #2)
| > Why would this cause wrong code?  It's just a cast from an enum to an int, 
| > I don't see what's wrong with that except that it's ugly. 
| >  
| > You have to actually show that this causes wrong code, not just make your 
| > usual exaggerated claims ;-) 
| 
| Ok, the reason why it is wrong code is because we return an address to a temporary which we can 
| overwrite.  Yes this is wrong code.  The diagnostic is just an indication that it will be wrong code.

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

| When references are involved, like this, reduction to MAX_EXPR is wrong to 
| begin with.  The cast is only an additional symptom.

Both Andrew and RTH are obviously right.  This is wrong-code
generation. As a "proof", I've slighlty modified the testcase to
include an assertion on the address of the object that realizes the
min.  It fails, as can be expected from the warning.
Here we go.

% cat 19199.C && g++ 19199.C -o 19199 && ./19199    
#include <assert.h>

enum Foo { A, B };

template<typename T> const T &qMin(const T &a, const T &b)
{ return a < b ? a : b; }

int main(int,  char **)
{
   Foo f = A;
   Foo g = B;
   const Foo& h = qMin(f, g);
   assert(&h == &f || &h == &g);
   return 0;
}
19199.C: In function 'const T& qMin(const T&, const T&) [with T = Foo]':
19199.C:12:   instantiated from here
19199.C:6: warning: returning reference to temporary
19199: 19199.C:13: int main(int, char**): Assertion `&h == &f || &h == &g' failed.
zsh: 10279 abort      ./19199


-- Gaby



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2005-01-07 01:11 -------
Hmm, even though 3.1 did not produce the diagnostic we did produce wrong code.
So the only part which is a regression is the diagnostic but producing correct code would be very nice.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|3.1                         |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[mmitchel at gcc dot gnu dot org]

------- Additional Comments From mmitchel at gcc dot gnu dot org  2005-01-07 08:08 -------
Roger, this is a middle-end problem in fond_cond_expr_with_comparision; that is
the routine which makes the invalid transformation.  (It has a similar bug for
MAX_EXPR.)  If the result of the COND_EXPR is an lvalue, I do not believe this
transformation is safe.  As we have no language-independent means of testing
lvalue-ness, I'm not sure that there's a straightforward fix.

Would you care to take a look at this issue?

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |roger at eyesopen dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[rth at gcc dot gnu dot org]

------- Additional Comments From rth at gcc dot gnu dot org  2005-01-09 04:46 -------
Actually, I have a standards question here.

Assume for the purposes of discussion here that a source-level reference variable
"X" is represented as a pointer variable "x" in the intermediate language.  E.g.

      int &A, &B, C; C = A + B;
lowers to
      int *a, *b, C; C = *a + *b;

It appears that [expr.cond]/3 *does* preserve references.  Thus it would
seem that the front end should lower

      int &A, &B, &C; C = (A < B ? A : B);
to
      int *a, *b, C; c = (*a < *b ? a : b);
and not
      int *a, *b, *c; c = &(*a < *b ? *a : *b);

Note that comment #1 indicates that we're emitting the later.

If we'd actually written (*a < *b ? *a : *b) at the source level in C, it 
would be illegal for us to take the address of the result, and so the 
transformation to MAX_EXPR would be legitimate.

But I'm not sure what the actual rules for expressions for which references
are legitimate in C++.  If it's legitimate to write

      int A, &B, &C; C = (A < B ? A : B);
->
      int A, *b, *c; c = &(A < *b ? A : *b);

then I think perhaps a more accurate representation would be

      int A, *b, *c, *t; c = &*((A < *b ? t = &A : t = &*b), t);

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mmitchel at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[amylaar at gcc dot gnu dot org]

------- Additional Comments From amylaar at gcc dot gnu dot org  2005-01-26 18:02 -------
Is the folowing the same bug?

extern void abort (void);

int i0 = 999;
int *const p = &i0;

int const *const &
foo ()
{
  return p;
}

int
main ()
{
  int i = *foo ();

  if (i != i0)
    abort ();
  return 0;
}

This warns, and fails at -O1.  At -O0 the code is also wrong, but it passes
because the assignment to the stack temporary happens, and the stack is not
scribbled over.

If I make the creation of a const * const & temporary explicit, the warning
is not emitted, but the code is still wrong in the same way:

extern void abort (void);

int i0 = 999;
int *const p = &i0;

int const *const &
foo ()
{
  int const *const &p0 = p;

  return p0;
}

int
main ()
{
  int i = *foo ();

  if (i != i0)
    abort ();
  return 0;
}

If I define p instead with
int const *const p = &i0;
no bogus temporary is generated.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[amylaar at gcc dot gnu dot org]

------- Additional Comments From amylaar at gcc dot gnu dot org  2005-01-27 23:08 -------
(In reply to comment #9)
Hmmm, closer inspection of the standard seems to indicate that this is
what the standard says should happen.  I think that is a defect, though,
propably best addressed together with DR 450.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[mmitchel at gcc dot gnu dot org]

------- Additional Comments From mmitchel at gcc dot gnu dot org  2005-03-04 08:13 -------
(Of course, the root cause of this problem is that "fold" is being called before
gimplification, which Nathan and I have sermonized about previously.)

There's interesting interplay between this PR and PR 20280.  In particular,
RTH's proposed transformation in Comment #8 is invalid if A and B are
bit-fields, because the address of a bitfield cannot be taken.

Consider:
  
  struct S { int i : 7; };
  S s;
  extern int &b;
  const int &a = (x ? s.i : b);

This cannot be transformed into:

  int *a = &*((x ? t = &s.i : t = &*b), t);

as per Comment #8.

Indeed fold transforms ((s.i > i)? s.i : i) into MAX_EXPR (s.i, i) in this program:

struct S { int i : 7; };
S s;
extern int &i;
bool b;
void g() {
  ((s.i > i) ? s.i : i) = 3;
}

That happens to work only because of the hack where we allow MAX_EXPR as an
lvalue in an assignment in build_modify_expr if neither argument has
side-effects.  (We recreate the COND_EXPR there.)  This change went in as the
fix for PR 7503.  

Roger, I think you need the same hack where you bind COND_EXPRs to references,
take their addresses, etc.  In fact, you should probably just make real_lvalue_p
accept MAX_EXPRs with side-effect free operands.  And, then, wherever we need
lvalues do the conversion that you presently do in build_modify_expr.  That's
consistent with your "MAX_EXPRs are just the canonical form of certain
COND_EXPRs" logic.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-03-04 19:08 -------
Subject: [PR c++/19199] don't turn cond_expr lvalue into min_expr rvalue
        (continued from PR c++/20280)

On Mar  4, 2005, Mark Mitchell <mark@codesourcery.com> wrote:

> Actually, looking at this more closely, I think that something is
> forgetting to call rationalize_conditional_expr, which is normally
> called from unary_complex_lvalue.  When the conditional expression is
> used in the lvalue context, something should be calling that.
> Normally, that happens because something calls build_unary_op
> (ADDR_EXPR, ...) on the COND_EXPR.

> It may be that I changed something to call build_addr (instead of
> build_unary_op) in a case where that's not safe.  Can you confirm or
> deny that hypothesis?

I'm not sure you're still talking about PR 20280, or about PR 19199,
that is marked as a blocker because it appears to be the same problem,
but AFAICT really isn't.

Here's a patch that fixes PR c++/19199, by avoiding the transformation
from:

<cond <lt <nop(int) <parm a> > <nop(int) <parm b> > >
      <parm a> <parm b> >

into:

<nop(enum) <min <nop(int) <parm a> > <nop(int) <parm b> > >

since the latter is clearly not an lvalue, and
rationalize_conditional_expr doesn't manage to turn it back into one
(I don't think it should).  I realize we might miss some optimization
opportunities because of this change in C, because the optimization
would be valid there, but I suppose we could arrange for cond_expr
operands in C to be forced into rvalues.

Still testing on x86_64-linux-gnu.  Ok to install if it passes?

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/19199
	* fold-const.c (non_lvalue): Split tests into...
	(maybe_lvalue_p): New function.
	(fold_ternary): Use it to avoid turning a COND_EXPR lvalue into
	an MIN_EXPR rvalue.

Index: gcc/fold-const.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v
retrieving revision 1.523
diff -u -p -r1.523 fold-const.c
--- gcc/fold-const.c 4 Mar 2005 06:24:09 -0000 1.523
+++ gcc/fold-const.c 4 Mar 2005 19:04:26 -0000
@@ -2004,16 +2004,12 @@ fold_convert (tree type, tree arg)
     }
 }
 \f
-/* Return an expr equal to X but certainly not valid as an lvalue.  */
+/* Return false if expr can be assumed to not be an lvalue, true
+   otherwise.  */
 
-tree
-non_lvalue (tree x)
+static bool
+maybe_lvalue_p (tree x)
 {
-  /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to
-     us.  */
-  if (in_gimple_form)
-    return x;
-
   /* We only need to wrap lvalue tree codes.  */
   switch (TREE_CODE (x))
   {
@@ -2053,8 +2049,24 @@ non_lvalue (tree x)
     /* Assume the worst for front-end tree codes.  */
     if ((int)TREE_CODE (x) >= NUM_TREE_CODES)
       break;
-    return x;
+    return false;
   }
+
+  return true;
+}
+
+/* Return an expr equal to X but certainly not valid as an lvalue.  */
+
+tree
+non_lvalue (tree x)
+{
+  /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to
+     us.  */
+  if (in_gimple_form)
+    return x;
+
+  if (! maybe_lvalue_p (x))
+    return x;
   return build1 (NON_LVALUE_EXPR, TREE_TYPE (x), x);
 }
 
@@ -7095,10 +7107,17 @@ fold_ternary (tree expr)
 	 of B and C.  Signed zeros prevent all of these transformations,
 	 for reasons given above each one.
 
+	 We don't want to use operand_equal_for_comparison_p here,
+	 because it might turn an lvalue COND_EXPR into an rvalue one,
+	 see PR c++/19199.
+
          Also try swapping the arguments and inverting the conditional.  */
       if (COMPARISON_CLASS_P (arg0)
-	  && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0),
-					     arg1, TREE_OPERAND (arg0, 1))
+	  && ((maybe_lvalue_p (arg1)
+	       && maybe_lvalue_p (TREE_OPERAND (t, 2)))
+	      ? operand_equal_p (TREE_OPERAND (arg0, 0), arg1, OEP_ONLY_CONST)
+	      : operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0),
+						arg1, TREE_OPERAND (arg0, 1)))
 	  && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (arg1))))
 	{
 	  tem = fold_cond_expr_with_comparison (type, arg0,
@@ -7109,9 +7128,13 @@ fold_ternary (tree expr)
 	}
 
       if (COMPARISON_CLASS_P (arg0)
-	  && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0),
-					     TREE_OPERAND (t, 2),
-					     TREE_OPERAND (arg0, 1))
+	  && ((maybe_lvalue_p (arg1)
+	       && maybe_lvalue_p (TREE_OPERAND (t, 2)))
+	      ? operand_equal_p (TREE_OPERAND (arg0, 0),
+				 TREE_OPERAND (t, 2), OEP_ONLY_CONST)
+	      : operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0),
+						TREE_OPERAND (t, 2),
+						TREE_OPERAND (arg0, 1)))
 	  && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (TREE_OPERAND (t, 2)))))
 	{
 	  tem = invert_truthvalue (arg0);
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/19199
	* g++.dg/expr/lval2.C: New.

Index: gcc/testsuite/g++.dg/expr/lval2.C
===================================================================
RCS file: gcc/testsuite/g++.dg/expr/lval2.C
diff -N gcc/testsuite/g++.dg/expr/lval2.C
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/g++.dg/expr/lval2.C 4 Mar 2005 19:04:42 -0000
@@ -0,0 +1,26 @@
+// PR c++/19199
+
+// { dg-do run }
+
+// We used to turn the COND_EXPR lvalue into a MIN_EXPR rvalue, and
+// then return a reference to a temporary in qMin.
+
+#include <assert.h>
+
+enum Foo { A, B };
+
+template<typename T> T &qMin(T &a, T &b) 
+{
+  return a < b ? a : b;
+}
+
+int main (int,  char **)
+{
+  Foo f = A;
+  Foo g = B;
+  Foo &h = qMin(f, g);
+  assert (&h == &f || &h == &g);
+  const Foo &i = qMin((const Foo&)f, (const Foo&)g);
+  assert (&i == &f || &i == &g);
+  return 0;
+}

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-03-06 00:14 -------
Subject: Re: [PR c++/19199] don't turn cond_expr lvalue into min_expr rvalue
        (continued from PR c++/20280)

Alexandre Oliva wrote:

> Here's a patch that fixes PR c++/19199, by avoiding the transformation
> from:
> 
> <cond <lt <nop(int) <parm a> > <nop(int) <parm b> > >
>       <parm a> <parm b> >
> 
> into:
> 
> <nop(enum) <min <nop(int) <parm a> > <nop(int) <parm b> > >
> 
> since the latter is clearly not an lvalue, and
> rationalize_conditional_expr doesn't manage to turn it back into one
> (I don't think it should).  I realize we might miss some optimization
> opportunities because of this change in C, because the optimization
> would be valid there, but I suppose we could arrange for cond_expr
> operands in C to be forced into rvalues.

Roger has objected to this change in the past.  As I noted in the audit 
trail for 19199, there's stuff in build_modify_expr to handle 
MIN_EXPR/MAX_EXPR as lvalues -- but, if Roger wants to stick with that 
approach, it needs to be spread throughout the C++ front end.  I'd be 
happier with your approach overall, in part because I think fold is 
doing too much, too early, in general, but I don't feel comfortable 
approving the patch.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-03-07 03:28 -------
Subject: Re: [PR c++/19199] don't turn cond_expr lvalue into min_expr rvalue (continued from PR c++/20280)

On Mar  5, 2005, Mark Mitchell <mark@codesourcery.com> wrote:

> Roger has objected to this change in the past.  As I noted in the
> audit trail for 19199, there's stuff in build_modify_expr to handle
> MIN_EXPR/MAX_EXPR as lvalues

Problem is, with the transformation performed by fold, it's no longer
an lvalue, and forcing it back into an lvalue just because it looks
like it should be one could break other programs.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-03-07 04:19 -------
Subject: Re: [PR c++/19199] don't turn cond_expr lvalue into min_expr rvalue
 (continued from PR c++/20280)

Alexandre Oliva wrote:
> On Mar  5, 2005, Mark Mitchell <mark@codesourcery.com> wrote:
> 
> 
>>Roger has objected to this change in the past.  As I noted in the
>>audit trail for 19199, there's stuff in build_modify_expr to handle
>>MIN_EXPR/MAX_EXPR as lvalues
> 
> 
> Problem is, with the transformation performed by fold, it's no longer
> an lvalue, and forcing it back into an lvalue just because it looks
> like it should be one could break other programs.

Yes, I understand.  You still need to take it up with Roger, though.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-03-08 23:23 -------
Subject: Re:  [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

On Mar  7, 2005, Roger Sayle <roger@eyesopen.com> wrote:

> For example, I believe that Alex's proposed solution to PR c++/19199
> isn't an appropriate fix.  It's perfectly reasonable for fold to convert
> a C++ COND_EXPR into a MIN_EXPR or MAX_EXPR, as according to the C++
> front-end all three of these tree nodes are valid lvalues.  Hence it's
> not this transformation in fold that's in error.

Bugzilla was kept out of the long discussion that ensued, so I'll try
to summarize.  The problem is that the conversion is turning a
COND_EXPR such as:

  ((int)a < (int)b ? a : b)

into

  (__typeof(a)) ((int)a <? (int)b)

which is not an lvalue.  This is the transformation we have to avoid.


> Simply disabling the COND_EXPR -> MIN_EXPR/MAX_EXPR transformation is
> also likely to be a serious performance penalty, especially on targets
> that support efficient sequences for "min" and "max".

This was not what I intended to do with my patch, FWIW.
Unfortunately, I goofed in the added call to operand_equal_p, limiting
too much the situations in which the optimization could be applied.
The patch fixes this problem, and updates the patch such that it
applies cleanly again.

As for other languages whose COND_EXPRs can't be lvalues, we can
probably arrange quite easily for them to ensure at least one of their
result operands is not an lvalue, so as to enable the transformation
again.

Comments?  Ok to install?

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* fold-const.c (non_lvalue): Split tests into...
	(maybe_lvalue_p): New function.
	(fold_ternary): Use it to avoid turning a COND_EXPR lvalue into
	a MIN_EXPR rvalue.

Index: gcc/fold-const.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v
retrieving revision 1.535
diff -u -p -r1.535 fold-const.c
--- gcc/fold-const.c 7 Mar 2005 21:24:21 -0000 1.535
+++ gcc/fold-const.c 8 Mar 2005 22:07:52 -0000
@@ -2005,16 +2005,12 @@ fold_convert (tree type, tree arg)
     }
 }
 \f
-/* Return an expr equal to X but certainly not valid as an lvalue.  */
+/* Return false if expr can be assumed to not be an lvalue, true
+   otherwise.  */
 
-tree
-non_lvalue (tree x)
+static bool
+maybe_lvalue_p (tree x)
 {
-  /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to
-     us.  */
-  if (in_gimple_form)
-    return x;
-
   /* We only need to wrap lvalue tree codes.  */
   switch (TREE_CODE (x))
   {
@@ -2054,8 +2050,24 @@ non_lvalue (tree x)
     /* Assume the worst for front-end tree codes.  */
     if ((int)TREE_CODE (x) >= NUM_TREE_CODES)
       break;
-    return x;
+    return false;
   }
+
+  return true;
+}
+
+/* Return an expr equal to X but certainly not valid as an lvalue.  */
+
+tree
+non_lvalue (tree x)
+{
+  /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to
+     us.  */
+  if (in_gimple_form)
+    return x;
+
+  if (! maybe_lvalue_p (x))
+    return x;
   return build1 (NON_LVALUE_EXPR, TREE_TYPE (x), x);
 }
 
@@ -9734,10 +9746,16 @@ fold_ternary (tree expr)
 	 of B and C.  Signed zeros prevent all of these transformations,
 	 for reasons given above each one.
 
+	 We don't want to use operand_equal_for_comparison_p here,
+	 because it might turn an lvalue COND_EXPR into an rvalue one,
+	 see PR c++/19199.
+
          Also try swapping the arguments and inverting the conditional.  */
       if (COMPARISON_CLASS_P (arg0)
-	  && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0),
-					     arg1, TREE_OPERAND (arg0, 1))
+	  && ((maybe_lvalue_p (op1) && maybe_lvalue_p (op2))
+	      ? operand_equal_p (TREE_OPERAND (arg0, 0), op1, 0)
+	      : operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0),
+						arg1, TREE_OPERAND (arg0, 1)))
 	  && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (arg1))))
 	{
 	  tem = fold_cond_expr_with_comparison (type, arg0, op1, op2);
@@ -9746,9 +9764,10 @@ fold_ternary (tree expr)
 	}
 
       if (COMPARISON_CLASS_P (arg0)
-	  && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0),
-					     op2,
-					     TREE_OPERAND (arg0, 1))
+	  && ((maybe_lvalue_p (op1) && maybe_lvalue_p (op2))
+	      ? operand_equal_p (TREE_OPERAND (arg0, 0), op2, 0)
+	      : operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0),
+						op2, TREE_OPERAND (arg0, 1)))
 	  && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (op2))))
 	{
 	  tem = invert_truthvalue (arg0);
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/19199
	* g++.dg/expr/lval2.C: New.

Index: gcc/testsuite/g++.dg/expr/lval2.C
===================================================================
RCS file: gcc/testsuite/g++.dg/expr/lval2.C
diff -N gcc/testsuite/g++.dg/expr/lval2.C
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/g++.dg/expr/lval2.C 8 Mar 2005 22:08:07 -0000
@@ -0,0 +1,26 @@
+// PR c++/19199
+
+// { dg-do run }
+
+// We used to turn the COND_EXPR lvalue into a MIN_EXPR rvalue, and
+// then return a reference to a temporary in qMin.
+
+#include <assert.h>
+
+enum Foo { A, B };
+
+template<typename T> T &qMin(T &a, T &b) 
+{
+  return a < b ? a : b;
+}
+
+int main (int,  char **)
+{
+  Foo f = A;
+  Foo g = B;
+  Foo &h = qMin(f, g);
+  assert (&h == &f || &h == &g);
+  const Foo &i = qMin((const Foo&)f, (const Foo&)g);
+  assert (&i == &f || &i == &g);
+  return 0;
+}
Index: gcc/testsuite/g++.old-deja/g++.oliva/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/19199
	* expr2.C: Fixed.

Index: gcc/testsuite/g++.old-deja/g++.oliva/expr2.C
===================================================================
RCS file: /cvs/gcc/gcc/gcc/testsuite/g++.old-deja/g++.oliva/expr2.C,v
retrieving revision 1.5
diff -u -p -r1.5 expr2.C
--- gcc/testsuite/g++.old-deja/g++.oliva/expr2.C 1 May 2003 02:02:47 -0000 1.5
+++ gcc/testsuite/g++.old-deja/g++.oliva/expr2.C 8 Mar 2005 22:08:07 -0000
@@ -1,4 +1,4 @@
-// { dg-do run { xfail *-*-* } }
+// { dg-do run }
 
 // Copyright (C) 2000 Free Software Foundation
 

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2005-03-09 01:28 -------
Subject: Re:  [3.3/3.4/4.0/4.1 Regression] Wrong warning about
 returning a reference to a temporary


On 8 Mar 2005, Alexandre Oliva wrote:
>
>       * fold-const.c (non_lvalue): Split tests into...
>       (maybe_lvalue_p): New function.
>       (fold_ternary): Use it to avoid turning a COND_EXPR lvalue into
>       a MIN_EXPR rvalue.

This version is Ok for mainline, and currently open release branches.

Thanks,

Roger
--



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-03-09 04:11 -------
Subject: Re:  [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

On Mar  8, 2005, Roger Sayle <roger@eyesopen.com> wrote:

> On 8 Mar 2005, Alexandre Oliva wrote:
>> 
>> * fold-const.c (non_lvalue): Split tests into...
>> (maybe_lvalue_p): New function.
>> (fold_ternary): Use it to avoid turning a COND_EXPR lvalue into
>> a MIN_EXPR rvalue.

> This version is Ok for mainline, and currently open release branches.

Unfortunately, the problem in g++.oliva/expr2.C resurfaced, because,
as it turns out, the transformation (I != J ? I : J) => I yields an
lvalue as required, but not the *correct* lvalue in all cases.  I
tried what you'd suggested on IRC (testing whether the thing is an
lvalue after the fact), but that obviously failed in this case as
well.

So I figured a better approach would be to perform lvalue tests to
fold_cond_expr_with_comparison(), where we can avoid specific
inappropriate transformations while still trying other alternatives.

While at that, I figured we could use pedantic_lvalues to avoid
refraining from applying optimizations just because it looks like the
cond-expr might be an lvalue, even though the language requires it not
to be.

This is currently bootstrapping on x86_64-linux-gnu.  Ok to install if
bootstrap completes and regtesting passes?

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/19199
	* fold-const.c (non_lvalue): Split tests into...
	(maybe_lvalue_p): New function.
	(fold_cond_expr_with_comparison): Preserve lvalue-ness.

Index: gcc/fold-const.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v
retrieving revision 1.535
diff -u -p -r1.535 fold-const.c
--- gcc/fold-const.c 7 Mar 2005 21:24:21 -0000 1.535
+++ gcc/fold-const.c 9 Mar 2005 03:59:18 -0000
@@ -2005,16 +2005,12 @@ fold_convert (tree type, tree arg)
     }
 }
 \f
-/* Return an expr equal to X but certainly not valid as an lvalue.  */
+/* Return false if expr can be assumed to not be an lvalue, true
+   otherwise.  */
 
-tree
-non_lvalue (tree x)
+static bool
+maybe_lvalue_p (tree x)
 {
-  /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to
-     us.  */
-  if (in_gimple_form)
-    return x;
-
   /* We only need to wrap lvalue tree codes.  */
   switch (TREE_CODE (x))
   {
@@ -2054,8 +2050,24 @@ non_lvalue (tree x)
     /* Assume the worst for front-end tree codes.  */
     if ((int)TREE_CODE (x) >= NUM_TREE_CODES)
       break;
-    return x;
+    return false;
   }
+
+  return true;
+}
+
+/* Return an expr equal to X but certainly not valid as an lvalue.  */
+
+tree
+non_lvalue (tree x)
+{
+  /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to
+     us.  */
+  if (in_gimple_form)
+    return x;
+
+  if (! maybe_lvalue_p (x))
+    return x;
   return build1 (NON_LVALUE_EXPR, TREE_TYPE (x), x);
 }
 
@@ -4162,7 +4174,15 @@ fold_cond_expr_with_comparison (tree typ
   tree arg00 = TREE_OPERAND (arg0, 0);
   tree arg01 = TREE_OPERAND (arg0, 1);
   tree arg1_type = TREE_TYPE (arg1);
-  tree tem;
+  tree tem = NULL;
+  /* If the COND_EXPR can possibly be an lvalue, we don't want to
+     perform transformations that return a simplified result that will
+     be recognized as lvalue, but that will not match the expected
+     result.  We may still return other expressions that would be
+     incorrect, but those are going to be rvalues, and the caller is
+     supposed to discard them.  */
+  bool lvalue = !pedantic_lvalues
+    && maybe_lvalue_p (arg1) && maybe_lvalue_p (arg2);
 
   STRIP_NOPS (arg1);
   STRIP_NOPS (arg2);
@@ -4196,10 +4216,12 @@ fold_cond_expr_with_comparison (tree typ
       case EQ_EXPR:
       case UNEQ_EXPR:
 	tem = fold_convert (arg1_type, arg1);
-	return pedantic_non_lvalue (fold_convert (type, negate_expr (tem)));
+	tem = pedantic_non_lvalue (fold_convert (type, negate_expr (tem)));
+	break;
       case NE_EXPR:
       case LTGT_EXPR:
-	return pedantic_non_lvalue (fold_convert (type, arg1));
+	tem = pedantic_non_lvalue (fold_convert (type, arg1));
+	break;
       case UNGE_EXPR:
       case UNGT_EXPR:
 	if (flag_trapping_math)
@@ -4211,7 +4233,8 @@ fold_cond_expr_with_comparison (tree typ
 	  arg1 = fold_convert (lang_hooks.types.signed_type
 			       (TREE_TYPE (arg1)), arg1);
 	tem = fold (build1 (ABS_EXPR, TREE_TYPE (arg1), arg1));
-	return pedantic_non_lvalue (fold_convert (type, tem));
+	tem = pedantic_non_lvalue (fold_convert (type, tem));
+	break;
       case UNLE_EXPR:
       case UNLT_EXPR:
 	if (flag_trapping_math)
@@ -4222,12 +4245,18 @@ fold_cond_expr_with_comparison (tree typ
 	  arg1 = fold_convert (lang_hooks.types.signed_type
 			       (TREE_TYPE (arg1)), arg1);
 	tem = fold (build1 (ABS_EXPR, TREE_TYPE (arg1), arg1));
-	return negate_expr (fold_convert (type, tem));
+	tem = negate_expr (fold_convert (type, tem));
+	break;
       default:
 	gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison);
 	break;
       }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   /* A != 0 ? A : 0 is simply A, unless A is -0.  Likewise
      A == 0 ? A : 0 is always 0 unless A is -0.  Note that
      both transformations are correct when A is NaN: A != 0
@@ -4236,11 +4265,16 @@ fold_cond_expr_with_comparison (tree typ
   if (integer_zerop (arg01) && integer_zerop (arg2))
     {
       if (comp_code == NE_EXPR)
-	return pedantic_non_lvalue (fold_convert (type, arg1));
+	tem = pedantic_non_lvalue (fold_convert (type, arg1));
       else if (comp_code == EQ_EXPR)
-	return fold_convert (type, integer_zero_node);
+	tem = fold_convert (type, integer_zero_node);
     }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   /* Try some transformations of A op B ? A : B.
 
      A == B? A : B    same as B
@@ -4284,9 +4318,15 @@ fold_cond_expr_with_comparison (tree typ
       switch (comp_code)
 	{
 	case EQ_EXPR:
-	  return pedantic_non_lvalue (fold_convert (type, arg2));
+	  if (lvalue)
+	    break;
+	  tem = pedantic_non_lvalue (fold_convert (type, arg2));
+	  break;
 	case NE_EXPR:
-	  return pedantic_non_lvalue (fold_convert (type, arg1));
+	  if (lvalue)
+	    break;
+	  tem = pedantic_non_lvalue (fold_convert (type, arg1));
+	  break;
 	case LE_EXPR:
 	case LT_EXPR:
 	case UNLE_EXPR:
@@ -4302,7 +4342,7 @@ fold_cond_expr_with_comparison (tree typ
 	      tem = (comp_code == LE_EXPR || comp_code == UNLE_EXPR)
 		    ? fold (build2 (MIN_EXPR, comp_type, comp_op0, comp_op1))
 		    : fold (build2 (MIN_EXPR, comp_type, comp_op1, comp_op0));
-	      return pedantic_non_lvalue (fold_convert (type, tem));
+	      tem = pedantic_non_lvalue (fold_convert (type, tem));
 	    }
 	  break;
 	case GE_EXPR:
@@ -4316,16 +4356,16 @@ fold_cond_expr_with_comparison (tree typ
 	      tem = (comp_code == GE_EXPR || comp_code == UNGE_EXPR)
 		    ? fold (build2 (MAX_EXPR, comp_type, comp_op0, comp_op1))
 		    : fold (build2 (MAX_EXPR, comp_type, comp_op1, comp_op0));
-	      return pedantic_non_lvalue (fold_convert (type, tem));
+	      tem = pedantic_non_lvalue (fold_convert (type, tem));
 	    }
 	  break;
 	case UNEQ_EXPR:
-	  if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
-	    return pedantic_non_lvalue (fold_convert (type, arg2));
+	  if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
+	    tem = pedantic_non_lvalue (fold_convert (type, arg2));
 	  break;
 	case LTGT_EXPR:
-	  if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
-	    return pedantic_non_lvalue (fold_convert (type, arg1));
+	  if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
+	    tem = pedantic_non_lvalue (fold_convert (type, arg1));
 	  break;
 	default:
 	  gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison);
@@ -4333,6 +4373,11 @@ fold_cond_expr_with_comparison (tree typ
 	}
     }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   /* If this is A op C1 ? A : C2 with C1 and C2 constant integers,
      we might still be able to simplify this.  For example,
      if C1 is one less or one more than C2, this might have started
@@ -4347,7 +4392,7 @@ fold_cond_expr_with_comparison (tree typ
       case EQ_EXPR:
 	/* We can replace A with C1 in this case.  */
 	arg1 = fold_convert (type, arg01);
-	return fold (build3 (COND_EXPR, type, arg0, arg1, arg2));
+	tem = fold (build3 (COND_EXPR, type, arg0, arg1, arg2));
 
       case LT_EXPR:
 	/* If C1 is C2 + 1, this is min(A, C2).  */
@@ -4357,8 +4402,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (PLUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold (build2 (MIN_EXPR,
-						    type, arg1, arg2)));
+	  tem = pedantic_non_lvalue (fold (build2 (MIN_EXPR,
+						   type, arg1, arg2)));
 	break;
 
       case LE_EXPR:
@@ -4369,8 +4414,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (MINUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold (build2 (MIN_EXPR,
-						    type, arg1, arg2)));
+	  tem = pedantic_non_lvalue (fold (build2 (MIN_EXPR,
+						   type, arg1, arg2)));
 	break;
 
       case GT_EXPR:
@@ -4381,8 +4426,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (MINUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold (build2 (MAX_EXPR,
-						    type, arg1, arg2)));
+	  tem = pedantic_non_lvalue (fold (build2 (MAX_EXPR,
+						   type, arg1, arg2)));
 	break;
 
       case GE_EXPR:
@@ -4393,8 +4438,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (PLUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold (build2 (MAX_EXPR,
-						    type, arg1, arg2)));
+	  tem = pedantic_non_lvalue (fold (build2 (MAX_EXPR,
+						   type, arg1, arg2)));
 	break;
       case NE_EXPR:
 	break;
@@ -4402,6 +4447,11 @@ fold_cond_expr_with_comparison (tree typ
 	gcc_unreachable ();
       }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   return NULL_TREE;
 }
 
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/19199
	* g++.dg/expr/lval2.C: New.

Index: gcc/testsuite/g++.dg/expr/lval2.C
===================================================================
RCS file: gcc/testsuite/g++.dg/expr/lval2.C
diff -N gcc/testsuite/g++.dg/expr/lval2.C
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/g++.dg/expr/lval2.C 9 Mar 2005 03:59:32 -0000
@@ -0,0 +1,26 @@
+// PR c++/19199
+
+// { dg-do run }
+
+// We used to turn the COND_EXPR lvalue into a MIN_EXPR rvalue, and
+// then return a reference to a temporary in qMin.
+
+#include <assert.h>
+
+enum Foo { A, B };
+
+template<typename T> T &qMin(T &a, T &b) 
+{
+  return a < b ? a : b;
+}
+
+int main (int,  char **)
+{
+  Foo f = A;
+  Foo g = B;
+  Foo &h = qMin(f, g);
+  assert (&h == &f || &h == &g);
+  const Foo &i = qMin((const Foo&)f, (const Foo&)g);
+  assert (&i == &f || &i == &g);
+  return 0;
+}
Index: gcc/testsuite/g++.old-deja/g++.oliva/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/19199
	* expr2.C: Fixed.

Index: gcc/testsuite/g++.old-deja/g++.oliva/expr2.C
===================================================================
RCS file: /cvs/gcc/gcc/gcc/testsuite/g++.old-deja/g++.oliva/expr2.C,v
retrieving revision 1.5
diff -u -p -r1.5 expr2.C
--- gcc/testsuite/g++.old-deja/g++.oliva/expr2.C 1 May 2003 02:02:47 -0000 1.5
+++ gcc/testsuite/g++.old-deja/g++.oliva/expr2.C 9 Mar 2005 03:59:32 -0000
@@ -1,4 +1,4 @@
-// { dg-do run { xfail *-*-* } }
+// { dg-do run }
 
 // Copyright (C) 2000 Free Software Foundation
 

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2005-03-25 06:03 -------
Splitting non_value into maybe_lvalue_p is a good thing, is totally safe and is
preapproved for both mainline and the 4.0 branch.  The remaining change to
fold_ternary/fold_cond_expr_with_comparison are more controversial, and can
theoretically be discussed independently.

Reading through each of the transformations of COND_EXPR in fold, all of the
problematic transformations are guarded in the block beginning at line 4268
of fold-const.c.  This is the set of "A op B ? A : B" transformations.  All
other transformations are either lvalue-safe, or require either operand 2 or
operand 3 to be a non-lvalue (typically operand 3 must be a constant).

I believe a suitable 4.0-timescale (grotesque hack workaround) is (untested):

--- 4265,4275 ----
       a number and A is not.  The conditions in the original
       expressions will be false, so all four give B.  The min()
       and max() versions would give a NaN instead.  */
!   if (operand_equal_for_comparison_p (arg01, arg2, arg00)
!       && (in_gimple_form
!           || strcmp (lang_hooks.name, "GNU C++") != 0
!           || ! maybe_lvalue_p (arg1)
!           || ! maybe_lvalue_p (arg2)))
      {
        tree comp_op0 = arg00;
        tree comp_op1 = arg01;

The maybe_lvalue_p tests should be obvious from the previous versions of
Alexandre's patch.  The remaining two lines are both hideous hacks.  The
first is that these transformations only need to be disabled for the C++
front-end.  This is (AFAIK) the only language front-end that uses COND_EXPRs
as lvalues, and disabling "x > y ? x : y" into MAX_EXPR (where x and y are
VAR_DECLs) is less than ideal for the remaining front-ends.  The second equally
bad hack is to re-allow this transformation even for C++ once we're in the
tree-ssa optimizers.  I believe once we're in gimple, COND_EXPR is no longer
allowed as the lhs of an assignment, hence the MAX_EXPR/MIN_EXPR recognition
transformations are the gimple-level should be able to clean-up any rvalue
COND_EXPRs they're presented with.

Clearly, testing lang_hooks.name is an option of last resort.  I'm increasingly
convinced that the correct long term solution is to introduce a new LCOND_EXPR
tree node for use by the C++ end.  Either as a C++-only tree code, or a generic
tree code.  Additionally, depending upon whether we go ahead and deprecate >?=
and <?= operators, we may or may not also require LMIN_EXPR and LMAX_EXPR. This
allows us to correctly separate the semantics: MIN_EXPR is commutative,
LMIN_EXPR isn't, MIN_EXPR is not an lvalue, LMIN_EXPR is.  If either operand
of a L*_EXPR is not an lvalue, it can be folded to the rvalue form.  The
problematic transformations above can then be controlled via COND_EXPR vs.
LCOND_EXPR rather than checking the front-end.  cp/call.c's
build_conditional_expr is then  tweaked to use LCOND_EXPR instead of COND_EXPR.
Finally either with tweaks to the grammar, gimplification or g++'s other
tree manipulation machinery we can introduce a lower_to_rvalue call to
recursively convert LCOND_EXPR into COND_EXPR, as soon as we know the
expression can't be used as an lvalue.  [Aside: In fact, a fold_rvalue
could be used by the middle-end, to additionally handle cases such as
const_array[0] which can't currently be handled due to the fear of being
used in an lvalue-context, such as &(const_array[0]).

Whilst I see LCOND_EXPR (and maybe LMIN_EXPR, LMAX_EXPR) as the long-term
way to go, this is unquestionably too disruptive for the gcc-4_0-branch.
This leaves us with two options, either suffer an rare miscompilation  or
take a performance hit in the sake of correctness.  I'm in favour of the
latter provided we don't unfairly impact the C/fortran/ada and Java front-ends
that aren't otherwise affected.  The above changes even attempt to allow the
std::min and std::max operators to continue to be optimized, in an attempt to
limit the performance impact to pre-gimple tree-folding during C++ parsing.

 
Alex, could you confirm that the above suggestion resolves the PR when used
in combination with your maybe_lvalue_p split?  Mark, do you agree that this
is a reasonable (the most reasonable?) compromise in the 4.0 timeframe?  I'm
not proud of querying the front-end in fold-const.c, but the semantics
required of COND_EXPR by the C++ front-end conflict with those of the other
front-ends.  Normally this means different tree codes, but that's a luxury
I don't think we can currently afford.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-03-25 06:29 -------
Subject: Re:  [3.3/3.4/4.0/4.1 Regression] Wrong warning about
 returning a reference to a temporary


> Alex, could you confirm that the above suggestion resolves the PR when used
> in combination with your maybe_lvalue_p split?  Mark, do you agree that this
> is a reasonable (the most reasonable?) compromise in the 4.0 timeframe? 

Thank you for looking at this again.

Yes, I think your patch is a reasonable solution in the 4.0 timeframe. 
It does, of course, mean that there will be programs that are less 
well-optimized in C++ than in C, but I agree with you that there's no 
good way around that in the short term.  If you're worried about the 
cost of the strcmp, you could cache the result, but I have no reason to 
think that's a real problem.  Also, I think this patch should go on the 
mainline too, until we get a better fix -- but I think we should work on 
that, so that this doesn't actually make it into 4.1.

(In 4.1, I disagree that we need new tree codes.  I think all we need to 
do is wait to call fold until gimplification time in C++; then, the 
middle end will never see COND_EXPRs used as lvalues.  In C++, 
conditionals can be lvalues, but we actually fix that up before passing 
them to the middle end.  The middle end sees COND_EXPRs that would be 
lvalues in C++, but they are only used as rvalues, so the transformation 
in fold would be fine.

But, we can have that debate later!)

Thanks,



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[aoliva at gcc dot gnu dot org]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-03-30 02:55 -------
Excuse me for asking, but what is it that makes the latest patch I posted not
reasonable for the 4.0 timeframe?

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |aoliva at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2004-12-30 14:48:58         |2005-03-30 02:55:13
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-03-30 07:20 -------
Subject: Re:  [3.3/3.4/4.0/4.1 Regression] Wrong warning about
 returning a reference to a temporary

aoliva at gcc dot gnu dot org wrote:
> ------- Additional Comments From aoliva at gcc dot gnu dot org  2005-03-30 02:55 -------
> Excuse me for asking, but what is it that makes the latest patch I posted not
> reasonable for the 4.0 timeframe?

I'm not sure if there is anything that makes it unreasonable; I don't 
know either way.  I hope Roger will comment.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-04-02 17:29 -------
Subject: Re:  [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

On Mar  9, 2005, Alexandre Oliva <aoliva@redhat.com> wrote:

> 	PR c++/19199
> 	* fold-const.c (non_lvalue): Split tests into...
> 	(maybe_lvalue_p): New function.
> 	(fold_cond_expr_with_comparison): Preserve lvalue-ness.

Ping?

http://gcc.gnu.org/PR19199

I have further local changes to adjust for other changes that went in,
that I'll post upon request, or when the patch goes in.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2005-04-03 03:20 -------
> Excuse me for asking, but what is it that makes the latest patch I posted not
> reasonable for the 4.0 timeframe?

The performance regression on C, Java, Ada and fortran code, that isn't affected
by this bug.  The bug is marked has the "c++" component because it only affects
the C++ front-end.  A fix that disables MIN_EXPR/MAX_EXPR optimizations in all
front-ends is not suitable for a release branch without SPEC testing to show how
badly innocent targets will get burnt!  There may be lots of places in the Linux
kernel that depend upon generating min/max insns for performance reasons...

I'd hoped I'd made this clear when I proposed the alternate strategy of only
disabling this optimization *only* in the C++ front-end, and *only* prior to
tree-ssa.  Whilst I agree this is a serious bug, it currently affects all
release branches, so taking a universal performance hit to resolve it without
considering the consequences seems a bad decision.  Just grep for "MIN (" and
"MAX (" in GCC's own source code to see how badly this could impact the
compiler's own code/performance/bootstrap times.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-04-04 00:37 -------
Subject: Re:  [3.3/3.4/4.0/4.1 Regression] Wrong warning about
 returning a reference to a temporary

roger at eyesopen dot com wrote:

> I'd hoped I'd made this clear when I proposed the alternate strategy of only
> disabling this optimization *only* in the C++ front-end, and *only* prior to
> tree-ssa.

Roger, Alexandre, will you to please coordinate to determine who will 
take responsibility for preparing a patch?

What's difficult for me is that I can't quite figure out who's going to 
actually take this bug and run with it.  I think Alexandre is trying 
hard to fix it, but I'm not confident that he understands what Roger 
wants.  If you two aren't able to work through the issues, or don't have 
time, please let me know; I want to find some way to get this problem 
dealt with before 4.0.

Thanks,



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2005-04-04 05:02 -------
Subject: Bug 19199

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	sayle@gcc.gnu.org	2005-04-04 05:02:10

Modified files:
	gcc            : ChangeLog fold-const.c 
	gcc/testsuite  : ChangeLog 
	gcc/testsuite/g++.old-deja/g++.oliva: ChangeLog expr2.C 
Added files:
	gcc/testsuite/g++.dg/expr: lval2.C 

Log message:
	2005-04-03  Roger Sayle  <roger@eyesopen.com>
	Alexandre Oliva  <aoliva@redhat.com>
	
	PR c++/19199
	* fold-const.c (non_lvalue): Split tests into...
	(maybe_lvalue_p): New function.
	(fold_cond_expr_with_comparison): Preserve lvalue-ness for the
	C++ front-end prior to lowering into gimple form.
	
	* g++.dg/expr/lval2.C: New.
	
	* expr2.C: Fixed.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.8104&r2=2.8105
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/fold-const.c.diff?cvsroot=gcc&r1=1.552&r2=1.553
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.5273&r2=1.5274
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/expr/lval2.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/ChangeLog.diff?cvsroot=gcc&r1=1.35&r2=1.36
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/expr2.C.diff?cvsroot=gcc&r1=1.5&r2=1.6



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-04-04 13:32 -------
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold

On Apr  4, 2005, Roger Sayle <roger@eyesopen.com> wrote:

> 	(fold_cond_expr_with_comparison): Preserve lvalue-ness for the
> 	C++ front-end prior to lowering into gimple form.

> 	* expr2.C: Fixed.

Err...  Why did you choose to drop the portion of the patch below,
that would have avoided the ugliness of comparing langhooks.name, but
still retained it in the ChangeLog entry?

Index: fold-const.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v
retrieving revision 1.554
diff -u -p -d -u -p -d -u -p -r1.554 fold-const.c
--- fold-const.c	4 Apr 2005 08:50:25 -0000	1.554
+++ fold-const.c	4 Apr 2005 13:25:47 -0000
@@ -4173,7 +4173,15 @@ fold_cond_expr_with_comparison (tree typ
   tree arg00 = TREE_OPERAND (arg0, 0);
   tree arg01 = TREE_OPERAND (arg0, 1);
   tree arg1_type = TREE_TYPE (arg1);
-  tree tem;
+  tree tem = NULL;
+  /* If the COND_EXPR can possibly be an lvalue, we don't want to
+     perform transformations that return a simplified result that will
+     be recognized as lvalue, but that will not match the expected
+     result.  We may still return other expressions that would be
+     incorrect, but those are going to be rvalues, and the caller is
+     supposed to discard them.  */
+  bool lvalue = !pedantic_lvalues
+    && maybe_lvalue_p (arg1) && maybe_lvalue_p (arg2);
 
   STRIP_NOPS (arg1);
   STRIP_NOPS (arg2);
@@ -4215,10 +4223,12 @@ fold_cond_expr_with_comparison (tree typ
       case EQ_EXPR:
       case UNEQ_EXPR:
 	tem = fold_convert (arg1_type, arg1);
-	return pedantic_non_lvalue (fold_convert (type, negate_expr (tem)));
+	tem = pedantic_non_lvalue (fold_convert (type, negate_expr (tem)));
+	break;
       case NE_EXPR:
       case LTGT_EXPR:
-	return pedantic_non_lvalue (fold_convert (type, arg1));
+	tem = pedantic_non_lvalue (fold_convert (type, arg1));
+	break;
       case UNGE_EXPR:
       case UNGT_EXPR:
 	if (flag_trapping_math)
@@ -4230,7 +4240,8 @@ fold_cond_expr_with_comparison (tree typ
 	  arg1 = fold_convert (lang_hooks.types.signed_type
 			       (TREE_TYPE (arg1)), arg1);
 	tem = fold_build1 (ABS_EXPR, TREE_TYPE (arg1), arg1);
-	return pedantic_non_lvalue (fold_convert (type, tem));
+	tem = pedantic_non_lvalue (fold_convert (type, tem));
+	break;
       case UNLE_EXPR:
       case UNLT_EXPR:
 	if (flag_trapping_math)
@@ -4241,12 +4252,18 @@ fold_cond_expr_with_comparison (tree typ
 	  arg1 = fold_convert (lang_hooks.types.signed_type
 			       (TREE_TYPE (arg1)), arg1);
 	tem = fold_build1 (ABS_EXPR, TREE_TYPE (arg1), arg1);
-	return negate_expr (fold_convert (type, tem));
+	tem = negate_expr (fold_convert (type, tem));
+	break;
       default:
 	gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison);
 	break;
       }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   /* A != 0 ? A : 0 is simply A, unless A is -0.  Likewise
      A == 0 ? A : 0 is always 0 unless A is -0.  Note that
      both transformations are correct when A is NaN: A != 0
@@ -4255,11 +4272,16 @@ fold_cond_expr_with_comparison (tree typ
   if (integer_zerop (arg01) && integer_zerop (arg2))
     {
       if (comp_code == NE_EXPR)
-	return pedantic_non_lvalue (fold_convert (type, arg1));
+	tem = pedantic_non_lvalue (fold_convert (type, arg1));
       else if (comp_code == EQ_EXPR)
-	return fold_convert (type, integer_zero_node);
+	tem = fold_convert (type, integer_zero_node);
     }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   /* Try some transformations of A op B ? A : B.
 
      A == B? A : B    same as B
@@ -4309,9 +4331,15 @@ fold_cond_expr_with_comparison (tree typ
       switch (comp_code)
 	{
 	case EQ_EXPR:
-	  return pedantic_non_lvalue (fold_convert (type, arg2));
+	  if (lvalue)
+	    break;
+	  tem = pedantic_non_lvalue (fold_convert (type, arg2));
+	  break;
 	case NE_EXPR:
-	  return pedantic_non_lvalue (fold_convert (type, arg1));
+	  if (lvalue)
+	    break;
+	  tem = pedantic_non_lvalue (fold_convert (type, arg1));
+	  break;
 	case LE_EXPR:
 	case LT_EXPR:
 	case UNLE_EXPR:
@@ -4327,7 +4355,7 @@ fold_cond_expr_with_comparison (tree typ
 	      tem = (comp_code == LE_EXPR || comp_code == UNLE_EXPR)
 		    ? fold_build2 (MIN_EXPR, comp_type, comp_op0, comp_op1)
 		    : fold_build2 (MIN_EXPR, comp_type, comp_op1, comp_op0);
-	      return pedantic_non_lvalue (fold_convert (type, tem));
+	      tem = pedantic_non_lvalue (fold_convert (type, tem));
 	    }
 	  break;
 	case GE_EXPR:
@@ -4341,16 +4369,16 @@ fold_cond_expr_with_comparison (tree typ
 	      tem = (comp_code == GE_EXPR || comp_code == UNGE_EXPR)
 		    ? fold_build2 (MAX_EXPR, comp_type, comp_op0, comp_op1)
 		    : fold_build2 (MAX_EXPR, comp_type, comp_op1, comp_op0);
-	      return pedantic_non_lvalue (fold_convert (type, tem));
+	      tem = pedantic_non_lvalue (fold_convert (type, tem));
 	    }
 	  break;
 	case UNEQ_EXPR:
-	  if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
-	    return pedantic_non_lvalue (fold_convert (type, arg2));
+	  if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
+	    tem = pedantic_non_lvalue (fold_convert (type, arg2));
 	  break;
 	case LTGT_EXPR:
-	  if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
-	    return pedantic_non_lvalue (fold_convert (type, arg1));
+	  if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
+	    tem = pedantic_non_lvalue (fold_convert (type, arg1));
 	  break;
 	default:
 	  gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison);
@@ -4358,6 +4386,11 @@ fold_cond_expr_with_comparison (tree typ
 	}
     }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   /* If this is A op C1 ? A : C2 with C1 and C2 constant integers,
      we might still be able to simplify this.  For example,
      if C1 is one less or one more than C2, this might have started
@@ -4372,7 +4405,7 @@ fold_cond_expr_with_comparison (tree typ
       case EQ_EXPR:
 	/* We can replace A with C1 in this case.  */
 	arg1 = fold_convert (type, arg01);
-	return fold_build3 (COND_EXPR, type, arg0, arg1, arg2);
+	tem = fold_build3 (COND_EXPR, type, arg0, arg1, arg2);
 
       case LT_EXPR:
 	/* If C1 is C2 + 1, this is min(A, C2).  */
@@ -4382,8 +4415,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (PLUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold_build2 (MIN_EXPR,
-						   type, arg1, arg2));
+	  tem = pedantic_non_lvalue (fold_build2 (MIN_EXPR,
+						  type, arg1, arg2));
 	break;
 
       case LE_EXPR:
@@ -4394,8 +4427,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (MINUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold_build2 (MIN_EXPR,
-						   type, arg1, arg2));
+	  tem = pedantic_non_lvalue (fold_build2 (MIN_EXPR,
+						  type, arg1, arg2));
 	break;
 
       case GT_EXPR:
@@ -4406,8 +4439,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (MINUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold_build2 (MAX_EXPR,
-						   type, arg1, arg2));
+	  tem = pedantic_non_lvalue (fold_build2 (MAX_EXPR,
+						  type, arg1, arg2));
 	break;
 
       case GE_EXPR:
@@ -4418,8 +4451,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (PLUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold_build2 (MAX_EXPR,
-						   type, arg1, arg2));
+	  tem = pedantic_non_lvalue (fold_build2 (MAX_EXPR,
+						  type, arg1, arg2));
 	break;
       case NE_EXPR:
 	break;
@@ -4427,6 +4460,11 @@ fold_cond_expr_with_comparison (tree typ
 	gcc_unreachable ();
       }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   return NULL_TREE;
 }
 


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[joseph at codesourcery dot com]

------- Additional Comments From joseph at codesourcery dot com  2005-04-04 13:41 -------
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold

On Mon, 4 Apr 2005, Alexandre Oliva wrote:

> Err...  Why did you choose to drop the portion of the patch below,
> that would have avoided the ugliness of comparing langhooks.name, but
> still retained it in the ChangeLog entry?

pedantic_lvalues means "this is C, until I manage to disentangle enough 
C/C++/ObjC interactions to keep lvalueness internal to the C front end so 
pedantic_lvalues can go away".  Checking langhooks.name tests for C++, 
i.e. they differ in their effects for all languages other than C/ObjC/C++: 
only C/ObjC require that conditional expressions not be turned into 
lvalues if they weren't such to start with, while only C++ requires that 
conditional expressions which were lvalues be preserved in their original 
form.  The proper hook to check is one meaning "this is C++" rather than 
"this is not C", though perhaps adding a new hook 
"fold_preserve_cond_expr_p" would be cleaner than checking langhooks.name.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-04-04 13:50 -------
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold

On Apr  4, 2005, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Apr  4, 2005, Roger Sayle <roger@eyesopen.com> wrote:

>> long-term solutions have been proposed for g++ 4.1, the patch below is
>> the best "4.0 timeframe" work-around that doesn't adversely affect
>> performance for GCC's non C++ front-ends,

> I don't understand your claim.  My patch used pedantic_lvalues, which
> is set in such a way by the C and C++ front-ends that makes it even
> faster than the string comparison you're using.

>> and even retains the ability to synthesize MIN_EXPR and MAX_EXPR for
>> C++ during the tree-ssa passes.

> This was never removed in my latest patch.

BTW, here's a patch for mainline that removes the bogus comment the
patch you checked in introduced, and adjusts the lvalueness tests such
that they're performed early, and used at any point we need to make a
decision on whether the transformation is valid.  At the point you
perform the test, we may have already stripped some nop_exprs, which
might make rvalues look like lvalues, removing some optimization
possibilities.

Would you like me to add the langhook to this, or do you oppose the
entire approach for some reason you still haven't explained?

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/19199
	* fold-const.c (maybe_lvalue_p): Remove bogus comment introduced
	in previous patch.
	(fold_cond_expr_with_comparison): Determine whether lvalueness is
	needed upfront, and proceed accordingly in each case.

Index: gcc/fold-const.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v
retrieving revision 1.554
diff -u -p -r1.554 fold-const.c
--- gcc/fold-const.c 4 Apr 2005 08:50:25 -0000 1.554
+++ gcc/fold-const.c 4 Apr 2005 13:42:43 -0000
@@ -2005,7 +2005,6 @@ fold_convert (tree type, tree arg)
 \f
 /* Return false if expr can be assumed not to be an value, true
    otherwise.  */
-/* Return an expr equal to X but certainly not valid as an lvalue.  */
 
 static bool
 maybe_lvalue_p (tree x)
@@ -4173,7 +4172,15 @@ fold_cond_expr_with_comparison (tree typ
   tree arg00 = TREE_OPERAND (arg0, 0);
   tree arg01 = TREE_OPERAND (arg0, 1);
   tree arg1_type = TREE_TYPE (arg1);
-  tree tem;
+  tree tem = NULL;
+  /* If the COND_EXPR can possibly be an lvalue, we don't want to
+     perform transformations that return a simplified result that will
+     be recognized as lvalue, but that will not match the expected
+     result.  We may still return other expressions that would be
+     incorrect, but those are going to be rvalues, and the caller is
+     supposed to discard them.  */
+  bool lvalue = !pedantic_lvalues
+    && maybe_lvalue_p (arg1) && maybe_lvalue_p (arg2);
 
   STRIP_NOPS (arg1);
   STRIP_NOPS (arg2);
@@ -4215,10 +4222,12 @@ fold_cond_expr_with_comparison (tree typ
       case EQ_EXPR:
       case UNEQ_EXPR:
 	tem = fold_convert (arg1_type, arg1);
-	return pedantic_non_lvalue (fold_convert (type, negate_expr (tem)));
+	tem = pedantic_non_lvalue (fold_convert (type, negate_expr (tem)));
+	break;
       case NE_EXPR:
       case LTGT_EXPR:
-	return pedantic_non_lvalue (fold_convert (type, arg1));
+	tem = pedantic_non_lvalue (fold_convert (type, arg1));
+	break;
       case UNGE_EXPR:
       case UNGT_EXPR:
 	if (flag_trapping_math)
@@ -4230,7 +4239,8 @@ fold_cond_expr_with_comparison (tree typ
 	  arg1 = fold_convert (lang_hooks.types.signed_type
 			       (TREE_TYPE (arg1)), arg1);
 	tem = fold_build1 (ABS_EXPR, TREE_TYPE (arg1), arg1);
-	return pedantic_non_lvalue (fold_convert (type, tem));
+	tem = pedantic_non_lvalue (fold_convert (type, tem));
+	break;
       case UNLE_EXPR:
       case UNLT_EXPR:
 	if (flag_trapping_math)
@@ -4241,12 +4251,18 @@ fold_cond_expr_with_comparison (tree typ
 	  arg1 = fold_convert (lang_hooks.types.signed_type
 			       (TREE_TYPE (arg1)), arg1);
 	tem = fold_build1 (ABS_EXPR, TREE_TYPE (arg1), arg1);
-	return negate_expr (fold_convert (type, tem));
+	tem = negate_expr (fold_convert (type, tem));
+	break;
       default:
 	gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison);
 	break;
       }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   /* A != 0 ? A : 0 is simply A, unless A is -0.  Likewise
      A == 0 ? A : 0 is always 0 unless A is -0.  Note that
      both transformations are correct when A is NaN: A != 0
@@ -4255,11 +4271,16 @@ fold_cond_expr_with_comparison (tree typ
   if (integer_zerop (arg01) && integer_zerop (arg2))
     {
       if (comp_code == NE_EXPR)
-	return pedantic_non_lvalue (fold_convert (type, arg1));
+	tem = pedantic_non_lvalue (fold_convert (type, arg1));
       else if (comp_code == EQ_EXPR)
-	return fold_convert (type, integer_zero_node);
+	tem = fold_convert (type, integer_zero_node);
     }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   /* Try some transformations of A op B ? A : B.
 
      A == B? A : B    same as B
@@ -4286,13 +4307,7 @@ fold_cond_expr_with_comparison (tree typ
      a number and A is not.  The conditions in the original
      expressions will be false, so all four give B.  The min()
      and max() versions would give a NaN instead.  */
-  if (operand_equal_for_comparison_p (arg01, arg2, arg00)
-      /* Avoid these transformations if the COND_EXPR may be used
-	 as an lvalue in the C++ front-end.  PR c++/19199.  */
-      && (in_gimple_form
-	  || strcmp (lang_hooks.name, "GNU C++") != 0
-	  || ! maybe_lvalue_p (arg1)
-	  || ! maybe_lvalue_p (arg2)))
+  if (operand_equal_for_comparison_p (arg01, arg2, arg00))
     {
       tree comp_op0 = arg00;
       tree comp_op1 = arg01;
@@ -4309,9 +4324,15 @@ fold_cond_expr_with_comparison (tree typ
       switch (comp_code)
 	{
 	case EQ_EXPR:
-	  return pedantic_non_lvalue (fold_convert (type, arg2));
+	  if (lvalue)
+	    break;
+	  tem = pedantic_non_lvalue (fold_convert (type, arg2));
+	  break;
 	case NE_EXPR:
-	  return pedantic_non_lvalue (fold_convert (type, arg1));
+	  if (lvalue)
+	    break;
+	  tem = pedantic_non_lvalue (fold_convert (type, arg1));
+	  break;
 	case LE_EXPR:
 	case LT_EXPR:
 	case UNLE_EXPR:
@@ -4327,7 +4348,7 @@ fold_cond_expr_with_comparison (tree typ
 	      tem = (comp_code == LE_EXPR || comp_code == UNLE_EXPR)
 		    ? fold_build2 (MIN_EXPR, comp_type, comp_op0, comp_op1)
 		    : fold_build2 (MIN_EXPR, comp_type, comp_op1, comp_op0);
-	      return pedantic_non_lvalue (fold_convert (type, tem));
+	      tem = pedantic_non_lvalue (fold_convert (type, tem));
 	    }
 	  break;
 	case GE_EXPR:
@@ -4341,16 +4362,16 @@ fold_cond_expr_with_comparison (tree typ
 	      tem = (comp_code == GE_EXPR || comp_code == UNGE_EXPR)
 		    ? fold_build2 (MAX_EXPR, comp_type, comp_op0, comp_op1)
 		    : fold_build2 (MAX_EXPR, comp_type, comp_op1, comp_op0);
-	      return pedantic_non_lvalue (fold_convert (type, tem));
+	      tem = pedantic_non_lvalue (fold_convert (type, tem));
 	    }
 	  break;
 	case UNEQ_EXPR:
-	  if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
-	    return pedantic_non_lvalue (fold_convert (type, arg2));
+	  if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
+	    tem = pedantic_non_lvalue (fold_convert (type, arg2));
 	  break;
 	case LTGT_EXPR:
-	  if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
-	    return pedantic_non_lvalue (fold_convert (type, arg1));
+	  if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1))))
+	    tem = pedantic_non_lvalue (fold_convert (type, arg1));
 	  break;
 	default:
 	  gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison);
@@ -4358,6 +4379,11 @@ fold_cond_expr_with_comparison (tree typ
 	}
     }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   /* If this is A op C1 ? A : C2 with C1 and C2 constant integers,
      we might still be able to simplify this.  For example,
      if C1 is one less or one more than C2, this might have started
@@ -4372,7 +4398,7 @@ fold_cond_expr_with_comparison (tree typ
       case EQ_EXPR:
 	/* We can replace A with C1 in this case.  */
 	arg1 = fold_convert (type, arg01);
-	return fold_build3 (COND_EXPR, type, arg0, arg1, arg2);
+	tem = fold_build3 (COND_EXPR, type, arg0, arg1, arg2);
 
       case LT_EXPR:
 	/* If C1 is C2 + 1, this is min(A, C2).  */
@@ -4382,8 +4408,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (PLUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold_build2 (MIN_EXPR,
-						   type, arg1, arg2));
+	  tem = pedantic_non_lvalue (fold_build2 (MIN_EXPR,
+						  type, arg1, arg2));
 	break;
 
       case LE_EXPR:
@@ -4394,8 +4420,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (MINUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold_build2 (MIN_EXPR,
-						   type, arg1, arg2));
+	  tem = pedantic_non_lvalue (fold_build2 (MIN_EXPR,
+						  type, arg1, arg2));
 	break;
 
       case GT_EXPR:
@@ -4406,8 +4432,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (MINUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold_build2 (MAX_EXPR,
-						   type, arg1, arg2));
+	  tem = pedantic_non_lvalue (fold_build2 (MAX_EXPR,
+						  type, arg1, arg2));
 	break;
 
       case GE_EXPR:
@@ -4418,8 +4444,8 @@ fold_cond_expr_with_comparison (tree typ
 				const_binop (PLUS_EXPR, arg2,
 					     integer_one_node, 0),
 				OEP_ONLY_CONST))
-	  return pedantic_non_lvalue (fold_build2 (MAX_EXPR,
-						   type, arg1, arg2));
+	  tem = pedantic_non_lvalue (fold_build2 (MAX_EXPR,
+						  type, arg1, arg2));
 	break;
       case NE_EXPR:
 	break;
@@ -4427,6 +4453,11 @@ fold_cond_expr_with_comparison (tree typ
 	gcc_unreachable ();
       }
 
+  if (tem && (! lvalue || maybe_lvalue_p (tem)))
+    return tem;
+  else
+    tem = NULL;
+
   return NULL_TREE;
 }
 

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-04-04 15:02 -------
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold

On Apr  4, 2005, Alexandre Oliva <aoliva@redhat.com> wrote:

> +     result.  We may still return other expressions that would be
> +     incorrect, but those are going to be rvalues, and the caller is
> +     supposed to discard them.  */

Uhh...  This sentence is no longer true.  I'm removing it from my
local copy of the patch.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2005-04-04 16:02 -------
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold


Hi Alex,

My apologies yet again for not being more explicit about all of the
things that were wrong (and or I was unhappy with) with your proposed
solution.  I'd hoped that I was clear in the comments in the bugzilla
thread, and that you'd appreciate the issues it addressed.

Problems with your approach:

[1] The use of pedantic_lvalues to identify the non-C front-ends,
adversely affects code generation in the Java, Fortran and Ada
front-ends.  The use of COND_EXPRs as lvalues is unique to the
C++ front-end, so ideally a fix shouldn't regress code quality on
innocent front-ends.  Certainly, not without benchmarking to
indicate how significant a performance hit, these other languages
are taking prior to a release.

[2] The pedantic_lvalues flag is itself a hack used by the C
front-end, that is currently being removed by Joseph in his clean-up
of the C parser.  Adding this use would block his efforts, until an
alternate solution is found.  Admittedly, this isn't an issue for
the 4.0 release, but creates more work or a regression once this is
removed from mainline.

[3] Your patch is too invasive.  Compared to the four-line counter
proposal that disables just the problematic class of transformations,
your much larger patch inherently contains a much larger risk.  For
example, there is absolutely no need to modify the source code on the
"A >= 0 ? A : -A  ->   abs (A)" paths as these transformations could
never interfere with the lvalueness of an expression.

Additionally, once one of the longer term solutions proposed by Mark
or me is implemented, all of these workarounds will have to be undone/
reverted.  By only affecting a single clause, we avoid the potential
for leaving historic code bit-rotting in the tree.

[4] Despite your counter claims you approach *does* inhibit the ability
of the the tree-ssa optimizers to synthesize MIN_EXPR and MAX_EXPR
nodes.  Once the in_gimple_form flag is set, fold-const.c is able to
optimize "A == B ? A : B  ->  B" even when compiling C++, as it knows
that a COND_EXPR can't be used as an lvalue in the late middle-end.

[5] For the immediate term, I don't think its worth worrying about
converting non-lvalues into lvalues, the wrong-code bugs and diagnostic
issues are related solely to the lvalue -> non-lvalue transition.
At this stage of pre-release, lower risk changes are cleary preferrable,
and making this change will break code that may have erroneously compiled
in the past.  Probably, OK for 4.0, but not for 3.4 (which also exhibits
this problem).


And although, not serious enough to warrant a [6], it should be pointed
out that several of your recent patches have introduced regressions.
Indeed, you've not yet reported that your patch has been sucessfully
bootstraped or regression tested on any target triple.  Indeed, your
approach of posting a patch before completing the prerequisite testing
sprecified/stressed in contribute.html, on more than one occassion
recently resulted in the patch having to be rewritten/tweaked.  Indeed,
as witnessed in "comment #17", I've already approved an earlier version
your patch once, only to later discover you were wasting my time.


As a middle-end maintainer, having been pinged by the release manager
that we/you weren't making sufficient progress towards addressing the
issues with your patch, I took the opportunity to apply a fix that
was within my authority to commit.  If you'd had made and tested the
changes that I requested in a timely manner, I'm sure I'd have approved
those efforts by now.

My apologies again for not being blunt earlier.  My intention was
that by listing all of the benefits of an alternate approach in
comment #19 of the bugzilla PR, I wouldn't have to explicitly list
them as the defficiencies of your approach.  Some people prefer the
carrot to the stick with patch reviews [others like RTH's "No"].


Perhaps, I should ask the counter question to your comment #21?
In what way do you feel that the committed patch isn't clearly
superior to your proposed solution?  p.s. Thanks for spotting my
mistake of leaving a bogus comment above maybe_lvalue_p.

Roger
--



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-04-04 16:39 -------
Subject: Re:  [3.3/3.4/4.0/4.1 Regression] Wrong warning about
 returning a reference to a temporary

Roger --

Thank you for fixing this PR!  Very much appreciated.

If I'm reading correctly, the patch is now on the mainline, so the 4.1 
regression listing in the subject line is now incorrect?  Your patch is 
definitely suitable for 4.0 as well; I shall look forward to seeing it 
there.

Thanks,



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-04-04 20:18 -------
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold

On Apr  4, 2005, Roger Sayle <roger@eyesopen.com> wrote:

> My apologies yet again for not being more explicit about all of the
> things that were wrong (and or I was unhappy with) with your proposed
> solution.  I'd hoped that I was clear in the comments in the bugzilla
> thread, and that you'd appreciate the issues it addressed.

I didn't.  In fact, I only saw your responses in bugzilla early last
week, since bugzilla e-mail I get goes to a folder that I don't read
very often because of all the traffic in gcc-prs.

> [1] The use of pedantic_lvalues to identify the non-C front-ends,
> adversely affects code generation in the Java, Fortran and Ada
> front-ends.

Indeed.  I'd misunderstood what pedantic_lvalues stood for, and
thought it could be used to distinguish C++ from other languages, not
C from other languages.  My mistake.  That's why it was never clear to
me that you disliked its use, and why.

> The use of COND_EXPRs as lvalues is unique to the
> C++ front-end

I don't know about that.  Among all the languages supported by GCC,
I'm only familiar with C, C++ and Java.

> [3] Your patch is too invasive.  Compared to the four-line counter
> proposal that disables just the problematic class of transformations,
> your much larger patch inherently contains a much larger risk.  For
> example, there is absolutely no need to modify the source code on the
> "A >= 0 ? A : -A  ->   abs (A)" paths as these transformations could
> never interfere with the lvalueness of an expression.

Granted.  OTOH, the patch faulted in trying to be overly consistent in
potentially-broken code, as opposed to keeping potentially-broken code
broken.

> Additionally, once one of the longer term solutions proposed by Mark
> or me is implemented, all of these workarounds will have to be undone/
> reverted.  By only affecting a single clause, we avoid the potential
> for leaving historic code bit-rotting in the tree.

If you're convinced that's the only clause that triggers the problem,
great.  I wasn't.

> [4] Despite your counter claims you approach *does* inhibit the ability
> of the the tree-ssa optimizers to synthesize MIN_EXPR and MAX_EXPR
> nodes.

Indeed.  I still had the behavior from an earlier version of the
patch, in which cond_expr returned different values from
maybe_lvalue_p depending on whether we were in gimple form or not.
Sorry about that.

> [5] For the immediate term, I don't think its worth worrying about
> converting non-lvalues into lvalues,

No worries here, all of my effort was toward preserving lvalueness.

> And although, not serious enough to warrant a [6], it should be pointed
> out that several of your recent patches have introduced regressions.

Oh, like the patch in which I did check in a testcase after
bootstrapping it on amd64-linux-gnu, although I didn't realize it
still failed on i686-pc-linux-gnu, a host on which I don't bootstrap
very often any more because my 32-bit hardware has become too slow?

> Indeed, you've not yet reported that your patch has been sucessfully
> bootstraped or regression tested on any target triple.

Did I have to, before checking the patch in?  I didn't think so.

> Indeed, your
> approach of posting a patch before completing the prerequisite testing
> sprecified/stressed in contribute.html, on more than one occassion
> recently resulted in the patch having to be rewritten/tweaked.

And the need for rewriting/tweaking is exactly the reason why I choose
to post patches early.  Quite often, patches don't go in as they're
posted, since maintainers often request additional minor tweaks.  At
about 24 hours per test cycle, and about as much for patch review, I
don't think posting a patch proposal early would hurt anyone.  In
general, I'm asking for feedback on the approach, rather than on the
exact patch spelling.  If people find the patch to be good as is,
great.  Otherwise, sometimes I can still tweak the patch and get it
into the next build&test cycle.

And, more importantly, I like to describe the problem and the solution
while it's still fresh in my mind.  If I wait 1 or 2 days to post it,
I won't be as precise in my description, and if questions arise, I may
fail to answer correctly.

> Indeed, as witnessed in "comment #17", I've already approved an
> earlier version your patch once, only to later discover you were
> wasting my time.

If you find that reviewing patch proposals is wasting your time, I'm
sorry.  In general, I run a `make all; make check-gcc or make
check-g++' on a previously-built tree before starting a full bootstrap
and posting the patch, but this is not enough to catch all errors.  In
fact, a full bootstrap and test cycle isn't enough either, since we
don't all use the same hosts.  So, patches are going to be posted that
need revision.  Since most of my patches have minor revisions
requested, not posting the patch for early feedback wastes a lot of my
time.  How do we find a balance?

> As a middle-end maintainer, having been pinged by the release manager
> that we/you weren't making sufficient progress towards addressing the
> issues with your patch,

Only days after that did I find you'd added notes to bugzilla, while
an e-mail reply would have caught my attention immediately.

> Perhaps, I should ask the counter question to your comment #21?
> In what way do you feel that the committed patch isn't clearly
> superior to your proposed solution?  p.s. Thanks for spotting my
> mistake of leaving a bogus comment above maybe_lvalue_p.

I can't immediately convince myself that it covers all the
potentially-faulty cases, and it tests for lvalueness after stripping
NOP_EXPRs, which may prevent optimization in case the operand of the
NOP_EXPR looks like an lvalue, but the NOP_EXPR itself is clearly an
rvalue.

It's easy enough to get the lvalue variable to take in_gimple_form
into account, and we can move the test for the language name to the
assignment to lvalue instead of using pedantic_lvalues, if you really
think that's better than a langhook.  Personally, I'd go with a
lang-specific boolean that defaults to false, to tell whether a
cond_expr/min_expr/max_expr can be an lvalue.  I thought
pedantic_lvalues was just that, but it's now obvious that I was
mistaken.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-04-04 23:01 -------
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold

Alexandre Oliva wrote:

> If you find that reviewing patch proposals is wasting your time, I'm
> sorry. 

Of course, untested patches should be clearly marked as such, which 
yours always are.  Given that, I don't feel like you're wasting my time 
with such a posting; if I see something obviously disconcerting I can 
comment.  However, I do find that I'm less interested in reviewing such 
patches.  So, from my point of view, I'd prefer that you post most 
patches after a full test cycle.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2005-04-05 05:15 -------
Subject: Bug 19199

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-4_0-branch
Changes by:	sayle@gcc.gnu.org	2005-04-05 05:15:23

Modified files:
	gcc            : ChangeLog fold-const.c 
	gcc/testsuite  : ChangeLog 
	gcc/testsuite/g++.old-deja/g++.oliva: ChangeLog expr2.C 
Added files:
	gcc/testsuite/g++.dg/expr: lval2.C 

Log message:
	2005-04-04  Roger Sayle  <roger@eyesopen.com>
	Alexandre Oliva  <aoliva@redhat.com>
	
	PR c++/19199
	* fold-const.c (non_lvalue): Split tests into...
	(maybe_lvalue_p): New function.
	(fold_cond_expr_with_comparison): Preserve lvalue-ness for the
	C++ front-end prior to lowering into gimple form.
	
	* g++.dg/expr/lval2.C: New.
	
	* expr2.C: Fixed.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.123&r2=2.7592.2.124
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/fold-const.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.517.2.5&r2=1.517.2.6
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.5084.2.96&r2=1.5084.2.97
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/expr/lval2.C.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.2.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.35&r2=1.35.62.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/expr2.C.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.5&r2=1.5.76.1



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4 Regression] Wrong warning about returning a reference to a temporary

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2005-04-05 23:13 -------
Now that a fix has been applied to both mainline and the 4.0 branch, I've been
investigating backporting the fix to 3.4.  Unfortunately, a significant feature
of the fixes proposed by Alex and me are that they disable the problematic
transformations during parsing, but allow them to be caught by later tree-ssa
passes.  Unfortunately, in gcc 3.4.x if we disable constant folding of
COND_EXPRs for C++, there are no tree-level optimizers that can take up the
slack, and instead we'd rely on if-conversion to do what it can at the RTL
level. I'm not convinced that 3.4 if-conversion is particularly effective in
this respect (certainly across all targets), and I've not yet checked whether
all of the affected transformations are implementated in ifcvt.c (even on
mainline).

Might I propose that we close this bug as won't fix for both the 3.3 and 3.4
branches.  GCC has always generated wrong code for this case, and the current
state of 3.4.4 is that we issue an incorrect warning (which seems better than
silently generating wrong code as we did with 3.1).  It's a trade-off; but I'm
keen to avoid degrading g++ 3.4.5 (in the common case), for a diagnostic
regression.  Or we could leave the PR open (and perhaps unassign it) in the
hope that a 3.4 solution will be discovered, as the mainline/4.x fixes aren't
suitable.  Mark?  Alexandre?


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|3.4.0 4.0.0 3.3.2           |3.4.0 3.3.2
      Known to work|                            |4.0.0 4.1.0
            Summary|[3.3/3.4/4.0 Regression]    |[3.3/3.4 Regression] Wrong
                   |Wrong warning about         |warning about returning a
                   |returning a reference to a  |reference to a temporary
                   |temporary                   |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4 Regression] Wrong warning about returning a reference to a temporary

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-04-05 23:49 -------
Subject: Re:  [3.3/3.4 Regression] Wrong warning about returning
 a reference to a temporary

roger at eyesopen dot com wrote:

> Might I propose that we close this bug as won't fix for both the 3.3 and 3.4
> branches.  GCC has always generated wrong code for this case, and the current
> state of 3.4.4 is that we issue an incorrect warning (which seems better than
> silently generating wrong code as we did with 3.1). 

Just so that I'm clear, what I think you're saying that the wrong-code 
problem has been there all along, and the only regression was actually 
the warning.  Investigating the warning revealed that we were generating 
wrong-code, and fixing the wrong-code problem fixed the warning, but the 
only actual regression per se was the warning.  If that's accurate, 
then, yes, we should add a note saying that this won't be fixed for 
3.4.x, and close the PR as FIXED.

Thanks,



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4 Regression] Wrong warning about returning a reference to a temporary

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2005-04-06 00:03 -------
That's my interpretation of (Andrew Pinki's) comment #6 in the bugzilla PR
[n.b. I haven't reconfirmed his analysis personally]


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199

[Bug c++/19199] [3.3/3.4 Regression] Wrong warning about returning a reference to a temporary

[mmitchel at gcc dot gnu dot org]

------- Additional Comments From mmitchel at gcc dot gnu dot org  2005-04-29 18:28 -------
As per comment #37, closing as WONTFIX.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
           Keywords|wrong-code                  |
         Resolution|                            |WONTFIX


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199