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

Reply via email to