2015-03-24 11:33 GMT+03:00 Jakub Jelinek <[email protected]>:
> 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.
Agree, overhead for not instrumented code should be minimized.
Unfortunately there is no option check I can use to guard chkp codes
due to LTO. Currently it is allowed to pass -fcheck-pointer-bounds for
IL generation and don't pass it for final code generation. I suppose I
may set this (or some new) flag if see instrumented node when read
cgraph and then use it to guard chkp related codes. Would it be
acceptable?
>
> 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).
Maybe replace attribute usage with a new flag in tree_decl_with_vis structure?
>
>> + 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.
Will fix it.
>
> 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.
The most popular guard call should be chkp_function_instrumented_p.
Replacing attribute with a flag should help.
Thanks for review!
Ilya
>
> Jakub
