On Thu, Mar 19, 2015 at 11:29:44AM +0300, Ilya Enkovich wrote:
> + /* We might propagate instrumented function pointer into
> + not instrumented function and vice versa. In such a
> + case we need to either fix function declaration or
> + remove bounds from call statement. */
> + if (callee)
> + skip_bounds = chkp_redirect_edge (e);
I just want to say that I'm not really excited about all this compile time
cost that is added everywhere unconditionally for chkp.
I think much better would be to guard most of it with proper option check
first and only do the more expensive part if the option has been used.
In particular, the above call isn't inlined,
> +bool
> +chkp_redirect_edge (cgraph_edge *e)
> +{
> + bool instrumented = false;
> + tree decl = e->callee->decl;
> +
> + if (e->callee->instrumentation_clone
> + || chkp_function_instrumented_p (decl))
> + instrumented = true;
Calls here for non-instrumented code another function that calls
lookup_attribute (cheap if DECL_ATTRIBUTES is NULL, not really cheap
otherwise).
> + if (instrumented
> + && !gimple_call_with_bounds_p (e->call_stmt))
> + e->redirect_callee (cgraph_node::get_create (e->callee->orig_decl));
> + else if (!instrumented
> + && !chkp_gimple_call_builtin_p (e->call_stmt, BUILT_IN_CHKP_BNDCL)
> + && !chkp_gimple_call_builtin_p (e->call_stmt, BUILT_IN_CHKP_BNDCU)
> + && !chkp_gimple_call_builtin_p (e->call_stmt, BUILT_IN_CHKP_BNDSTX)
> + && gimple_call_with_bounds_p (e->call_stmt))
Plus the ordering of the conditions above is bad, you first check
for 3 out of a few thousands builtin and only then call the predicate,
which should be probably done right after the !instrumented case.
So, for the very likely case of -fcheck-pointer-bounds not being used at
all, you've added at least 7-8 non-inlinable calls here.
There are dozens of similar calls inserted just about everywhere.
Jakub
