| Message ID | 20190204204835.GD3884@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | C++ PATCH for c++/89158 - by-value capture of constexpr variable broken | expand |
On 2/4/19 3:48 PM, Marek Polacek wrote: > In this test we have a user-defined conversion converting const int & to > T, and we're binding a const int to const int & -- the parameter of the > converting ctor. We call Func with "VIEW_CONVERT_EXPR<const int>(Val)" > as an argument. I like to use a diagram for the conversion like this: > > ck_rvalue <- ck_user <- ck_identity > T T const int > V_C_E<const int>(Val) > > when processing the ck_user part we call __ct_comp, whose ICS for its > argument is > > ck_ref_bind <- ck_identity > const int & <- const int > V_C_E<const int>(Val) > > this ICS comes from reference_binding, and at that time we still had "Val" > so need_temporary_p is 0. But when processing the ck_user conversion > before calling build_over_call for the __ct_comp, we call > 7009 expr = mark_rvalue_use (expr); > for the argument which turns the V_C_E to NON_LVALUE_EXPR<42>. Binding > "42" to const int & is possible, but requires a temporary, which we don't > create precisely because need_temporary_p is 0. I.e. the original expr > of the conversion changed behind reference_binding's back. > > Now, the call to mark_rvalue_use was added in r159096 to handle > -Wunused-but-set-variable. Since r261121, we're actually able to turn a V_C_E > into an rvalue. But it seems we shouldn't be trying to turn the argument into > an rvalue in the ck_user case; mark_lvalue_use should do the job (= mark is as > used) without changing an lvalue into an rvalue, breaking the ICS as above. > > I'm not sure if I'm describing the issue clearly enough, but I hope this > will do. Yes, thanks, mark_rvalue_use is definitely wrong here. But mark_lvalue_use might be wrong as well; we don't know here how the expression is used by the inner conversions for the user-defined conversion. Can we remove the call entirely? It doesn't seem to break any Wunused* tests. Jason
On Tue, Feb 05, 2019 at 11:24:14AM -0500, Jason Merrill wrote: > Yes, thanks, mark_rvalue_use is definitely wrong here. But mark_lvalue_use > might be wrong as well; we don't know here how the expression is used by the > inner conversions for the user-defined conversion. Can we remove the call > entirely? It doesn't seem to break any Wunused* tests. It was added in r159096 for -Wunused-but-set*. It is surely possible it is now covered by other mark_*_use calls. Or could we call there just mark_exp_read instead? Jakub
On 2/5/19 11:31 AM, Jakub Jelinek wrote: > On Tue, Feb 05, 2019 at 11:24:14AM -0500, Jason Merrill wrote: >> Yes, thanks, mark_rvalue_use is definitely wrong here. But mark_lvalue_use >> might be wrong as well; we don't know here how the expression is used by the >> inner conversions for the user-defined conversion. Can we remove the call >> entirely? It doesn't seem to break any Wunused* tests. > > It was added in r159096 for -Wunused-but-set*. It is surely possible it is > now covered by other mark_*_use calls. Or could we call there just > mark_exp_read instead? That would be fine too. Jason
On Tue, Feb 05, 2019 at 01:24:15PM -0500, Jason Merrill wrote: > On 2/5/19 11:31 AM, Jakub Jelinek wrote: > > On Tue, Feb 05, 2019 at 11:24:14AM -0500, Jason Merrill wrote: > > > Yes, thanks, mark_rvalue_use is definitely wrong here. But mark_lvalue_use > > > might be wrong as well; we don't know here how the expression is used by the > > > inner conversions for the user-defined conversion. Can we remove the call > > > entirely? It doesn't seem to break any Wunused* tests. > > > > It was added in r159096 for -Wunused-but-set*. It is surely possible it is > > now covered by other mark_*_use calls. Or could we call there just > > mark_exp_read instead? > > That would be fine too. So how about this if it passes testing? Bootstrapped/regtested running on x86_64-linux, ok for trunk/8? 2019-02-05 Marek Polacek <polacek@redhat.com> PR c++/89158 - by-value capture of constexpr variable broken. * call.c (convert_like_real) <case ck_user>: Call mark_exp_read instead of mark_rvalue_use. * g++.dg/cpp0x/lambda/lambda-89158.C: New test. diff --git gcc/cp/call.c gcc/cp/call.c index c74d1b4ebdf..020a095dade 100644 --- gcc/cp/call.c +++ gcc/cp/call.c @@ -7006,7 +7006,9 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum, return expr; } - expr = mark_rvalue_use (expr); + /* Calling mark_rvalue_use could turn a decl into a constant, breaking + e.g. the need_temporary_p assumption in the ICS. */ + mark_exp_read (expr); /* Pass LOOKUP_NO_CONVERSION so rvalue/base handling knows not to allow any more UDCs. */ diff --git gcc/testsuite/g++.dg/cpp0x/lambda/lambda-89158.C gcc/testsuite/g++.dg/cpp0x/lambda/lambda-89158.C new file mode 100644 index 00000000000..15f15b46875 --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/lambda/lambda-89158.C @@ -0,0 +1,11 @@ +// PR c++/89158 +// { dg-do compile { target c++11 } } + +struct T { T(const int&); }; +void Func(T); + +void test() +{ + constexpr int Val = 42; + [Val]() { Func(Val); }; +}
On 2/5/19 3:43 PM, Marek Polacek wrote: > On Tue, Feb 05, 2019 at 01:24:15PM -0500, Jason Merrill wrote: >> On 2/5/19 11:31 AM, Jakub Jelinek wrote: >>> On Tue, Feb 05, 2019 at 11:24:14AM -0500, Jason Merrill wrote: >>>> Yes, thanks, mark_rvalue_use is definitely wrong here. But mark_lvalue_use >>>> might be wrong as well; we don't know here how the expression is used by the >>>> inner conversions for the user-defined conversion. Can we remove the call >>>> entirely? It doesn't seem to break any Wunused* tests. >>> >>> It was added in r159096 for -Wunused-but-set*. It is surely possible it is >>> now covered by other mark_*_use calls. Or could we call there just >>> mark_exp_read instead? >> >> That would be fine too. > > So how about this if it passes testing? > > Bootstrapped/regtested running on x86_64-linux, ok for trunk/8? > > 2019-02-05 Marek Polacek <polacek@redhat.com> > > PR c++/89158 - by-value capture of constexpr variable broken. > * call.c (convert_like_real) <case ck_user>: Call mark_exp_read > instead of mark_rvalue_use. > > * g++.dg/cpp0x/lambda/lambda-89158.C: New test. > > diff --git gcc/cp/call.c gcc/cp/call.c > index c74d1b4ebdf..020a095dade 100644 > --- gcc/cp/call.c > +++ gcc/cp/call.c > @@ -7006,7 +7006,9 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum, > return expr; > } > > - expr = mark_rvalue_use (expr); > + /* Calling mark_rvalue_use could turn a decl into a constant, breaking > + e.g. the need_temporary_p assumption in the ICS. */ > + mark_exp_read (expr); I'd describe the issue at a higher level, e.g. "we don't know here whether expr is being used as an lvalue or rvalue". mark_rvalue_use is wrong for this testcase because expr is actually used as an lvalue. OK with that change. Jason
On Tue, Feb 05, 2019 at 04:17:48PM -0500, Jason Merrill wrote: > On 2/5/19 3:43 PM, Marek Polacek wrote: > > On Tue, Feb 05, 2019 at 01:24:15PM -0500, Jason Merrill wrote: > > > On 2/5/19 11:31 AM, Jakub Jelinek wrote: > > > > On Tue, Feb 05, 2019 at 11:24:14AM -0500, Jason Merrill wrote: > > > > > Yes, thanks, mark_rvalue_use is definitely wrong here. But mark_lvalue_use > > > > > might be wrong as well; we don't know here how the expression is used by the > > > > > inner conversions for the user-defined conversion. Can we remove the call > > > > > entirely? It doesn't seem to break any Wunused* tests. > > > > > > > > It was added in r159096 for -Wunused-but-set*. It is surely possible it is > > > > now covered by other mark_*_use calls. Or could we call there just > > > > mark_exp_read instead? > > > > > > That would be fine too. > > > > So how about this if it passes testing? > > > > Bootstrapped/regtested running on x86_64-linux, ok for trunk/8? > > > > 2019-02-05 Marek Polacek <polacek@redhat.com> > > > > PR c++/89158 - by-value capture of constexpr variable broken. > > * call.c (convert_like_real) <case ck_user>: Call mark_exp_read > > instead of mark_rvalue_use. > > > > * g++.dg/cpp0x/lambda/lambda-89158.C: New test. > > > > diff --git gcc/cp/call.c gcc/cp/call.c > > index c74d1b4ebdf..020a095dade 100644 > > --- gcc/cp/call.c > > +++ gcc/cp/call.c > > @@ -7006,7 +7006,9 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum, > > return expr; > > } > > - expr = mark_rvalue_use (expr); > > + /* Calling mark_rvalue_use could turn a decl into a constant, breaking > > + e.g. the need_temporary_p assumption in the ICS. */ > > + mark_exp_read (expr); > > I'd describe the issue at a higher level, e.g. "we don't know here whether > expr is being used as an lvalue or rvalue". mark_rvalue_use is wrong for > this testcase because expr is actually used as an lvalue. OK with that > change. Will do, thanks! Marek
diff --git gcc/cp/call.c gcc/cp/call.c index c74d1b4ebdf..68395503e1e 100644 --- gcc/cp/call.c +++ gcc/cp/call.c @@ -7006,7 +7006,9 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum, return expr; } - expr = mark_rvalue_use (expr); + /* Calling mark_rvalue_use could turn a decl into a constant, breaking + e.g. the need_temporary_p assumption in the ICS. */ + mark_lvalue_use (expr); /* Pass LOOKUP_NO_CONVERSION so rvalue/base handling knows not to allow any more UDCs. */ diff --git gcc/testsuite/g++.dg/cpp0x/lambda/lambda-89158.C gcc/testsuite/g++.dg/cpp0x/lambda/lambda-89158.C new file mode 100644 index 00000000000..15f15b46875 --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/lambda/lambda-89158.C @@ -0,0 +1,11 @@ +// PR c++/89158 +// { dg-do compile { target c++11 } } + +struct T { T(const int&); }; +void Func(T); + +void test() +{ + constexpr int Val = 42; + [Val]() { Func(Val); }; +}
In this test we have a user-defined conversion converting const int & to T, and we're binding a const int to const int & -- the parameter of the converting ctor. We call Func with "VIEW_CONVERT_EXPR<const int>(Val)" as an argument. I like to use a diagram for the conversion like this: ck_rvalue <- ck_user <- ck_identity T T const int V_C_E<const int>(Val) when processing the ck_user part we call __ct_comp, whose ICS for its argument is ck_ref_bind <- ck_identity const int & <- const int V_C_E<const int>(Val) this ICS comes from reference_binding, and at that time we still had "Val" so need_temporary_p is 0. But when processing the ck_user conversion before calling build_over_call for the __ct_comp, we call 7009 expr = mark_rvalue_use (expr); for the argument which turns the V_C_E to NON_LVALUE_EXPR<42>. Binding "42" to const int & is possible, but requires a temporary, which we don't create precisely because need_temporary_p is 0. I.e. the original expr of the conversion changed behind reference_binding's back. Now, the call to mark_rvalue_use was added in r159096 to handle -Wunused-but-set-variable. Since r261121, we're actually able to turn a V_C_E into an rvalue. But it seems we shouldn't be trying to turn the argument into an rvalue in the ck_user case; mark_lvalue_use should do the job (= mark is as used) without changing an lvalue into an rvalue, breaking the ICS as above. I'm not sure if I'm describing the issue clearly enough, but I hope this will do. Bootstrapped/regtested on x86_64-linux, ok for trunk/8? 2019-02-04 Marek Polacek <polacek@redhat.com> PR c++/89158 - by-value capture of constexpr variable broken. * call.c (convert_like_real) <case ck_user>: Call mark_lvalue_use instead of mark_rvalue_use. * g++.dg/cpp0x/lambda/lambda-89158.C: New test.