Message ID | 20210204000319.299127-1-polacek@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: Fix bogus -Wvolatile warning in C++20 [PR98947] | expand |
On 2/3/21 7:03 PM, Marek Polacek wrote: > Since most of volatile is deprecated in C++20, we are required to warn > for compound assignments to volatile variables and so on. But here we > have > > volatile int x, y, z; > (b ? x : y) = 1; > > and we shouldn't warn, because simple assignments like x = 24; should > not provoke the warning when they are a discarded-value expression. > > We warn here because when ?: is used as an lvalue, we transform it in > cp_build_modify_expr/COND_EXPR from (a ? b : c) = rhs to > > (a ? (b = rhs) : (c = rhs)) > > and build_conditional_expr then calls mark_lvalue_use for the new > artificial assignments Hmm, that seems wrong; the ?: expression itself does not use lvalue operands any more than ',' does. I notice that removing those mark_lvalue_use calls doesn't regress Wunused-var-10.c, which was added with them in r160289. , which then evokes the warning. So use > a warning sentinel. But since we should still warn for > > (b ? x : y) += 1; // compound assignment > (b ? (x = 2) : y) = 1; // lvalue-to-rvalue conv on x = 2 > > I've tweaked mark_use to only set TREE_THIS_VOLATILE when the warning > is enabled. > > I'd argue this is a regression because GCC 9 doesn't warn. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? > > gcc/cp/ChangeLog: > > PR c++/98947 > * expr.c (mark_use) <case MODIFY_EXPR>: Only set TREE_THIS_VOLATILE > if warn_volatile. > * typeck.c (cp_build_modify_expr) <case COND_EXPR>: Add a warning > sentinel for -Wvolatile around build_conditional_expr. > > gcc/testsuite/ChangeLog: > > PR c++/98947 > * g++.dg/cpp2a/volatile5.C: New test. > --- > gcc/cp/expr.c | 3 ++- > gcc/cp/typeck.c | 4 ++++ > gcc/testsuite/g++.dg/cpp2a/volatile5.C | 15 +++++++++++++++ > 3 files changed, 21 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/volatile5.C > > diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c > index 480e740f08c..d697978ce19 100644 > --- a/gcc/cp/expr.c > +++ b/gcc/cp/expr.c > @@ -228,7 +228,8 @@ mark_use (tree expr, bool rvalue_p, bool read_p, > && !cp_unevaluated_operand > && (TREE_THIS_VOLATILE (lhs) > || CP_TYPE_VOLATILE_P (TREE_TYPE (lhs))) > - && !TREE_THIS_VOLATILE (expr)) > + && !TREE_THIS_VOLATILE (expr) > + && warn_volatile) Or you could check the return value of the warning? > { > warning_at (location_of (expr), OPT_Wvolatile, > "using value of simple assignment with %<volatile%>-" > diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c > index a87d5e5f2ac..52c2344530d 100644 > --- a/gcc/cp/typeck.c > +++ b/gcc/cp/typeck.c > @@ -8617,6 +8617,10 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, > tree op2 = TREE_OPERAND (lhs, 2); > if (TREE_CODE (op2) != THROW_EXPR) > op2 = cp_build_modify_expr (loc, op2, modifycode, rhs, complain); > + /* build_conditional_expr calls mark_lvalue_use for op1/op2, > + which are now assignments due to the above transformation, > + generating bogus C++20 warnings. */ > + warning_sentinel w (warn_volatile); > tree cond = build_conditional_expr (input_location, > TREE_OPERAND (lhs, 0), op1, op2, > complain); > diff --git a/gcc/testsuite/g++.dg/cpp2a/volatile5.C b/gcc/testsuite/g++.dg/cpp2a/volatile5.C > new file mode 100644 > index 00000000000..1f9d23845b4 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/volatile5.C > @@ -0,0 +1,15 @@ > +// PR c++/98947 > +// { dg-do compile } > + > +volatile int x, y, z; > + > +void > +f (bool b) > +{ > + (b ? x : y) = 1; > + (b ? x : y) += 1; // { dg-warning "compound assignment" "" { target c++20 } } > + z = (b ? x : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } > + ((z = 2) ? x : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } > + (b ? (x = 2) : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } > + (b ? x : (y = 5)) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } > +} > > base-commit: 34215a7a3a359d700a520f1d5bdaec835f0b5180 >
On Thu, Feb 04, 2021 at 10:59:21AM -0500, Jason Merrill wrote: > On 2/3/21 7:03 PM, Marek Polacek wrote: > > Since most of volatile is deprecated in C++20, we are required to warn > > for compound assignments to volatile variables and so on. But here we > > have > > > > volatile int x, y, z; > > (b ? x : y) = 1; > > > > and we shouldn't warn, because simple assignments like x = 24; should > > not provoke the warning when they are a discarded-value expression. > > > > We warn here because when ?: is used as an lvalue, we transform it in > > cp_build_modify_expr/COND_EXPR from (a ? b : c) = rhs to > > > > (a ? (b = rhs) : (c = rhs)) > > > > and build_conditional_expr then calls mark_lvalue_use for the new > > artificial assignments > > Hmm, that seems wrong; the ?: expression itself does not use lvalue operands > any more than ',' does. I notice that removing those mark_lvalue_use calls > doesn't regress Wunused-var-10.c, which was added with them in r160289. The mark_lvalue_use calls didn't strike me as wrong because [expr.cond]/7 says that lvalue-to-rvalue conversion is performed on the second and third operands. With those mark_lvalue_use calls removed, we'd not issue the warning for (b ? (x = 2) : y) = 1; (b ? x : (y = 5)) = 1; which I think we want (and clang++ warns here too). > > --- a/gcc/cp/expr.c > > +++ b/gcc/cp/expr.c > > @@ -228,7 +228,8 @@ mark_use (tree expr, bool rvalue_p, bool read_p, > > && !cp_unevaluated_operand > > && (TREE_THIS_VOLATILE (lhs) > > || CP_TYPE_VOLATILE_P (TREE_TYPE (lhs))) > > - && !TREE_THIS_VOLATILE (expr)) > > + && !TREE_THIS_VOLATILE (expr) > > + && warn_volatile) > > Or you could check the return value of the warning? Works too. Happy to change that. Marek
On 2/4/21 1:11 PM, Marek Polacek wrote: > On Thu, Feb 04, 2021 at 10:59:21AM -0500, Jason Merrill wrote: >> On 2/3/21 7:03 PM, Marek Polacek wrote: >>> Since most of volatile is deprecated in C++20, we are required to warn >>> for compound assignments to volatile variables and so on. But here we >>> have >>> >>> volatile int x, y, z; >>> (b ? x : y) = 1; >>> >>> and we shouldn't warn, because simple assignments like x = 24; should >>> not provoke the warning when they are a discarded-value expression. >>> >>> We warn here because when ?: is used as an lvalue, we transform it in >>> cp_build_modify_expr/COND_EXPR from (a ? b : c) = rhs to >>> >>> (a ? (b = rhs) : (c = rhs)) >>> >>> and build_conditional_expr then calls mark_lvalue_use for the new >>> artificial assignments >> >> Hmm, that seems wrong; the ?: expression itself does not use lvalue operands >> any more than ',' does. I notice that removing those mark_lvalue_use calls >> doesn't regress Wunused-var-10.c, which was added with them in r160289. > > The mark_lvalue_use calls didn't strike me as wrong because [expr.cond]/7 > says that lvalue-to-rvalue conversion is performed on the second and third > operands. Only after we've decided (in /6) that the result is a prvalue. > With those mark_lvalue_use calls removed, we'd not issue the > warning for > > (b ? (x = 2) : y) = 1; > (b ? x : (y = 5)) = 1; Why wouldn't we? The assignment should call mark_lvalue_use for the LHS, which recursively applies it to the arms of the ?:. Jason
On Thu, Feb 04, 2021 at 03:47:54PM -0500, Jason Merrill via Gcc-patches wrote: > On 2/4/21 1:11 PM, Marek Polacek wrote: > > On Thu, Feb 04, 2021 at 10:59:21AM -0500, Jason Merrill wrote: > > > On 2/3/21 7:03 PM, Marek Polacek wrote: > > > > Since most of volatile is deprecated in C++20, we are required to warn > > > > for compound assignments to volatile variables and so on. But here we > > > > have > > > > > > > > volatile int x, y, z; > > > > (b ? x : y) = 1; > > > > > > > > and we shouldn't warn, because simple assignments like x = 24; should > > > > not provoke the warning when they are a discarded-value expression. > > > > > > > > We warn here because when ?: is used as an lvalue, we transform it in > > > > cp_build_modify_expr/COND_EXPR from (a ? b : c) = rhs to > > > > > > > > (a ? (b = rhs) : (c = rhs)) > > > > > > > > and build_conditional_expr then calls mark_lvalue_use for the new > > > > artificial assignments > > > > > > Hmm, that seems wrong; the ?: expression itself does not use lvalue operands > > > any more than ',' does. I notice that removing those mark_lvalue_use calls > > > doesn't regress Wunused-var-10.c, which was added with them in r160289. > > > > The mark_lvalue_use calls didn't strike me as wrong because [expr.cond]/7 > > says that lvalue-to-rvalue conversion is performed on the second and third > > operands. > > Only after we've decided (in /6) that the result is a prvalue. Oh, I see. :~ > > With those mark_lvalue_use calls removed, we'd not issue the > > warning for > > > > (b ? (x = 2) : y) = 1; > > (b ? x : (y = 5)) = 1; > > Why wouldn't we? The assignment should call mark_lvalue_use for the LHS, > which recursively applies it to the arms of the ?:. Aha: we call mark_lvalue_use_*nonread* and the warning was guarded by read_p. I don't know why I did it, I see no regressions if I just remove the check. And then I get the expected results. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? -- >8 -- Since most of volatile is deprecated in C++20, we are required to warn for compound assignments to volatile variables and so on. But here we have volatile int x, y, z; (b ? x : y) = 1; and we shouldn't warn, because simple assignments like x = 24; should not provoke the warning when they are a discarded-value expression. We warn here because when ?: is used as an lvalue, we transform it in cp_build_modify_expr/COND_EXPR from (a ? b : c) = rhs to (a ? (b = rhs) : (c = rhs)) and build_conditional_expr then calls mark_lvalue_use for the new artificial assignments, which then evokes the warning. The calls to mark_lvalue_use were added in r160289 to suppress warnings in Wunused-var-10.c, but looks like they're no longer needed. To warn on (b ? (x = 2) : y) = 1; (b ? x : (y = 5)) = 1; I've tweaked a check in mark_use/MODIFY_EXPR. I'd argue this is a regression because GCC 9 doesn't warn. gcc/cp/ChangeLog: PR c++/98947 * call.c (build_conditional_expr_1): Don't call mark_lvalue_use on arg2/arg3. * expr.c (mark_use) <case MODIFY_EXPR>: Don't check read_p when issuing the -Wvolatile warning. Only set TREE_THIS_VOLATILE if a warning was emitted. gcc/testsuite/ChangeLog: PR c++/98947 * g++.dg/cpp2a/volatile5.C: New test. --- gcc/cp/call.c | 2 -- gcc/cp/expr.c | 14 +++++++------- gcc/testsuite/g++.dg/cpp2a/volatile5.C | 15 +++++++++++++++ 3 files changed, 22 insertions(+), 9 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp2a/volatile5.C diff --git a/gcc/cp/call.c b/gcc/cp/call.c index c7e13f3a22b..4744c9768ec 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -5559,8 +5559,6 @@ build_conditional_expr_1 (const op_location_t &loc, && same_type_p (arg2_type, arg3_type)) { result_type = arg2_type; - arg2 = mark_lvalue_use (arg2); - arg3 = mark_lvalue_use (arg3); goto valid_operands; } diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c index 480e740f08c..d16d1896f2d 100644 --- a/gcc/cp/expr.c +++ b/gcc/cp/expr.c @@ -224,17 +224,17 @@ mark_use (tree expr, bool rvalue_p, bool read_p, a volatile-qualified type is deprecated unless the assignment is either a discarded-value expression or appears in an unevaluated context." */ - if (read_p - && !cp_unevaluated_operand + if (!cp_unevaluated_operand && (TREE_THIS_VOLATILE (lhs) || CP_TYPE_VOLATILE_P (TREE_TYPE (lhs))) && !TREE_THIS_VOLATILE (expr)) { - warning_at (location_of (expr), OPT_Wvolatile, - "using value of simple assignment with %<volatile%>-" - "qualified left operand is deprecated"); - /* Make sure not to warn about this assignment again. */ - TREE_THIS_VOLATILE (expr) = true; + if (warning_at (location_of (expr), OPT_Wvolatile, + "using value of simple assignment with " + "%<volatile%>-qualified left operand is " + "deprecated")) + /* Make sure not to warn about this assignment again. */ + TREE_THIS_VOLATILE (expr) = true; } break; } diff --git a/gcc/testsuite/g++.dg/cpp2a/volatile5.C b/gcc/testsuite/g++.dg/cpp2a/volatile5.C new file mode 100644 index 00000000000..1f9d23845b4 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/volatile5.C @@ -0,0 +1,15 @@ +// PR c++/98947 +// { dg-do compile } + +volatile int x, y, z; + +void +f (bool b) +{ + (b ? x : y) = 1; + (b ? x : y) += 1; // { dg-warning "compound assignment" "" { target c++20 } } + z = (b ? x : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } + ((z = 2) ? x : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } + (b ? (x = 2) : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } + (b ? x : (y = 5)) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } +} base-commit: 4e7c24d97dd65083a770252ce942f43d408fe11d
On 2/4/21 6:09 PM, Marek Polacek wrote: > On Thu, Feb 04, 2021 at 03:47:54PM -0500, Jason Merrill via Gcc-patches wrote: >> On 2/4/21 1:11 PM, Marek Polacek wrote: >>> On Thu, Feb 04, 2021 at 10:59:21AM -0500, Jason Merrill wrote: >>>> On 2/3/21 7:03 PM, Marek Polacek wrote: >>>>> Since most of volatile is deprecated in C++20, we are required to warn >>>>> for compound assignments to volatile variables and so on. But here we >>>>> have >>>>> >>>>> volatile int x, y, z; >>>>> (b ? x : y) = 1; >>>>> >>>>> and we shouldn't warn, because simple assignments like x = 24; should >>>>> not provoke the warning when they are a discarded-value expression. >>>>> >>>>> We warn here because when ?: is used as an lvalue, we transform it in >>>>> cp_build_modify_expr/COND_EXPR from (a ? b : c) = rhs to >>>>> >>>>> (a ? (b = rhs) : (c = rhs)) >>>>> >>>>> and build_conditional_expr then calls mark_lvalue_use for the new >>>>> artificial assignments >>>> >>>> Hmm, that seems wrong; the ?: expression itself does not use lvalue operands >>>> any more than ',' does. I notice that removing those mark_lvalue_use calls >>>> doesn't regress Wunused-var-10.c, which was added with them in r160289. >>> >>> The mark_lvalue_use calls didn't strike me as wrong because [expr.cond]/7 >>> says that lvalue-to-rvalue conversion is performed on the second and third >>> operands. >> >> Only after we've decided (in /6) that the result is a prvalue. > > Oh, I see. :~ > >>> With those mark_lvalue_use calls removed, we'd not issue the >>> warning for >>> >>> (b ? (x = 2) : y) = 1; >>> (b ? x : (y = 5)) = 1; >> >> Why wouldn't we? The assignment should call mark_lvalue_use for the LHS, >> which recursively applies it to the arms of the ?:. > > Aha: we call mark_lvalue_use_*nonread* and the warning was guarded by > read_p. I don't know why I did it, I see no regressions if I just > remove the check. And then I get the expected results. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? OK. > -- >8 -- > Since most of volatile is deprecated in C++20, we are required to warn > for compound assignments to volatile variables and so on. But here we > have > > volatile int x, y, z; > (b ? x : y) = 1; > > and we shouldn't warn, because simple assignments like x = 24; should > not provoke the warning when they are a discarded-value expression. > > We warn here because when ?: is used as an lvalue, we transform it in > cp_build_modify_expr/COND_EXPR from (a ? b : c) = rhs to > > (a ? (b = rhs) : (c = rhs)) > > and build_conditional_expr then calls mark_lvalue_use for the new > artificial assignments, which then evokes the warning. The calls > to mark_lvalue_use were added in r160289 to suppress warnings in > Wunused-var-10.c, but looks like they're no longer needed. > > To warn on > > (b ? (x = 2) : y) = 1; > (b ? x : (y = 5)) = 1; > > I've tweaked a check in mark_use/MODIFY_EXPR. > > I'd argue this is a regression because GCC 9 doesn't warn. > > gcc/cp/ChangeLog: > > PR c++/98947 > * call.c (build_conditional_expr_1): Don't call mark_lvalue_use > on arg2/arg3. > * expr.c (mark_use) <case MODIFY_EXPR>: Don't check read_p when > issuing the -Wvolatile warning. Only set TREE_THIS_VOLATILE if > a warning was emitted. > > gcc/testsuite/ChangeLog: > > PR c++/98947 > * g++.dg/cpp2a/volatile5.C: New test. > --- > gcc/cp/call.c | 2 -- > gcc/cp/expr.c | 14 +++++++------- > gcc/testsuite/g++.dg/cpp2a/volatile5.C | 15 +++++++++++++++ > 3 files changed, 22 insertions(+), 9 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/volatile5.C > > diff --git a/gcc/cp/call.c b/gcc/cp/call.c > index c7e13f3a22b..4744c9768ec 100644 > --- a/gcc/cp/call.c > +++ b/gcc/cp/call.c > @@ -5559,8 +5559,6 @@ build_conditional_expr_1 (const op_location_t &loc, > && same_type_p (arg2_type, arg3_type)) > { > result_type = arg2_type; > - arg2 = mark_lvalue_use (arg2); > - arg3 = mark_lvalue_use (arg3); > goto valid_operands; > } > > diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c > index 480e740f08c..d16d1896f2d 100644 > --- a/gcc/cp/expr.c > +++ b/gcc/cp/expr.c > @@ -224,17 +224,17 @@ mark_use (tree expr, bool rvalue_p, bool read_p, > a volatile-qualified type is deprecated unless the assignment > is either a discarded-value expression or appears in an > unevaluated context." */ > - if (read_p > - && !cp_unevaluated_operand > + if (!cp_unevaluated_operand > && (TREE_THIS_VOLATILE (lhs) > || CP_TYPE_VOLATILE_P (TREE_TYPE (lhs))) > && !TREE_THIS_VOLATILE (expr)) > { > - warning_at (location_of (expr), OPT_Wvolatile, > - "using value of simple assignment with %<volatile%>-" > - "qualified left operand is deprecated"); > - /* Make sure not to warn about this assignment again. */ > - TREE_THIS_VOLATILE (expr) = true; > + if (warning_at (location_of (expr), OPT_Wvolatile, > + "using value of simple assignment with " > + "%<volatile%>-qualified left operand is " > + "deprecated")) > + /* Make sure not to warn about this assignment again. */ > + TREE_THIS_VOLATILE (expr) = true; > } > break; > } > diff --git a/gcc/testsuite/g++.dg/cpp2a/volatile5.C b/gcc/testsuite/g++.dg/cpp2a/volatile5.C > new file mode 100644 > index 00000000000..1f9d23845b4 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/volatile5.C > @@ -0,0 +1,15 @@ > +// PR c++/98947 > +// { dg-do compile } > + > +volatile int x, y, z; > + > +void > +f (bool b) > +{ > + (b ? x : y) = 1; > + (b ? x : y) += 1; // { dg-warning "compound assignment" "" { target c++20 } } > + z = (b ? x : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } > + ((z = 2) ? x : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } > + (b ? (x = 2) : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } > + (b ? x : (y = 5)) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } > +} > > base-commit: 4e7c24d97dd65083a770252ce942f43d408fe11d >
diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c index 480e740f08c..d697978ce19 100644 --- a/gcc/cp/expr.c +++ b/gcc/cp/expr.c @@ -228,7 +228,8 @@ mark_use (tree expr, bool rvalue_p, bool read_p, && !cp_unevaluated_operand && (TREE_THIS_VOLATILE (lhs) || CP_TYPE_VOLATILE_P (TREE_TYPE (lhs))) - && !TREE_THIS_VOLATILE (expr)) + && !TREE_THIS_VOLATILE (expr) + && warn_volatile) { warning_at (location_of (expr), OPT_Wvolatile, "using value of simple assignment with %<volatile%>-" diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index a87d5e5f2ac..52c2344530d 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -8617,6 +8617,10 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, tree op2 = TREE_OPERAND (lhs, 2); if (TREE_CODE (op2) != THROW_EXPR) op2 = cp_build_modify_expr (loc, op2, modifycode, rhs, complain); + /* build_conditional_expr calls mark_lvalue_use for op1/op2, + which are now assignments due to the above transformation, + generating bogus C++20 warnings. */ + warning_sentinel w (warn_volatile); tree cond = build_conditional_expr (input_location, TREE_OPERAND (lhs, 0), op1, op2, complain); diff --git a/gcc/testsuite/g++.dg/cpp2a/volatile5.C b/gcc/testsuite/g++.dg/cpp2a/volatile5.C new file mode 100644 index 00000000000..1f9d23845b4 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/volatile5.C @@ -0,0 +1,15 @@ +// PR c++/98947 +// { dg-do compile } + +volatile int x, y, z; + +void +f (bool b) +{ + (b ? x : y) = 1; + (b ? x : y) += 1; // { dg-warning "compound assignment" "" { target c++20 } } + z = (b ? x : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } + ((z = 2) ? x : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } + (b ? (x = 2) : y) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } + (b ? x : (y = 5)) = 1; // { dg-warning "using value of simple assignment" "" { target c++20 } } +}