Bug 20280 - [4.0/4.1 regression] ICE in create_tmp_var, at gimplify.c:368
Summary: [4.0/4.1 regression] ICE in create_tmp_var, at gimplify.c:368
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.0.0
: P2 critical
Target Milestone: 4.0.0
Assignee: Alexandre Oliva
URL:
Keywords: ice-on-valid-code, monitored
Depends on: 19199
Blocks: 30132
  Show dependency treegraph
 
Reported: 2005-03-02 09:38 UTC by Caolan McNamara
Modified: 2007-03-14 00:42 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2005-03-03 07:01:44


Attachments
bzip2'ed dump (248.76 KB, application/x-bzip)
2005-03-02 09:39 UTC, Caolan McNamara
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Caolan McNamara 2005-03-02 09:38:25 UTC
gcc (GCC) 4.0.0 20050228 (Red Hat 4.0.0-0.30)
Copyright (C) 2005 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

---

Making: ../../../unxlngi6.pro/slo/fltini.obj
g++ -Wreturn-type -fmessage-length=0 -c -I.  -I. -I../inc -I../../../inc
-I../../../unx/inc -I../../../unxlngi6.pro/inc -I.
-I/usr/src/redhat/BUILD/SRC680_m80/solver/680/unxlngi6.pro/inc/stl
-I/usr/src/redhat/BUILD/SRC680_m80/solver/680/unxlngi6.pro/inc/external
-I/usr/src/redhat/BUILD/SRC680_m80/solver/680/unxlngi6.pro/inc
-I/usr/src/redhat/BUILD/SRC680_m80/solenv/unxlngi6/inc
-I/usr/src/redhat/BUILD/SRC680_m80/solenv/inc
-I/usr/src/redhat/BUILD/SRC680_m80/res
-I/usr/src/redhat/BUILD/SRC680_m80/solver/680/unxlngi6.pro/inc/stl
-I/usr/src/redhat/BUILD/SRC680_m80/solenv/inc/Xp31 -I/usr/include
-I/usr/X11R6/include     -I. -I../../../res -I. -Wuninitialized -g1 -Os
-fno-strict-aliasing   -fvisibility=hidden -pipe -mtune=pentiumpro
-Wno-ctor-dtor-privacy -fvisibility-inlines-hidden -fexceptions
-fno-enforce-eh-specs   -fpic -DLINUX -DUNX -DVCL -DGCC -DC341 -DINTEL
-DGXX_INCLUDE_PATH=/usr/lib/gcc/i386-redhat-linux/4.0.0/../../../../include/c++/4.0.0
-DCVER=C341 -D_USE_NAMESPACE -DGLIBC=2 -DX86 -D_PTHREADS -D_REENTRANT
-DNEW_SOLAR -D_USE_NAMESPACE=1 -DSTLPORT_VERSION=400
-DHAVE_GCC_VISIBILITY_FEATURE -D__DMAKE -DUNIX -DCPPU_ENV=gcc3 -DSUPD=680
-DPRODUCT -DNDEBUG -DPRODUCT_FULL -DOSL_DEBUG_LEVEL=0 -DOPTIMIZE -DEXCEPTIONS_ON
-DCUI -DSOLAR_JAVA -DSRC680   -DNUM_RELSPACE -DVERTICAL_LAYOUT
-DACCESSIBLE_LAYOUT -DBIDI -DSW_DLLIMPLEMENTATION -DSHAREDLIB -D_DLL_ 
-DMULTITHREAD  -o ../../../unxlngi6.pro/slo/fltini.o
/usr/src/redhat/BUILD/SRC680_m80/sw/source/filter/basflt/fltini.cxx
/usr/src/redhat/BUILD/SRC680_m80/sw/source/filter/basflt/fltini.cxx: In function
\uffff\uffff\uffffvoid CalculateFlySize(SfxItemSet&, const SwNodeIndex&,
SwTwips)\uffff\uffff\uffff:
/usr/src/redhat/BUILD/SRC680_m80/sw/source/filter/basflt/fltini.cxx:810:
internal compiler error: in create_tmp_var, at gimplify.c:368
Please submit a full bug report,
with preprocessed source if appropriate.
See <URL:http://bugzilla.redhat.com/bugzilla> for instructions.
Preprocessed source stored into /tmp/ccAHLsI1.out file, please attach this to
your bugreport.
dmake:  Error code 1, while making '../../../unxlngi6.pro/slo/fltini.obj'
'---* tg_merge.mk *---'
dmake:  Error code 255, while making 'do_it_exceptions'
'---* tg_merge.mk *---'
Comment 1 Caolan McNamara 2005-03-02 09:39:45 UTC
Created attachment 8311 [details]
bzip2'ed dump
Comment 2 Volker Reichelt 2005-03-02 13:31:27 UTC
Confirmed. Reduced testcase:

==================================
struct A
{
    ~A();
};

struct B : A {};

A& foo();

void bar(bool b)
{
    (B&) (b ? foo() : foo());
}
==================================
Comment 3 Alexandre Oliva 2005-03-03 07:51:01 UTC
Subject: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs

When passing an lvalue cond_expr to a function taking a reference that
binds directly to either operand of ?:, we'd fail gimplification when
the type of the expressions was addressable, because, once again,
create_tmp_var would reject addressable types.

The solution I came up with was to hoist the indirect_ref out of the
cond_expr at the time it was created, such that we create a tmp_var
with a reference to the result of the cond_expr, and when we take the
address of the cond_expr to pass its result by reference, it cancels
out with the indirect_ref, so it works beautifully.

I went ahead and verified that I didn't break bit-field lvalues in
conditional expressions (my first attempt did), but I was surprised to
find out that the calls to h() pass.  I understand why they do (we
create a temporary and bind to that), but I'm not sure this is correct
behavior.  Opinions?

I'm bootstrapping this on x86_64-linux-gnu, along with the patch for
PR c++/20103; it's also passed C++ regression testing.  Ok to install
if bootstrap and all-languages regression testing passes?

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

	PR c++/20280
	* call.c (build_conditional_expr): Hoist indirect_ref out of
	cond_expr if the result is an addressable lvalue.

Index: gcc/cp/call.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cp/call.c,v
retrieving revision 1.531
diff -u -p -r1.531 call.c
--- gcc/cp/call.c 24 Feb 2005 21:55:10 -0000 1.531
+++ gcc/cp/call.c 3 Mar 2005 07:42:24 -0000
@@ -3111,6 +3111,7 @@ build_conditional_expr (tree arg1, tree 
   tree result = NULL_TREE;
   tree result_type = NULL_TREE;
   bool lvalue_p = true;
+  bool indirect_p = false;
   struct z_candidate *candidates = 0;
   struct z_candidate *cand;
   void *p;
@@ -3292,7 +3293,15 @@ build_conditional_expr (tree arg1, tree 
       && real_lvalue_p (arg3) 
       && same_type_p (arg2_type, arg3_type))
     {
-      result_type = arg2_type;
+      if (TREE_ADDRESSABLE (arg2_type) && TREE_ADDRESSABLE (arg3_type))
+	{
+	  indirect_p = true;
+	  result_type = build_pointer_type (arg2_type);
+	  arg2 = fold_if_not_in_template (build_address (arg2));
+	  arg3 = fold_if_not_in_template (build_address (arg3));
+	}
+      else
+	result_type = arg2_type;
       goto valid_operands;
     }
 
@@ -3458,6 +3467,14 @@ build_conditional_expr (tree arg1, tree 
   /* We can't use result_type below, as fold might have returned a
      throw_expr.  */
 
+  if (indirect_p)
+    {
+      if (TREE_CODE (TREE_TYPE (result)) == POINTER_TYPE)
+	result = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (result)), result);
+      else
+	gcc_assert (TREE_CODE (result) == THROW_EXPR);
+    }
+
   /* Expand both sides into the same slot, hopefully the target of the
      ?: expression.  We used to check for TARGET_EXPRs here, but now we
      sometimes wrap them in NOP_EXPRs so the test would fail.  */
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* g++.dg/tree-ssa/pr20103.C: New.

Index: gcc/testsuite/g++.dg/tree-ssa/pr20280.C
===================================================================
RCS file: gcc/testsuite/g++.dg/tree-ssa/pr20280.C
diff -N gcc/testsuite/g++.dg/tree-ssa/pr20280.C
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/g++.dg/tree-ssa/pr20280.C 3 Mar 2005 07:42:38 -0000
@@ -0,0 +1,62 @@
+// PR c++/20280
+
+// { dg-do compile }
+
+// Gimplification of the COND_EXPR used to fail because it had an
+// addressable type, and create_tmp_var rejected that.
+
+struct A
+{
+    ~A();
+};
+
+struct B : A {};
+
+A& foo();
+
+void bar(bool b)
+{
+    (B&) (b ? foo() : foo());
+}
+
+// Make sure bit-fields and addressable types don't cause crashes.
+// These were not in the original bug report.
+
+// Added by Alexandre Oliva <aoliva@redhat.com>
+
+// Copyright 2005 Free Software Foundation
+
+struct X
+{
+  long i : 32, j, k : 32;
+};
+
+void g(long&);
+void h(const long&);
+
+void f(X &x, bool b)
+{
+  (b ? x.i : x.j) = 1;
+  (b ? x.j : x.k) = 2;
+  (b ? x.i : x.k) = 3;
+
+  (void)(b ? x.i : x.j);
+  (void)(b ? x.i : x.k);
+  (void)(b ? x.j : x.k);
+
+  g (b ? x.i : x.j); // { dg-error "cannot bind bitfield" }
+  g (b ? x.i : x.k); // { dg-error "cannot bind bitfield" }
+  g (b ? x.j : x.k); // { dg-error "cannot bind bitfield" }
+
+  // Hmm...  I don't think these should be accepted.  The conditional
+  // expressions are lvalues for sure, and 8.5.3/5 exempts lvalues
+  // that are bit-fields, but not lvalues that are conditional
+  // expressions involving bit-fields.
+  h (b ? x.i : x.j);
+  h (b ? x.i : x.k);
+  h (b ? x.j : x.k);
+
+  (long &)(b ? x.i : x.j); // { dg-error "address of bit-field" }
+  (long &)(b ? x.i : x.k); // { dg-error "address of bit-field" }
+  (long &)(b ? x.j : x.k); // { dg-error "address of bit-field" }
+}

-- 
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}
Comment 4 Andrew Pinski 2005-03-03 12:54:17 UTC
I think this is very much related to PR 19199.  I almost think when that bug is fixed this one will also be 
fixed.

Your current patch will not fix that PR either.
Comment 5 Andrew Pinski 2005-03-03 12:58:55 UTC
Subject: Re: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs


On Mar 3, 2005, at 2:50 AM, Alexandre Oliva wrote:

> I'm bootstrapping this on x86_64-linux-gnu, along with the patch for
> PR c++/20103; it's also passed C++ regression testing.  Ok to install
> if bootstrap and all-languages regression testing passes?

I think this is the wrong approach, we should be doing the same for all
types (well except for bitfields) and not just "addressable" types, see
PR 19199 for a testcase where we get this wrong and there is a proposed
way of fixing this from RTH.

Thanks,
Andrew Pinski

Comment 6 Mark Mitchell 2005-03-04 01:36:43 UTC
Subject: Re: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs

Alexandre Oliva wrote:
\
> I went ahead and verified that I didn't break bit-field lvalues in
> conditional expressions (my first attempt did), but I was surprised to
> find out that the calls to h() pass.  I understand why they do (we
> create a temporary and bind to that), but I'm not sure this is correct
> behavior.  Opinions?

> +  // Hmm...  I don't think these should be accepted.  The conditional
> +  // expressions are lvalues for sure, and 8.5.3/5 exempts lvalues
> +  // that are bit-fields, but not lvalues that are conditional
> +  // expressions involving bit-fields.
> +  h (b ? x.i : x.j);
> +  h (b ? x.i : x.k);
> +  h (b ? x.j : x.k);

That's legal because "h" takes a "const &", which permits the compiler 
to create a temporary.  If it takes a non-const reference, you should 
get an error.

And, I think these kinds of transformations (if necessary) should be 
done in a langhook during gimplification, not at COND_EXPR-creation 
time.  We really want the C++ front-end's data structures to be an 
accurate mirror of the input program for as long as possible.

Comment 7 Alexandre Oliva 2005-03-04 06:01:28 UTC
Subject: Re: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs

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

> Alexandre Oliva wrote:
> \
>> I went ahead and verified that I didn't break bit-field lvalues in
>> conditional expressions (my first attempt did), but I was surprised to
>> find out that the calls to h() pass.  I understand why they do (we
>> create a temporary and bind to that), but I'm not sure this is correct
>> behavior.  Opinions?

>> +  // Hmm...  I don't think these should be accepted.  The conditional
>> +  // expressions are lvalues for sure, and 8.5.3/5 exempts lvalues
>> +  // that are bit-fields, but not lvalues that are conditional
>> +  // expressions involving bit-fields.
>> +  h (b ? x.i : x.j);
>> +  h (b ? x.i : x.k);
>> +  h (b ? x.j : x.k);

> That's legal because "h" takes a "const &", which permits the compiler
> to create a temporary.

Yeah, it permits, but only in certain circumstances that AFAICT aren't
met.  This expression AFAICT is an lvalue that isn't a bit-field, so
it has to bind directly, per the first bullet in 8.5.3/5.  Since it
meets the conditions of this first bullet, it doesn't get to use the
`otherwise' portion of that paragraph, that creates a temporary.  Or
am I misreading anything?

> And, I think these kinds of transformations (if necessary) should be
> done in a langhook during gimplification, not at COND_EXPR-creation
> time.  We really want the C++ front-end's data structures to be an
> accurate mirror of the input program for as long as possible.

Err...  But in what sense does my patch change that?  See, what I'm
doing is hoisting the indirect_refs that are inserted by
stabilize_reference out of the cond_expr.  They weren't in the
original code.  There's no dereferencing going on unless the whole
expression undergoes lvalue-to-rvalue decay, so I'd argue that the
transformation I'm proposing actually matches even more accurately the
meaning of the original source code.

Comment 8 Andrew Pinski 2005-03-04 06:30:41 UTC
(In reply to comment #7)
> Yeah, it permits, but only in certain circumstances that AFAICT aren't
> met.  This expression AFAICT is an lvalue that isn't a bit-field, so
> it has to bind directly, per the first bullet in 8.5.3/5.  Since it
> meets the conditions of this first bullet, it doesn't get to use the
> `otherwise' portion of that paragraph, that creates a temporary.  Or
> am I misreading anything?
Yes these expressions are lvalues, see again PR 19199 where we get this wrong.  Even Mark and RTH 
commented on it.  Again it looks like the C++ parser should be creating a tempary to store the address 
and have the indirect reference out of the COND_EXPR, this can only happen for non bitfields.
Comment 9 Alexandre Oliva 2005-03-04 06:34:32 UTC
Subject: Re: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs

On Mar  3, 2005, Andrew Pinski <pinskia@physics.uc.edu> wrote:

> On Mar 3, 2005, at 2:50 AM, Alexandre Oliva wrote:

>> I'm bootstrapping this on x86_64-linux-gnu, along with the patch for
>> PR c++/20103; it's also passed C++ regression testing.  Ok to install
>> if bootstrap and all-languages regression testing passes?

> I think this is the wrong approach,

Err...  But AFAICT this is exactly the approach RTH suggested to cope
with the issue, except for the removal of the unnecessary artificial
decl before gimplification.

> we should be doing the same for all types (well except for
> bitfields) and not just "addressable" types,

Agreed.  That's relatively easy to fix.

Improved patch follows.  Ok to install?

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

	PR c++/19199
	PR c++/20280
	* call.c (build_conditional_expr): Hoist indirect_ref out of
	cond_expr if the result is a non-bitfield lvalue.

Index: gcc/cp/call.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cp/call.c,v
retrieving revision 1.531
diff -u -p -r1.531 call.c
--- gcc/cp/call.c 24 Feb 2005 21:55:10 -0000 1.531
+++ gcc/cp/call.c 4 Mar 2005 06:32:44 -0000
@@ -3111,6 +3111,9 @@ build_conditional_expr (tree arg1, tree 
   tree result = NULL_TREE;
   tree result_type = NULL_TREE;
   bool lvalue_p = true;
+  bool indirect_p = false;
+  cp_lvalue_kind arg2_lvalue_kind;
+  cp_lvalue_kind arg3_lvalue_kind;
   struct z_candidate *candidates = 0;
   struct z_candidate *cand;
   void *p;
@@ -3288,11 +3291,26 @@ build_conditional_expr (tree arg1, tree 
 
      If the second and third operands are lvalues and have the same
      type, the result is of that type and is an lvalue.  */
-  if (real_lvalue_p (arg2) 
-      && real_lvalue_p (arg3) 
+  if ((arg2_lvalue_kind = real_lvalue_p (arg2))
+      && (arg3_lvalue_kind = real_lvalue_p (arg3))
       && same_type_p (arg2_type, arg3_type))
     {
-      result_type = arg2_type;
+      if ((arg2_lvalue_kind & clk_bitfield) == clk_none
+	  && (arg3_lvalue_kind & clk_bitfield) == clk_none)
+	{
+	  indirect_p = true;
+	  result_type = build_pointer_type (arg2_type);
+	  if (TREE_CODE (arg2) == INDIRECT_REF)
+	    arg2 = TREE_OPERAND (arg2, 0);
+	  else
+	    arg2 = fold_if_not_in_template (build_address (arg2));
+	  if (TREE_CODE (arg3) == INDIRECT_REF)
+	    arg3 = TREE_OPERAND (arg3, 0);
+	  else
+	    arg3 = fold_if_not_in_template (build_address (arg3));
+	}
+      else
+	result_type = arg2_type;
       goto valid_operands;
     }
 
@@ -3458,6 +3476,14 @@ build_conditional_expr (tree arg1, tree 
   /* We can't use result_type below, as fold might have returned a
      throw_expr.  */
 
+  if (indirect_p)
+    {
+      if (TREE_CODE (TREE_TYPE (result)) == POINTER_TYPE)
+	result = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (result)), result);
+      else
+	gcc_assert (TREE_CODE (result) == THROW_EXPR);
+    }
+
   /* Expand both sides into the same slot, hopefully the target of the
      ?: expression.  We used to check for TARGET_EXPRs here, but now we
      sometimes wrap them in NOP_EXPRs so the test would fail.  */
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/19199
	PR c++/20280
	* g++.dg/tree-ssa/pr20280.C: New.

Index: gcc/testsuite/g++.dg/tree-ssa/pr20280.C
===================================================================
RCS file: gcc/testsuite/g++.dg/tree-ssa/pr20280.C
diff -N gcc/testsuite/g++.dg/tree-ssa/pr20280.C
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/g++.dg/tree-ssa/pr20280.C 4 Mar 2005 06:32:58 -0000
@@ -0,0 +1,80 @@
+// PR c++/20280
+
+// { dg-do compile }
+
+// Gimplification of the COND_EXPR used to fail because it had an
+// addressable type, and create_tmp_var rejected that.
+
+struct A
+{
+    ~A();
+};
+
+struct B : A {};
+
+A& foo();
+
+void bar(bool b)
+{
+    (B&) (b ? foo() : foo());
+}
+
+// Make sure bit-fields and addressable types don't cause crashes.
+// These were not in the original bug report.
+
+// Added by Alexandre Oliva <aoliva@redhat.com>
+
+// Copyright 2005 Free Software Foundation
+
+struct X
+{
+  long i : 32, j, k : 32;
+};
+
+void g(long&);
+void h(const long&);
+
+void f(X &x, bool b)
+{
+  (b ? x.i : x.j) = 1;
+  (b ? x.j : x.k) = 2;
+  (b ? x.i : x.k) = 3;
+
+  (void)(b ? x.i : x.j);
+  (void)(b ? x.i : x.k);
+  (void)(b ? x.j : x.k);
+
+  g (b ? x.i : x.j); // { dg-error "cannot bind bitfield" }
+  g (b ? x.i : x.k); // { dg-error "cannot bind bitfield" }
+  g (b ? x.j : x.k); // { dg-error "cannot bind bitfield" }
+
+  // Hmm...  I don't think these should be accepted.  The conditional
+  // expressions are lvalues for sure, and 8.5.3/5 exempts lvalues
+  // that are bit-fields, but not lvalues that are conditional
+  // expressions involving bit-fields.
+  h (b ? x.i : x.j);
+  h (b ? x.i : x.k);
+  h (b ? x.j : x.k);
+
+  (long &)(b ? x.i : x.j); // { dg-error "address of bit-field" }
+  (long &)(b ? x.i : x.k); // { dg-error "address of bit-field" }
+  (long &)(b ? x.j : x.k); // { dg-error "address of bit-field" }
+}
+
+// What follows is from PR c++/19199, yet another bug on cond-expr
+// lvalues.  We used to return a reference to a temporary in qMin.
+
+enum Foo { A, B };
+
+template<typename T> const T &qMin(const T &a, const T &b) 
+{
+  return a < b ? a : b;
+}
+
+int testref (int,  char **)
+{
+  Foo f = A;
+  Foo g = B;
+  Foo h = qMin(f, 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}
Comment 10 Alexandre Oliva 2005-03-04 06:59:40 UTC
Subject: Re: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs

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

>> we should be doing the same for all types (well except for
>> bitfields) and not just "addressable" types,

> Agreed.  That's relatively easy to fix.

Rats.  Not that easy.  A number of regressions showed up with the
`improved' patch :-(

It has to do with the uses of build_address, that marks variables and
fields as addressable and used, so we end up having to emit them,
instead of optimizing them out as intended.

It seems like we may indeed need something more elaborate at
gimplification time, instead of modifying the up-front representation.

I'll keep digging.

Comment 11 Mark Mitchell 2005-03-04 07:26:44 UTC
Subject: Re: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs

Alexandre Oliva wrote:

> 
>>>+  // Hmm...  I don't think these should be accepted.  The conditional
>>>+  // expressions are lvalues for sure, and 8.5.3/5 exempts lvalues
>>>+  // that are bit-fields, but not lvalues that are conditional
>>>+  // expressions involving bit-fields.
>>>+  h (b ? x.i : x.j);
>>>+  h (b ? x.i : x.k);
>>>+  h (b ? x.j : x.k);
> 
> 
>>That's legal because "h" takes a "const &", which permits the compiler
>>to create a temporary.
> 
> 
> Yeah, it permits, but only in certain circumstances that AFAICT aren't
> met.  This expression AFAICT is an lvalue that isn't a bit-field, so
> it has to bind directly, per the first bullet in 8.5.3/5.  Since it
> meets the conditions of this first bullet, it doesn't get to use the
> `otherwise' portion of that paragraph, that creates a temporary.  Or
> am I misreading anything?

The situation is a little unclear.

EDG also accepts this code, which is part of what confused me.

Your reading is logical, but it depends on exactly what "lvalue for a 
bit-field" means.  (Note that it does not say "lvalue *is* a bit-field"; 
it says "lvalue *for* a bit-field".)

Consider:

   h((0, x.i));

It would be odd not to allow that, but to allow "h(x.i)".  The 
comma-expression isn't changing what's being passed to "h".  The same 
goes for "h((x.i = 3))".

I think that, if anything, there's a possible defect in the standard 
here, not a defect in the compiler.

>>And, I think these kinds of transformations (if necessary) should be
>>done in a langhook during gimplification, not at COND_EXPR-creation
>>time.  We really want the C++ front-end's data structures to be an
>>accurate mirror of the input program for as long as possible.
>  
> Err...  But in what sense does my patch change that?  See, what I'm
> doing is hoisting the indirect_refs that are inserted by
> stabilize_reference out of the cond_expr.  They weren't in the
> original code.  There's no dereferencing going on unless the whole
> expression undergoes lvalue-to-rvalue decay, so I'd argue that the
> transformation I'm proposing actually matches even more accurately the
> meaning of the original source code.

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?

Thanks,

Comment 12 Alexandre Oliva 2005-03-04 19:23:15 UTC
Subject: Re: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs

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

> Your reading is logical, but it depends on exactly what "lvalue for a
> bit-field" means.  (Note that it does not say "lvalue *is* a
> bit-field"; it says "lvalue *for* a bit-field".)

Good point.  Here's an all-new patch, with the comment updated to
reflect our discussion.

Still testing on x86_64-linux-gnu, ok to install if it succeeds?

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

	PR c++/20280
	* gimplify.c (gimplify_cond_expr): Add fallback argument.  Use a
	temporary variable of pointer type if an lvalues is required.
	(gimplify_modify_expr_rhs): Request an rvalue from it.
	(gimplify_expr): Pass fallback on.

Index: gcc/gimplify.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gimplify.c,v
retrieving revision 2.113
diff -u -p -r2.113 gimplify.c
--- gcc/gimplify.c 18 Feb 2005 19:35:37 -0000 2.113
+++ gcc/gimplify.c 4 Mar 2005 19:18:49 -0000
@@ -2123,7 +2123,8 @@ gimple_boolify (tree expr)
      *EXPR_P should be stored.  */
 
 static enum gimplify_status
-gimplify_cond_expr (tree *expr_p, tree *pre_p, tree *post_p, tree target)
+gimplify_cond_expr (tree *expr_p, tree *pre_p, tree *post_p, tree target,
+		    fallback_t fallback)
 {
   tree expr = *expr_p;
   tree tmp, tmp2, type;
@@ -2137,18 +2138,40 @@ gimplify_cond_expr (tree *expr_p, tree *
      the arms.  */
   else if (! VOID_TYPE_P (type))
     {
+      tree result;
+
       if (target)
 	{
 	  ret = gimplify_expr (&target, pre_p, post_p,
 			       is_gimple_min_lval, fb_lvalue);
 	  if (ret != GS_ERROR)
 	    ret = GS_OK;
-	  tmp = target;
+	  result = tmp = target;
 	  tmp2 = unshare_expr (target);
 	}
+      else if ((fallback & fb_lvalue) == 0)
+	{
+	  result = tmp2 = tmp = create_tmp_var (TREE_TYPE (expr), "iftmp");
+	  ret = GS_ALL_DONE;
+	}
       else
 	{
-	  tmp2 = tmp = create_tmp_var (TREE_TYPE (expr), "iftmp");
+	  tree type = build_pointer_type (TREE_TYPE (expr));
+
+	  if (TREE_TYPE (TREE_OPERAND (expr, 1)) != void_type_node)
+	    TREE_OPERAND (expr, 1) =
+	      build_fold_addr_expr (TREE_OPERAND (expr, 1));
+
+	  if (TREE_TYPE (TREE_OPERAND (expr, 2)) != void_type_node)
+	    TREE_OPERAND (expr, 2) =
+	      build_fold_addr_expr (TREE_OPERAND (expr, 2));
+	  
+	  tmp2 = tmp = create_tmp_var (type, "iftmp");
+
+	  expr = build (COND_EXPR, void_type_node, TREE_OPERAND (expr, 0),
+			TREE_OPERAND (expr, 1), TREE_OPERAND (expr, 2));
+
+	  result = build_fold_indirect_ref (tmp);
 	  ret = GS_ALL_DONE;
 	}
 
@@ -2169,7 +2192,7 @@ gimplify_cond_expr (tree *expr_p, tree *
       /* Move the COND_EXPR to the prequeue.  */
       gimplify_and_add (expr, pre_p);
 
-      *expr_p = tmp;
+      *expr_p = result;
       return ret;
     }
 
@@ -2907,7 +2930,8 @@ gimplify_modify_expr_rhs (tree *expr_p, 
 	if (!is_gimple_reg_type (TREE_TYPE (*from_p)))
 	  {
 	    *expr_p = *from_p;
-	    return gimplify_cond_expr (expr_p, pre_p, post_p, *to_p);
+	    return gimplify_cond_expr (expr_p, pre_p, post_p, *to_p,
+				       fb_rvalue);
 	  }
 	else
 	  ret = GS_UNHANDLED;
@@ -3721,7 +3745,8 @@ gimplify_expr (tree *expr_p, tree *pre_p
 	  break;
 
 	case COND_EXPR:
-	  ret = gimplify_cond_expr (expr_p, pre_p, post_p, NULL_TREE);
+	  ret = gimplify_cond_expr (expr_p, pre_p, post_p, NULL_TREE,
+				    fallback);
 	  break;
 
 	case CALL_EXPR:
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR c++/20280
	* g++.dg/tree-ssa/pr20280.C: New.

Index: gcc/testsuite/g++.dg/tree-ssa/pr20280.C
===================================================================
RCS file: gcc/testsuite/g++.dg/tree-ssa/pr20280.C
diff -N gcc/testsuite/g++.dg/tree-ssa/pr20280.C
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/g++.dg/tree-ssa/pr20280.C 4 Mar 2005 19:19:03 -0000
@@ -0,0 +1,63 @@
+// PR c++/20280
+
+// { dg-do compile }
+
+// Gimplification of the COND_EXPR used to fail because it had an
+// addressable type, and create_tmp_var rejected that.
+
+struct A
+{
+    ~A();
+};
+
+struct B : A {};
+
+A& foo();
+
+void bar(bool b)
+{
+    (B&) (b ? foo() : foo());
+}
+
+// Make sure bit-fields and addressable types don't cause crashes.
+// These were not in the original bug report.
+
+// Added by Alexandre Oliva <aoliva@redhat.com>
+
+// Copyright 2005 Free Software Foundation
+
+struct X
+{
+  long i : 32, j, k : 32;
+};
+
+void g(long&);
+void h(const long&);
+
+void f(X &x, bool b)
+{
+  (b ? x.i : x.j) = 1;
+  (b ? x.j : x.k) = 2;
+  (b ? x.i : x.k) = 3;
+
+  (void)(b ? x.i : x.j);
+  (void)(b ? x.i : x.k);
+  (void)(b ? x.j : x.k);
+
+  g (b ? x.i : x.j); // { dg-error "cannot bind bitfield" }
+  g (b ? x.i : x.k); // { dg-error "cannot bind bitfield" }
+  g (b ? x.j : x.k); // { dg-error "cannot bind bitfield" }
+
+  // It's not entirely clear whether these should be accepted.  The
+  // conditional expressions are lvalues for sure, and 8.5.3/5 exempts
+  // lvalues for bit-fields, but it's not clear that conditional
+  // expressions that are lvalues and that have at least one possible
+  // result that is a bit-field lvalue meets this condition.
+  h (b ? x.i : x.j);
+  h (b ? x.i : x.k);
+  h (b ? x.j : x.k);
+
+  (long &)(b ? x.i : x.j); // { dg-error "address of bit-field" }
+  (long &)(b ? x.i : x.k); // { dg-error "address of bit-field" }
+  (long &)(b ? x.j : x.k); // { dg-error "address of bit-field" }
+}

-- 
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}
Comment 13 Jason Merrill 2005-03-08 06:47:23 UTC
Subject: Re: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs

On Thu, 03 Mar 2005 23:26:04 -0800, Mark Mitchell <mark@codesourcery.com> wrote:

> Your reading is logical, but it depends on exactly what "lvalue for a
> bit-field" means.  (Note that it does not say "lvalue *is* a bit-field"; it
> says "lvalue *for* a bit-field".)

In fact, there's a core issue for exactly that question; Steve's proposed
wording clarifies that these expressions are lvalues for a bit-field.

Jason
Comment 14 Richard Henderson 2005-03-14 12:59:38 UTC
Subject: Re: [PR c++/20280] hoist indirect_ref out of addressable cond_exprs

On Fri, Mar 04, 2005 at 04:21:53PM -0300, Alexandre Oliva wrote:
> 	* gimplify.c (gimplify_cond_expr): Add fallback argument.  Use a
> 	temporary variable of pointer type if an lvalues is required.
> 	(gimplify_modify_expr_rhs): Request an rvalue from it.
> 	(gimplify_expr): Pass fallback on.

Ok.


r~
Comment 15 CVS Commits 2005-03-14 20:02:40 UTC
Subject: Bug 20280

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	aoliva@gcc.gnu.org	2005-03-14 20:02:11

Modified files:
	gcc            : ChangeLog gimplify.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/g++.dg/tree-ssa: pr20280.C 

Log message:
	gcc/ChangeLog:
	PR c++/20280
	* gimplify.c (gimplify_cond_expr): Add fallback argument.  Use a
	temporary variable of pointer type if an lvalues is required.
	(gimplify_modify_expr_rhs): Request an rvalue from it.
	(gimplify_expr): Pass fallback on.
	gcc/testsuite/ChangeLog:
	PR c++/20280
	* g++.dg/tree-ssa/pr20280.C: New.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.7848&r2=2.7849
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/gimplify.c.diff?cvsroot=gcc&r1=2.117&r2=2.118
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.5165&r2=1.5166
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/tree-ssa/pr20280.C.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 16 CVS Commits 2005-03-14 20:35:00 UTC
Subject: Bug 20280

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-4_0-branch
Changes by:	aoliva@gcc.gnu.org	2005-03-14 20:34:53

Modified files:
	gcc            : ChangeLog gimplify.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/g++.dg/tree-ssa: pr20280.C 

Log message:
	gcc/ChangeLog:
	PR c++/20280
	* gimplify.c (gimplify_cond_expr): Add fallback argument.  Use a
	temporary variable of pointer type if an lvalues is required.
	(gimplify_modify_expr_rhs): Request an rvalue from it.
	(gimplify_expr): Pass fallback on.
	gcc/testsuite/ChangeLog:
	PR c++/20280
	* g++.dg/tree-ssa/pr20280.C: New.

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.50&r2=2.7592.2.51
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/gimplify.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.113.2.1&r2=2.113.2.2
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.45&r2=1.5084.2.46
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/tree-ssa/pr20280.C.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.2.1

Comment 17 Alexandre Oliva 2005-03-14 23:43:35 UTC
Fixed
Comment 18 Andrew Pinski 2006-12-10 20:50:24 UTC
The gimplifier patch here causes PR 30132 and I still think it is the wrong approach as we have a fall back of either here.  If I change "fallback & fb_lvalue) == 0" to "fallback != fb_lvalue", then we get an ICE still on this testcase but since the fallback says it can be either.
Comment 19 Andrew Pinski 2007-03-14 00:42:44 UTC
I have a patch which fixes after my patch for PR 30132.  The way I fixed it is an all front-end fix:
Index: typeck.c
===================================================================
--- typeck.c    (revision 122880)
+++ typeck.c    (working copy)
@@ -4060,6 +4060,14 @@ build_address (tree t)
   if (error_operand_p (t) || !cxx_mark_addressable (t))
     return error_mark_node;

+  if (TREE_CODE (t) == COND_EXPR)
+    {
+      tree ptrtype = build_pointer_type (TREE_TYPE (t));
+      return build3 (COND_EXPR, ptrtype, TREE_OPERAND (t, 0),
+                    build_address (TREE_OPERAND (t, 1)),
+                    build_address (TREE_OPERAND (t, 2)));
+    }
+
   addr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (t)), t);

   return addr;


Create a new COND_EXPR for when we want to create an address of a COND_EXPR, by this point, we should have errored out.