[C++ RFC/Patch] PR c++/71665

[C++ RFC/Patch] PR c++/71665

[Paolo Carlini]

Hi,

we have this pretty old regression where we ICE on the invalid snippet:

class A
{
   int f ();
   enum { a = f };
};

in fact we hit the gcc_checking_assert in cxx_eval_constant_expression

     case COMPONENT_REF:
       if (is_overloaded_fn (t))
     {
       /* We can only get here in checking mode via
          build_non_dependent_expr,  because any expression that
          calls or takes the address of the function will have
          pulled a FUNCTION_DECL out of the COMPONENT_REF.  */
       gcc_checking_assert (ctx->quiet || errorcount);
       *non_constant_p = true;
       return t;
     }

which goes back to my fix for c++/58647 (that's why Jakub CC-ed me). 
Three years later some archeology is required, this is the original thread:

https://gcc.gnu.org/ml/gcc-patches/2013-11/msg03059.html

Obviously the comment above isn't (fully) correct (anymore), we don't 
get there via build_non_dependent_expr, the backtrace is simple:

#0  cxx_eval_constant_expression (ctx=0x7fffffffd170, t=0x7ffff68559c0, 
lval=false, non_constant_p=0x7fffffffd1a7, overflow_p=0x7fffffffd1a6, 
jump_target=0x0) at ../../trunk/gcc/cp/constexpr.c:3918
#1  0x0000000000929dd1 in cxx_eval_outermost_constant_expr 
(t=0x7ffff68559c0, allow_non_constant=false, strict=true, object=0x0) at 
../../trunk/gcc/cp/constexpr.c:4211
#2  0x000000000092a46f in cxx_constant_value (t=0x7ffff68559c0, 
decl=0x0) at ../../trunk/gcc/cp/constexpr.c:4309
#3  0x00000000006aebf2 in build_enumerator (name=0x7ffff69ce000, 
value=0x7ffff68559c0, enumtype=0x7ffff69bd690, attributes=0x0, 
loc=250592) at ../../trunk/gcc/cp/decl.c:13588
#4  0x00000000007c9b35 in cp_parser_enumerator_definition 
(parser=0x7ffff7fbeab0, type=0x7ffff69bd690) at 
../../trunk/gcc/cp/parser.c:17431

thus from the parser straight through build_enumerator and then 
cxx_constant_value.

Now, I suspect a fix for this issue is simple, eg, does it really make 
sense to keep the gcc_checking_assert in the face of this kind of broken 
but quite straightforward snippet? Of course ctx-quiet is false in this 
case and errorcount is still 0 but when we return from 
cxx_constant_value with such a COMPONENT_REF, build_enumerator 
immediately does:

           if (TREE_CODE (value) != INTEGER_CST
           || ! INTEGRAL_OR_ENUMERATION_TYPE_P (TREE_TYPE (value)))
         {
           error ("enumerator value for %qD is not an integer constant",
              name);
           value = NULL_TREE;
         }

and we are Ok diagnostic-wise (if we want we could say something more 
specific in build_enumerator, eg, clang says "call to non-static member 
function without an object argument", edg does not).

What do you think?

Thanks,
Paolo.

Re: [C++ RFC/Patch] PR c++/71665

[Paolo Carlini]

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

Hi Jason,

and sorry about the delay in following up, a few days of vacations...

On 30/06/2016 19:49, Jason Merrill wrote:
>
> I think we should check the type before calling cxx_constant_value.
>
Ok, I got the point. I'm not sure however how far we want to go with 
this and which kind of consistency we want to achieve (vs error messages 
issued in other similar circumstances). The below certainly passes 
testing on x86_64-linux.

Thanks,
Paolo.

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


[-- Attachment #2: patch_71665 --]
[-- Type: text/plain, Size: 901 bytes --]

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 238239)
+++ cp/decl.c	(working copy)
@@ -13585,7 +13585,9 @@ build_enumerator (tree name, tree value, tree enum
 
 	  if (value != NULL_TREE)
 	    {
-	      value = cxx_constant_value (value);
+	      if (TREE_CODE (value) != COMPONENT_REF
+		  || !is_overloaded_fn (value))
+		value = cxx_constant_value (value);
 
 	      if (TREE_CODE (value) != INTEGER_CST
 		  || ! INTEGRAL_OR_ENUMERATION_TYPE_P (TREE_TYPE (value)))
Index: testsuite/g++.dg/cpp0x/pr71665.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr71665.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/pr71665.C	(working copy)
@@ -0,0 +1,8 @@
+// PR c++/71665
+// { dg-do compile { target c++11 } }
+
+class A 
+{
+  int f ();
+  enum { a = f }; // { dg-error "enumerator value" }
+};

Re: [C++ RFC/Patch] PR c++/71665

[Jason Merrill]

On Tue, Jul 12, 2016 at 10:30 AM, Paolo Carlini
<paolo.carlini@oracle.com> wrote:
> On 30/06/2016 19:49, Jason Merrill wrote:
>> I think we should check the type before calling cxx_constant_value.
>>
> Ok, I got the point. I'm not sure however how far we want to go with this
> and which kind of consistency we want to achieve (vs error messages issued
> in other similar circumstances). The below certainly passes testing on
> x86_64-linux.

I meant the actual type of the expression: that is, check
INTEGRAL_OR_ENUMERATION_TYPE_P before calling cxx_constant_value.

Jason

Re: [C++ RFC/Patch] PR c++/71665

[Paolo Carlini]

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

Hi,

On 18/07/2016 20:16, Jason Merrill wrote:
> On Tue, Jul 12, 2016 at 10:30 AM, Paolo Carlini
> <paolo.carlini@oracle.com> wrote:
>> On 30/06/2016 19:49, Jason Merrill wrote:
>>> I think we should check the type before calling cxx_constant_value.
>>>
>> Ok, I got the point. I'm not sure however how far we want to go with this
>> and which kind of consistency we want to achieve (vs error messages issued
>> in other similar circumstances). The below certainly passes testing on
>> x86_64-linux.
> I meant the actual type of the expression: that is, check
> INTEGRAL_OR_ENUMERATION_TYPE_P before calling cxx_constant_value.
Ah sorry, I missed the *type* bit. The below passes testing on 
x86_64-linux. I don't think we need to check the type again after 
cxx_constant_value?!?

While finally spending a decent amount of time on this issue I noticed 
that current clang appears to enforce integral or *unscoped* enumeration 
type and tweaking our code in the obvious way doesn't cause regressions, 
we of course reject earlier (ie, not as "could not convert ‘(E)1’ from 
‘E’ to ‘unsigned int’") in build_enumeraror snippets like:

enum class E { e = 1 };

class A
{
   enum { a = E::e };
};

I didn't investigate the issue further, however.

Thanks,
Paolo.

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


[-- Attachment #2: patch_71665_2 --]
[-- Type: text/plain, Size: 2970 bytes --]

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 238807)
+++ cp/decl.c	(working copy)
@@ -13587,15 +13587,23 @@ build_enumerator (tree name, tree value, tree enum
 
 	  if (value != NULL_TREE)
 	    {
-	      value = cxx_constant_value (value);
-
-	      if (TREE_CODE (value) != INTEGER_CST
-		  || ! INTEGRAL_OR_ENUMERATION_TYPE_P (TREE_TYPE (value)))
+	      if (! INTEGRAL_OR_ENUMERATION_TYPE_P (TREE_TYPE (value)))
 		{
-		  error ("enumerator value for %qD is not an integer constant",
-			 name);
+		  error ("enumerator for %qD must have integral or "
+			 "enumeration type", name);
 		  value = NULL_TREE;
 		}
+	      else
+		{
+		  value = cxx_constant_value (value);
+
+		  if (TREE_CODE (value) != INTEGER_CST)
+		    {
+		      error ("enumerator value for %qD is not an integer "
+			     "constant", name);
+		      value = NULL_TREE;
+		    }
+		}
 	    }
 	}
 
Index: testsuite/g++.dg/cpp0x/enum29.C
===================================================================
--- testsuite/g++.dg/cpp0x/enum29.C	(revision 238807)
+++ testsuite/g++.dg/cpp0x/enum29.C	(working copy)
@@ -38,7 +38,7 @@ enum E0 { e0 = X0() };
 enum E1 { e1 = X1() };
 enum E2 { e2 = X2() };
 enum E3 { e3 = X3() };
-enum E4 { e4 = X4() };  // { dg-error "integer constant" }
+enum E4 { e4 = X4() };  // { dg-error "integral" }
 enum E5 { e5 = X5() };  // { dg-error "ambiguous" }
 
 enum F0 : int { f0 = X0() };
Index: testsuite/g++.dg/cpp0x/pr71665.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr71665.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/pr71665.C	(working copy)
@@ -0,0 +1,8 @@
+// PR c++/71665
+// { dg-do compile { target c++11 } }
+
+class A 
+{
+  int f ();
+  enum { a = f }; // { dg-error "enumerator" }
+};
Index: testsuite/g++.dg/ext/label10.C
===================================================================
--- testsuite/g++.dg/ext/label10.C	(revision 238807)
+++ testsuite/g++.dg/ext/label10.C	(working copy)
@@ -4,7 +4,7 @@
 
 template<int N> struct A
 {
-  enum { M = && N };	// { dg-error "referenced outside|cannot appear in|not an integer constant" }
+  enum { M = && N };	// { dg-error "referenced outside|cannot appear in|integral" }
 };
 
 A<0> a;
@@ -12,6 +12,6 @@ A<0> a;
 void foo ()
 {
   __label__ P;
-  enum { O = && P };	// { dg-error "cannot appear in|not an integer constant" }
+  enum { O = && P };	// { dg-error "cannot appear in|integral" }
   P:;
 }
Index: testsuite/g++.dg/parse/constant5.C
===================================================================
--- testsuite/g++.dg/parse/constant5.C	(revision 238807)
+++ testsuite/g++.dg/parse/constant5.C	(working copy)
@@ -1,7 +1,7 @@
 // { dg-options "-std=c++98 -pedantic-errors" }
 
 enum E { 
-  a = 24.2, // { dg-error "constant" }
+  a = 24.2, // { dg-error "integral|constant" }
   b = (int)3.7, 
   c = int(4.2),
   d = (int)(4.2 + 3.7), // { dg-error "constant" }

Re: [C++ RFC/Patch] PR c++/71665

[Jason Merrill]

On Thu, Jul 28, 2016 at 7:48 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Ah sorry, I missed the *type* bit. The below passes testing on x86_64-linux.
> I don't think we need to check the type again after cxx_constant_value?!?

No, we don't.  The patch is OK.

> While finally spending a decent amount of time on this issue I noticed that
> current clang appears to enforce integral or *unscoped* enumeration type and
> tweaking our code in the obvious way doesn't cause regressions, we of course
> reject earlier (ie, not as "could not convert ‘(E)1’ from ‘E’ to ‘unsigned
> int’") in build_enumerator snippets like:
>
> enum class E { e = 1 };
>
> class A
> {
>   enum { a = E::e };
> };

Sure, that change could improve diagnostic quality a bit.

Jason

Re: [C++ RFC/Patch] PR c++/71665

[Paolo Carlini]

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

Hi,

On 28/07/2016 16:28, Jason Merrill wrote:
> On Thu, Jul 28, 2016 at 7:48 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Ah sorry, I missed the *type* bit. The below passes testing on x86_64-linux.
>> I don't think we need to check the type again after cxx_constant_value?!?
> No, we don't.  The patch is OK.
>
>> While finally spending a decent amount of time on this issue I noticed that
>> current clang appears to enforce integral or *unscoped* enumeration type and
>> tweaking our code in the obvious way doesn't cause regressions, we of course
>> reject earlier (ie, not as "could not convert ‘(E)1’ from ‘E’ to ‘unsigned
>> int’") in build_enumerator snippets like:
>>
>> enum class E { e = 1 };
>>
>> class A
>> {
>>    enum { a = E::e };
>> };
> Sure, that change could improve diagnostic quality a bit.
Thanks Jason. Then I'm regression testing again the below and I mean to 
commit it later today.

Thanks again,
Paolo.

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

[-- Attachment #2: CL_71665_3 --]
[-- Type: text/plain, Size: 450 bytes --]

/cp
2016-07-28  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/71665
	* decl.c (build_enumerator): Check the type of the enumerator before
	calling cxx_constant_value.

/testsuite
2016-07-28  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/71665
	* g++.dg/cpp0x/pr71665-1.C: New.
	* g++.dg/cpp0x/pr71665-2.C: Likewise.
	* g++.dg/cpp0x/enum29.C: Adjust dg-error string.
	* g++.dg/ext/label10.C: Likewise.
	* g++.dg/parse/constant5.C: Likewise.

[-- Attachment #3: patch_71665_3 --]
[-- Type: text/plain, Size: 3407 bytes --]

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 238822)
+++ cp/decl.c	(working copy)
@@ -13587,15 +13587,24 @@ build_enumerator (tree name, tree value, tree enum
 
 	  if (value != NULL_TREE)
 	    {
-	      value = cxx_constant_value (value);
-
-	      if (TREE_CODE (value) != INTEGER_CST
-		  || ! INTEGRAL_OR_ENUMERATION_TYPE_P (TREE_TYPE (value)))
+	      if (! INTEGRAL_OR_UNSCOPED_ENUMERATION_TYPE_P
+		  (TREE_TYPE (value)))
 		{
-		  error ("enumerator value for %qD is not an integer constant",
-			 name);
+		  error ("enumerator for %qD must have integral or "
+			 "unscoped enumeration type", name);
 		  value = NULL_TREE;
 		}
+	      else
+		{
+		  value = cxx_constant_value (value);
+
+		  if (TREE_CODE (value) != INTEGER_CST)
+		    {
+		      error ("enumerator value for %qD is not an integer "
+			     "constant", name);
+		      value = NULL_TREE;
+		    }
+		}
 	    }
 	}
 
Index: testsuite/g++.dg/cpp0x/enum29.C
===================================================================
--- testsuite/g++.dg/cpp0x/enum29.C	(revision 238822)
+++ testsuite/g++.dg/cpp0x/enum29.C	(working copy)
@@ -38,7 +38,7 @@ enum E0 { e0 = X0() };
 enum E1 { e1 = X1() };
 enum E2 { e2 = X2() };
 enum E3 { e3 = X3() };
-enum E4 { e4 = X4() };  // { dg-error "integer constant" }
+enum E4 { e4 = X4() };  // { dg-error "integral" }
 enum E5 { e5 = X5() };  // { dg-error "ambiguous" }
 
 enum F0 : int { f0 = X0() };
Index: testsuite/g++.dg/cpp0x/pr71665-1.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr71665-1.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/pr71665-1.C	(working copy)
@@ -0,0 +1,8 @@
+// PR c++/71665
+// { dg-do compile { target c++11 } }
+
+class A 
+{
+  int f ();
+  enum { a = f }; // { dg-error "enumerator" }
+};
Index: testsuite/g++.dg/cpp0x/pr71665-2.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr71665-2.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/pr71665-2.C	(working copy)
@@ -0,0 +1,8 @@
+// PR c++/71665
+// { dg-do compile { target c++11 } }
+
+class A 
+{
+  enum class E { e = 1 };
+  enum { a = E::e };  // { dg-error "integral or unscoped enumeration" }
+};
Index: testsuite/g++.dg/ext/label10.C
===================================================================
--- testsuite/g++.dg/ext/label10.C	(revision 238822)
+++ testsuite/g++.dg/ext/label10.C	(working copy)
@@ -4,7 +4,7 @@
 
 template<int N> struct A
 {
-  enum { M = && N };	// { dg-error "referenced outside|cannot appear in|not an integer constant" }
+  enum { M = && N };	// { dg-error "referenced outside|cannot appear in|integral" }
 };
 
 A<0> a;
@@ -12,6 +12,6 @@ A<0> a;
 void foo ()
 {
   __label__ P;
-  enum { O = && P };	// { dg-error "cannot appear in|not an integer constant" }
+  enum { O = && P };	// { dg-error "cannot appear in|integral" }
   P:;
 }
Index: testsuite/g++.dg/parse/constant5.C
===================================================================
--- testsuite/g++.dg/parse/constant5.C	(revision 238822)
+++ testsuite/g++.dg/parse/constant5.C	(working copy)
@@ -1,7 +1,7 @@
 // { dg-options "-std=c++98 -pedantic-errors" }
 
 enum E { 
-  a = 24.2, // { dg-error "constant" }
+  a = 24.2, // { dg-error "integral|constant" }
   b = (int)3.7, 
   c = int(4.2),
   d = (int)(4.2 + 3.7), // { dg-error "constant" }