[C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[Paolo Carlini]

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

Hi,

this issue should be easy to fix. Broken code like:

void foo ()
{
   decltype (auto) a = foo;
}

triggers the gcc_assert in digest_init_r:

   /* Come here only for aggregates: records, arrays, unions, complex 
numbers
      and vectors.  */
   gcc_assert (TREE_CODE (type) == ARRAY_TYPE
           || VECTOR_TYPE_P (type)
           || TREE_CODE (type) == RECORD_TYPE
           || TREE_CODE (type) == UNION_TYPE
           || TREE_CODE (type) == COMPLEX_TYPE);

because of course TREE_CODE (type) == FUNCTION_TYPE, none of the above. 
I said should be easy to fix because in fact convert_for_initialization 
is perfectly able to handle these cases and emit proper diagnostic, if 
called. What shall we do then? The patchlet below passes testing but we 
could also relax the gcc_assert itself, include FUNCTION_TYPE 
with/without checking cxx_dialect >= cxx14. We could drop the latter 
check in my patchlet. Or something else entirely.

Thanks!
Paolo.

////////////////////////

[-- Attachment #2: patch_70572 --]
[-- Type: text/plain, Size: 1295 bytes --]

Index: cp/typeck2.c
===================================================================
--- cp/typeck2.c	(revision 236400)
+++ cp/typeck2.c	(working copy)
@@ -1074,10 +1074,14 @@ digest_init_r (tree type, tree init, bool nested,
 	}
     }
 
-  /* Handle scalar types (including conversions) and references.  */
+  /* Handle scalar types (including conversions) and references.
+     Also handle cases of erroneous C++14 code involving function types
+     like (c++/70572): void foo () { decltype (auto) a = foo; }
+     and get a proper error message from convert_for_initialization.  */
   if ((TREE_CODE (type) != COMPLEX_TYPE
        || BRACE_ENCLOSED_INITIALIZER_P (init))
-      && (SCALAR_TYPE_P (type) || code == REFERENCE_TYPE))
+      && (SCALAR_TYPE_P (type) || code == REFERENCE_TYPE
+	  || (TREE_CODE (type) == FUNCTION_TYPE && cxx_dialect >= cxx14)))
     {
       if (nested)
 	flags |= LOOKUP_NO_NARROWING;
Index: testsuite/g++.dg/cpp1y/auto-fn31.C
===================================================================
--- testsuite/g++.dg/cpp1y/auto-fn31.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/auto-fn31.C	(working copy)
@@ -0,0 +1,7 @@
+// PR c++/70572
+// { dg-do compile { target c++14 } }
+
+void foo ()
+{
+  decltype (auto) a = foo;  // { dg-error "cannot convert" }
+}

Re: [C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[Jason Merrill]

On 05/18/2016 11:48 AM, Paolo Carlini wrote:
> Hi,
>
> this issue should be easy to fix. Broken code like:
>
> void foo ()
> {
>   decltype (auto) a = foo;
> }
>
> triggers the gcc_assert in digest_init_r:
>
>   /* Come here only for aggregates: records, arrays, unions, complex
> numbers
>      and vectors.  */
>   gcc_assert (TREE_CODE (type) == ARRAY_TYPE
>           || VECTOR_TYPE_P (type)
>           || TREE_CODE (type) == RECORD_TYPE
>           || TREE_CODE (type) == UNION_TYPE
>           || TREE_CODE (type) == COMPLEX_TYPE);
>
> because of course TREE_CODE (type) == FUNCTION_TYPE, none of the above.
> I said should be easy to fix because in fact convert_for_initialization
> is perfectly able to handle these cases and emit proper diagnostic, if
> called. What shall we do then? The patchlet below passes testing but we
> could also relax the gcc_assert itself, include FUNCTION_TYPE
> with/without checking cxx_dialect >= cxx14. We could drop the latter
> check in my patchlet. Or something else entirely.
>
> Thanks!
> Paolo.
>
> ////////////////////////

Shouldn't we have complained about declaring a variable with function 
type before we get here?

Jason

Re: [C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[Paolo Carlini]

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

Hi,

On 18/05/2016 23:13, Jason Merrill wrote:
> Shouldn't we have complained about declaring a variable with function 
> type before we get here?
Ah, interesting, I think all the other compilers I have at hand don't 
even try to catch the issue so early.

In any case, something as simple as the below appears to work, I'm 
finishing testing it. An earlier version didn't have the assignment of 
error_mark_node, which I added to avoid cases of redundant diagnostic 
later (eg, in check_field_decls for fields) and had a VAR_P (decl) check 
in the condition which doesn't seem necessary. What do you think?

Thanks,
Paolo.

/////////////////////

[-- Attachment #2: patch_70572_2 --]
[-- Type: text/plain, Size: 958 bytes --]

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 236433)
+++ cp/decl.c	(working copy)
@@ -6609,6 +6609,12 @@ cp_finish_decl (tree decl, tree init, bool init_co
                                                    adc_variable_type);
       if (type == error_mark_node)
 	return;
+      if (TREE_CODE (type) == FUNCTION_TYPE)
+	{
+	  error ("cannot declare variable %q+D with function type", decl);
+	  TREE_TYPE (decl) = error_mark_node;
+	  return;
+	}
       cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
     }
 
Index: testsuite/g++.dg/cpp1y/auto-fn31.C
===================================================================
--- testsuite/g++.dg/cpp1y/auto-fn31.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/auto-fn31.C	(working copy)
@@ -0,0 +1,7 @@
+// PR c++/70572
+// { dg-do compile { target c++14 } }
+
+void foo ()
+{
+  decltype (auto) a = foo;  // { dg-error "cannot declare" }
+}

Re: [C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[Jason Merrill]

On 05/18/2016 07:13 PM, Paolo Carlini wrote:
> +	  error ("cannot declare variable %q+D with function type", decl);

I think the error message would be more helpful if it mentioned 
decltype(auto), maybe

"initializer for %<decltype(auto) %D%> has function type, did you forget 
the %<()%>?", DECL_NAME (decl)

(or some other way to print the variable type as declared rather than as 
deduced).

Jason

Re: [C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[Paolo Carlini]

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

Hi,

On 19/05/2016 15:58, Jason Merrill wrote:
> On 05/18/2016 07:13 PM, Paolo Carlini wrote:
>> +      error ("cannot declare variable %q+D with function type", decl);
>
> I think the error message would be more helpful if it mentioned 
> decltype(auto), maybe
>
> "initializer for %<decltype(auto) %D%> has function type, did you 
> forget the %<()%>?", DECL_NAME (decl)
>
> (or some other way to print the variable type as declared rather than 
> as deduced).

The below passes testing. There are a few minor changes wrt your 
suggestions (I think we want & as hint; spacing consistent with 
typeck2.c; DECL_NAME doesn't seem necessary). I wondered if we want to 
tighten the condition consistently with the wording of the error 
message, thus patchlet *2 below, which of course also passes testing.

Thanks,
Paolo.

///////////////////////

[-- Attachment #2: patch_70572_3 --]
[-- Type: text/plain, Size: 999 bytes --]

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 236496)
+++ cp/decl.c	(working copy)
@@ -6609,6 +6609,13 @@ cp_finish_decl (tree decl, tree init, bool init_co
                                                    adc_variable_type);
       if (type == error_mark_node)
 	return;
+      if (TREE_CODE (type) == FUNCTION_TYPE)
+	{
+	  error ("initializer for %<decltype(auto) %D%> has function type "
+		 "(did you forget the %<&%> ?)", decl);
+	  TREE_TYPE (decl) = error_mark_node;
+	  return;
+	}
       cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
     }
 
Index: testsuite/g++.dg/cpp1y/auto-fn31.C
===================================================================
--- testsuite/g++.dg/cpp1y/auto-fn31.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/auto-fn31.C	(working copy)
@@ -0,0 +1,7 @@
+// PR c++/70572
+// { dg-do compile { target c++14 } }
+
+void foo ()
+{
+  decltype (auto) a = foo;  // { dg-error "initializer" }
+}

[-- Attachment #3: patch_70572_3b --]
[-- Type: text/plain, Size: 1054 bytes --]

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 236496)
+++ cp/decl.c	(working copy)
@@ -6609,6 +6609,14 @@ cp_finish_decl (tree decl, tree init, bool init_co
                                                    adc_variable_type);
       if (type == error_mark_node)
 	return;
+      if (TREE_CODE (type) == FUNCTION_TYPE
+	  && TREE_CODE (TREE_TYPE (d_init)) == FUNCTION_TYPE)
+	{
+	  error ("initializer for %<decltype(auto) %D%> has function type "
+		 "(did you forget the %<&%> ?)", decl);
+	  TREE_TYPE (decl) = error_mark_node;
+	  return;
+	}
       cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
     }
 
Index: testsuite/g++.dg/cpp1y/auto-fn31.C
===================================================================
--- testsuite/g++.dg/cpp1y/auto-fn31.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/auto-fn31.C	(working copy)
@@ -0,0 +1,7 @@
+// PR c++/70572
+// { dg-do compile { target c++14 } }
+
+void foo ()
+{
+  decltype (auto) a = foo;  // { dg-error "initializer" }
+}

Re: [C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[Jason Merrill]

On 05/20/2016 07:17 AM, Paolo Carlini wrote:
> The below passes testing. There are a few minor changes wrt your
> suggestions (I think we want & as hint;

I disagree; if what the user wanted was a function pointer, there's no 
reason to use decltype(auto) over plain auto.  Much more likely that 
they meant to capture the (possibly reference) result of a call.

> spacing consistent with
> typeck2.c; DECL_NAME doesn't seem necessary). I wondered if we want to
> tighten the condition consistently with the wording of the error
> message, thus patchlet *2 below, which of course also passes testing.

No, I don't think there's any way to deduce function type without the 
initializer having function type.

So, the first patch is OK with the message changed to ().

Jason

Re: [C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[Paolo Carlini]

Hi,

On 20/05/2016 17:24, Jason Merrill wrote:
> On 05/20/2016 07:17 AM, Paolo Carlini wrote:
>> The below passes testing. There are a few minor changes wrt your
>> suggestions (I think we want & as hint;
>
> I disagree; if what the user wanted was a function pointer, there's no 
> reason to use decltype(auto) over plain auto.  Much more likely that 
> they meant to capture the (possibly reference) result of a call.
... ok, if you are sure that the user (ok, novice programmer) will not 
be puzzled when he will see that decltype (auto) a = foo () also does 
not work... but I agree hints are hints, will never be 100% correct and 
informative...
>> spacing consistent with
>> typeck2.c; DECL_NAME doesn't seem necessary). I wondered if we want to
>> tighten the condition consistently with the wording of the error
>> message, thus patchlet *2 below, which of course also passes testing.
>
> No, I don't think there's any way to deduce function type without the 
> initializer having function type.
>
> So, the first patch is OK with the message changed to ().
Ok.

Paolo.

Re: [C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[Paolo Carlini]

.. shall we fix this in gcc-6-branch too or not? It's just an ICE on 
invalid but we don't emit any diagnostic before the crash.

Thanks,
Paolo.

Re: [C++ Patch/RFC] PR 70572 ("[4.9/5/6/7 Regression] ICE on code with decltype (auto) on x86_64-linux-gnu in digest_init_r")

[Jason Merrill]

On Wed, Jun 8, 2016 at 4:21 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> .. shall we fix this in gcc-6-branch too or not? It's just an ICE on invalid
> but we don't emit any diagnostic before the crash.

Sure, it should be safe enough.

Jason