diff mbox series

[C++,PING,C++] PR 85065 ("[concepts] ICE with invalid use of a concept")

Message ID 93851680-6ba2-cdec-4dc0-e2dd9f5767cd@oracle.com
State New
Headers show
Series [C++,PING,C++] PR 85065 ("[concepts] ICE with invalid use of a concept") | expand

Commit Message

Paolo Carlini Sept. 17, 2018, 5:53 p.m. UTC 
Hi again,

On 9/3/18 10:59 PM, Paolo Carlini wrote:
> in this error-recovery ICE, upon the error make_constrained_auto 
> assigns error_mark_node to PLACEHOLDER_TYPE_CONSTRAINTS (type) which 
> then causes a crash later when hash_placeholder_constraint is called 
> on it. I think we should cope with this somehow, I believe that 
> consistency with the way we use error_mark_node in this part of the 
> front-end prevents us from avoiding to assign the error_mark_node in 
> the first place and, for the reasons explained in my previous patch, 
> we want to unconditionally call make_constrained_auto. This said, 
> catching in practice the error_mark_node would normally mean 
> renouncing to the pattern 'if (tree c = ...)' which we lately appear 
> to like a lot and seems indeed neat. Thus I'm wondering if we want 
> instead to add a macro like ERROR_AS_NULL, which of course would be 
> also useful in many other places - essentially in all the 
> circumstances where we want to check for a kosher node, thus neither 
> null nor error_mark_node. What do you think? What about the name, in 
> case? Tested x86_64-linux.

Today I reviewed again this issue, for which I sent a tentative patch a 
couple of weeks ago. All in all, I still believe that is the right place 
to catch the error_mark_node and avoid ICE-ing later, the quick 
rationale being that PLACEHOLDER_TYPE_CONSTRAINTS can be error_mark_node 
for other reasons too. As regards the ERROR_AS_NULL idea, I'm still not 
sure, on one hand it would allow for more compact and neat code in some 
cases, on the other hand could be seen as some sort of obfuscation - 
well, some people out there consider an obfuscation the very 'if (c 
=...)' pattern ;) Anyway, I'm attaching the normal versions of the fix, 
which, per a recent message from Jason, probably is almost obvious...

Thanks, Paolo.

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

Comments

Jason Merrill Sept. 18, 2018, 1:12 p.m. UTC | #1 
On Mon, Sep 17, 2018 at 1:53 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi again,
>
> On 9/3/18 10:59 PM, Paolo Carlini wrote:
>>
>> in this error-recovery ICE, upon the error make_constrained_auto assigns
>> error_mark_node to PLACEHOLDER_TYPE_CONSTRAINTS (type) which then causes a
>> crash later when hash_placeholder_constraint is called on it. I think we
>> should cope with this somehow, I believe that consistency with the way we
>> use error_mark_node in this part of the front-end prevents us from avoiding
>> to assign the error_mark_node in the first place and, for the reasons
>> explained in my previous patch, we want to unconditionally call
>> make_constrained_auto. This said, catching in practice the error_mark_node
>> would normally mean renouncing to the pattern 'if (tree c = ...)' which we
>> lately appear to like a lot and seems indeed neat. Thus I'm wondering if we
>> want instead to add a macro like ERROR_AS_NULL, which of course would be
>> also useful in many other places - essentially in all the circumstances
>> where we want to check for a kosher node, thus neither null nor
>> error_mark_node. What do you think? What about the name, in case? Tested
>> x86_64-linux.
>
>
> Today I reviewed again this issue, for which I sent a tentative patch a
> couple of weeks ago. All in all, I still believe that is the right place to
> catch the error_mark_node and avoid ICE-ing later, the quick rationale being
> that PLACEHOLDER_TYPE_CONSTRAINTS can be error_mark_node for other reasons
> too. As regards the ERROR_AS_NULL idea, I'm still not sure, on one hand it
> would allow for more compact and neat code in some cases, on the other hand
> could be seen as some sort of obfuscation - well, some people out there
> consider an obfuscation the very 'if (c =...)' pattern ;) Anyway, I'm
> attaching the normal versions of the fix, which, per a recent message from
> Jason, probably is almost obvious...

Hmm, I do kind of like the ERROR_AS_NULL idea.  I might call it
NON_ERROR, though.  OK with that change.

Jason
diff mbox series

Patch

Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 264362)
+++ cp/pt.c	(working copy)
@@ -26121,7 +26121,8 @@  struct auto_hash : default_hash_traits<tree>
 inline hashval_t
 auto_hash::hash (tree t)
 {
-  if (tree c = PLACEHOLDER_TYPE_CONSTRAINTS (t))
+  tree c = PLACEHOLDER_TYPE_CONSTRAINTS (t);
+  if (c && c != error_mark_node)
     /* Matching constrained-type-specifiers denote the same template
        parameter, so hash the constraint.  */
     return hash_placeholder_constraint (c);
@@ -26880,50 +26881,53 @@  do_auto_deduction (tree type, tree init, tree auto
 
   /* Check any placeholder constraints against the deduced type. */
   if (flag_concepts && !processing_template_decl)
-    if (tree constr = PLACEHOLDER_TYPE_CONSTRAINTS (auto_node))
-      {
-        /* Use the deduced type to check the associated constraints. If we
-           have a partial-concept-id, rebuild the argument list so that
-           we check using the extra arguments. */
-        gcc_assert (TREE_CODE (constr) == CHECK_CONSTR);
-        tree cargs = CHECK_CONSTR_ARGS (constr);
-        if (TREE_VEC_LENGTH (cargs) > 1)
-          {
-            cargs = copy_node (cargs);
-            TREE_VEC_ELT (cargs, 0) = TREE_VEC_ELT (targs, 0);
-          }
-        else
-          cargs = targs;
-        if (!constraints_satisfied_p (constr, cargs))
-          {
-            if (complain & tf_warning_or_error)
-              {
-		auto_diagnostic_group d;
-                switch (context)
-                  {
-                  case adc_unspecified:
-		  case adc_unify:
-                    error("placeholder constraints not satisfied");
-                    break;
-                  case adc_variable_type:
-		  case adc_decomp_type:
-                    error ("deduced initializer does not satisfy "
-                           "placeholder constraints");
-                    break;
-                  case adc_return_type:
-                    error ("deduced return type does not satisfy "
-                           "placeholder constraints");
-                    break;
-                  case adc_requirement:
-		    error ("deduced expression type does not satisfy "
-                           "placeholder constraints");
-                    break;
-                  }
-                diagnose_constraints (input_location, constr, targs);
-              }
-            return error_mark_node;
-          }
-      }
+    {
+      tree constr = PLACEHOLDER_TYPE_CONSTRAINTS (auto_node);
+      if (constr && constr != error_mark_node)
+	{
+	  /* Use the deduced type to check the associated constraints. If we
+	     have a partial-concept-id, rebuild the argument list so that
+	     we check using the extra arguments. */
+	  gcc_assert (TREE_CODE (constr) == CHECK_CONSTR);
+	  tree cargs = CHECK_CONSTR_ARGS (constr);
+	  if (TREE_VEC_LENGTH (cargs) > 1)
+	    {
+	      cargs = copy_node (cargs);
+	      TREE_VEC_ELT (cargs, 0) = TREE_VEC_ELT (targs, 0);
+	    }
+	  else
+	    cargs = targs;
+	  if (!constraints_satisfied_p (constr, cargs))
+	    {
+	      if (complain & tf_warning_or_error)
+		{
+		  auto_diagnostic_group d;
+		  switch (context)
+		    {
+		    case adc_unspecified:
+		    case adc_unify:
+		      error("placeholder constraints not satisfied");
+		      break;
+		    case adc_variable_type:
+		    case adc_decomp_type:
+		      error ("deduced initializer does not satisfy "
+			     "placeholder constraints");
+		      break;
+		    case adc_return_type:
+		      error ("deduced return type does not satisfy "
+			     "placeholder constraints");
+		      break;
+		    case adc_requirement:
+		      error ("deduced expression type does not satisfy "
+			     "placeholder constraints");
+		      break;
+		    }
+		  diagnose_constraints (input_location, constr, targs);
+		}
+	      return error_mark_node;
+	    }
+	}
+    }
 
   if (processing_template_decl && context != adc_unify)
     outer_targs = current_template_args ();
Index: testsuite/g++.dg/concepts/pr85065.C
===================================================================
--- testsuite/g++.dg/concepts/pr85065.C	(nonexistent)
+++ testsuite/g++.dg/concepts/pr85065.C	(working copy)
@@ -0,0 +1,6 @@ 
+// { dg-do compile { target c++14 } }
+// { dg-additional-options "-fconcepts" }
+
+template<int> concept bool C = true;
+
+C c = 0;  // { dg-error "invalid reference to concept" }