| Message ID | 20180216082814.GM5867@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] Fix ICE with return in statement expression in constexpr.c (PR c++/84192) | expand |
On Fri, Feb 16, 2018 at 3:28 AM, Jakub Jelinek <jakub@redhat.com> wrote: > pop_stmt_list, if there is just a single stmt inside statement expression > moves the stmt out of the STATEMENT_LIST (and I think cp_fold does too). > We only initialize jump_target to non-NULL in cxx_eval_statement_list > or for calls, so before we have a chance to diagnose the error of using > an expression with void type, we ICE trying to dereference NULL jump_target. > > This can't happen with BREAK_STMT nor CONTINUE_STMT, because they are not > potential constant expressions, and I think can only happen when ctx->quiet > is true, otherwise it should have been diagnosed already before. > If a RETURN_EXPR (or in theory break/continue) appears in a (potential) constant > expression we want to evaluate, not doing anything with jump_target if we > aren't inside a statement list makes sense to me, there is no following > statement to bypass. I think we should also set *non_constant_p. Jason
On Fri, Feb 16, 2018 at 08:52:10AM -0500, Jason Merrill wrote: > On Fri, Feb 16, 2018 at 3:28 AM, Jakub Jelinek <jakub@redhat.com> wrote: > > pop_stmt_list, if there is just a single stmt inside statement expression > > moves the stmt out of the STATEMENT_LIST (and I think cp_fold does too). > > We only initialize jump_target to non-NULL in cxx_eval_statement_list > > or for calls, so before we have a chance to diagnose the error of using > > an expression with void type, we ICE trying to dereference NULL jump_target. > > > > This can't happen with BREAK_STMT nor CONTINUE_STMT, because they are not > > potential constant expressions, and I think can only happen when ctx->quiet > > is true, otherwise it should have been diagnosed already before. > > If a RETURN_EXPR (or in theory break/continue) appears in a (potential) constant > > expression we want to evaluate, not doing anything with jump_target if we > > aren't inside a statement list makes sense to me, there is no following > > statement to bypass. > > I think we should also set *non_constant_p. Just like this? Tested so far just on the testcase, but given that we'd ICE on the *jump_target before, it can't really regress anything else (though of course I'll bootstrap/regtest it normally). 2018-02-16 Marek Polacek <polacek@redhat.com> Jakub Jelinek <jakub@redhat.com> PR c++/84192 * constexpr.c (cxx_eval_constant_expression) <case RETURN_EXPR>: Don't set *jump_target to anything if jump_target is NULL. * g++.dg/cpp1y/constexpr-84192.C: New test. --- gcc/cp/constexpr.c.jj 2018-02-12 19:17:37.937216029 +0100 +++ gcc/cp/constexpr.c 2018-02-15 16:10:56.630572360 +0100 @@ -4254,7 +4254,16 @@ cxx_eval_constant_expression (const cons r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0), lval, non_constant_p, overflow_p); - *jump_target = t; + if (jump_target) + *jump_target = t; + else + { + /* Can happen with ({ return true; }) && false; passed to + maybe_constant_value. There is nothing to jump over in this + case, and the bug will be diagnosed later. */ + gcc_assert (ctx->quiet); + *non_constant_p = true; + } break; case SAVE_EXPR: --- gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C.jj 2018-02-15 16:00:58.242588914 +0100 +++ gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C 2018-02-15 16:01:30.219585291 +0100 @@ -0,0 +1,41 @@ +// PR c++/84192 +// { dg-do compile { target c++14 } } +// { dg-options "" } + +bool +f1 () +{ + return ({ return true; }) && false; // { dg-error "could not convert" } +} + +void +f2 () +{ + for (;;) + constexpr bool b = ({ break; false; }) && false; // { dg-error "statement is not a constant expression" } +} + +constexpr bool +f3 (int n) +{ + bool b = false; + for (int i = 0; i < n; i++) + b = ({ break; }); // { dg-error "void value not ignored as it ought to be" } + return b; +} + +constexpr bool b = f3 (4); + +bool +f4 () +{ + constexpr bool b = ({ return true; }) && false; // { dg-error "could not convert" } + return false; +} + +constexpr bool +f5 (int x) +{ + constexpr bool b = ({ switch (x) case 0: true; }) && false; // { dg-error "could not convert" } + return false; +} Jakub
OK. On Fri, Feb 16, 2018 at 10:19 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Feb 16, 2018 at 08:52:10AM -0500, Jason Merrill wrote: >> On Fri, Feb 16, 2018 at 3:28 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> > pop_stmt_list, if there is just a single stmt inside statement expression >> > moves the stmt out of the STATEMENT_LIST (and I think cp_fold does too). >> > We only initialize jump_target to non-NULL in cxx_eval_statement_list >> > or for calls, so before we have a chance to diagnose the error of using >> > an expression with void type, we ICE trying to dereference NULL jump_target. >> > >> > This can't happen with BREAK_STMT nor CONTINUE_STMT, because they are not >> > potential constant expressions, and I think can only happen when ctx->quiet >> > is true, otherwise it should have been diagnosed already before. >> > If a RETURN_EXPR (or in theory break/continue) appears in a (potential) constant >> > expression we want to evaluate, not doing anything with jump_target if we >> > aren't inside a statement list makes sense to me, there is no following >> > statement to bypass. >> >> I think we should also set *non_constant_p. > > Just like this? Tested so far just on the testcase, but given that we'd ICE > on the *jump_target before, it can't really regress anything else (though of > course I'll bootstrap/regtest it normally). > > 2018-02-16 Marek Polacek <polacek@redhat.com> > Jakub Jelinek <jakub@redhat.com> > > PR c++/84192 > * constexpr.c (cxx_eval_constant_expression) <case RETURN_EXPR>: Don't > set *jump_target to anything if jump_target is NULL. > > * g++.dg/cpp1y/constexpr-84192.C: New test. > > --- gcc/cp/constexpr.c.jj 2018-02-12 19:17:37.937216029 +0100 > +++ gcc/cp/constexpr.c 2018-02-15 16:10:56.630572360 +0100 > @@ -4254,7 +4254,16 @@ cxx_eval_constant_expression (const cons > r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0), > lval, > non_constant_p, overflow_p); > - *jump_target = t; > + if (jump_target) > + *jump_target = t; > + else > + { > + /* Can happen with ({ return true; }) && false; passed to > + maybe_constant_value. There is nothing to jump over in this > + case, and the bug will be diagnosed later. */ > + gcc_assert (ctx->quiet); > + *non_constant_p = true; > + } > break; > > case SAVE_EXPR: > --- gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C.jj 2018-02-15 16:00:58.242588914 +0100 > +++ gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C 2018-02-15 16:01:30.219585291 +0100 > @@ -0,0 +1,41 @@ > +// PR c++/84192 > +// { dg-do compile { target c++14 } } > +// { dg-options "" } > + > +bool > +f1 () > +{ > + return ({ return true; }) && false; // { dg-error "could not convert" } > +} > + > +void > +f2 () > +{ > + for (;;) > + constexpr bool b = ({ break; false; }) && false; // { dg-error "statement is not a constant expression" } > +} > + > +constexpr bool > +f3 (int n) > +{ > + bool b = false; > + for (int i = 0; i < n; i++) > + b = ({ break; }); // { dg-error "void value not ignored as it ought to be" } > + return b; > +} > + > +constexpr bool b = f3 (4); > + > +bool > +f4 () > +{ > + constexpr bool b = ({ return true; }) && false; // { dg-error "could not convert" } > + return false; > +} > + > +constexpr bool > +f5 (int x) > +{ > + constexpr bool b = ({ switch (x) case 0: true; }) && false; // { dg-error "could not convert" } > + return false; > +} > > > Jakub
--- gcc/cp/constexpr.c.jj 2018-02-12 19:17:37.937216029 +0100 +++ gcc/cp/constexpr.c 2018-02-15 16:10:56.630572360 +0100 @@ -4254,7 +4254,13 @@ cxx_eval_constant_expression (const cons r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0), lval, non_constant_p, overflow_p); - *jump_target = t; + if (jump_target) + *jump_target = t; + else + /* Can happen with ({ return true; }) && false; passed to + maybe_constant_value. There is nothing to jump over in this + case, and the bug will be diagnosed later. */ + gcc_assert (ctx->quiet); break; case SAVE_EXPR: --- gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C.jj 2018-02-15 16:00:58.242588914 +0100 +++ gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C 2018-02-15 16:01:30.219585291 +0100 @@ -0,0 +1,41 @@ +// PR c++/84192 +// { dg-do compile { target c++14 } } +// { dg-options "" } + +bool +f1 () +{ + return ({ return true; }) && false; // { dg-error "could not convert" } +} + +void +f2 () +{ + for (;;) + constexpr bool b = ({ break; false; }) && false; // { dg-error "statement is not a constant expression" } +} + +constexpr bool +f3 (int n) +{ + bool b = false; + for (int i = 0; i < n; i++) + b = ({ break; }); // { dg-error "void value not ignored as it ought to be" } + return b; +} + +constexpr bool b = f3 (4); + +bool +f4 () +{ + constexpr bool b = ({ return true; }) && false; // { dg-error "could not convert" } + return false; +} + +constexpr bool +f5 (int x) +{ + constexpr bool b = ({ switch (x) case 0: true; }) && false; // { dg-error "could not convert" } + return false; +}