Re: daily report on extending static analyzer project [GSoC]

[David Malcolm <dmalcolm@redhat.com>]

On Thu, 2021-07-29 at 18:20 +0530, Ankur Saini wrote:
> I have attached the patches(one is the updated version of previous
> patch to 
> detect calls via function pointers) of the changed done to make the
> analyzer 
> understand the calls to virtual functions for initial review. 
> 
> 1. I decided to make a dedicated function to create enodes and eedges
> for the 
> dynamically discovered calls as I found myself using the exact same
> peice of 
> code again to analyse vfunc calls.

Makes sense.

> 
> 2. Boostaraping and testing of these changes are underway.
> 
> 3. Regarding the regress tests that have to be added to test
> functionality of 
> vfunc extension patch :
> Should I add many test files for different types of inheritences or
> should I 
> add one ( or two ) test files, with a lot of fucntions in them testing
> different 
> types of calls ?

Both approaches have merit, and there's an element of personal taste.

I find that during development and debugging it's handy to have the
tests broken out into individual files, but it's good to eventually
combine the tests to minimize the number of invocations that the test
harness has to do.

That said, interprocedural tests tend to be fiddly, so it's often good
to keep these in separate files.

I tend to combine my tests and add them to git, and then to temporarily
trim them down when debugging them to minimize the amoung of unrelated
stuff I'm having to look at when debugging, knowing that git has the
full version saved.

I hope that answers your question.

> 
> ---
> Patches :

This isn't a full review, but...

fn_ptr.patch:

> 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..c62510c026f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/function-ptr-4.c
> @@ -0,0 +1,25 @@
> +/*Test to see if the analyzer detect and analyze calls via 
> +  fucntion pointers or not. */
> +
> +#include <stdio.h>
> +#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);
> +	(*fun_ptr)(int_ptr);
> +}

...thinking back to our discussion about events, it would be good to
verify that the analyzer is emitting them.  You can put directives
like:

   /* { dg-message "calling 'fun' from 'double_call'" } */

on the appropriate lines to test this via DejaGnu.

"analyzer: detect and analyzer vfunc calls"

[...snip...]

> @@ -1242,6 +1243,17 @@ exploded_node::on_stmt (exploded_graph &eg,
>  	unknown_side_effects = false;
>      }
>  
> +  /* If the statmement is a polymorphic call then assume 
> +     there are no side effects.  */
> +  gimple *call_stmt = const_cast<gimple *>(stmt);
> +  if (gcall *call = dyn_cast<gcall *> (call_stmt))
> +  {
> +    function *fun = this->get_function();
> +    cgraph_edge *e = cgraph_node::get (fun->decl)->get_edge (call);
> +    if ((e && e->indirect_info) && (e->indirect_info->polymorphic))
> +        unknown_side_effects = false;
> +  }
> +

This seems wrong; surely it depends on what the call is - or am I
missing something?  Is the issue that we're speculating lots of
possibilities as dynamic calls?  If so, would it be better to terminate
the remaining analysis path (if that makes sense), and assume that any
further analysis happens on extra edges added for the speculated calls?

FWIW I've been experimenting with adding "bifurcation" support so that
you can do:
  program_state *other = ctxt->bifurcate ();
and have it split the analysis into states (e.g. for handling realloc,
so that we can split into 3 states: "succeeded", "succeeded but moved",
"failed").  Unfortunately my code for this is a mess (it's a hacked up
prototype).  Should I try to post what I have for this?


[...snip...]


> @@ -3327,9 +3338,44 @@ exploded_graph::process_node (exploded_node *node)
>                  region_model *model = state.m_region_model;
>  
>                  /* Call is possibly happening via a function pointer.  */
> -                if (tree fn_decl = model->get_fndecl_for_call(call,&ctxt))
> -                  create_dynamic_call (call, fn_decl, node, next_state,
> -                                       next_point, &uncertainty);
> +                if (tree fn_decl = model->get_fndecl_for_call (call,&ctxt))
> +                  create_dynamic_call (call,
> +                  		       fn_decl,
> +                  		       node,
> +                  		       next_state,
> +                                       next_point,
> +                                       &uncertainty);
> +                else
> +                  {
> +                    /* Call is possibly a polymorphic call.
> +
> +                       In such case, use devirtisation tools to find 
> +                       possible callees of this function call and let the 
> +                       analyzer specluate them all.  */
> +		    function *fun = node->get_function ();
> +                    gcall *stmt  = const_cast<gcall *>(call);
> +                    cgraph_edge *e
> +                      = cgraph_node::get (fun->decl)->get_edge (stmt);
> +                    if (e->indirect_info->polymorphic)
> +                      {
> +			void *cache_token;
> +			bool final;
> +			vec <cgraph_node *> targets
> +			  = possible_polymorphic_call_targets (e,
> +							       &final,
> +							       &cache_token,
> +							       true);
> +	       		for (cgraph_node *x : targets)
> +	       		  {
> +	       		    create_dynamic_call (stmt,
> +						 x->decl,
> +						 node,
> +						 next_state,
> +						 next_point,
> +						 &uncertainty);
> +	       		  }
> +                      }
> +                  }

Interesting.

Caveat: I'm not familiar with the devirt code.

If we're speculating that a particular call happens, do we gain
information about what kind of object we're dealing with?

If we have a repeated call to the same vfunc, do we know that we're
calling the same function?

[...snip...]

I'm interested in seeing what test cases you have.


Hope this is helpful
Dave