Re: [PATCH] Support asan-instrumentation-with-call-threshold in GCC

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, May 26, 2014 at 07:58:17PM +0400, Yury Gribov wrote:
> This patch adds support for outline Asan instrumentation (i.e.
> function calls instead of inline checks). This has been recently
> added to LLVM to
> * reduce long compiler runtimes on large functions with huge (over
> 10K) number of memory accesses
> (http://llvm.org/bugs/show_bug.cgi?id=12653)
> * a step to full Kasan support in GCC
> * reduce code size overhead
> 
> Implementation is a bit different from LLVM: GCC starts generating
> function calls after overflowing the threshold whereas LLVM replaces
> all instrumentations once total number is estimated to be larger
> than treshold. Making implementations more similar would require
> bigger rewrite of Asan which I would prefer to avoid.
> 
> The patch consists of two parts:
> * asan_negative_params_1.diff - support for negative parameters

I don't like this.  Why do you need it?
Negative params look like a bad idea to me.  If you want to
have different values for none, all and some, then you can use
e.g. 0 as a special value and number of accesses >= param
for the other one, or one can be 0, another INT_MAX, etc.,
-1, 0 and positive doesn't make things any clearer.

> * asan_calls_1.diff - the proper

Note, the patch conflicts with the recently posted asan patches,
please wait until those are reviewed.

> --- 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;

I don't see this variable ever changed, so don't see how the patch can
actually work.
> @@ -1476,17 +1506,30 @@ 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;
> +  bool use_calls = asan_num_accesses > ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;

Too long line.

> @@ -1687,6 +1798,8 @@ 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;

Likewise.

> @@ -2265,6 +2386,9 @@ initialize_sanitizer_builtins (void)
>    tree BT_FN_VOID_PTR_PTR
>      = build_function_type_list (void_type_node, ptr_type_node,
>  				ptr_type_node, NULL_TREE);
> +  tree BT_FN_VOID_PTR_SIZE
> +    = build_function_type_list (void_type_node, ptr_type_node,
> +				size_type_node, NULL_TREE);

I thought the functions use uptr arguments, aka BT_FN_VOID_PTR_PTRMODE.

	Jakub