[C++ PATCH] Re: gcc trunk rev. 127646 breaks bootstrap during stage 3

[C++ PATCH] Re: gcc trunk rev. 127646 breaks bootstrap during stage 3

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Mon, Aug 20, 2007 at 07:26:02AM -0700, Andrew Pinski wrote:
> > The statement which we are gimplifying:
> > <<<change_dynamic_type (struct __recursive_mutex *) &fake_mutex)>>>
> >
> > 5887            case CHANGE_DYNAMIC_TYPE_EXPR:
> > 5888              ret = gimplify_expr (&CHANGE_DYNAMIC_TYPE_LOCATION (*expr_p),
> > 5889                                   pre_p, post_p, is_gimple_reg, fb_lvalue);
> >
> >
> > So why fb_lvalue?
> 
> Actually this is correct, we need a decl here as we need to mark that
> for pointing to anything.  Jakub, how did you test your patch again?

I'm sorry, I bet I forgot to make in libstdc++-v3 this time, only tested
make -C check-g++ and make -C x86_64*/libstdc++-v3.

The problem is that save_expr on constant arg (which &fake_mutex is)
does nothing at all and we really need the SAVE_EXPR around, so that
during gimplification a temporary is created (but we can't create it
right away in the FE, it must be initialized in the correct spot).

Attached are two possible fixes, one is to force creation of SAVE_EXPR
when it wasn't added by save_expr, the other one is to use a TARGET_EXPR
instead (force creation thereof).
I have briefly tested the former, the latter was tested with make
in libstdc++-v3, make check-g++ and make check in libstdc++-v3.

Ok for trunk (which one)?

	Jakub

[-- Attachment #2: G --]
[-- Type: text/plain, Size: 1210 bytes --]

2007-08-20  Jakub Jelinek  <jakub@redhat.com>

	* init.c (build_new_1): Force SAVE_EXPR if placement_expr
	is a constant.

	* g++.dg/init/new24.C: New test.

--- gcc/cp/init.c.jj	2007-08-20 09:52:49.000000000 +0200
+++ gcc/cp/init.c	2007-08-20 17:11:10.000000000 +0200
@@ -1758,6 +1758,9 @@ build_new_1 (tree placement, tree type, 
   else
     {
       placement_expr = save_expr (TREE_VALUE (placement));
+      if (TREE_CODE (placement_expr) != SAVE_EXPR)
+	placement_expr = build1 (SAVE_EXPR, TREE_TYPE (placement_expr),
+				 placement_expr);
       placement = tree_cons (NULL_TREE, placement_expr, NULL_TREE);
     }
 
--- gcc/testsuite/g++.dg/init/new24.C.jj	2007-08-20 17:13:28.000000000 +0200
+++ gcc/testsuite/g++.dg/init/new24.C	2007-08-20 17:01:52.000000000 +0200
@@ -0,0 +1,18 @@
+// PR c++/33025
+// { dg-do run }
+// { dg-options "-O2" }
+
+typedef __SIZE_TYPE__ size_t;
+inline void *operator new (size_t, void *p) throw () { return p; }
+extern "C" void abort ();
+
+int
+main()
+{
+  const unsigned num = 10;
+  unsigned *data = new unsigned[num];
+  unsigned *ptr = new (data) unsigned (num);
+  static unsigned data2[10];
+  unsigned *ptr2 = new (&data2[0]) unsigned (10);
+  return 0;
+}

[-- Attachment #3: G2 --]
[-- Type: text/plain, Size: 1213 bytes --]

2007-08-20  Jakub Jelinek  <jakub@redhat.com>

	* init.c (build_new_1): Use force_target_expr instead of
	save_expr.

	* g++.dg/init/new24.C: New test.

--- gcc/cp/init.c.jj	2007-08-20 09:52:49.000000000 +0200
+++ gcc/cp/init.c	2007-08-20 17:16:35.000000000 +0200
@@ -1757,7 +1757,8 @@ build_new_1 (tree placement, tree type, 
     placement_expr = NULL_TREE;
   else
     {
-      placement_expr = save_expr (TREE_VALUE (placement));
+      placement_expr = force_target_expr (TREE_TYPE (TREE_VALUE (placement)),
+					  TREE_VALUE (placement));
       placement = tree_cons (NULL_TREE, placement_expr, NULL_TREE);
     }
 
--- gcc/testsuite/g++.dg/init/new24.C.jj	2007-08-20 17:13:28.000000000 +0200
+++ gcc/testsuite/g++.dg/init/new24.C	2007-08-20 17:19:14.000000000 +0200
@@ -0,0 +1,18 @@
+// PR c++/33025
+// { dg-do compile }
+// { dg-options "-O2" }
+
+typedef __SIZE_TYPE__ size_t;
+inline void *operator new (size_t, void *p) throw () { return p; }
+extern "C" void abort ();
+
+int
+main()
+{
+  const unsigned num = 10;
+  unsigned *data = new unsigned[num];
+  unsigned *ptr = new (data) unsigned (num);
+  static unsigned data2[10];
+  unsigned *ptr2 = new (&data2[0]) unsigned (10);
+  return 0;
+}

Re: [C++ PATCH] Re: gcc trunk rev. 127646 breaks bootstrap during stage 3

[Andrew Pinski]

On 8/20/07, Jakub Jelinek <jakub@redhat.com> wrote:
> I'm sorry, I bet I forgot to make in libstdc++-v3 this time, only tested
> make -C check-g++ and make -C x86_64*/libstdc++-v3.

And you forgot to check to build libjava also which is required when
you changed the C++ front-end.
As documented on http://gcc.gnu.org/contribute.html :).

I would say revert the patch until you fully (which includes building
libjava too) tested your patches.

--- Pinski

Re: [C++ PATCH] Re: gcc trunk rev. 127646 breaks bootstrap during stage 3

[Jakub Jelinek]

On Mon, Aug 20, 2007 at 08:39:57PM -0700, Andrew Pinski wrote:
> On 8/20/07, Jakub Jelinek <jakub@redhat.com> wrote:
> > I'm sorry, I bet I forgot to make in libstdc++-v3 this time, only tested
> > make -C check-g++ and make -C x86_64*/libstdc++-v3.
> 
> And you forgot to check to build libjava also which is required when
> you changed the C++ front-end.
> As documented on http://gcc.gnu.org/contribute.html :).
> 
> I would say revert the patch until you fully (which includes building
> libjava too) tested your patches.

The force_target_expr variant has been fully tested (all languages except
java, bootstrap + make check).

	Jakub

Re: [C++ PATCH] Re: gcc trunk rev. 127646 breaks bootstrap during stage 3

[Andrew Pinski]

On 8/21/07, Jakub Jelinek <jakub@redhat.com> wrote:
> The force_target_expr variant has been fully tested (all languages except
> java, bootstrap + make check).

Yes and java testing is required by our documentation for a C++
front-end change so again, you did not follow directions.

-- Pinski

Re: [C++ PATCH] Re: gcc trunk rev. 127646 breaks bootstrap during stage 3

[Jakub Jelinek]

On Tue, Aug 21, 2007 at 02:04:12AM -0700, Andrew Pinski wrote:
> On 8/21/07, Jakub Jelinek <jakub@redhat.com> wrote:
> > The force_target_expr variant has been fully tested (all languages except
> > java, bootstrap + make check).
> 
> Yes and java testing is required by our documentation for a C++
> front-end change so again, you did not follow directions.

Typo, meant to write all languages except ada.

	Jakub

Re: [C++ PATCH] Re: gcc trunk rev. 127646 breaks bootstrap during stage 3

[Richard Guenther]

On 8/20/07, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Aug 20, 2007 at 07:26:02AM -0700, Andrew Pinski wrote:
> > > The statement which we are gimplifying:
> > > <<<change_dynamic_type (struct __recursive_mutex *) &fake_mutex)>>>
> > >
> > > 5887            case CHANGE_DYNAMIC_TYPE_EXPR:
> > > 5888              ret = gimplify_expr (&CHANGE_DYNAMIC_TYPE_LOCATION (*expr_p),
> > > 5889                                   pre_p, post_p, is_gimple_reg, fb_lvalue);
> > >
> > >
> > > So why fb_lvalue?
> >
> > Actually this is correct, we need a decl here as we need to mark that
> > for pointing to anything.  Jakub, how did you test your patch again?
>
> I'm sorry, I bet I forgot to make in libstdc++-v3 this time, only tested
> make -C check-g++ and make -C x86_64*/libstdc++-v3.
>
> The problem is that save_expr on constant arg (which &fake_mutex is)
> does nothing at all and we really need the SAVE_EXPR around, so that
> during gimplification a temporary is created (but we can't create it
> right away in the FE, it must be initialized in the correct spot).
>
> Attached are two possible fixes, one is to force creation of SAVE_EXPR
> when it wasn't added by save_expr, the other one is to use a TARGET_EXPR
> instead (force creation thereof).
> I have briefly tested the former, the latter was tested with make
> in libstdc++-v3, make check-g++ and make check in libstdc++-v3.
>
> Ok for trunk (which one)?

I think the target expr one is better though I cannot approve the patch.

Richard.

Re: [C++ PATCH] Re: gcc trunk rev. 127646 breaks bootstrap during stage 3

[Jakub Jelinek]

On Tue, Aug 21, 2007 at 02:14:41PM +0200, Richard Guenther wrote:
> I think the target expr one is better though I cannot approve the patch.

Jason approved offline following 3rd version which I have
bootstrapped/regtested and committed:

2007-08-21  Jakub Jelinek  <jakub@redhat.com>

	* init.c (build_new_1): Use get_target_expr instead of save_expr.

	* g++.dg/init/new24.C: New test.

--- gcc/cp/init.c.jj	2007-08-21 08:13:43.000000000 +0200
+++ gcc/cp/init.c	2007-08-21 17:35:08.000000000 +0200
@@ -1755,7 +1755,7 @@ build_new_1 (tree placement, tree type, 
     placement_expr = NULL_TREE;
   else
     {
-      placement_expr = save_expr (TREE_VALUE (placement));
+      placement_expr = get_target_expr (TREE_VALUE (placement));
       placement = tree_cons (NULL_TREE, placement_expr, NULL_TREE);
     }
 
--- gcc/testsuite/g++.dg/init/new24.C.jj	2007-08-21 08:19:22.000000000 +0200
+++ gcc/testsuite/g++.dg/init/new24.C	2007-08-21 08:19:22.000000000 +0200
@@ -0,0 +1,18 @@
+// PR c++/33025
+// { dg-do compile }
+// { dg-options "-O2" }
+
+typedef __SIZE_TYPE__ size_t;
+inline void *operator new (size_t, void *p) throw () { return p; }
+extern "C" void abort ();
+
+int
+main()
+{
+  const unsigned num = 10;
+  unsigned *data = new unsigned[num];
+  unsigned *ptr = new (data) unsigned (num);
+  static unsigned data2[10];
+  unsigned *ptr2 = new (&data2[0]) unsigned (10);
+  return 0;
+}


	Jakub