| Message ID | f763a8ed-ad24-8054-ddca-9380b30d4612@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] PR 65579 ("gcc requires definition of a static constexpr member...") | expand |
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
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.
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. ///////////////////// 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; +}
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
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. /////////////////////// 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; +}
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. > > ///////////////////////
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; +}
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. /////////////////// /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.