[Ping] [Patch C++] RFC Fix PR57958

[Ping] [Patch C++] RFC Fix PR57958

[Dinar Temirbulatov]

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

Hi,
I revised the patch from
http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00083.html.
This change fixes PR57958 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57958)
For this code:
auto fn = [] (const Foo<Data>& x) {
    return (x);
  };

  {
    Foo<Data> a;
    fn(a);
  }
Current version of trunk generates following gimple:

main(int, char**)::<lambda(const Foo<Data>&)> (const struct __lambda0
* const __closure, const struct Foo & x)
{
  struct Foo D.2089;

  Foo<Data>::Foo (&D.2089, x);
  try
    {
      return <retval>;
    }
  finally
    {
      Foo<Data>::~Foo (&D.2089);
      D.2089 = {CLOBBER};
    }
}

And after fix applied gimple product looks like this:

main(int, char**)::<lambda(const Foo<Data>&)> (const struct __lambda0
* const __closure, const struct Foo & x)
{
  Foo<Data>::Foo (<retval>, x);
  return <retval>;
}

looking at the code that I think caused the issue:
cp/typeck.c:8600
      /* We can't initialize a register from a AGGR_INIT_EXPR.  */
      else if (! cfun->returns_struct
               && TREE_CODE (retval) == TARGET_EXPR
               && TREE_CODE (TREE_OPERAND (retval, 1)) == AGGR_INIT_EXPR)
        retval = build2 (COMPOUND_EXPR, TREE_TYPE (retval), retval,
                         TREE_OPERAND (retval, 0));
It is not clear to me the propose of constructing COMPOUND_EXPR here,
comment says: "We can't initialize a register from a AGGR_INIT_EXPR.",
but how on the front-end side we could determine what is going to end
up in a register and what is not? The code looks like a hack for some
middle-end issue awhile back?  I hoped to trigger any code generation
problem by removing the code and testing regression testsuite on
x86_64-pc-linux-gnu, but no regression have occured. Attached patch
was tested on x86_64-pc-linux-gnu with no new regressions. Ok to apply
to trunk?
                         Thanks in advance, Dinar.

[-- Attachment #2: PR57958.patch --]
[-- Type: application/octet-stream, Size: 1397 bytes --]

diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 5fc0e6b..082a5f5 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -8593,13 +8593,7 @@ check_return_expr (tree retval, bool *no_warning)
       /* If the conversion failed, treat this just like `return;'.  */
       if (retval == error_mark_node)
 	return retval;
-      /* We can't initialize a register from a AGGR_INIT_EXPR.  */
-      else if (! cfun->returns_struct
-	       && TREE_CODE (retval) == TARGET_EXPR
-	       && TREE_CODE (TREE_OPERAND (retval, 1)) == AGGR_INIT_EXPR)
-	retval = build2 (COMPOUND_EXPR, TREE_TYPE (retval), retval,
-			 TREE_OPERAND (retval, 0));
-      else
+      else 
 	maybe_warn_about_returning_address_of_local (retval);
     }
 
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr57958.C b/gcc/testsuite/g++.dg/cpp1y/pr57958.C
new file mode 100644
index 0000000..c9c99e8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr57958.C
@@ -0,0 +1,40 @@
+// { dg-options -std=c++11 }
+// { dg-do run }
+
+#define assert(E) if(!(E))__builtin_abort();
+
+int n = 0;
+
+template <class T>
+class Foo {
+ public:
+  Foo() { 
+   n--; 
+  }
+  Foo(const Foo&) { 
+   n--; 
+  }
+  ~Foo() { 
+   n++; 
+  }
+};
+
+struct Data {};
+
+void a()
+{
+	Data b;
+}
+
+int main(int argc, char *argv[]) {
+  auto fn = [] (const Foo<Data>& x) {
+    return (x);
+  };
+
+  {
+    Foo<Data> a;
+    fn(a);
+  }
+
+  assert(n == 0);
+}

Re: [Ping] [Patch C++] RFC Fix PR57958

[Jason Merrill]

On 04/07/2014 12:56 PM, Dinar Temirbulatov wrote:
>       /* We can't initialize a register from a AGGR_INIT_EXPR.  */
>       else if (! cfun->returns_struct
>                && TREE_CODE (retval) == TARGET_EXPR
>                && TREE_CODE (TREE_OPERAND (retval, 1)) == AGGR_INIT_EXPR)
>         retval = build2 (COMPOUND_EXPR, TREE_TYPE (retval), retval,
>                          TREE_OPERAND (retval, 0));

That code is extremely ancient.  I agree that it seems kind of useless, 
but the fact that we're hitting it indicates a bug: cfun->returns_struct 
should have been set by apply_deduced_return_type.  I guess we need to 
call complete_type before aggregate_value_p.  Let's fix that now, and 
then remove this chunk of code after 4.9 branches.

> +++ b/gcc/testsuite/g++.dg/cpp1y/pr57958.C

The cpp1y directory is for C++14 tests; C++11 tests should go in cpp0x. 
  Perhaps we should rename the directories.  :)

> +// { dg-options -std=c++11 }
> +// { dg-do run }

Please use { dg-do run { target c++11 } } rather than specify -std in 
dg-options.

Jason

Re: [Ping] [Patch C++] RFC Fix PR57958

[Jason Merrill]

On 04/07/2014 03:46 PM, Jason Merrill wrote:
> I guess we need to call complete_type before aggregate_value_p.

complete_type_or_else, actually.

Jason

Re: [Ping] [Patch C++] RFC Fix PR57958

[Dinar Temirbulatov]

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

Hello,
Here is another version of fix. This time, I added
complete_type_or_else call just before aggregate_value_p. Bootstraped
and tested on x86_64-pc-linux-gnu with no new regressions.  Ok to
apply
to trunk?
                         Thanks, Dinar.


On Mon, Apr 7, 2014 at 11:47 PM, Jason Merrill <jason@redhat.com> wrote:
> On 04/07/2014 03:46 PM, Jason Merrill wrote:
>>
>> I guess we need to call complete_type before aggregate_value_p.
>
>
> complete_type_or_else, actually.
>
> Jason
>
>

[-- Attachment #2: Changelog.txt --]
[-- Type: text/plain, Size: 190 bytes --]

2014-04-18  Dinar Temirbulatov  <dtemirbulatov@gmail.com>

	PR c++/57958
	* semantics.c (apply_deduced_return_type): Complete non-void type
	before estimating whether the type is aggregate.

[-- Attachment #3: fix.patch --]
[-- Type: application/octet-stream, Size: 1225 bytes --]

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 9fb4fc0..72cbe98 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -10627,6 +10627,8 @@ apply_deduced_return_type (tree fco, tree return_type)
 
   if (!processing_template_decl)
     {
+      if (TREE_CODE (TREE_TYPE(result)) != VOID_TYPE)
+      	complete_type_or_else (TREE_TYPE(result), NULL_TREE);
       bool aggr = aggregate_value_p (result, fco);
 #ifdef PCC_STATIC_STRUCT_RETURN
       cfun->returns_pcc_struct = aggr;
diff -ruNp a/gcc/testsuite/g++.dg/cpp0x/pr57958.C b/gcc/testsuite/g++.dg/cpp0x/pr57958.C
--- a/gcc/testsuite/g++.dg/cpp0x/pr57958.C	1970-01-01 03:00:00.000000000 +0300
+++ b/gcc/testsuite/g++.dg/cpp0x/pr57958.C	2014-04-07 18:29:40.510479277 +0400
@@ -0,0 +1,40 @@
+// { dg-options -std=c++11 }
+// { dg-do run }
+
+#define assert(E) if(!(E))__builtin_abort();
+
+int n = 0;
+
+template <class T>
+class Foo {
+ public:
+  Foo() { 
+   n--; 
+  }
+  Foo(const Foo&) { 
+   n--; 
+  }
+  ~Foo() { 
+   n++; 
+  }
+};
+
+struct Data {};
+
+void a()
+{
+	Data b;
+}
+
+int main(int argc, char *argv[]) {
+  auto fn = [] (const Foo<Data>& x) {
+    return (x);
+  };
+
+  {
+    Foo<Data> a;
+    fn(a);
+  }
+
+  assert(n == 0);
+}

Re: [Ping] [Patch C++] RFC Fix PR57958

[Dinar Temirbulatov]

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

I found typo in the testcase header, fixed. Ok to apply to trunk?
                thanks, Dinar.

On Fri, Apr 18, 2014 at 1:13 PM, Dinar Temirbulatov
<dtemirbulatov@gmail.com> wrote:
> Hello,
> Here is another version of fix. This time, I added
> complete_type_or_else call just before aggregate_value_p. Bootstraped
> and tested on x86_64-pc-linux-gnu with no new regressions.  Ok to
> apply
> to trunk?
>                          Thanks, Dinar.
>
>
> On Mon, Apr 7, 2014 at 11:47 PM, Jason Merrill <jason@redhat.com> wrote:
>> On 04/07/2014 03:46 PM, Jason Merrill wrote:
>>>
>>> I guess we need to call complete_type before aggregate_value_p.
>>
>>
>> complete_type_or_else, actually.
>>
>> Jason
>>
>>

[-- Attachment #2: Changelog.txt --]
[-- Type: text/plain, Size: 190 bytes --]

2014-04-18  Dinar Temirbulatov  <dtemirbulatov@gmail.com>

	PR c++/57958
	* semantics.c (apply_deduced_return_type): Complete non-void type
	before estimating whether the type is aggregate.

[-- Attachment #3: fix.patch --]
[-- Type: application/octet-stream, Size: 1212 bytes --]

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 9fb4fc0..72cbe98 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -10627,6 +10627,8 @@ apply_deduced_return_type (tree fco, tree return_type)
 
   if (!processing_template_decl)
     {
+      if (TREE_CODE (TREE_TYPE(result)) != VOID_TYPE)
+      	complete_type_or_else (TREE_TYPE(result), NULL_TREE);
       bool aggr = aggregate_value_p (result, fco);
 #ifdef PCC_STATIC_STRUCT_RETURN
       cfun->returns_pcc_struct = aggr;
diff -ruNp a/gcc/testsuite/g++.dg/cpp0x/pr57958.C b/gcc/testsuite/g++.dg/cpp0x/pr57958.C
--- a/gcc/testsuite/g++.dg/cpp0x/pr57958.C	1970-01-01 03:00:00.000000000 +0300
+++ b/gcc/testsuite/g++.dg/cpp0x/pr57958.C	2014-04-18 16:40:27.270374461 +0400
@@ -0,0 +1,39 @@
+// { dg-do run { target c++11 } }
+
+#define assert(E) if(!(E))__builtin_abort();
+
+int n = 0;
+
+template <class T>
+class Foo {
+ public:
+  Foo() { 
+   n--; 
+  }
+  Foo(const Foo&) { 
+   n--; 
+  }
+  ~Foo() { 
+   n++; 
+  }
+};
+
+struct Data {};
+
+void a()
+{
+	Data b;
+}
+
+int main(int argc, char *argv[]) {
+  auto fn = [] (const Foo<Data>& x) {
+    return (x);
+  };
+
+  {
+    Foo<Data> a;
+    fn(a);
+  }
+
+  assert(n == 0);
+}

Re: [Ping] [Patch C++] RFC Fix PR57958

[Jason Merrill]

On 04/18/2014 08:53 AM, Dinar Temirbulatov wrote:
> +      if (TREE_CODE (TREE_TYPE(result)) != VOID_TYPE)

Space after TREE_TYPE.  OK with that change.

Jason

Re: [Ping] [Patch C++] RFC Fix PR57958

[Paolo Carlini]

Hi,

On 18 aprile 2014 17:20:40 CEST, Jason Merrill <jason@redhat.com> wrote:
>On 04/18/2014 08:53 AM, Dinar Temirbulatov wrote:
>> +      if (TREE_CODE (TREE_TYPE(result)) != VOID_TYPE)
>
>Space after TREE_TYPE.  OK with that change.

I'm traveling and I can't really check, but I seem to remember that we do have a VOID_TYPE_P.

Paolo

Re: [Ping] [Patch C++] RFC Fix PR57958

[Jason Merrill]

On 04/18/2014 11:39 AM, Paolo Carlini wrote:
> I'm traveling and I can't really check, but I seem to remember that we do have a VOID_TYPE_P.

True, I suppose that would be preferable.

Jason