[C++] PR 81054 ("[7/8 Regression] ICE with volatile variable in constexpr function") [Take 2]

Message ID 9dc00709-5f5e-6bf3-1ece-999edade8a84@oracle.com
State New
Headers show
Series
  • [C++] PR 81054 ("[7/8 Regression] ICE with volatile variable in constexpr function") [Take 2]
Related show

Commit Message

Paolo Carlini Jan. 16, 2018, 8:32 p.m.
Hi again,

thus I figured out what was badly wrong in my first try: I misread 
ensure_literal_type_for_constexpr_object and missed that it can return 
NULL_TREE without emitting an hard error. Thus my first try even caused 
miscompilations :( Anyway, when DECL_DECLARED_CONSTEXPR_P is true we are 
safe and indeed we want to clear it as matter of error recovery. Then, 
in this safe case the only change in the below is returning early, thus 
avoiding any internal inconsistencies later and also the redundant / 
misleading diagnostic which I already mentioned. Testing on x86_64-linux 
is still in progress - in libstdc++ - but I separately checked that all 
the regressions are gone.

Thanks! Paolo.

/////////////////////
/cp
2018-01-16  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/81054
	* decl.c (cp_finish_decl): Early return when
	ensure_literal_type_for_constexpr_object returns NULL_TREE 
	and DECL_DECLARED_CONSTEXPR_P is true.

/testsuite
2018-01-16  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/81054
	* g++.dg/cpp0x/constexpr-ice19.C: New.

Comments

Jason Merrill Jan. 16, 2018, 9:35 p.m. | #1
On Tue, Jan 16, 2018 at 3:32 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> thus I figured out what was badly wrong in my first try: I misread

> ensure_literal_type_for_constexpr_object and missed that it can return

> NULL_TREE without emitting an hard error. Thus my first try even caused

> miscompilations :( Anyway, when DECL_DECLARED_CONSTEXPR_P is true we are

> safe and indeed we want to clear it as matter of error recovery. Then, in

> this safe case the only change in the below is returning early, thus

> avoiding any internal inconsistencies later and also the redundant /

> misleading diagnostic which I already mentioned.


I can't see how this could be right.  In the cases where we don't give
an error (e.g. because we're dealing with an instantiation of a
variable template) there is no error, so we need to proceed with the
rest of cp_finish_decl as normal.

Jason
Paolo Carlini Jan. 16, 2018, 9:40 p.m. | #2
Hi Jason

On 16/01/2018 22:35, Jason Merrill wrote:
> On Tue, Jan 16, 2018 at 3:32 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:

>> thus I figured out what was badly wrong in my first try: I misread

>> ensure_literal_type_for_constexpr_object and missed that it can return

>> NULL_TREE without emitting an hard error. Thus my first try even caused

>> miscompilations :( Anyway, when DECL_DECLARED_CONSTEXPR_P is true we are

>> safe and indeed we want to clear it as matter of error recovery. Then, in

>> this safe case the only change in the below is returning early, thus

>> avoiding any internal inconsistencies later and also the redundant /

>> misleading diagnostic which I already mentioned.

> I can't see how this could be right.  In the cases where we don't give

> an error (e.g. because we're dealing with an instantiation of a

> variable template) there is no error, so we need to proceed with the

> rest of cp_finish_decl as normal.

The cases where we don't give an error all fall under 
DECL_DECLARED_CONSTEXPR_P == false, thus aren't affected at all.

Unless I'm again misreading ensure_literal_type_for_constexpr_object, I 
hope not.

Paolo.
Paolo Carlini Jan. 17, 2018, 12:54 a.m. | #3
.. regression testing actually completed successfully.

Paolo.
Jason Merrill Jan. 17, 2018, 2:58 p.m. | #4
On Tue, Jan 16, 2018 at 4:40 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 16/01/2018 22:35, Jason Merrill wrote:

>> On Tue, Jan 16, 2018 at 3:32 PM, Paolo Carlini <paolo.carlini@oracle.com>

>> wrote:

>>>

>>> thus I figured out what was badly wrong in my first try: I misread

>>> ensure_literal_type_for_constexpr_object and missed that it can return

>>> NULL_TREE without emitting an hard error. Thus my first try even caused

>>> miscompilations :( Anyway, when DECL_DECLARED_CONSTEXPR_P is true we are

>>> safe and indeed we want to clear it as matter of error recovery. Then, in

>>> this safe case the only change in the below is returning early, thus

>>> avoiding any internal inconsistencies later and also the redundant /

>>> misleading diagnostic which I already mentioned.

>>

>> I can't see how this could be right.  In the cases where we don't give

>> an error (e.g. because we're dealing with an instantiation of a

>> variable template) there is no error, so we need to proceed with the

>> rest of cp_finish_decl as normal.

>

> The cases where we don't give an error all fall under

> DECL_DECLARED_CONSTEXPR_P == false, thus aren't affected at all.


Ah, true.  Though that's a bit subtle; maybe change ensure_... to
return error_mark_node in the error case?

Jason
Paolo Carlini Jan. 17, 2018, 4:16 p.m. | #5
Hi,

On 17/01/2018 15:58, Jason Merrill wrote:
> On Tue, Jan 16, 2018 at 4:40 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:

>> On 16/01/2018 22:35, Jason Merrill wrote:

>>> On Tue, Jan 16, 2018 at 3:32 PM, Paolo Carlini <paolo.carlini@oracle.com>

>>> wrote:

>>>> thus I figured out what was badly wrong in my first try: I misread

>>>> ensure_literal_type_for_constexpr_object and missed that it can return

>>>> NULL_TREE without emitting an hard error. Thus my first try even caused

>>>> miscompilations :( Anyway, when DECL_DECLARED_CONSTEXPR_P is true we are

>>>> safe and indeed we want to clear it as matter of error recovery. Then, in

>>>> this safe case the only change in the below is returning early, thus

>>>> avoiding any internal inconsistencies later and also the redundant /

>>>> misleading diagnostic which I already mentioned.

>>> I can't see how this could be right.  In the cases where we don't give

>>> an error (e.g. because we're dealing with an instantiation of a

>>> variable template) there is no error, so we need to proceed with the

>>> rest of cp_finish_decl as normal.

>> The cases where we don't give an error all fall under

>> DECL_DECLARED_CONSTEXPR_P == false, thus aren't affected at all.

> Ah, true.  Though that's a bit subtle; maybe change ensure_... to

> return error_mark_node in the error case?

Agreed. The below does that and I'm finishing testing it (in libstdc++ 
at the moment, so far so good and I checked separately for those nasty 
breakages I had yesterday). Note, however, this isn't exactly equivalent 
to my previous patch: it's definitely cleaner and less subtle IMO but 
more aggressive because we immediately bail out of cp_finish_decl in all 
cases of error, not just when DECL_DECLARED_CONSTEXPR_P == true. Ok if 
it passes?

Thanks!
Paolo.

////////////////////////
/cp
2018-01-17  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/81054
	* constexpr.c (ensure_literal_type_for_constexpr_object): Return
	error_mark_node when we give an error.
	decl.c (cp_finish_decl): Use the latter.

/testsuite
2018-01-17  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/81054
	* g++.dg/cpp0x/constexpr-ice19.C: New.
Index: cp/constexpr.c
===================================================================
--- cp/constexpr.c	(revision 256794)
+++ cp/constexpr.c	(working copy)
@@ -75,7 +75,8 @@ literal_type_p (tree t)
 }
 
 /* If DECL is a variable declared `constexpr', require its type
-   be literal.  Return the DECL if OK, otherwise NULL.  */
+   be literal.  Return error_mark_node if we give an error, the
+   DECL otherwise.  */
 
 tree
 ensure_literal_type_for_constexpr_object (tree decl)
@@ -97,6 +98,7 @@ ensure_literal_type_for_constexpr_object (tree dec
 	      error ("the type %qT of %<constexpr%> variable %qD "
 		     "is not literal", type, decl);
 	      explain_non_literal_class (type);
+	      decl = error_mark_node;
 	    }
 	  else
 	    {
@@ -105,10 +107,10 @@ ensure_literal_type_for_constexpr_object (tree dec
 		  error ("variable %qD of non-literal type %qT in %<constexpr%> "
 			 "function", decl, type);
 		  explain_non_literal_class (type);
+		  decl = error_mark_node;
 		}
 	      cp_function_chain->invalid_constexpr = true;
 	    }
-	  return NULL;
 	}
     }
   return decl;
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 256794)
+++ cp/decl.c	(working copy)
@@ -6810,8 +6810,12 @@ cp_finish_decl (tree decl, tree init, bool init_co
       cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
     }
 
-  if (!ensure_literal_type_for_constexpr_object (decl))
-    DECL_DECLARED_CONSTEXPR_P (decl) = 0;
+  if (ensure_literal_type_for_constexpr_object (decl)
+      == error_mark_node)
+    {
+      DECL_DECLARED_CONSTEXPR_P (decl) = 0;
+      return;
+    }
 
   if (VAR_P (decl)
       && DECL_CLASS_SCOPE_P (decl)
Index: testsuite/g++.dg/cpp0x/constexpr-ice19.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-ice19.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/constexpr-ice19.C	(working copy)
@@ -0,0 +1,13 @@
+// PR c++/81054
+// { dg-do compile { target c++11 } }
+
+struct A
+{
+  volatile int i;
+  constexpr A() : i() {}
+};
+
+struct B
+{
+  static constexpr A a {};  // { dg-error "not literal" }
+};
Jason Merrill Jan. 17, 2018, 4:36 p.m. | #6
OK.

On Wed, Jan 17, 2018 at 11:16 AM, Paolo Carlini
<paolo.carlini@oracle.com> wrote:
> Hi,

>

> On 17/01/2018 15:58, Jason Merrill wrote:

>>

>> On Tue, Jan 16, 2018 at 4:40 PM, Paolo Carlini <paolo.carlini@oracle.com>

>> wrote:

>>>

>>> On 16/01/2018 22:35, Jason Merrill wrote:

>>>>

>>>> On Tue, Jan 16, 2018 at 3:32 PM, Paolo Carlini

>>>> <paolo.carlini@oracle.com>

>>>> wrote:

>>>>>

>>>>> thus I figured out what was badly wrong in my first try: I misread

>>>>> ensure_literal_type_for_constexpr_object and missed that it can return

>>>>> NULL_TREE without emitting an hard error. Thus my first try even caused

>>>>> miscompilations :( Anyway, when DECL_DECLARED_CONSTEXPR_P is true we

>>>>> are

>>>>> safe and indeed we want to clear it as matter of error recovery. Then,

>>>>> in

>>>>> this safe case the only change in the below is returning early, thus

>>>>> avoiding any internal inconsistencies later and also the redundant /

>>>>> misleading diagnostic which I already mentioned.

>>>>

>>>> I can't see how this could be right.  In the cases where we don't give

>>>> an error (e.g. because we're dealing with an instantiation of a

>>>> variable template) there is no error, so we need to proceed with the

>>>> rest of cp_finish_decl as normal.

>>>

>>> The cases where we don't give an error all fall under

>>> DECL_DECLARED_CONSTEXPR_P == false, thus aren't affected at all.

>>

>> Ah, true.  Though that's a bit subtle; maybe change ensure_... to

>> return error_mark_node in the error case?

>

> Agreed. The below does that and I'm finishing testing it (in libstdc++ at

> the moment, so far so good and I checked separately for those nasty

> breakages I had yesterday). Note, however, this isn't exactly equivalent to

> my previous patch: it's definitely cleaner and less subtle IMO but more

> aggressive because we immediately bail out of cp_finish_decl in all cases of

> error, not just when DECL_DECLARED_CONSTEXPR_P == true. Ok if it passes?

>

> Thanks!

> Paolo.

>

> ////////////////////////

>

>

Patch

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 256753)
+++ cp/decl.c	(working copy)
@@ -6810,8 +6810,12 @@  cp_finish_decl (tree decl, tree init, bool init_co
       cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
     }
 
-  if (!ensure_literal_type_for_constexpr_object (decl))
-    DECL_DECLARED_CONSTEXPR_P (decl) = 0;
+  if (!ensure_literal_type_for_constexpr_object (decl)
+      && DECL_DECLARED_CONSTEXPR_P (decl))
+    {
+      DECL_DECLARED_CONSTEXPR_P (decl) = 0;
+      return;
+    }
 
   if (VAR_P (decl)
       && DECL_CLASS_SCOPE_P (decl)
Index: testsuite/g++.dg/cpp0x/constexpr-ice19.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-ice19.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/constexpr-ice19.C	(working copy)
@@ -0,0 +1,13 @@ 
+// PR c++/81054
+// { dg-do compile { target c++11 } }
+
+struct A
+{
+  volatile int i;
+  constexpr A() : i() {}
+};
+
+struct B
+{
+  static constexpr A a {};  // { dg-error "not literal" }
+};