Re: [PATCH] Optimize ASAN_CHECK checks

[Dodji Seketeli <dodji at redhat dot com>]

Jakub Jelinek <jakub@redhat.com> writes:

> On Wed, Nov 12, 2014 at 02:09:59PM +0300, Yury Gribov wrote:
> > >For asan you're right, we can have addresses of decls there etc.
> > >If you have spare cycles, feel free to take over the patch and adjust it.
> > 
> > I guess I'd wait when this gets to trunk?
> 
> Ok, in that case I've bootstrapped/regtested on x86_64-linux/i686-linux what I have with
> the ASAN_CHECK_NON_ZERO_LEN stuff removed from it (all non-INTEGER_CST
> lengths ignored).  Dodji, is this ok for trunk?

[...]

> +++ gcc/sanopt.c	2014-11-12 21:04:50.007325020 +0100
> 
>  /* This is used to carry information about basic blocks.  It is
> @@ -56,7 +57,29 @@ along with GCC; see the file COPYING3.
>  
>  struct sanopt_info
>  {

[...]

> +  /* True if there is a block with HAS_FREEING_CALL_P flag set
> +     on any a path between imm(BB) and BB.  */

s/a//.

Also, I'd rather say "on any path between an immediate dominator of
BB, denoted imm(BB), and BB".  That way, subsequent uses of imm(BB)
makes sense more for the new comer.  This is only a suggestion.  If
you feel that formulation is obvious enough, then please ignore my
comment.

> +  bool imm_dom_path_with_freeing_call_p;

[...]

 };

[...]
 
> +/* Return true if there might be any call to free/munmap operation
> +   on any path in between DOM (which should be imm(BB)) and BB.  */
> +
> +static bool
> +imm_dom_path_with_freeing_call (basic_block bb, basic_block dom)
> +{

To ease maintainability, maybe we could assert that:

   gcc_assert (dom == get_immediate_dominator(CDI_DOMINATORS, bb));

?

And thus remove the assert that is at the caller site of this
function, later in maybe_optimize_asan_check_ifn:

> +	  basic_block imm = get_immediate_dominator (CDI_DOMINATORS, last_bb);
> +	  gcc_assert (imm);
> +	  if (imm_dom_path_with_freeing_call (last_bb, imm))
> +	    break;


Also, when the 'dom' basic block is NULL, couldn't we just return
immediately?

[...]

> +}


[...]

> +/* Optimize away redundant ASAN_CHECK calls.  */
> +
> +static bool
> +maybe_optimize_asan_check_ifn (struct sanopt_ctx *ctx, gimple stmt)
> +{
> +  gcc_assert (gimple_call_num_args (stmt) == 4);
> +  tree ptr = gimple_call_arg (stmt, 1);
> +  tree len = gimple_call_arg (stmt, 2);
> +  basic_block bb = gimple_bb (stmt);
> +  sanopt_info *info = (sanopt_info *) bb->aux;
> +
> +  if (TREE_CODE (len) != INTEGER_CST)
> +    return false;
> +  if (integer_zerop (len))
> +    return false;
> +
> +  gimple_set_uid (stmt, info->freeing_call_events);

I am not sure, but I am wondering if we shouldn't save the previous uid
of 'stmt' here before setting it, and then restore it before getting out
of this function.

[...]

> +}
> +
>  /* Try to optimize away redundant UBSAN_NULL checks.
>     
>     We walk blocks in the CFG via a depth first search of the dominator
> @@ -89,111 +402,77 @@ sanopt_optimize_walker (basic_block bb,

I think the comment of this sanopt_optimize_walker function should now
be adapted to say that it optimizes away redundant UBSAN_NULL *and*
ASAN_CHECK internal function calls.

>  {

[...]

>  }

[...]

OK with those changes.

Thanks.

-- 
		Dodji