| Message ID | 20141106121041.GA44122@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On 11/06/14 05:10, Ilya Enkovich wrote: > Hi, > > This patch enables instrumentation of chosen builtin calls. > > Thanks, > Ilya > -- > 2014-11-06 Ilya Enkovich <ilya.enkovich@intel.com> > > * ipa-chkp.c (chkp_versioning): Clone builtin functions. > (chkp_instrument_normal_builtin): New. > (chkp_add_bounds_to_call_stmt): Support builtin functions. > (chkp_replace_function_pointer): Likewise. > > > > diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c > index df7d425..9e2efdb 100644 > --- a/gcc/tree-chkp.c > +++ b/gcc/tree-chkp.c > @@ -1586,6 +1586,50 @@ chkp_find_bound_slots (const_tree type, bitmap res) > chkp_find_bound_slots_1 (type, res, 0); > } > > +/* Return 1 if call to FNDECL should be instrumented > + and 0 otherwise. */ > + > +static bool > +chkp_instrument_normal_builtin (tree fndecl) > +{ > + switch (DECL_FUNCTION_CODE (fndecl)) > + { > + case BUILT_IN_STRLEN: > + case BUILT_IN_STRCPY: > + case BUILT_IN_STRNCPY: > + case BUILT_IN_STPCPY: > + case BUILT_IN_STPNCPY: > + case BUILT_IN_STRCAT: > + case BUILT_IN_STRNCAT: > + case BUILT_IN_MEMCPY: > + case BUILT_IN_MEMPCPY: > + case BUILT_IN_MEMSET: > + case BUILT_IN_MEMMOVE: > + case BUILT_IN_BZERO: > + case BUILT_IN_STRCMP: > + case BUILT_IN_STRNCMP: > + case BUILT_IN_BCMP: > + case BUILT_IN_MEMCMP: > + case BUILT_IN_MEMCPY_CHK: > + case BUILT_IN_MEMPCPY_CHK: > + case BUILT_IN_MEMMOVE_CHK: > + case BUILT_IN_MEMSET_CHK: > + case BUILT_IN_STRCPY_CHK: > + case BUILT_IN_STRNCPY_CHK: > + case BUILT_IN_STPCPY_CHK: > + case BUILT_IN_STPNCPY_CHK: > + case BUILT_IN_STRCAT_CHK: > + case BUILT_IN_STRNCAT_CHK: > + case BUILT_IN_MALLOC: > + case BUILT_IN_CALLOC: > + case BUILT_IN_REALLOC: > + return 1; > + > + default: > + return 0; > + } > +} OK, this gates creation of the additional builtin and ensures we don't try to create an instrumention clone for anything outside the list above. > @@ -1686,11 +1730,18 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi) > if (!flag_chkp_instrument_calls) > return; > > - /* Avoid instrumented builtin functions for now. Due to IPA > - it also means we have to avoid instrumentation of indirect > - calls. */ > - if (fndecl && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN) > - return; > + /* We instrument only some subset of builtins. We also instrument > + builtin calls to be inlined. */ > + if (fndecl > + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL > + && !chkp_instrument_normal_builtin (fndecl)) > + { > + struct cgraph_node *clone = chkp_maybe_create_clone (fndecl); > + if (!clone > + || !gimple_has_body_p (clone->decl) > + || !lookup_attribute ("always_inline", DECL_ATTRIBUTES (fndecl))) > + return; > + } Is that outer conditional right? If we have a fndecl and it's a normal builtin, but it's _not_ one of hte builtins we're instrumenting, then call chkp_maybe_create_clone? Will reserve OK/Not OK decision until after you respond to that issue. jeff
2014-11-14 9:49 GMT+03:00 Jeff Law <law@redhat.com>: > On 11/06/14 05:10, Ilya Enkovich wrote: >> >> Hi, >> >> This patch enables instrumentation of chosen builtin calls. >> >> Thanks, >> Ilya >> -- >> 2014-11-06 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * ipa-chkp.c (chkp_versioning): Clone builtin functions. >> (chkp_instrument_normal_builtin): New. >> (chkp_add_bounds_to_call_stmt): Support builtin functions. >> (chkp_replace_function_pointer): Likewise. >> >> >> >> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c >> index df7d425..9e2efdb 100644 >> --- a/gcc/tree-chkp.c >> +++ b/gcc/tree-chkp.c >> @@ -1586,6 +1586,50 @@ chkp_find_bound_slots (const_tree type, bitmap res) >> chkp_find_bound_slots_1 (type, res, 0); >> } >> >> +/* Return 1 if call to FNDECL should be instrumented >> + and 0 otherwise. */ >> + >> +static bool >> +chkp_instrument_normal_builtin (tree fndecl) >> +{ >> + switch (DECL_FUNCTION_CODE (fndecl)) >> + { >> + case BUILT_IN_STRLEN: >> + case BUILT_IN_STRCPY: >> + case BUILT_IN_STRNCPY: >> + case BUILT_IN_STPCPY: >> + case BUILT_IN_STPNCPY: >> + case BUILT_IN_STRCAT: >> + case BUILT_IN_STRNCAT: >> + case BUILT_IN_MEMCPY: >> + case BUILT_IN_MEMPCPY: >> + case BUILT_IN_MEMSET: >> + case BUILT_IN_MEMMOVE: >> + case BUILT_IN_BZERO: >> + case BUILT_IN_STRCMP: >> + case BUILT_IN_STRNCMP: >> + case BUILT_IN_BCMP: >> + case BUILT_IN_MEMCMP: >> + case BUILT_IN_MEMCPY_CHK: >> + case BUILT_IN_MEMPCPY_CHK: >> + case BUILT_IN_MEMMOVE_CHK: >> + case BUILT_IN_MEMSET_CHK: >> + case BUILT_IN_STRCPY_CHK: >> + case BUILT_IN_STRNCPY_CHK: >> + case BUILT_IN_STPCPY_CHK: >> + case BUILT_IN_STPNCPY_CHK: >> + case BUILT_IN_STRCAT_CHK: >> + case BUILT_IN_STRNCAT_CHK: >> + case BUILT_IN_MALLOC: >> + case BUILT_IN_CALLOC: >> + case BUILT_IN_REALLOC: >> + return 1; >> + >> + default: >> + return 0; >> + } >> +} > > OK, this gates creation of the additional builtin and ensures we don't try > to create an instrumention clone for anything outside the list above. > > >> @@ -1686,11 +1730,18 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator >> *gsi) >> if (!flag_chkp_instrument_calls) >> return; >> >> - /* Avoid instrumented builtin functions for now. Due to IPA >> - it also means we have to avoid instrumentation of indirect >> - calls. */ >> - if (fndecl && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN) >> - return; >> + /* We instrument only some subset of builtins. We also instrument >> + builtin calls to be inlined. */ >> + if (fndecl >> + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL >> + && !chkp_instrument_normal_builtin (fndecl)) >> + { >> + struct cgraph_node *clone = chkp_maybe_create_clone (fndecl); >> + if (!clone >> + || !gimple_has_body_p (clone->decl) >> + || !lookup_attribute ("always_inline", DECL_ATTRIBUTES >> (fndecl))) >> + return; >> + } > > Is that outer conditional right? If we have a fndecl and it's a normal > builtin, but it's _not_ one of hte builtins we're instrumenting, then call > chkp_maybe_create_clone? Some builtin functions (especially their *_chk version) are defined as always_inline functions in headers. In this case we handle them as regular functions (clone and instrument) because they will be inlined anyway. Seems gimple_has_body_p should be applied to fndecl and moved into outer if-statement along with attribute check. Thus unneeded clones would be avoided. Thanks, Ilya > > > Will reserve OK/Not OK decision until after you respond to that issue. > > jeff
On 11/14/14 01:06, Ilya Enkovich wrote: >>> - /* Avoid instrumented builtin functions for now. Due to IPA >>> - it also means we have to avoid instrumentation of indirect >>> - calls. */ >>> - if (fndecl && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN) >>> - return; >>> + /* We instrument only some subset of builtins. We also instrument >>> + builtin calls to be inlined. */ >>> + if (fndecl >>> + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL >>> + && !chkp_instrument_normal_builtin (fndecl)) >>> + { >>> + struct cgraph_node *clone = chkp_maybe_create_clone (fndecl); >>> + if (!clone >>> + || !gimple_has_body_p (clone->decl) >>> + || !lookup_attribute ("always_inline", DECL_ATTRIBUTES >>> (fndecl))) >>> + return; >>> + } >> >> Is that outer conditional right? If we have a fndecl and it's a normal >> builtin, but it's _not_ one of hte builtins we're instrumenting, then call >> chkp_maybe_create_clone? > > Some builtin functions (especially their *_chk version) are defined as > always_inline functions in headers. In this case we handle them as > regular functions (clone and instrument) because they will be inlined > anyway. Seems gimple_has_body_p should be applied to fndecl and moved > into outer if-statement along with attribute check. Thus unneeded > clones would be avoided. So the outer condition is, essentially checking for a _chk builtin and if we find it, try to create a clone for instrumentation purposes. I think the always_inline attribute lookup can move to the outer-if. Less sure about the gimple_has_body check though. Presumably clone->decl is going to refer to fndecl? If so, then that can move to the outer if as well. As you say, that may cut down on the number of clones that get created. OK as-is, or with those two conditions moved out of level as long as all prerequisites are approved and it passes a bootstrap & regression test. JEff
diff --git a/gcc/ipa-chkp.c b/gcc/ipa-chkp.c index 691adfd..ee7de48 100644 --- a/gcc/ipa-chkp.c +++ b/gcc/ipa-chkp.c @@ -577,8 +577,9 @@ chkp_versioning (void) && (!flag_chkp_instrument_marked_only || lookup_attribute ("bnd_instrument", DECL_ATTRIBUTES (node->decl))) - /* No builtins instrumentation for now. */ - && DECL_BUILT_IN_CLASS (node->decl) == NOT_BUILT_IN) + && (!DECL_BUILT_IN (node->decl) + || (DECL_BUILT_IN_CLASS (node->decl) == BUILT_IN_NORMAL + && DECL_FUNCTION_CODE (node->decl) < BEGIN_CHKP_BUILTINS))) chkp_maybe_create_clone (node->decl); } diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c index df7d425..9e2efdb 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -1586,6 +1586,50 @@ chkp_find_bound_slots (const_tree type, bitmap res) chkp_find_bound_slots_1 (type, res, 0); } +/* Return 1 if call to FNDECL should be instrumented + and 0 otherwise. */ + +static bool +chkp_instrument_normal_builtin (tree fndecl) +{ + switch (DECL_FUNCTION_CODE (fndecl)) + { + case BUILT_IN_STRLEN: + case BUILT_IN_STRCPY: + case BUILT_IN_STRNCPY: + case BUILT_IN_STPCPY: + case BUILT_IN_STPNCPY: + case BUILT_IN_STRCAT: + case BUILT_IN_STRNCAT: + case BUILT_IN_MEMCPY: + case BUILT_IN_MEMPCPY: + case BUILT_IN_MEMSET: + case BUILT_IN_MEMMOVE: + case BUILT_IN_BZERO: + case BUILT_IN_STRCMP: + case BUILT_IN_STRNCMP: + case BUILT_IN_BCMP: + case BUILT_IN_MEMCMP: + case BUILT_IN_MEMCPY_CHK: + case BUILT_IN_MEMPCPY_CHK: + case BUILT_IN_MEMMOVE_CHK: + case BUILT_IN_MEMSET_CHK: + case BUILT_IN_STRCPY_CHK: + case BUILT_IN_STRNCPY_CHK: + case BUILT_IN_STPCPY_CHK: + case BUILT_IN_STPNCPY_CHK: + case BUILT_IN_STRCAT_CHK: + case BUILT_IN_STRNCAT_CHK: + case BUILT_IN_MALLOC: + case BUILT_IN_CALLOC: + case BUILT_IN_REALLOC: + return 1; + + default: + return 0; + } +} + /* Add bound arguments to call statement pointed by GSI. Also performs a replacement of user checker builtins calls with internal ones. */ @@ -1619,7 +1663,7 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi) && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_OBJECT_SIZE) return; - /* Donothing for calls to legacy functions. */ + /* Do nothing for calls to legacy functions. */ if (fndecl && lookup_attribute ("bnd_legacy", DECL_ATTRIBUTES (fndecl))) return; @@ -1686,11 +1730,18 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi) if (!flag_chkp_instrument_calls) return; - /* Avoid instrumented builtin functions for now. Due to IPA - it also means we have to avoid instrumentation of indirect - calls. */ - if (fndecl && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN) - return; + /* We instrument only some subset of builtins. We also instrument + builtin calls to be inlined. */ + if (fndecl + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL + && !chkp_instrument_normal_builtin (fndecl)) + { + struct cgraph_node *clone = chkp_maybe_create_clone (fndecl); + if (!clone + || !gimple_has_body_p (clone->decl) + || !lookup_attribute ("always_inline", DECL_ATTRIBUTES (fndecl))) + return; + } /* If function decl is available then use it for formal arguments list. Otherwise use function type. */ @@ -1764,14 +1815,6 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi) } new_args.release (); - /* If we call built-in function and pass no bounds then - we do not need to change anything. */ - if (new_call == call - && fndecl - && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL - && fndecl == builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl))) - return; - /* For direct calls fndecl is replaced with instrumented version. */ if (fndecl) { @@ -3905,15 +3948,21 @@ chkp_replace_function_pointer (tree *op, int *walk_subtrees, { if (TREE_CODE (*op) == FUNCTION_DECL && !lookup_attribute ("bnd_legacy", DECL_ATTRIBUTES (*op)) - /* Do not replace builtins for now. */ - && DECL_BUILT_IN_CLASS (*op) == NOT_BUILT_IN) + && (DECL_BUILT_IN_CLASS (*op) == NOT_BUILT_IN + /* For builtins we replace pointers only for selected + function and functions having definitions. */ + || (DECL_BUILT_IN_CLASS (*op) == BUILT_IN_NORMAL + && (chkp_instrument_normal_builtin (*op) + || gimple_has_body_p (*op))))) { struct cgraph_node *node = cgraph_node::get_create (*op); + struct cgraph_node *clone = NULL; if (!node->instrumentation_clone) - chkp_maybe_create_clone (*op); + clone = chkp_maybe_create_clone (*op); - *op = node->instrumented_version->decl; + if (clone) + *op = clone->decl; *walk_subtrees = 0; }