Re: [PING^2] Re: [PATCH] Caller instrumentation with -finstrument-calls

[Jan Hubicka <hubicka at ucw dot cz>]

Hi,
I apologize for long time for the review.

> > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> > index 8f7f5e5..f3ad003 100644
> > --- a/gcc/c-family/c-common.c
> > +++ b/gcc/c-family/c-common.c
> > @@ -343,6 +343,8 @@ static tree handle_tls_model_attribute (tree *, tree, tree, int,
> >  					bool *);
> >  static tree handle_no_instrument_function_attribute (tree *, tree,
> >  						     tree, int, bool *);
> > +static tree handle_no_instrument_calls_attribute (tree *, tree,
> > +						     tree, int, bool *);
> >  static tree handle_malloc_attribute (tree *, tree, tree, int, bool *);
> >  static tree handle_returns_twice_attribute (tree *, tree, tree, int, bool *);
> >  static tree handle_no_limit_stack_attribute (tree *, tree, tree, int,
> > @@ -658,6 +660,9 @@ const struct attribute_spec c_common_attribute_table[] =
> >    { "no_instrument_function", 0, 0, true,  false, false,
> >  			      handle_no_instrument_function_attribute,
> >  			      false },
> > +  { "no_instrument_calls", 0, 0, true,  false, false,
> > +			      handle_no_instrument_calls_attribute,
> > +			      false },

So you are adding no_instrument_calls in addition to no_instrument_function and moreover you add
command line option to disable/enable this per line basis.  I see this follow the pre-existing
functio ninstrumentatio ncode, but can't we just make this part of the function instrumentation
instead of adding three new knobs for this? (i.e. have just -finstrument-calls that will be controlled
same way as function instrumentation).

> > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> > index 8170a80..4657e48 100644
> > --- a/gcc/c/c-decl.c
> > +++ b/gcc/c/c-decl.c
> > @@ -2287,6 +2287,8 @@ merge_decls (tree newdecl, tree olddecl, tree newtype, tree oldtype)
> >  	  DECL_NO_LIMIT_STACK (newdecl) |= DECL_NO_LIMIT_STACK (olddecl);
> >  	  DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (newdecl)
> >  	    |= DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (olddecl);
> > +	  DECL_NO_INSTRUMENT_CALLS_BEFORE_AFTER (newdecl)
> > +	    |= DECL_NO_INSTRUMENT_CALLS_BEFORE_AFTER (olddecl);
> >  	  TREE_THIS_VOLATILE (newdecl) |= TREE_THIS_VOLATILE (olddecl);
> >  	  DECL_IS_MALLOC (newdecl) |= DECL_IS_MALLOC (olddecl);
> >  	  DECL_IS_OPERATOR_NEW (newdecl) |= DECL_IS_OPERATOR_NEW (olddecl);

You do not need to introduce new DECL flag for this.  We used to do this in dark ages before
we had attributes in place, but this time we usually just look up the given attribute.

> > +@item -finstrument-calls
> > +@opindex finstrument-calls
> > +Generate instrumentation calls immediately before and after each
> > +function call. The following profiling functions will be called with
> > +the address of the function that is called between them. Use
> > +@code{__builtin_return_address(0)} inside the profiling functions to
> > +get the addresses from where they are called.
> > +
> > +@smallexample
> > +void __gnu_profile_call_before (void *fn);
> > +void __gnu_profile_call_after  (void *fn);
> > +@end smallexample

I would expect this to be useful i.e. for dynamic callgraph construction.
In this case I would expect to have two parameters (calling function
and called function) instead of just one (calling function, right?)
> > diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> > index e2ae893..8401278 100644
> > --- a/gcc/gimplify.c
> > +++ b/gcc/gimplify.c
> > @@ -89,6 +89,8 @@ static struct gimplify_omp_ctx *gimplify_omp_ctxp;
> >  
> >  /* Forward declaration.  */
> >  static enum gimplify_status gimplify_compound_expr (tree *, gimple_seq *, bool);
> > +static bool flag_instrument_calls_exclude_p (tree fndecl);
> > +static bool flag_instrument_functions_exclude_p (tree fndecl);
> >  
> >  /* Mark X addressable.  Unlike the langhook we expect X to be in gimple
> >     form and we don't do any syntax checking.  */
> > @@ -1153,6 +1155,63 @@ build_stack_save_restore (gimple *save, gimple *restore)
> >  			 1, tmp_var);
> >  }
> >  
> > +/* Returns the function decl that corresponds the function called in
> > +   CALL_EXPR if call instrumentation is enabled.  */
> > +
> > +static tree
> > +addr_expr_for_call_instrumentation (tree call_expr)
> > +{
> > +  tree addr_expr = NULL_TREE;
> > +
> > +  if (!gimplify_ctxp->into_ssa && flag_instrument_calls_before_after
> > +      && !DECL_NO_INSTRUMENT_CALLS_BEFORE_AFTER (current_function_decl)
> > +      && !flag_instrument_calls_exclude_p (current_function_decl))
> > +    {
> > +      tree fndecl = get_callee_fndecl (call_expr);
> > +      if (fndecl)
> > +        {
> > +	  if (!DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (fndecl)
> > +	      && !flag_instrument_functions_exclude_p (fndecl))

Here you can just use lookup_attribute ("no_instrument", attributes) != NULL
instead of DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT.
> > @@ -2684,15 +2743,22 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value)
> >       gimplify_modify_expr.  */
> >    if (!want_value)
> >      {
> > +      tree addr_expr = addr_expr_for_call_instrumentation (*expr_p);
> > +      gimple_stmt_iterator gsi;
> > +
> > +      maybe_add_profile_call (addr_expr, BUILT_IN_PROFILE_CALL_BEFORE, pre_p);
> > +
> >        /* The CALL_EXPR in *EXPR_P is already in GIMPLE form, so all we
> >  	 have to do is replicate it as a GIMPLE_CALL tuple.  */
> > -      gimple_stmt_iterator gsi;
> >        call = gimple_build_call_from_tree (*expr_p);
> >        gimple_call_set_fntype (call, TREE_TYPE (fnptrtype));
> >        notice_special_calls (call);
> >        gimplify_seq_add_stmt (pre_p, call);
> >        gsi = gsi_last (*pre_p);
> >        fold_stmt (&gsi);
> > +
> > +      maybe_add_profile_call (addr_expr, BUILT_IN_PROFILE_CALL_AFTER, pre_p);
> > +
> >        *expr_p = NULL_TREE;
> >      }
> >    else

Note that adding instrumentation at gimplification time will make you to miss calls produced later
(i.e. by libcalls, or EH exapnsion or GOMP) and it will also make you to record calls that got inlined.
I suppose this is intended behaviour, what is the main use case for this code?

> > +
> > +      name = lang_hooks.decl_printable_name (fndecl, 0);

The printable names get uninformative for C++ (i.e. all ctors/dtors are called the same way).
I would think of favour of dropping those knobs in favour of functio nattributes...
> > diff --git a/gcc/libfuncs.h b/gcc/libfuncs.h
> > index 04a4dc2..4c7faa4 100644
> > --- a/gcc/libfuncs.h
> > +++ b/gcc/libfuncs.h
> > @@ -40,6 +40,9 @@ enum libfunc_index
> >    LTI_profile_function_entry,
> >    LTI_profile_function_exit,
> >  
> > +  LTI_profile_call_before,
> > +  LTI_profile_call_after,
> > +
> >    LTI_synchronize,
> >  
> >    LTI_gcov_flush,
> > @@ -98,6 +101,9 @@ extern struct target_libfuncs *this_target_libfuncs;
> >  #define profile_function_entry_libfunc	(libfunc_table[LTI_profile_function_entry])
> >  #define profile_function_exit_libfunc	(libfunc_table[LTI_profile_function_exit])
> >  
> > +#define profile_call_before_libfunc	(libfunc_table[LTI_profile_call_before])
> > +#define profile_call_after_libfunc	(libfunc_table[LTI_profile_call_after])
> > +
> >  #define synchronize_libfunc	(libfunc_table[LTI_synchronize])
> >  
> >  #define gcov_flush_libfunc	(libfunc_table[LTI_gcov_flush])
> > diff --git a/gcc/optabs.c b/gcc/optabs.c
> > index a3051ad..04d149c 100644
> > --- a/gcc/optabs.c
> > +++ b/gcc/optabs.c
> > @@ -6202,6 +6202,12 @@ init_optabs (void)
> >    profile_function_exit_libfunc
> >      = init_one_libfunc ("__cyg_profile_func_exit");
> >  
> > +  /* For call before/after instrumentation.  */
> > +  profile_call_before_libfunc
> > +    = init_one_libfunc ("__gnu_profile_call_before");
> > +  profile_call_after_libfunc
> > +    = init_one_libfunc ("__gnu_profile_call_after");

I do not think we need libcalls for this, since the conversio happens on expansion time.
It seems that the function_entry/function_exit was not removed when the code was upgraded.
So please rmeove those, too.

The patch seems reasonable to me with the changes above. Afte removal of the DECL bits
it should become quite small and localized, so it seems the maintenance costs of the
code should not be too big.  Do you have FSF copyright assignment?

Honza