Re: [PATCH] Support asan-instrumentation-with-call-threshold in GCC (second try)

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Jun 03, 2014 at 12:03:02PM +0400, Yury Gribov wrote:
> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -257,6 +257,8 @@ struct asan_mem_ref
>  
>  static alloc_pool asan_mem_ref_alloc_pool;
>  
> +static int asan_num_accesses;
> +
>  /* This creates the alloc pool used to store the instances of
>     asan_mem_ref that are stored in the hash table asan_mem_ref_ht.  */
>  
> @@ -1335,6 +1337,26 @@ report_error_func (bool is_store, HOST_WIDE_INT size_in_bytes, bool slow_p)
>    return builtin_decl_implicit (report[is_store][exact_log2 (size_in_bytes)]);
>  }
>  
> +/* Construct a function tree for __asan_{load,store}{1,2,4,8,16,_n}.
> +   IS_STORE is either 1 (for a store) or 0 (for a load).  */
> +
> +static tree
> +check_func (bool is_store, int size_in_bytes, int slow_p)
> +{
> +  static enum built_in_function check[2][6]
> +    = { { BUILT_IN_ASAN_CHECK_LOAD1, BUILT_IN_ASAN_CHECK_LOAD2,
> +      BUILT_IN_ASAN_CHECK_LOAD4, BUILT_IN_ASAN_CHECK_LOAD8,
> +      BUILT_IN_ASAN_CHECK_LOAD16, BUILT_IN_ASAN_CHECK_LOAD_N },
> +	{ BUILT_IN_ASAN_CHECK_STORE1, BUILT_IN_ASAN_CHECK_STORE2,
> +      BUILT_IN_ASAN_CHECK_STORE4, BUILT_IN_ASAN_CHECK_STORE8,
> +      BUILT_IN_ASAN_CHECK_STORE16, BUILT_IN_ASAN_CHECK_STORE_N } };

Any reason why the BUILT_IN_* names so differ from the actual function
names?  I.e. why not use BUILT_IN_ASAN_{LOAD,STORE}{1,2,4,8,16,N}
(no underscore before N, no CHECK_)?

> @@ -1520,9 +1542,11 @@ build_check_stmt (location_t location, tree base, gimple_stmt_iterator *iter,
>    tree shadow_type = TREE_TYPE (shadow_ptr_type);
>    tree uintptr_type
>      = build_nonstandard_integer_type (TYPE_PRECISION (TREE_TYPE (base)), 1);
> -  tree base_ssa = base;
> +  tree base_ssa;
>    HOST_WIDE_INT real_size_in_bytes = size_in_bytes;
>    tree sz_arg = NULL_TREE;
> +  bool use_calls =
> +    asan_num_accesses++ >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;

Wouldn't it be better to do
  bool use_calls
    = asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;
(note, = on next line per GNU Coding Conventions), and then
  if (!use_calls)
    asan_num_accesses++;
so that you don't risk signed overflow?  Maybe also
  if (ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD == INT_MAX)
    use_calls = false;
before that, so that there is a way to turn the calls off completely.

> @@ -1534,15 +1558,27 @@ build_check_stmt (location_t location, tree base, gimple_stmt_iterator *iter,
>        slow_p = true;
>      }
>  
> -  /* Get an iterator on the point where we can add the condition
> -     statement for the instrumentation.  */
> -  gsi = create_cond_insert_point (iter, before_p,
> -				  /*then_more_likely_p=*/false,
> -				  /*create_then_fallthru_edge=*/false,
> -				  &then_bb,
> -				  &else_bb);
> +  base_ssa = base = unshare_expr (base);
>  
> -  base = unshare_expr (base);
> +  if (use_calls)
> +    {
> +      gsi = *iter;
> +      g = gimple_build_nop ();
> +      if (before_p)
> +	gsi_insert_before (&gsi, g, GSI_NEW_STMT);
> +      else
> +	gsi_insert_after (&gsi, g, GSI_NEW_STMT);

This looks ugly.  I don't like adding GIMPLE_NOP, it is going
to take many passes before it is going to be DCEd.
Just handle the use_calls && before_p case in the two spots
where a new stmt is inserted.

Also note (but, already preexisting issue) is that the
__asan_report* and __asan_{load,store}* calls are declared in
sanitizer.def as having 1st argument PTR type, while in the library
it is uptr (aka PTRMODE).  So, either we should fix up sanitizer.def
to match that (we were passing already base_addr which is integral,
not pointer), or if we keep sanitizer.def as is, we should pass
to the calls base_ssa instead of base_addr.

> @@ -1642,6 +1694,61 @@ build_check_stmt (location_t location, tree base, gimple_stmt_iterator *iter,
>    *iter = gsi_start_bb (else_bb);
>  }
>  
> +static void
> +build_check_stmt_sized (location_t location, tree base, tree len,
> +		  gimple_stmt_iterator *iter, bool is_store)

Formatting, gimple_stmt_iterator needs to be below location_t.

Furthermore, this looks just like a partial transition, I don't see
why we should emit one __asan_loadN call but two separate
__asan_report_load_1 for the instrumented stringops.

Instead, build_check_stmt should be changed, so that it is possible to pass
it a variable length (checked by the caller for non-zero already),
and in that case force using the slow_p stuff, but instead of adding a
constant size add the variable length - 1.

> @@ -1774,6 +1881,9 @@ instrument_mem_region_access (tree base, tree len,
>  			      gimple_stmt_iterator *iter,
>  			      location_t location, bool is_store)
>  {
> +  bool use_calls =
> +    asan_num_accesses++ >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;
> +

See above.

> @@ -1795,6 +1905,14 @@ instrument_mem_region_access (tree base, tree len,
>    if (start_instrumented && end_instrumented)
>      return;
>  
> +  if (use_calls)
> +    {
> +      build_check_stmt_sized (location, base, len, iter, is_store);
> +      update_mem_ref_hash_table (base, 1);
> +      update_mem_ref_hash_table (end, 1);
> +      return;
> +    }
> +

See above, here we should not handle use_calls any differently from
!use_calls.

	Jakub