Re: [PATCH] avoid -Wnonull for dynamic_cast (PR 99251)
On 3/1/21 7:44 PM, Martin Sebor wrote: On 3/1/21 1:33 PM, Jason Merrill wrote: On 3/1/21 12:10 PM, Martin Sebor wrote: On 2/24/21 8:13 PM, Jason Merrill wrote: On 2/24/21 5:25 PM, Martin Sebor wrote: In r11-6900 (PR 98646 - static_cast confuses -Wnonnull) we decided that issuing -Wnonnull for dereferencing the result of dynamic_cast was helpful despite the false positives it causes when the pointer is guaranteed not to be null because of a prior test. The test case in PR 99251 along with the feedback I got from Martin Liska have convinced me it was not the right decision. The attached patch arranges for dynamic_cast to also suppress -Wnonnull analogously to static_cast. Since there already is a helper function that builds the if-not-null test (ifnonnull) and sets TREE_NO_WARNING, I factored out the corresponding code from build_base_path that sets the additional TREE_NO_WARNING bit for static_cast into the function and called it from both places. I also renamed the function to make its purpose clearer and for consistency with other build_xxx APIs. Let's call it build_if_nonnull, as it builds the COND_EXPR as well as the test. Done. + /* The dynamic_cast might fail but so a warning might be justified + but not when the operand is guarded. See pr99251. */ + if (B *q = p->bptr ()) + dynamic_cast(q)->g (); // { dg-bogus "\\\[-Wnonnull" } This guard is no more necessary than it is for static_cast; both cases deal with null arguments. Let's not add these checks to the testcases. Done. This guard doesn't check for the mentioned case of dynamic_cast failing because the B* does not in fact point to a C. I think we can just change the dg-warning to dg-bogus. Sure, dynamic_cast might fail, but AFAICT -Wnonnull isn't supposed to warn about arguments that *might* be null, only arguments that are *known* to be null. Done. I had added the 'if' to the test to make it clear why the warning isn't expected. I replaced it with a comment to that effect. FWIW, I do think a warning for dynamic_cast to a pointer would be helpful in the absence of an if statement in these cases: void f (B *p) { dynamic_cast(p)->g (); } Not because p may be null but because the result of the cast may be for a nonnull p. If it's f's precondition that D is derived from B (in addition to p being nonnull) the cast would be better written as one to a reference to D. Agreed, or if it isn't a precondition, if (D* dp = dynamic_cast(p)) dp->g(); Such a warning would need to be issued by the middle end and although it could be controlled by a new option (say -Wdynamic-cast, along with the cases discussed in PR 12277) Sounds good. I don't think issuing -Wnonnull in this case would be inappropriate. I disagree; issuing -Wnonnull for this case would be wrong. Again, -Wnonnull is supposed to warn when an argument *is* null, not when it *might* be null. I think warning about this case is a good idea, just not as part of -Wnonnull. Anyway, that's something to consider for GCC 12. For now, please see the revised patch. * rtti.c (ifnonnull): Rename... (build_nonnull_test): ...to this. Set no-warning bit on COND_EXPR. The ChangeLog needs updating. + /* Unlike static_cast, dynamic cast may fail for a nonnull operand but Yes, but... + since the front end can't see if the cast is guarded against being + invoked with a null No; my point was that whether the cast is guarded against being invoked with a null is no more relevant for dynamic_cast than it is for static_cast, as both casts give a null result for a null argument. For this test, dynamic_cast is not significantly different from static_cast. For both casts, the bug was the compiler warning about a nullptr that it introduced itself. It seems like splitting hairs. The comment (much as the original if guard) is just meant to explain why -Wnonnull isn't expected in case a new warning is added that detects the bad assumption above. I want to make it clear that if/when that happens a failure here doesn't mislead the author into thinking we don't want any warning there at all. I have reworded the comments yet again. verify there's no warning. See also pr99251. */ Yes. - dynamic_cast(p->bptr ())->g (); // { dg-warning "\\\[-Wnonnull" } + dynamic_cast(p)->g (); // { dg-bogus "\\\[-Wnonnull" } Please put back the ->bptr()s. Done. OK, thanks. Jason
Re: [PATCH] avoid -Wnonull for dynamic_cast (PR 99251)
On 3/1/21 1:33 PM, Jason Merrill wrote: On 3/1/21 12:10 PM, Martin Sebor wrote: On 2/24/21 8:13 PM, Jason Merrill wrote: On 2/24/21 5:25 PM, Martin Sebor wrote: In r11-6900 (PR 98646 - static_cast confuses -Wnonnull) we decided that issuing -Wnonnull for dereferencing the result of dynamic_cast was helpful despite the false positives it causes when the pointer is guaranteed not to be null because of a prior test. The test case in PR 99251 along with the feedback I got from Martin Liska have convinced me it was not the right decision. The attached patch arranges for dynamic_cast to also suppress -Wnonnull analogously to static_cast. Since there already is a helper function that builds the if-not-null test (ifnonnull) and sets TREE_NO_WARNING, I factored out the corresponding code from build_base_path that sets the additional TREE_NO_WARNING bit for static_cast into the function and called it from both places. I also renamed the function to make its purpose clearer and for consistency with other build_xxx APIs. Let's call it build_if_nonnull, as it builds the COND_EXPR as well as the test. Done. + /* The dynamic_cast might fail but so a warning might be justified + but not when the operand is guarded. See pr99251. */ + if (B *q = p->bptr ()) + dynamic_cast(q)->g (); // { dg-bogus "\\\[-Wnonnull" } This guard is no more necessary than it is for static_cast; both cases deal with null arguments. Let's not add these checks to the testcases. Done. This guard doesn't check for the mentioned case of dynamic_cast failing because the B* does not in fact point to a C. I think we can just change the dg-warning to dg-bogus. Sure, dynamic_cast might fail, but AFAICT -Wnonnull isn't supposed to warn about arguments that *might* be null, only arguments that are *known* to be null. Done. I had added the 'if' to the test to make it clear why the warning isn't expected. I replaced it with a comment to that effect. FWIW, I do think a warning for dynamic_cast to a pointer would be helpful in the absence of an if statement in these cases: void f (B *p) { dynamic_cast(p)->g (); } Not because p may be null but because the result of the cast may be for a nonnull p. If it's f's precondition that D is derived from B (in addition to p being nonnull) the cast would be better written as one to a reference to D. Agreed, or if it isn't a precondition, if (D* dp = dynamic_cast(p)) dp->g(); Such a warning would need to be issued by the middle end and although it could be controlled by a new option (say -Wdynamic-cast, along with the cases discussed in PR 12277) Sounds good. I don't think issuing -Wnonnull in this case would be inappropriate. I disagree; issuing -Wnonnull for this case would be wrong. Again, -Wnonnull is supposed to warn when an argument *is* null, not when it *might* be null. I think warning about this case is a good idea, just not as part of -Wnonnull. Anyway, that's something to consider for GCC 12. For now, please see the revised patch. * rtti.c (ifnonnull): Rename... (build_nonnull_test): ...to this. Set no-warning bit on COND_EXPR. The ChangeLog needs updating. + /* Unlike static_cast, dynamic cast may fail for a nonnull operand but Yes, but... + since the front end can't see if the cast is guarded against being + invoked with a null No; my point was that whether the cast is guarded against being invoked with a null is no more relevant for dynamic_cast than it is for static_cast, as both casts give a null result for a null argument. For this test, dynamic_cast is not significantly different from static_cast. For both casts, the bug was the compiler warning about a nullptr that it introduced itself. It seems like splitting hairs. The comment (much as the original if guard) is just meant to explain why -Wnonnull isn't expected in case a new warning is added that detects the bad assumption above. I want to make it clear that if/when that happens a failure here doesn't mislead the author into thinking we don't want any warning there at all. I have reworded the comments yet again. verify there's no warning. See also pr99251. */ Yes. - dynamic_cast(p->bptr ())->g (); // { dg-warning "\\\[-Wnonnull" } + dynamic_cast(p)->g (); // { dg-bogus "\\\[-Wnonnull" } Please put back the ->bptr()s. Done. Martin Jason PR c++/99251 - inconsistent -Wnonnull warning behaviour with dynamic_cast gcc/cp/ChangeLog: PR c++/99251 * class.c (build_base_path): Call build_if_nonnull. * cp-tree.h (build_if_nonnull): Declare. * rtti.c (ifnonnull): Rename... (build_if_nonnull): ...to this. Set no-warning bit on COND_EXPR. (build_dynamic_cast_1): Adjust to name change. gcc/testsuite/ChangeLog: PR c++/99251 * g++.dg/warn/Wnonnull9.C: Expect no warnings. * g++.dg/warn/Wnonnull12.C: New test. diff --git a/gcc/cp/class.c b/gcc/cp/class.c index
Re: [PATCH] avoid -Wnonull for dynamic_cast (PR 99251)
On 3/1/21 12:10 PM, Martin Sebor wrote: On 2/24/21 8:13 PM, Jason Merrill wrote: On 2/24/21 5:25 PM, Martin Sebor wrote: In r11-6900 (PR 98646 - static_cast confuses -Wnonnull) we decided that issuing -Wnonnull for dereferencing the result of dynamic_cast was helpful despite the false positives it causes when the pointer is guaranteed not to be null because of a prior test. The test case in PR 99251 along with the feedback I got from Martin Liska have convinced me it was not the right decision. The attached patch arranges for dynamic_cast to also suppress -Wnonnull analogously to static_cast. Since there already is a helper function that builds the if-not-null test (ifnonnull) and sets TREE_NO_WARNING, I factored out the corresponding code from build_base_path that sets the additional TREE_NO_WARNING bit for static_cast into the function and called it from both places. I also renamed the function to make its purpose clearer and for consistency with other build_xxx APIs. Let's call it build_if_nonnull, as it builds the COND_EXPR as well as the test. Done. + /* The dynamic_cast might fail but so a warning might be justified + but not when the operand is guarded. See pr99251. */ + if (B *q = p->bptr ()) + dynamic_cast(q)->g (); // { dg-bogus "\\\[-Wnonnull" } This guard is no more necessary than it is for static_cast; both cases deal with null arguments. Let's not add these checks to the testcases. Done. This guard doesn't check for the mentioned case of dynamic_cast failing because the B* does not in fact point to a C. I think we can just change the dg-warning to dg-bogus. Sure, dynamic_cast might fail, but AFAICT -Wnonnull isn't supposed to warn about arguments that *might* be null, only arguments that are *known* to be null. Done. I had added the 'if' to the test to make it clear why the warning isn't expected. I replaced it with a comment to that effect. FWIW, I do think a warning for dynamic_cast to a pointer would be helpful in the absence of an if statement in these cases: void f (B *p) { dynamic_cast(p)->g (); } Not because p may be null but because the result of the cast may be for a nonnull p. If it's f's precondition that D is derived from B (in addition to p being nonnull) the cast would be better written as one to a reference to D. Agreed, or if it isn't a precondition, if (D* dp = dynamic_cast(p)) dp->g(); Such a warning would need to be issued by the middle end and although it could be controlled by a new option (say -Wdynamic-cast, along with the cases discussed in PR 12277) Sounds good. I don't think issuing -Wnonnull in this case would be inappropriate. I disagree; issuing -Wnonnull for this case would be wrong. Again, -Wnonnull is supposed to warn when an argument *is* null, not when it *might* be null. I think warning about this case is a good idea, just not as part of -Wnonnull. Anyway, that's something to consider for GCC 12. For now, please see the revised patch. * rtti.c (ifnonnull): Rename... (build_nonnull_test): ...to this. Set no-warning bit on COND_EXPR. The ChangeLog needs updating. + /* Unlike static_cast, dynamic cast may fail for a nonnull operand but Yes, but... + since the front end can't see if the cast is guarded against being + invoked with a null No; my point was that whether the cast is guarded against being invoked with a null is no more relevant for dynamic_cast than it is for static_cast, as both casts give a null result for a null argument. For this test, dynamic_cast is not significantly different from static_cast. For both casts, the bug was the compiler warning about a nullptr that it introduced itself. verify there's no warning. See also pr99251. */ Yes. - dynamic_cast(p->bptr ())->g (); // { dg-warning "\\\[-Wnonnull" } + dynamic_cast(p)->g ();// { dg-bogus "\\\[-Wnonnull" } Please put back the ->bptr()s. Jason
Re: [PATCH] avoid -Wnonull for dynamic_cast (PR 99251)
On 2/24/21 8:13 PM, Jason Merrill wrote: On 2/24/21 5:25 PM, Martin Sebor wrote: In r11-6900 (PR 98646 - static_cast confuses -Wnonnull) we decided that issuing -Wnonnull for dereferencing the result of dynamic_cast was helpful despite the false positives it causes when the pointer is guaranteed not to be null because of a prior test. The test case in PR 99251 along with the feedback I got from Martin Liska have convinced me it was not the right decision. The attached patch arranges for dynamic_cast to also suppress -Wnonnull analogously to static_cast. Since there already is a helper function that builds the if-not-null test (ifnonnull) and sets TREE_NO_WARNING, I factored out the corresponding code from build_base_path that sets the additional TREE_NO_WARNING bit for static_cast into the function and called it from both places. I also renamed the function to make its purpose clearer and for consistency with other build_xxx APIs. Let's call it build_if_nonnull, as it builds the COND_EXPR as well as the test. Done. + /* The dynamic_cast might fail but so a warning might be justified + but not when the operand is guarded. See pr99251. */ + if (B *q = p->bptr ()) + dynamic_cast(q)->g (); // { dg-bogus "\\\[-Wnonnull" } This guard is no more necessary than it is for static_cast; both cases deal with null arguments. Let's not add these checks to the testcases. Done. This guard doesn't check for the mentioned case of dynamic_cast failing because the B* does not in fact point to a C. I think we can just change the dg-warning to dg-bogus. Sure, dynamic_cast might fail, but AFAICT -Wnonnull isn't supposed to warn about arguments that *might* be null, only arguments that are *known* to be null. Done. I had added the 'if' to the test to make it clear why the warning isn't expected. I replaced it with a comment to that effect. FWIW, I do think a warning for dynamic_cast to a pointer would be helpful in the absence of an if statement in these cases: void f (B *p) { dynamic_cast(p)->g (); } Not because p may be null but because the result of the cast may be for a nonnull p. If it's f's precondition that D is derived from B (in addition to p being nonnull) the cast would be better written as one to a reference to D. Such a warning would need to be issued by the middle end and although it could be controlled by a new option (say -Wdynamic-cast, along with the cases discussed in PR 12277) I don't think issuing -Wnonnull in this case would be inappropriate. Anyway, that's something to consider for GCC 12. For now, please see the revised patch. Martin Jason PR c++/99251 - inconsistent -Wnonnull warning behaviour with dynamic_cast gcc/cp/ChangeLog: PR c++/99251 * class.c (build_base_path): Call build_nonnull_test. * cp-tree.h (build_nonnull_test): Declare. * rtti.c (ifnonnull): Rename... (build_nonnull_test): ...to this. Set no-warning bit on COND_EXPR. (build_dynamic_cast_1): Adjust to name change. gcc/testsuite/ChangeLog: PR c++/99251 * g++.dg/warn/Wnonnull9.C: Expect no warnings. * g++.dg/warn/Wnonnull12.C: New test. diff --git a/gcc/cp/class.c b/gcc/cp/class.c index 40f5fef7baa..b948b601d3f 100644 --- a/gcc/cp/class.c +++ b/gcc/cp/class.c @@ -402,16 +402,9 @@ build_base_path (enum tree_code code, if (TREE_SIDE_EFFECTS (expr) && (null_test || virtual_access)) expr = save_expr (expr); - /* Now that we've saved expr, build the real null test. */ + /* Store EXPR and build the real null test just before returning. */ if (null_test) -{ - tree zero = cp_convert (TREE_TYPE (expr), nullptr_node, complain); - null_test = build2_loc (input_location, NE_EXPR, boolean_type_node, - expr, zero); - /* This is a compiler generated comparison, don't emit - e.g. -Wnonnull-compare warning for it. */ - TREE_NO_WARNING (null_test) = 1; -} +null_test = expr; /* If this is a simple base reference, express it as a COMPONENT_REF. */ if (code == PLUS_EXPR && !virtual_access @@ -516,14 +509,8 @@ build_base_path (enum tree_code code, out: if (null_test) -{ - expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test, - expr, build_zero_cst (target_type)); - /* Avoid warning for the whole conditional expression (in addition - to NULL_TEST itself -- see above) in case the result is used in - a nonnull context that the front end -Wnonnull checks. */ - TREE_NO_WARNING (expr) = 1; -} +/* Wrap EXPR in a null test. */ +expr = build_if_nonnull (null_test, expr, complain); return expr; } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 38b31e3908f..a4f6bd67cef 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7271,6 +7271,7 @@ extern void emit_support_tinfos (void); extern bool emit_tinfo_decl (tree); extern unsigned get_pseudo_tinfo_index (tree); extern tree get_pseudo_tinfo_type (unsigned);
Re: [PATCH] avoid -Wnonull for dynamic_cast (PR 99251)
On 2/24/21 5:25 PM, Martin Sebor wrote: In r11-6900 (PR 98646 - static_cast confuses -Wnonnull) we decided that issuing -Wnonnull for dereferencing the result of dynamic_cast was helpful despite the false positives it causes when the pointer is guaranteed not to be null because of a prior test. The test case in PR 99251 along with the feedback I got from Martin Liska have convinced me it was not the right decision. The attached patch arranges for dynamic_cast to also suppress -Wnonnull analogously to static_cast. Since there already is a helper function that builds the if-not-null test (ifnonnull) and sets TREE_NO_WARNING, I factored out the corresponding code from build_base_path that sets the additional TREE_NO_WARNING bit for static_cast into the function and called it from both places. I also renamed the function to make its purpose clearer and for consistency with other build_xxx APIs. Let's call it build_if_nonnull, as it builds the COND_EXPR as well as the test. + /* The dynamic_cast might fail but so a warning might be justified + but not when the operand is guarded. See pr99251. */ + if (B *q = p->bptr ()) +dynamic_cast(q)->g ();// { dg-bogus "\\\[-Wnonnull" } This guard is no more necessary than it is for static_cast; both cases deal with null arguments. Let's not add these checks to the testcases. This guard doesn't check for the mentioned case of dynamic_cast failing because the B* does not in fact point to a C. I think we can just change the dg-warning to dg-bogus. Sure, dynamic_cast might fail, but AFAICT -Wnonnull isn't supposed to warn about arguments that *might* be null, only arguments that are *known* to be null. Jason
[PATCH] avoid -Wnonull for dynamic_cast (PR 99251)
In r11-6900 (PR 98646 - static_cast confuses -Wnonnull) we decided that issuing -Wnonnull for dereferencing the result of dynamic_cast was helpful despite the false positives it causes when the pointer is guaranteed not to be null because of a prior test. The test case in PR 99251 along with the feedback I got from Martin Liska have convinced me it was not the right decision. The attached patch arranges for dynamic_cast to also suppress -Wnonnull analogously to static_cast. Since there already is a helper function that builds the if-not-null test (ifnonnull) and sets TREE_NO_WARNING, I factored out the corresponding code from build_base_path that sets the additional TREE_NO_WARNING bit for static_cast into the function and called it from both places. I also renamed the function to make its purpose clearer and for consistency with other build_xxx APIs. Tested on x86_64-linux. Martin PR c++/99251 - inconsistent -Wnonnull warning behaviour with dynamic_cast gcc/cp/ChangeLog: PR c++/99251 * class.c (build_base_path): Call build_nonnull_test. * cp-tree.h (build_nonnull_test): Declare. * rtti.c (ifnonnull): Rename... (build_nonnull_test): ...to this. Set no-warning bit on COND_EXPR. (build_dynamic_cast_1): Adjust to name change. gcc/testsuite/ChangeLog: PR c++/99251 * g++.dg/warn/Wnonnull9.C: Expect no warnings. * g++.dg/warn/Wnonnull12.C: New test. diff --git a/gcc/cp/class.c b/gcc/cp/class.c index 40f5fef7baa..6c6e0564bf9 100644 --- a/gcc/cp/class.c +++ b/gcc/cp/class.c @@ -402,16 +402,9 @@ build_base_path (enum tree_code code, if (TREE_SIDE_EFFECTS (expr) && (null_test || virtual_access)) expr = save_expr (expr); - /* Now that we've saved expr, build the real null test. */ + /* Store EXPR and build the real null test just before returning. */ if (null_test) -{ - tree zero = cp_convert (TREE_TYPE (expr), nullptr_node, complain); - null_test = build2_loc (input_location, NE_EXPR, boolean_type_node, - expr, zero); - /* This is a compiler generated comparison, don't emit - e.g. -Wnonnull-compare warning for it. */ - TREE_NO_WARNING (null_test) = 1; -} +null_test = expr; /* If this is a simple base reference, express it as a COMPONENT_REF. */ if (code == PLUS_EXPR && !virtual_access @@ -516,14 +509,8 @@ build_base_path (enum tree_code code, out: if (null_test) -{ - expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test, - expr, build_zero_cst (target_type)); - /* Avoid warning for the whole conditional expression (in addition - to NULL_TEST itself -- see above) in case the result is used in - a nonnull context that the front end -Wnonnull checks. */ - TREE_NO_WARNING (expr) = 1; -} +/* Wrap EXPR in a null test. */ +expr = build_nonnull_test (null_test, expr, complain); return expr; } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 38b31e3908f..8c6cda8d1a6 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7271,6 +7271,7 @@ extern void emit_support_tinfos (void); extern bool emit_tinfo_decl (tree); extern unsigned get_pseudo_tinfo_index (tree); extern tree get_pseudo_tinfo_type (unsigned); +extern tree build_nonnull_test (tree, tree, tsubst_flags_t); /* in search.c */ extern tree get_parent_with_private_access (tree decl, tree binfo); diff --git a/gcc/cp/rtti.c b/gcc/cp/rtti.c index b41d95469c6..84482743392 100644 --- a/gcc/cp/rtti.c +++ b/gcc/cp/rtti.c @@ -121,7 +121,6 @@ vec *unemitted_tinfo_decls; and are generated as needed. */ static GTY (()) vec *tinfo_descs; -static tree ifnonnull (tree, tree, tsubst_flags_t); static tree tinfo_name (tree, bool); static tree build_dynamic_cast_1 (location_t, tree, tree, tsubst_flags_t); static tree throw_bad_cast (void); @@ -529,16 +528,23 @@ get_typeid (tree type, tsubst_flags_t complain) /* Check whether TEST is null before returning RESULT. If TEST is used in RESULT, it must have previously had a save_expr applied to it. */ -static tree -ifnonnull (tree test, tree result, tsubst_flags_t complain) +tree +build_nonnull_test (tree test, tree result, tsubst_flags_t complain) { - tree cond = build2 (NE_EXPR, boolean_type_node, test, - cp_convert (TREE_TYPE (test), nullptr_node, complain)); + tree null_ptr = cp_convert (TREE_TYPE (test), nullptr_node, complain); + tree cond = build2 (NE_EXPR, boolean_type_node, test, null_ptr); + /* This is a compiler generated comparison, don't emit e.g. -Wnonnull-compare warning for it. */ TREE_NO_WARNING (cond) = 1; - return build3 (COND_EXPR, TREE_TYPE (result), cond, result, - cp_convert (TREE_TYPE (result), nullptr_node, complain)); + + null_ptr = cp_convert (TREE_TYPE (result), nullptr_node, complain); + cond = build3 (COND_EXPR, TREE_TYPE (result), cond, result, null_ptr); + + /* Likewise, don't emit -Wnonnull for using the result to call + a member function. */ + TREE_NO_WARNING (cond) = 1;