On Sun, 2021-08-15 at 20:28 +0530, Ankur Saini wrote: > The patch extends the analyser’s functionality to understand and > analyze indirect calls (calls via a function pointer or calls to > virtual functions ) > > On successful merging, the patch should also fix the following bugs :- > > 1. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100546 > 2. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97114 >
Thanks, this is looking promising. Has this been rebased recently (e.g. since I merged https://gcc.gnu.org/pipermail/gcc-patches/2021-August/576737.html ) and have you run the test suite on this version? I've posted various minor nits below, and a request for more test coverage, but this is close to being ready to commit. > Subject: [PATCH 1/2] analyzer: detect and analyze calls via function pointer > 2021-07-29 Ankur Saini <arse...@sourceware.org>> > gcc/analyzer/ChangeLog: [...snip...] The ChangeLog entries should reference the pertinent bugs with: PR analyzer/97114 PR analyzer/100546 See the existing ChangeLog file for examples of how this is done. [...snip...] > /* class rewind_info_t : public exploded_edge::custom_info_t. */ > > /* Implementation of exploded_edge::custom_info_t::update_model vfunc > @@ -2980,6 +3024,61 @@ state_change_requires_new_enode_p (const program_state > &old_state, > return false; > } > > +/* Create enodes and eedges for the function calls that doesn't have an > + underlying call superedge. > + > + Such case occurs when GCC's middle end didn't know which function to > + call but analyzer do (with the help of current state). Grammar nit: "but analyzer do" -> "but the analyzer does" [...snip...] > @@ -3174,10 +3273,13 @@ exploded_graph::process_node (exploded_node *node) > break; > case PK_AFTER_SUPERNODE: > { > + bool found_a_superedge = false; > + bool is_an_exit_block = false; > /* If this is an EXIT BB, detect leaks, and potentially > create a function summary. */ > if (point.get_supernode ()->return_p ()) > { > + is_an_exit_block = true; > node->detect_leaks (*this); > if (flag_analyzer_call_summaries > && point.get_call_string ().empty_p ()) > @@ -3205,6 +3307,7 @@ exploded_graph::process_node (exploded_node *node) > superedge *succ; > FOR_EACH_VEC_ELT (point.get_supernode ()->m_succs, i, succ) > { > + found_a_superedge = true; > if (logger) > logger->log ("considering SN: %i -> SN: %i", > succ->m_src->m_index, succ->m_dest->m_index); > @@ -3214,6 +3317,54 @@ exploded_graph::process_node (exploded_node *node) > point.get_call_string ()); > program_state next_state (state); > uncertainty_t uncertainty; > + > + /* Make use the current state and try to discover and analyse > + indirect function calls (calls that doesn't hage underlying Grammar and spelling nit: "calls that doesn't hage" -> "a call that doesn't have an" [...snip...] > diff --git a/gcc/analyzer/exploded-graph.h b/gcc/analyzer/exploded-graph.h > index 8f48d8a286c..acfd5fd7167 100644 > --- a/gcc/analyzer/exploded-graph.h > +++ b/gcc/analyzer/exploded-graph.h > @@ -362,6 +362,37 @@ private: > DISABLE_COPY_AND_ASSIGN (exploded_edge); > }; > > +/* Extra data for an exploded_edge that represents a dynamic call info ( > calls > + that doesn't have a superedge representing the call ). */ Similar grammar nit here [...snip...] > diff --git a/gcc/analyzer/state-purge.cc b/gcc/analyzer/state-purge.cc > index e82ea87e735..c4050c2cf9a 100644 > --- a/gcc/analyzer/state-purge.cc > +++ b/gcc/analyzer/state-purge.cc > @@ -377,18 +377,32 @@ state_purge_per_ssa_name::process_point (const > function_point &point, > { > /* Add any intraprocedually edge for a call. */ > if (snode->m_returning_call) > - { > - cgraph_edge *cedge > + { > + gcall *returning_call = snode->m_returning_call; > + cgraph_edge *cedge > = supergraph_call_edge (snode->m_fun, > - snode->m_returning_call); > - gcc_assert (cedge); > - superedge *sedge > - = map.get_sg ().get_intraprocedural_edge_for_call (cedge); > - gcc_assert (sedge); > - add_to_worklist > - (function_point::after_supernode (sedge->m_src), > - worklist, logger); > - } > + returning_call); I think that in the following it would be more readable to reverse the sense of the condition, so that rather than: if (!cedge) { // code for NULL cedge } else { // code for non-NULL cedge } instead have: if (cedge) { // code for non-NULL cedge } else { // code for NULL cedge } to avoid having a double-negative when reasoning about the "else" clause. > + if(!cedge) > + { > + supernode *callernode > + = map.get_sg ().get_supernode_for_stmt (returning_call); > + > + gcc_assert (callernode); > + add_to_worklist > + (function_point::after_supernode (callernode), > + worklist, logger); > + } > + else > + { > + gcc_assert (cedge); > + superedge *sedge > + = map.get_sg ().get_intraprocedural_edge_for_call (cedge); > + gcc_assert (sedge); > + add_to_worklist > + (function_point::after_supernode (sedge->m_src), > + worklist, logger); > + } > + } > } > } [...snip...] > diff --git a/gcc/testsuite/gcc.dg/analyzer/function-ptr-4.c > b/gcc/testsuite/gcc.dg/analyzer/function-ptr-4.c > new file mode 100644 > index 00000000000..016351a83f6 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/analyzer/function-ptr-4.c > @@ -0,0 +1,24 @@ > +/* Test to see if the analyzer detect and analyze calls via > + function pointers or not. */ > + > +#include <stdlib.h> > + > +void fun(int *int_ptr) > +{ > + free(int_ptr); /* { dg-warning "double-'free' of 'int_ptr'" } */ > +} > + > +void single_call() > +{ > + int *int_ptr = (int*)malloc(sizeof(int)); > + void (*fun_ptr)(int *) = &fun; > + (*fun_ptr)(int_ptr); > +} > + > +void double_call() > +{ > + int *int_ptr = (int*)malloc(sizeof(int)); > + void (*fun_ptr)(int *) = &fun; > + (*fun_ptr)(int_ptr); /* { dg-message "calling 'fun' from 'double_call'" > } */ Would be good to also have a dg-message for the return event. > + (*fun_ptr)(int_ptr); > +} [...snip...] > From 173c36d4576ba548be58c07b0182c87434bd6854 Mon Sep 17 00:00:00 2001 > From: Ankur Saini <arse...@sourceware.org> > Date: Sun, 15 Aug 2021 19:19:07 +0530 > Subject: [PATCH 2/2] analyzer: detect and analyze virtul function calls Typo: "virtul" -> "virtual" > 2021-08-15 Ankur Saini <arse...@sourceware.org> ChangeLog entries should reference the pertinent bug IDs as above. > gcc/analyzer/ChangeLog: > * analyzer/region-model.cc (region_model::get_rvalue_1): Add case for > OBJ_TYPE_REF. > > gcc/testsuite/ChangeLog: > *g++.dg/analyzer/vfunc-2.C: New test. > *g++.dg/analyzer/vfunc-3.C: New test. [...snip...] > diff --git a/gcc/testsuite/g++.dg/analyzer/vfunc-2.C > b/gcc/testsuite/g++.dg/analyzer/vfunc-2.C > new file mode 100644 > index 00000000000..46b68e529e6 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/vfunc-2.C > @@ -0,0 +1,44 @@ > +#include <cstdio> > +#include <cstdlib> > + > +struct A > +{ > + int m_data; > + A() {m_data = 0;} > + virtual int deallocate (void) > + { > + return 42; > + } > +}; > + > +struct B: public A > +{ > + int *ptr; > + int m_data_b; > + B() {m_data_b = 0;} > + void allocate () > + { > + ptr = (int*)malloc(sizeof(int)); > + } > + int deallocate (void) > + { > + free(ptr); > + return 0; > + } > +}; > + > +void foo(A *a_ptr) > +{ > + printf("%d\n",a_ptr->deallocate()); > +} > + > +void test() > +{ > + B b; > + A a, *aptr; > + aptr = &b; > + b.allocate(); > + foo(aptr); > + aptr = &a; > + foo(aptr); > +} > diff --git a/gcc/testsuite/g++.dg/analyzer/vfunc-3.C > b/gcc/testsuite/g++.dg/analyzer/vfunc-3.C > new file mode 100644 > index 00000000000..bd00bb436c8 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/vfunc-3.C > @@ -0,0 +1,36 @@ > +#include <cstdlib> > + > +struct A > +{ > + virtual int foo (void) > + { > + return 42; > + } > +}; > + > +struct B: public A > +{ > + int *ptr; > + void alloc () > + { > + ptr = (int*)malloc(sizeof(int)); > + } > + int foo (void) > + { > + free(ptr); /* { dg-warning "double-'free' of 'b.B::ptr'" } */ > + return 0; > + } > +}; > + > +int test() > +{ > + struct B b, *bptr=&b; > + b.alloc(); > + bptr->foo(); /* { dg-message "calling 'B::foo' from 'test'" } */ > + return bptr->foo(); > +} > + > +int main() > +{ > + test(); > +} Looks good. Would be nice (but not needed) to have some more test coverage: * some even simpler test coverage e.g. using __analyzer_dump_path and __analyzer_eval to verify the most trivial vfunc calls that the optimizer can't figure out. * a test that verifies the ASCII art of the calls/returns of a vfunc. See e.g. gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-double-free.c for an example (you ought to be able to copy and paste the output into the test case). But this is looking close to ready to commit. Dave
