[C++ Patch] PR 65579 ("gcc requires definition of a static constexpr member...")

[C++ Patch] PR 65579 ("gcc requires definition of a static constexpr member...")

[Paolo Carlini]

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

Hi,

this is a relatively old bug already analyzed by Martin last year. He 
also proposed a patch:

     https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00593.html

After a short exchange Jason proposed a different approach based on 
simply completing the involved vars:

     https://gcc.gnu.org/ml/gcc-patches/2016-03/msg01420.html

Having verified that Martin wasn't actively working on the bug, I 
decided to try a very straightforward implementation of Jason's 
suggestion - see the attached, tested x86_64-linux - which appears to 
work fine as-is. Naturally, one could imagine restricting/enlarging the 
set of decls to complete: some choices don't seem good, like extending 
to non-constepr vars too (the corresponding snippet is ill formed anyway 
due to the initialization). I didn't try to test all the possible 
variants...

Thanks, Paolo.

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


[-- Attachment #2: CL_65579 --]
[-- Type: text/plain, Size: 315 bytes --]

/cp
2017-09-26  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/65579
	* decl.c (grokdeclarator): Before calling cp_apply_type_quals_to_decl
	on constexpr VAR_DECLs complete their type.

/testsuite
2017-09-26  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/65579
	* g++.dg/cpp0x/constexpr-template11.C: New.

[-- Attachment #3: patch_65579 --]
[-- Type: text/plain, Size: 1137 bytes --]

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 253134)
+++ cp/decl.c	(working copy)
@@ -12348,7 +12348,11 @@ grokdeclarator (const cp_declarator *declarator,
 
     /* Set constexpr flag on vars (functions got it in grokfndecl).  */
     if (constexpr_p && VAR_P (decl))
-      DECL_DECLARED_CONSTEXPR_P (decl) = true;
+      {
+	DECL_DECLARED_CONSTEXPR_P (decl) = true;
+	if (!processing_template_decl)
+	  TREE_TYPE (decl) = complete_type (TREE_TYPE (decl));
+      }
 
     /* Record constancy and volatility on the DECL itself .  There's
        no need to do this when processing a template; we'll do this
Index: testsuite/g++.dg/cpp0x/constexpr-template11.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-template11.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/constexpr-template11.C	(working copy)
@@ -0,0 +1,16 @@
+// PR c++/65579
+// { dg-do link { target c++11 } }
+
+template <typename>
+struct S {
+    int i;
+};
+
+struct T {
+  static constexpr S<int> s = { 1 };
+};
+
+int main()
+{
+  return T::s.i;
+}

Re: [C++ Patch] PR 65579 ("gcc requires definition of a static constexpr member...")

[Jason Merrill]

On Tue, Sep 26, 2017 at 4:28 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> this is a relatively old bug already analyzed by Martin last year. He also
> proposed a patch:
>
>     https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00593.html
>
> After a short exchange Jason proposed a different approach based on simply
> completing the involved vars:
>
>     https://gcc.gnu.org/ml/gcc-patches/2016-03/msg01420.html
>
> Having verified that Martin wasn't actively working on the bug, I decided to
> try a very straightforward implementation of Jason's suggestion - see the
> attached, tested x86_64-linux - which appears to work fine as-is. Naturally,
> one could imagine restricting/enlarging the set of decls to complete: some
> choices don't seem good, like extending to non-constepr vars too (the
> corresponding snippet is ill formed anyway due to the initialization). I
> didn't try to test all the possible variants...

This seems like an odd place to add the complete_type call.  What
happens if we change the COMPLETE_TYPE_P (type) in
cp_apply_type_quals_to_decl to COMPLETE_TYPE_P (complete_type (type))?

Jason

Re: [C++ Patch] PR 65579 ("gcc requires definition of a static constexpr member...")

[Paolo Carlini]

Hi Jason,

On 24/10/2017 20:58, Jason Merrill wrote:
> On Tue, Sep 26, 2017 at 4:28 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Hi,
>>
>> this is a relatively old bug already analyzed by Martin last year. He also
>> proposed a patch:
>>
>>      https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00593.html
>>
>> After a short exchange Jason proposed a different approach based on simply
>> completing the involved vars:
>>
>>      https://gcc.gnu.org/ml/gcc-patches/2016-03/msg01420.html
>>
>> Having verified that Martin wasn't actively working on the bug, I decided to
>> try a very straightforward implementation of Jason's suggestion - see the
>> attached, tested x86_64-linux - which appears to work fine as-is. Naturally,
>> one could imagine restricting/enlarging the set of decls to complete: some
>> choices don't seem good, like extending to non-constepr vars too (the
>> corresponding snippet is ill formed anyway due to the initialization). I
>> didn't try to test all the possible variants...
> This seems like an odd place to add the complete_type call.
Indeed. Wanted to at least try the minimal change, worked surprisingly well.

>    What
> happens if we change the COMPLETE_TYPE_P (type) in
> cp_apply_type_quals_to_decl to COMPLETE_TYPE_P (complete_type (type))?
I'll let you know asap, thanks for the suggestion!

Paolo.

Re: [C++ Patch] PR 65579 ("gcc requires definition of a static constexpr member...")

[Paolo Carlini]

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

Hi again,

On 24/10/2017 20:58, Jason Merrill wrote:
> This seems like an odd place to add the complete_type call.  What
> happens if we change the COMPLETE_TYPE_P (type) in
> cp_apply_type_quals_to_decl to COMPLETE_TYPE_P (complete_type (type))?
Finally I'm back with some information.

Simply doing the above doesn't fully work. The first symptom is the 
failure of g++.dg/init/mutable1.C which is precisely the testcase that 
you added together with the "|| !COMPLETE_TYPE_P (type)" itself: 
clearly, the additional condition isn't able anymore to do its work, 
because, first, when the type isn't complete, TYPE_HAS_MUTABLE_P (type) 
is false and then, when in fact it would be found true, we check 
!COMPLETE_TYPE_P (complete_type (type)) which is false, because 
completing succeeded.

Thus it seems we need at least something like:

    TREE_TYPE (decl) = type = complete_type (type);

    if (TYPE_HAS_MUTABLE_P (type) || !COMPLETE_TYPE_P (type))
      type_quals &= ~TYPE_QUAL_CONST;

But then, toward the end of the testsuite, we notice a more serious 
issue, which is unrelated to the above: g++.old-deja/g++.pt/poi1.C

// { dg-do assemble  }
// Origin: Gerald Pfeifer <pfeifer@dbai.tuwien.ac.at>

template <class T>
class TLITERAL : public T
     {
     int x;
     };

class GATOM;

typedef TLITERAL<GATOM> x;
extern TLITERAL<GATOM> y;

also fails:

poi1.C: In instantiation of ‘class TLITERAL<GATOM>’:
poi1.C:13:24:   required from here
poi1.C:5:7: error: invalid use of incomplete type ‘class GATOM’
  class TLITERAL : public T
poi1.C:10:7: note: forward declaration of ‘class GATOM’
  class GATOM;

that is, trying to complete GATOM at the 'extern TLITERAL<GATOM> y;" 
line obviously fails. Note, in case isn't obvious, that this happens 
exactly for the cp_apply_type_quals_to_decl call at the end of 
grokdeclarator which I tried to change in my first try: the failure of 
poi1.C seems rather useful to figure out what we want to do for this bug.

Well, as expected, explicitly checking VAR_P && 
DECL_DECLARED_CONSTEXPR_P works again - it seems to me that after all it 
could make sense given the comment precisely talking about the 
additional complexities related to constexpr. Anyway, I'm attaching the 
corresponding complete patch.

Thanks!
Paolo.

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

[-- Attachment #2: patch_65579_2 --]
[-- Type: text/plain, Size: 1006 bytes --]

Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 254071)
+++ cp/typeck.c	(working copy)
@@ -9544,6 +9544,9 @@ cp_apply_type_quals_to_decl (int type_quals, tree
 
   /* If the type has (or might have) a mutable component, that component
      might be modified.  */
+  if (VAR_P (decl) && DECL_DECLARED_CONSTEXPR_P (decl))
+    TREE_TYPE (decl) = type = complete_type (type);
+
   if (TYPE_HAS_MUTABLE_P (type) || !COMPLETE_TYPE_P (type))
     type_quals &= ~TYPE_QUAL_CONST;
 
Index: testsuite/g++.dg/cpp0x/constexpr-template11.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-template11.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/constexpr-template11.C	(working copy)
@@ -0,0 +1,16 @@
+// PR c++/65579
+// { dg-do link { target c++11 } }
+
+template <typename>
+struct S {
+    int i;
+};
+
+struct T {
+  static constexpr S<int> s = { 1 };
+};
+
+int main()
+{
+  return T::s.i;
+}

Re: [C++ Patch] PR 65579 ("gcc requires definition of a static constexpr member...")

[Jason Merrill]

On Thu, Oct 26, 2017 at 6:17 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi again,
>
> On 24/10/2017 20:58, Jason Merrill wrote:
>>
>> This seems like an odd place to add the complete_type call.  What
>> happens if we change the COMPLETE_TYPE_P (type) in
>> cp_apply_type_quals_to_decl to COMPLETE_TYPE_P (complete_type (type))?
>
> Finally I'm back with some information.
>
> Simply doing the above doesn't fully work. The first symptom is the failure
> of g++.dg/init/mutable1.C which is precisely the testcase that you added
> together with the "|| !COMPLETE_TYPE_P (type)" itself: clearly, the
> additional condition isn't able anymore to do its work, because, first, when
> the type isn't complete, TYPE_HAS_MUTABLE_P (type) is false and then, when
> in fact it would be found true, we check !COMPLETE_TYPE_P (complete_type
> (type)) which is false, because completing succeeded.

> Thus it seems we need at least something like:
>
>    TREE_TYPE (decl) = type = complete_type (type);
>
>    if (TYPE_HAS_MUTABLE_P (type) || !COMPLETE_TYPE_P (type))
>      type_quals &= ~TYPE_QUAL_CONST;
>
> But then, toward the end of the testsuite, we notice a more serious issue,
> which is unrelated to the above: g++.old-deja/g++.pt/poi1.C
>
> // { dg-do assemble  }
> // Origin: Gerald Pfeifer <pfeifer@dbai.tuwien.ac.at>
>
> template <class T>
> class TLITERAL : public T
>     {
>     int x;
>     };
>
> class GATOM;
>
> typedef TLITERAL<GATOM> x;
> extern TLITERAL<GATOM> y;
>
> also fails:
>
> poi1.C: In instantiation of ‘class TLITERAL<GATOM>’:
> poi1.C:13:24:   required from here
> poi1.C:5:7: error: invalid use of incomplete type ‘class GATOM’
>  class TLITERAL : public T
> poi1.C:10:7: note: forward declaration of ‘class GATOM’
>  class GATOM;
>
> that is, trying to complete GATOM at the 'extern TLITERAL<GATOM> y;" line
> obviously fails. Note, in case isn't obvious, that this happens exactly for
> the cp_apply_type_quals_to_decl call at the end of grokdeclarator which I
> tried to change in my first try: the failure of poi1.C seems rather useful
> to figure out what we want to do for this bug.
>
> Well, as expected, explicitly checking VAR_P && DECL_DECLARED_CONSTEXPR_P
> works again - it seems to me that after all it could make sense given the
> comment precisely talking about the additional complexities related to
> constexpr. Anyway, I'm attaching the corresponding complete patch.

Looking at the code again, it seems that the problem is the difference
between start_decl_1 and grokfield, in that the former has

  /* If an explicit initializer is present, or if this is a definition
     of an aggregate, then we need a complete type at this point.
     (Scalars are always complete types, so there is nothing to
     check.)  This code just sets COMPLETE_P; errors (if necessary)
     are issued below.  */
  if ((initialized || aggregate_definition_p)
      && !complete_p
      && COMPLETE_TYPE_P (complete_type (type)))
    {
      complete_p = true;
      /* We will not yet have set TREE_READONLY on DECL if the type
         was "const", but incomplete, before this point.  But, now, we
         have a complete type, so we can try again.  */
      cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
    }

and grokfield/finish_static_data_member_decl don't.  How about
completing the type and re-applying the quals in
finish_static_data_member_decl if there's an initializer?  Your most
recent patch ought to work, but is less parallel.  Sorry for the
churn.

Jason

Re: [C++ Patch] PR 65579 ("gcc requires definition of a static constexpr member...")

[Paolo Carlini]

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

Hi,

On 03/11/2017 18:56, Jason Merrill wrote:
> Looking at the code again, it seems that the problem is the difference
> between start_decl_1 and grokfield, in that the former has
>
>    /* If an explicit initializer is present, or if this is a definition
>       of an aggregate, then we need a complete type at this point.
>       (Scalars are always complete types, so there is nothing to
>       check.)  This code just sets COMPLETE_P; errors (if necessary)
>       are issued below.  */
>    if ((initialized || aggregate_definition_p)
>        && !complete_p
>        && COMPLETE_TYPE_P (complete_type (type)))
>      {
>        complete_p = true;
>        /* We will not yet have set TREE_READONLY on DECL if the type
>           was "const", but incomplete, before this point.  But, now, we
>           have a complete type, so we can try again.  */
>        cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
>      }
>
> and grokfield/finish_static_data_member_decl don't.  How about
> completing the type and re-applying the quals in
> finish_static_data_member_decl if there's an initializer?  Your most
> recent patch ought to work, but is less parallel.  Sorry for the
> churn.
No problem, I learned something! Anyway, yes, the below is passing 
testing, shall we go ahead with it?

Thanks,
Paolo.

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

[-- Attachment #2: patch_65579_3 --]
[-- Type: text/plain, Size: 1200 bytes --]

Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 254365)
+++ cp/decl2.c	(working copy)
@@ -787,6 +787,15 @@ finish_static_data_member_decl (tree decl,
       && TYPE_DOMAIN (TREE_TYPE (decl)) == NULL_TREE)
     SET_VAR_HAD_UNKNOWN_BOUND (decl);
 
+  if (init)
+    {
+      /* Similarly to start_decl_1, we want to complete the type in order
+	 to do the right thing in cp_apply_type_quals_to_decl, possibly
+	 clear TYPE_QUAL_CONST (c++/65579).  */
+      tree type = TREE_TYPE (decl) = complete_type (TREE_TYPE (decl));
+      cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
+    }
+
   cp_finish_decl (decl, init, init_const_expr_p, asmspec_tree, flags);
 }
 
Index: testsuite/g++.dg/cpp0x/constexpr-template11.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-template11.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/constexpr-template11.C	(working copy)
@@ -0,0 +1,16 @@
+// PR c++/65579
+// { dg-do link { target c++11 } }
+
+template <typename>
+struct S {
+    int i;
+};
+
+struct T {
+  static constexpr S<int> s = { 1 };
+};
+
+int main()
+{
+  return T::s.i;
+}

Re: [C++ Patch] PR 65579 ("gcc requires definition of a static constexpr member...")

[Jason Merrill]

OK, thanks.

On Fri, Nov 3, 2017 at 3:55 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 03/11/2017 18:56, Jason Merrill wrote:
>>
>> Looking at the code again, it seems that the problem is the difference
>> between start_decl_1 and grokfield, in that the former has
>>
>>    /* If an explicit initializer is present, or if this is a definition
>>       of an aggregate, then we need a complete type at this point.
>>       (Scalars are always complete types, so there is nothing to
>>       check.)  This code just sets COMPLETE_P; errors (if necessary)
>>       are issued below.  */
>>    if ((initialized || aggregate_definition_p)
>>        && !complete_p
>>        && COMPLETE_TYPE_P (complete_type (type)))
>>      {
>>        complete_p = true;
>>        /* We will not yet have set TREE_READONLY on DECL if the type
>>           was "const", but incomplete, before this point.  But, now, we
>>           have a complete type, so we can try again.  */
>>        cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
>>      }
>>
>> and grokfield/finish_static_data_member_decl don't.  How about
>> completing the type and re-applying the quals in
>> finish_static_data_member_decl if there's an initializer?  Your most
>> recent patch ought to work, but is less parallel.  Sorry for the
>> churn.
>
> No problem, I learned something! Anyway, yes, the below is passing testing,
> shall we go ahead with it?
>
> Thanks,
> Paolo.
>
> ///////////////////////