On Thu, 2024-11-14 at 11:19 +0100, Jakub Jelinek wrote: > Hi! Hi; sorry for the delay in responding to this.
> > On top of the > https://gcc.gnu.org/pipermail/gcc-patches/2024-November/668554.html > patch which introduces the nonnull_if_nonzero attribute (because > C2Y is allowing NULL arguments on various calls like memcpy, memset, > strncpy etc. as long as the count is 0) the following patch adds just > limited handling of the attribute in the analyzer. > > For nonnull attribute(s) we have the get_nonnull_args helper which > returns a bitmap, for nonnull_if_nonzero a function would need to > return a hash_map or something similar, I think it is better to > handle the attributes one by one. This patch just handles the > non-zero INTEGER_CST (integer_nonzerop) count arguments, in other > places > the above patch uses ranger to some extent, but I'm not familiar > enough > with the analyzer to know if one can use the ranger, or should > somehow > explain in data structures the conditional nature of the nonnull > property, I haven't used ranger yet within the analyzer, and I wonder if there is a philosophical divide here between the goals of optimization versus bug finding: the optimizer makes use of undefined behavior in order to add assumptions about the state of the code ("X can't happen"), whereas the analyzer ought to report on places where undefined behavior could happen ("if X happened here, it would be bad, let's warn the user"). One way of expressing the conditional nature of this property in the analyzer would be to "bifurcate" the analysis so that there are two out-edges in the exploded graph, covering the ptr == NULL vs ptr != NULL conditions explicitly and separately. But that's fiddly, alas, and not something to be doing for GCC 15 at this stage (sorry). So how you have it in this patch seems reasonable for GCC 15. > the argument is nonnull only if some other argument is nonzero. > > Also, analyzer uses get_nonnull_args in another spot when entering a > frame, > not sure if anything can be done there (note the conditional nonnull > somehow, pass from callers if the argument is nonzero, ...). > > Note, the testsuite changes aren't strictly necessary with just > the above and this patch, but will be with a patch I'm going to post > soon. > > Ok for trunk? I see that the code from state_t state = sm_ctxt.get_state (stmt, arg); onwards seems to be a copy-and-paste of the existing code from within the /* Handle "__attribute__((nonnull))". */ suite. malloc_state_machine::on_stmt is already longer than I'm comfortable with, so please can you introduce a subroutine to avoid the copy-and-paste (probably a private member function of malloc_state_machine). OK for trunk with that change (assuming usual testing, of course) > > Could you improve this in stage3? I can take a look now, but improving this in the analyzer is probably now stage 1 material; sorry again Dave > > 2024-11-14 Jakub Jelinek <ja...@redhat.com> > > PR c/117023 > gcc/analyzer/ > * sm-malloc.cc (malloc_state_machine::on_stmt): Handle > also nonnull_if_nonzero attributes. > gcc/testsuite/ > * c-c++-common/analyzer/call-summaries-malloc.c > (test_use_without_check): Pass 4 rather than sz to memset. > * c-c++-common/analyzer/strncpy-1.c (test_null_dst, > test_null_src): Pass 42 rather than count to strncpy. > > --- gcc/analyzer/sm-malloc.cc.jj 2024-10-24 > 22:56:14.111158433 +0200 > +++ gcc/analyzer/sm-malloc.cc 2024-11-14 10:39:37.590835715 +0100 > @@ -2150,6 +2150,58 @@ malloc_state_machine::on_stmt (sm_contex > } > BITMAP_FREE (nonnull_args); > } > + /* Handle __attribute__((nonnull_if_nonzero (x, y))). > */ > + if (fntype) > + for (tree attrs = TYPE_ATTRIBUTES (fntype); > + (attrs = lookup_attribute ("nonnull_if_nonzero", > attrs)); > + attrs = TREE_CHAIN (attrs)) > + { > + tree args = TREE_VALUE (attrs); > + unsigned int idx = TREE_INT_CST_LOW (TREE_VALUE > (args)) - 1; > + unsigned int idx2 > + = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN > (args))) - 1; > + if (idx < gimple_call_num_args (stmt) > + && idx2 < gimple_call_num_args (stmt)) > + { > + tree arg = gimple_call_arg (stmt, idx); > + tree arg2 = gimple_call_arg (stmt, idx2); > + if (TREE_CODE (TREE_TYPE (arg)) != > POINTER_TYPE > + || !INTEGRAL_TYPE_P (TREE_TYPE (arg2)) > + || integer_zerop (arg2)) > + continue; > + if (integer_nonzerop (arg2)) > + ; > + else > + /* FIXME: Use ranger here to query arg2 > range? */ > + continue; > + state_t state = sm_ctxt.get_state (stmt, arg); > + /* Can't use a switch as the states are non- > const. */ > + /* Do use the fndecl that caused the warning > so that the > + misused attributes are printed and the user > not > + confused. */ > + if (unchecked_p (state)) > + { > + tree diag_arg = > sm_ctxt.get_diagnostic_tree (arg); > + sm_ctxt.warn (node, stmt, arg, > + make_unique<possible_null_ar > g> > + (*this, diag_arg, fndecl, > idx)); > + const allocation_state *astate > + = as_a_allocation_state (state); > + sm_ctxt.set_next_state (stmt, arg, > + astate- > >get_nonnull ()); > + } > + else if (state == m_null) > + { > + tree diag_arg = > sm_ctxt.get_diagnostic_tree (arg); > + sm_ctxt.warn (node, stmt, arg, > + make_unique<null_arg> > + (*this, diag_arg, fndecl, > idx)); > + sm_ctxt.set_next_state (stmt, arg, > m_stop); > + } > + else if (state == m_start) > + maybe_assume_non_null (sm_ctxt, arg, stmt); > + } > + } > } > > /* Check for this after nonnull, so that if we have both > --- gcc/testsuite/c-c++-common/analyzer/call-summaries- > malloc.c.jj 2024-09-18 15:03:34.578092119 +0200 > +++ gcc/testsuite/c-c++-common/analyzer/call-summaries- > malloc.c 2024-11-14 10:54:35.445125885 +0100 > @@ -67,7 +67,7 @@ void test_use_after_free (void) > void test_use_without_check (size_t sz) > { > char *buf = (char *) wrapped_malloc (sz); /* { dg-message "this > call could return NULL" } */ > - memset (buf, 'x', sz); /* { dg-warning "use of possibly-NULL 'buf' > where non-null expected" } */ > + memset (buf, 'x', 4); /* { dg-warning "use of possibly-NULL 'buf' > where non-null expected" } */ > wrapped_free (buf); > } > > --- gcc/testsuite/c-c++-common/analyzer/strncpy-1.c.jj 2024-01-18 > 22:04:50.983005923 +0100 > +++ gcc/testsuite/c-c++-common/analyzer/strncpy-1.c 2024-11-14 > 10:55:26.035410085 +0100 > @@ -20,13 +20,13 @@ test_passthrough (char *dst, const char > char * > test_null_dst (const char *src, size_t count) > { > - return strncpy (NULL, src, count); /* { dg-warning "use of NULL > where non-null expected" } */ > + return strncpy (NULL, src, 42); /* { dg-warning "use of NULL where > non-null expected" } */ > } > > char * > test_null_src (char *dst, size_t count) > { > - return strncpy (dst, NULL, count); /* { dg-warning "use of NULL > where non-null expected" } */ > + return strncpy (dst, NULL, 42); /* { dg-warning "use of NULL where > non-null expected" } */ > } > > void > > Jakub >
