| 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] | expand |
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
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.
.. regression testing actually completed successfully. Paolo.
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
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" } +};
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. > > //////////////////////// > >
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" } +};