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<C*>(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<D*>(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<D*>(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<const C*>(p->bptr ())->g (); // { dg-warning
"\\\[-Wnonnull" }
+ dynamic_cast<const C*>(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 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);
+extern tree build_if_nonnull (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..5a33b83afd0 100644
--- a/gcc/cp/rtti.c
+++ b/gcc/cp/rtti.c
@@ -121,7 +121,6 @@ vec<tree, va_gc> *unemitted_tinfo_decls;
and are generated as needed. */
static GTY (()) vec<tinfo_s, va_gc> *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_if_nonnull (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;
+ return cond;
}
/* Execute a dynamic cast, as described in section 5.2.6 of the 9/93 working
@@ -671,7 +677,7 @@ build_dynamic_cast_1 (location_t loc, tree type, tree expr,
expr1 = build_headof (expr);
if (TREE_TYPE (expr1) != type)
expr1 = build1 (NOP_EXPR, type, expr1);
- return ifnonnull (expr, expr1, complain);
+ return build_if_nonnull (expr, expr1, complain);
}
else
{
@@ -786,7 +792,7 @@ build_dynamic_cast_1 (location_t loc, tree type, tree expr,
/* Now back to the type we want from a void*. */
result = cp_convert (type, result, complain);
- return ifnonnull (expr, result, complain);
+ return build_if_nonnull (expr, result, complain);
}
}
else
diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull12.C b/gcc/testsuite/g++.dg/warn/Wnonnull12.C
new file mode 100644
index 00000000000..7b2606302f5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wnonnull12.C
@@ -0,0 +1,29 @@
+/* PR c++/99251 - inconsistent -Wnonnull warning behaviour with dynamic_cast
+ { dg-do compile }
+ { dg-options "-Wall" } */
+
+struct A
+{
+ virtual ~A ();
+};
+
+struct B: A
+{
+ int f (int);
+};
+
+int f1 (A *p)
+{
+ if (!p)
+ return 0;
+
+ return (dynamic_cast<B *>(p))->f (1);
+}
+
+int f2 (A *p)
+{
+ if (!p)
+ return 0;
+
+ return dynamic_cast<B *>(p)->f (2); // { dg-bogus "\\\[-Wnonnull" }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull9.C b/gcc/testsuite/g++.dg/warn/Wnonnull9.C
index b6135c4a624..b0a70aa0d79 100644
--- a/gcc/testsuite/g++.dg/warn/Wnonnull9.C
+++ b/gcc/testsuite/g++.dg/warn/Wnonnull9.C
@@ -38,13 +38,17 @@ void static_cast_const_C_ptr (B *p)
void dynamic_cast_C_ptr (B *p)
{
- // The dynamic_cast might fail so a warning is justified.
- dynamic_cast<C*>(p->bptr ())->g (); // { dg-warning "\\\[-Wnonnull" }
+ /* Unlike static_cast, dynamic cast may return null even for a nonnull
+ operand but detecting assumptions to the contrary isn't -Wnonnull's
+ purpose. Verify -Wnonnull isn't issued, either for the implicitly
+ emitted null check or for other reasons (the latter may be worth
+ warning for by some other warning). See also pr99251. */
+ dynamic_cast<C*>(p->bptr ())->g (); // { dg-bogus "\\\[-Wnonnull" }
}
void dynamic_cast_const_C_ptr (B *p)
{
- dynamic_cast<const C*>(p->bptr ())->g (); // { dg-warning "\\\[-Wnonnull" }
+ dynamic_cast<const C*>(p->bptr ())->g (); // { dg-bogus "\\\[-Wnonnull" }
}
@@ -107,11 +111,15 @@ void static_cast_const_D_ptr (B *p)
void dynamic_cast_D_ptr (B *p)
{
- // The dynamic_cast might fail so a warning is justified.
- dynamic_cast<D*>(p->bptr ())->g (); // { dg-warning "\\\[-Wnonnull" }
+ /* Unlike static_cast, dynamic cast may return null even for a nonnull
+ operand but detecting assumptions to the contrary isn't -Wnonnull's
+ purpose. Verify -Wnonnull isn't issued, either for the implicitly
+ emitted null check or for other reasons (the latter may be worth
+ warning for by some other warning). See also pr99251. */
+ dynamic_cast<D*>(p->bptr ())->g (); // { dg-bogus "\\\[-Wnonnull" }
}
void dynamic_cast_const_D_ptr (B *p)
{
- dynamic_cast<const D*>(p->bptr ())->g (); // { dg-warning "\\\[-Wnonnull" }
+ dynamic_cast<const D*>(p->bptr ())->g (); // { dg-bogus "\\\[-Wnonnull" }
}
