On Tue, Feb 16, 2016 at 11:12:27AM -0700, Jeff Law wrote: > >Not sure if it matters but wouldn't walking over function parameters > >and their default def SSA names immediate uses be cheaper than > >matching all conditions? Esp. as nonnull_arg_p walks over all > >DECL_ARGUMENTS (up to the searched index) over and over again? > >[we should simply set a flag on the PARM_DECL!] > Seems like a waste to burn a flag bit for something like this. > > We're already walking all the statements in this code so it's really just > the cost of testing if the existing statement is "interesting" for this > warning. I guess you're suggesting jakub walk the arguments and if one is a > marked as non-null, then walk the immediate uses. Yea, that's probably > faster than doing the test for every statement.
I'm already bootstrapping/regtesting following variant. > Swapping the nonnull_arg_p and the test for whether or not op1 is 0 would be > a slight help as well, particularly if there was machine generated code with > oodles of arguments. > > I'm not sure why we're testing OFFSET_TYPE against -1. That is because NULL pointers to member are represented as -1 in the middle end. 2016-02-16 Jakub Jelinek <ja...@redhat.com> PR c/69835 * common.opt (Wnonnull-compare): New warning. * doc/invoke.texi (-Wnonnull): Remove text about comparison of arguments against NULL. (-Wnonnull-compare): Document. * Makefile.in (OBJS): Add gimple-ssa-nonnull-compare.o. * tree-pass.h (make_pass_warn_nonnull_compare): Declare. * passes.def (pass_warn_nonnull_compare): Add. * gimple-ssa-nonnull-compare.c: New file. c-family/ * c.opt (Wnonnull-compare): Enable for -Wall. c/ * c-typeck.c (build_binary_op): Revert 2015-09-09 change. cp/ * typeck.c (cp_build_binary_op): Revert 2015-09-09 change. testsuite/ * c-c++-common/nonnull-1.c: Use -Wnonnull-compare instead of -Wnonnull in dg-options. * c-c++-common/nonnull-2.c: New test. --- gcc/common.opt.jj 2016-02-16 16:14:40.787071893 +0100 +++ gcc/common.opt 2016-02-16 17:34:12.850648209 +0100 @@ -616,6 +616,10 @@ Wlarger-than= Common RejectNegative Joined UInteger Warning -Wlarger-than=<number> Warn if an object is larger than <number> bytes. +Wnonnull-compare +Var(warn_nonnull_compare) Warning +Warn if comparing pointer parameter with nonnull attribute with NULL. + Wnull-dereference Common Var(warn_null_dereference) Warning Warn if dereferencing a NULL pointer may lead to erroneous or undefined behavior. --- gcc/doc/invoke.texi.jj 2016-02-16 16:14:41.692059415 +0100 +++ gcc/doc/invoke.texi 2016-02-16 17:26:55.346587296 +0100 @@ -276,7 +276,8 @@ Objective-C and Objective-C++ Dialects}. -Wmain -Wmaybe-uninitialized -Wmemset-transposed-args @gol -Wmisleading-indentation -Wmissing-braces @gol -Wmissing-field-initializers -Wmissing-include-dirs @gol --Wno-multichar -Wnonnull -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol +-Wno-multichar -Wnonnull -Wnonnull-compare @gol +-Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol -Wnull-dereference -Wodr -Wno-overflow -Wopenmp-simd @gol -Woverride-init-side-effects -Woverlength-strings @gol -Wpacked -Wpacked-bitfield-compat -Wpadded @gol @@ -3537,6 +3538,7 @@ Options} and @ref{Objective-C and Object -Wmissing-braces @r{(only for C/ObjC)} @gol -Wnarrowing @r{(only for C++)} @gol -Wnonnull @gol +-Wnonnull-compare @gol -Wopenmp-simd @gol -Wparentheses @gol -Wpointer-sign @gol @@ -3795,12 +3797,18 @@ formats that may yield only a two-digit Warn about passing a null pointer for arguments marked as requiring a non-null value by the @code{nonnull} function attribute. -Also warns when comparing an argument marked with the @code{nonnull} -function attribute against null inside the function. - @option{-Wnonnull} is included in @option{-Wall} and @option{-Wformat}. It can be disabled with the @option{-Wno-nonnull} option. +@item -Wnonnull-compare +@opindex Wnonnull-compare +@opindex Wno-nonnull-compare +Warn when comparing an argument marked with the @code{nonnull} +function attribute against null inside the function. + +@option{-Wnonnull-compare} is included in @option{-Wall}. It +can be disabled with the @option{-Wno-nonnull-compare} option. + @item -Wnull-dereference @opindex Wnull-dereference @opindex Wno-null-dereference --- gcc/Makefile.in.jj 2016-02-11 20:28:54.000000000 +0100 +++ gcc/Makefile.in 2016-02-16 17:32:26.919086223 +0100 @@ -1278,6 +1278,7 @@ OBJS = \ gimple-pretty-print.o \ gimple-ssa-backprop.o \ gimple-ssa-isolate-paths.o \ + gimple-ssa-nonnull-compare.o \ gimple-ssa-split-paths.o \ gimple-ssa-strength-reduction.o \ gimple-streamer-in.o \ --- gcc/tree-pass.h.jj 2016-01-19 13:31:07.000000000 +0100 +++ gcc/tree-pass.h 2016-02-16 17:28:39.155178102 +0100 @@ -472,6 +472,7 @@ extern gimple_opt_pass *make_pass_oacc_k extern simple_ipa_opt_pass *make_pass_ipa_oacc (gcc::context *ctxt); extern simple_ipa_opt_pass *make_pass_ipa_oacc_kernels (gcc::context *ctxt); extern gimple_opt_pass *make_pass_gen_hsail (gcc::context *ctxt); +extern gimple_opt_pass *make_pass_warn_nonnull_compare (gcc::context *ctxt); /* IPA Passes */ extern simple_ipa_opt_pass *make_pass_ipa_lower_emutls (gcc::context *ctxt); --- gcc/passes.def.jj 2016-02-10 16:01:57.000000000 +0100 +++ gcc/passes.def 2016-02-16 17:27:46.515892676 +0100 @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. NEXT_PASS (pass_fixup_cfg); NEXT_PASS (pass_init_datastructures); NEXT_PASS (pass_build_ssa); + NEXT_PASS (pass_warn_nonnull_compare); NEXT_PASS (pass_ubsan); NEXT_PASS (pass_early_warn_uninitialized); NEXT_PASS (pass_nothrow); --- gcc/gimple-ssa-nonnull-compare.c.jj 2016-02-16 16:58:10.676272364 +0100 +++ gcc/gimple-ssa-nonnull-compare.c 2016-02-16 17:46:19.788692474 +0100 @@ -0,0 +1,150 @@ +/* -Wnonnull-compare warning support. + Copyright (C) 2016 Free Software Foundation, Inc. + +This file is part of GCC. + +GCC is free software; you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation; either version 3, or (at your option) +any later version. + +GCC is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with GCC; see the file COPYING3. If not see +<http://www.gnu.org/licenses/>. */ + +#include "config.h" +#include "system.h" +#include "coretypes.h" +#include "backend.h" +#include "tree.h" +#include "gimple.h" +#include "tree-pass.h" +#include "ssa.h" +#include "diagnostic-core.h" +#include "tree-dfa.h" + +/* Warn about comparison of nonnull_arg_p argument initial values + with NULL. */ + +static void +do_warn_nonnull_compare (function *fun, tree arg) +{ + if (!POINTER_TYPE_P (TREE_TYPE (arg)) + && TREE_CODE (TREE_TYPE (arg)) != OFFSET_TYPE) + return; + + if (!nonnull_arg_p (arg)) + return; + + tree d = ssa_default_def (fun, arg); + if (d == NULL_TREE) + return; + + use_operand_p use_p; + imm_use_iterator iter; + + FOR_EACH_IMM_USE_FAST (use_p, iter, d) + { + gimple *stmt = USE_STMT (use_p); + tree op = NULL_TREE; + location_t loc = gimple_location (stmt); + if (gimple_code (stmt) == GIMPLE_COND) + switch (gimple_cond_code (stmt)) + { + case EQ_EXPR: + case NE_EXPR: + if (gimple_cond_lhs (stmt) == d) + op = gimple_cond_rhs (stmt); + break; + default: + break; + } + else if (is_gimple_assign (stmt)) + switch (gimple_assign_rhs_code (stmt)) + { + case EQ_EXPR: + case NE_EXPR: + if (gimple_assign_rhs1 (stmt) == d) + op = gimple_assign_rhs2 (stmt); + break; + case COND_EXPR: + switch (TREE_CODE (gimple_assign_rhs1 (stmt))) + { + case EQ_EXPR: + case NE_EXPR: + op = gimple_assign_rhs1 (stmt); + if (TREE_OPERAND (op, 0) != d) + { + op = NULL_TREE; + break; + } + loc = EXPR_LOC_OR_LOC (op, loc); + op = TREE_OPERAND (op, 1); + break; + default: + break; + } + break; + default: + break; + } + if (op + && (POINTER_TYPE_P (TREE_TYPE (arg)) + ? integer_zerop (op) : integer_minus_onep (op))) + warning_at (loc, OPT_Wnonnull_compare, + "nonnull argument %qD compared to NULL", arg); + } +} + +namespace { + +const pass_data pass_data_warn_nonnull_compare = +{ + GIMPLE_PASS, /* type */ + "*nonnullcmp", /* name */ + OPTGROUP_NONE, /* optinfo_flags */ + TV_NONE, /* tv_id */ + PROP_ssa, /* properties_required */ + 0, /* properties_provided */ + 0, /* properties_destroyed */ + 0, /* todo_flags_start */ + 0, /* todo_flags_finish */ +}; + +class pass_warn_nonnull_compare : public gimple_opt_pass +{ +public: + pass_warn_nonnull_compare (gcc::context *ctxt) + : gimple_opt_pass (pass_data_warn_nonnull_compare, ctxt) + {} + + /* opt_pass methods: */ + virtual bool gate (function *) { return warn_nonnull_compare; } + + virtual unsigned int execute (function *); + +}; // class pass_warn_nonnull_compare + +unsigned int +pass_warn_nonnull_compare::execute (function *fun) +{ + if (fun->static_chain_decl) + do_warn_nonnull_compare (fun, fun->static_chain_decl); + + for (tree arg = DECL_ARGUMENTS (cfun->decl); arg; arg = DECL_CHAIN (arg)) + do_warn_nonnull_compare (fun, arg); + return 0; +} + +} // anon namespace + +gimple_opt_pass * +make_pass_warn_nonnull_compare (gcc::context *ctxt) +{ + return new pass_warn_nonnull_compare (ctxt); +} --- gcc/c-family/c.opt.jj 2016-02-16 16:14:40.895070404 +0100 +++ gcc/c-family/c.opt 2016-02-16 17:26:55.347587282 +0100 @@ -677,6 +677,10 @@ Wnonnull C ObjC C++ ObjC++ LangEnabledBy(C ObjC C++ ObjC++,Wall) ; +Wnonnull-compare +C ObjC C++ ObjC++ LangEnabledBy(C ObjC C++ ObjC++,Wall) +; + Wnormalized C ObjC C++ ObjC++ Warning Alias(Wnormalized=,nfc,none) ; --- gcc/c/c-typeck.c.jj 2016-02-16 16:14:40.636073975 +0100 +++ gcc/c/c-typeck.c 2016-02-16 17:26:55.350587242 +0100 @@ -11086,11 +11086,6 @@ build_binary_op (location_t location, en short_compare = 1; else if (code0 == POINTER_TYPE && null_pointer_constant_p (orig_op1)) { - if (warn_nonnull - && TREE_CODE (op0) == PARM_DECL && nonnull_arg_p (op0)) - warning_at (location, OPT_Wnonnull, - "nonnull argument %qD compared to NULL", op0); - if (TREE_CODE (op0) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0))) { @@ -11111,11 +11106,6 @@ build_binary_op (location_t location, en } else if (code1 == POINTER_TYPE && null_pointer_constant_p (orig_op0)) { - if (warn_nonnull - && TREE_CODE (op1) == PARM_DECL && nonnull_arg_p (op1)) - warning_at (location, OPT_Wnonnull, - "nonnull argument %qD compared to NULL", op1); - if (TREE_CODE (op1) == ADDR_EXPR && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0))) { --- gcc/cp/typeck.c.jj 2016-02-16 16:14:40.657073685 +0100 +++ gcc/cp/typeck.c 2016-02-16 17:26:55.352587215 +0100 @@ -4514,11 +4514,6 @@ cp_build_binary_op (location_t location, || (code0 == POINTER_TYPE && TYPE_PTR_P (type1) && integer_zerop (op1))) { - if (warn_nonnull - && TREE_CODE (op0) == PARM_DECL && nonnull_arg_p (op0)) - warning_at (location, OPT_Wnonnull, - "nonnull argument %qD compared to NULL", op0); - if (TYPE_PTR_P (type1)) result_type = composite_pointer_type (type0, type1, op0, op1, CPO_COMPARISON, complain); @@ -4558,11 +4553,6 @@ cp_build_binary_op (location_t location, || (code1 == POINTER_TYPE && TYPE_PTR_P (type0) && integer_zerop (op0))) { - if (warn_nonnull - && TREE_CODE (op1) == PARM_DECL && nonnull_arg_p (op1)) - warning_at (location, OPT_Wnonnull, - "nonnull argument %qD compared to NULL", op1); - if (TYPE_PTR_P (type0)) result_type = composite_pointer_type (type0, type1, op0, op1, CPO_COMPARISON, complain); --- gcc/testsuite/c-c++-common/nonnull-1.c.jj 2016-02-16 16:14:40.310078470 +0100 +++ gcc/testsuite/c-c++-common/nonnull-1.c 2016-02-16 17:26:55.352587215 +0100 @@ -1,7 +1,7 @@ /* Test for the bad usage of "nonnull" function attribute parms. */ /* */ /* { dg-do compile } */ -/* { dg-options "-Wnonnull" } */ +/* { dg-options "-Wnonnull-compare" } */ #include <stddef.h> #include <stdlib.h> --- gcc/testsuite/c-c++-common/nonnull-2.c.jj 2016-02-16 17:26:55.353587201 +0100 +++ gcc/testsuite/c-c++-common/nonnull-2.c 2016-02-16 17:26:55.353587201 +0100 @@ -0,0 +1,26 @@ +/* Test for the bad usage of "nonnull" function attribute parms. */ +/* { dg-do compile } */ +/* { dg-options "-Wnonnull-compare" } */ + +void bar (char **); + +__attribute__((nonnull (1, 3))) int +foo (char *cp1, char *cp2, char *cp3, char *cp4) +{ + if (cp1 == (char *) 0) /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */ + return 1; + + cp1 = cp2; + if (cp1 == (char *) 0) /* { dg-bogus "nonnull argument" } */ + return 2; + + if (!cp4) /* { dg-bogus "nonnull argument" } */ + return 3; + + char **p = &cp3; + bar (p); + if (cp3 == (char *) 0) /* { dg-bogus "nonnull argument" } */ + return 4; + + return 5; +} Jakub