| Message ID | 20141106114818.GB53257@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On 11/06/14 04:48, Ilya Enkovich wrote: > -- > 2014-11-06 Ilya Enkovich <ilya.enkovich@intel.com> > > * tree-core.h (built_in_class): Add builtin codes to be used > by Pointer Bounds Checker for instrumented builtin functions. > * tree-streamer-in.c: Include ipa-chkp.h. > (streamer_get_builtin_tree): Create instrumented decl if > required. > * ipa-chkp.h (chkp_maybe_clone_builtin_fndecl): New. > * ipa-chkp.c (chkp_build_instrumented_fndecl): Support builtin > function decls. > (chkp_maybe_clone_builtin_fndecl): New. > (chkp_maybe_create_clone): Support builtin function decls. Looks much better than prior versions. > > > @@ -355,6 +365,30 @@ chkp_add_bounds_params_to_function (tree fndecl) > chkp_copy_function_type_adding_bounds (TREE_TYPE (fndecl)); > } > > +tree > +chkp_maybe_clone_builtin_fndecl (tree fndecl) Need a function comment here. > /* Return clone created for instrumentation of NODE or NULL. */ > > cgraph_node * > @@ -365,6 +399,52 @@ chkp_maybe_create_clone (tree fndecl) > > gcc_assert (!node->instrumentation_clone); > > + if (DECL_BUILT_IN (fndecl) > + && (DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL > + || DECL_FUNCTION_CODE (fndecl) >= BEGIN_CHKP_BUILTINS)) > + return NULL; Just so I'm sure, the only way to get into chkp_maybe_clone_builtin_decl is if this test is false. Right? Can we ultimately end up with checking clones for any normal builtin? What filters out builtins that don't need a checking variant? > + > + clone = node->instrumented_version; > + > + /* For builtin functions we may loose and recreate > + cgraph node. We should check if we already have > + instrumented version. */ Can you describe to me under what circumstances this happens? It seems like we may be papering over an issue that would be better fixed elsewhere. > @@ -409,6 +489,15 @@ chkp_maybe_create_clone (tree fndecl) > actually copies args list from the original decl. */ > chkp_add_bounds_params_to_function (new_decl); > > + /* Remember builtin fndecl. */ > + if (DECL_BUILT_IN_CLASS (clone->decl) == BUILT_IN_NORMAL > + && fndecl == builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl))) > + { > + gcc_assert (!builtin_decl_explicit (DECL_FUNCTION_CODE (clone->decl))); > + set_builtin_decl (DECL_FUNCTION_CODE (clone->decl), > + clone->decl, false); > + } I'm not a big fan of slamming in a new DECL like this, but it may be OK. I'm not going to object to that now, but I worry about downstream impacts. Tentatively OK after adding the missing function comment. Please wait for entire kit to be approved before committing anything. I may come back to something as I dig deeper into the other patches in the series. jeff
2014-11-14 9:43 GMT+03:00 Jeff Law <law@redhat.com>: > On 11/06/14 04:48, Ilya Enkovich wrote: >> >> -- >> 2014-11-06 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * tree-core.h (built_in_class): Add builtin codes to be used >> by Pointer Bounds Checker for instrumented builtin functions. >> * tree-streamer-in.c: Include ipa-chkp.h. >> (streamer_get_builtin_tree): Create instrumented decl if >> required. >> * ipa-chkp.h (chkp_maybe_clone_builtin_fndecl): New. >> * ipa-chkp.c (chkp_build_instrumented_fndecl): Support builtin >> function decls. >> (chkp_maybe_clone_builtin_fndecl): New. >> (chkp_maybe_create_clone): Support builtin function decls. > > Looks much better than prior versions. >> >> >> >> @@ -355,6 +365,30 @@ chkp_add_bounds_params_to_function (tree fndecl) >> chkp_copy_function_type_adding_bounds (TREE_TYPE (fndecl)); >> } >> >> +tree >> +chkp_maybe_clone_builtin_fndecl (tree fndecl) > > Need a function comment here. > > >> /* Return clone created for instrumentation of NODE or NULL. */ >> >> cgraph_node * >> @@ -365,6 +399,52 @@ chkp_maybe_create_clone (tree fndecl) >> >> gcc_assert (!node->instrumentation_clone); >> >> + if (DECL_BUILT_IN (fndecl) >> + && (DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL >> + || DECL_FUNCTION_CODE (fndecl) >= BEGIN_CHKP_BUILTINS)) >> + return NULL; > > Just so I'm sure, the only way to get into chkp_maybe_clone_builtin_decl is > if this test is false. Right? > > Can we ultimately end up with checking clones for any normal builtin? What > filters out builtins that don't need a checking variant? As you already noticed the filter is in the second patch. > > >> + >> + clone = node->instrumented_version; >> + >> + /* For builtin functions we may loose and recreate >> + cgraph node. We should check if we already have >> + instrumented version. */ > > Can you describe to me under what circumstances this happens? It seems like > we may be papering over an issue that would be better fixed elsewhere. I don't think I'm hiding some problem here. Builtin function calls may be removed during various optimizations. Therefore we may remove all calls to some instrumented builtins and corresponding cgraph_node is removed as unreachable (but fndecl still exists). Later calls to removed function may be created again. IIRC in my test case it happened in strlen pass which may replace builtin calls with another ones. In this case cgraph_node is recreated but fndecl recreation should be avoided, existing one should be used instead. Thanks, Ilya > > >> @@ -409,6 +489,15 @@ chkp_maybe_create_clone (tree fndecl) >> actually copies args list from the original decl. */ >> chkp_add_bounds_params_to_function (new_decl); >> >> + /* Remember builtin fndecl. */ >> + if (DECL_BUILT_IN_CLASS (clone->decl) == BUILT_IN_NORMAL >> + && fndecl == builtin_decl_explicit (DECL_FUNCTION_CODE >> (fndecl))) >> + { >> + gcc_assert (!builtin_decl_explicit (DECL_FUNCTION_CODE >> (clone->decl))); >> + set_builtin_decl (DECL_FUNCTION_CODE (clone->decl), >> + clone->decl, false); >> + } > > I'm not a big fan of slamming in a new DECL like this, but it may be OK. > I'm not going to object to that now, but I worry about downstream impacts. > > > Tentatively OK after adding the missing function comment. Please wait for > entire kit to be approved before committing anything. I may come back to > something as I dig deeper into the other patches in the series. > > jeff
On 11/14/14 01:22, Ilya Enkovich wrote: > 2014-11-14 9:43 GMT+03:00 Jeff Law <law@redhat.com>: >> On 11/06/14 04:48, Ilya Enkovich wrote: >>> >>> -- >>> 2014-11-06 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> * tree-core.h (built_in_class): Add builtin codes to be used >>> by Pointer Bounds Checker for instrumented builtin functions. >>> * tree-streamer-in.c: Include ipa-chkp.h. >>> (streamer_get_builtin_tree): Create instrumented decl if >>> required. >>> * ipa-chkp.h (chkp_maybe_clone_builtin_fndecl): New. >>> * ipa-chkp.c (chkp_build_instrumented_fndecl): Support builtin >>> function decls. >>> (chkp_maybe_clone_builtin_fndecl): New. >>> (chkp_maybe_create_clone): Support builtin function decls. >> >> Looks much better than prior versions. >>> >>> >>> >>> @@ -355,6 +365,30 @@ chkp_add_bounds_params_to_function (tree fndecl) >>> chkp_copy_function_type_adding_bounds (TREE_TYPE (fndecl)); >>> } >>> >>> +tree >>> +chkp_maybe_clone_builtin_fndecl (tree fndecl) >> >> Need a function comment here. >> >> >>> /* Return clone created for instrumentation of NODE or NULL. */ >>> >>> cgraph_node * >>> @@ -365,6 +399,52 @@ chkp_maybe_create_clone (tree fndecl) >>> >>> gcc_assert (!node->instrumentation_clone); >>> >>> + if (DECL_BUILT_IN (fndecl) >>> + && (DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL >>> + || DECL_FUNCTION_CODE (fndecl) >= BEGIN_CHKP_BUILTINS)) >>> + return NULL; >> >> Just so I'm sure, the only way to get into chkp_maybe_clone_builtin_decl is >> if this test is false. Right? >> >> Can we ultimately end up with checking clones for any normal builtin? What >> filters out builtins that don't need a checking variant? > > As you already noticed the filter is in the second patch. > >> >> >>> + >>> + clone = node->instrumented_version; >>> + >>> + /* For builtin functions we may loose and recreate >>> + cgraph node. We should check if we already have >>> + instrumented version. */ >> >> Can you describe to me under what circumstances this happens? It seems like >> we may be papering over an issue that would be better fixed elsewhere. > > I don't think I'm hiding some problem here. Builtin function calls > may be removed during various optimizations. Therefore we may remove > all calls to some instrumented builtins and corresponding cgraph_node > is removed as unreachable (but fndecl still exists). Later calls to > removed function may be created again. IIRC in my test case it > happened in strlen pass which may replace builtin calls with another > ones. In this case cgraph_node is recreated but fndecl recreation > should be avoided, existing one should be used instead. OK. Please add those details to the comment. With that comment updated, OK for the trunk. jeff
On Thu, Nov 6, 2014 at 3:48 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > Hi, > > This is the first patch in a series to enable builtin function calls instrumentation by Poitner Bounds Checker. Previously builtins instrumentation was disabled in checker because it allowed two function calls with the same function code have different set of arguments. Here I try another approach where function codes are not reused by instrumented calls. > > Basic idea is to provide each function code with a pair to be used for instrumented call. It's achieved by additional include of builtins.def into built_in_class enum. Instrumented builtins shouldn't be visible for users. Also when checker is disabled we don't need decls for them at all. Therefore function decls for instrumented function are not created at compiler initialization, but lazily created on demand. These decls are mainly requested by checker itslef and by LTO streamer to read instrumented code. > > This patch introduces new builtin codes, functions for builtin clones creation and modifies LTO streamer. > > Thanks, > Ilya > -- > 2014-11-06 Ilya Enkovich <ilya.enkovich@intel.com> > > * tree-core.h (built_in_class): Add builtin codes to be used > by Pointer Bounds Checker for instrumented builtin functions. > * tree-streamer-in.c: Include ipa-chkp.h. > (streamer_get_builtin_tree): Create instrumented decl if > required. > * ipa-chkp.h (chkp_maybe_clone_builtin_fndecl): New. > * ipa-chkp.c (chkp_build_instrumented_fndecl): Support builtin > function decls. > (chkp_maybe_clone_builtin_fndecl): New. > (chkp_maybe_create_clone): Support builtin function decls. > > This caused: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64075
diff --git a/gcc/ipa-chkp.c b/gcc/ipa-chkp.c index f910583..691adfd 100644 --- a/gcc/ipa-chkp.c +++ b/gcc/ipa-chkp.c @@ -130,6 +130,16 @@ chkp_build_instrumented_fndecl (tree fndecl) make own copy. */ DECL_ATTRIBUTES (new_decl) = copy_list (DECL_ATTRIBUTES (fndecl)); + /* Change builtin function code. */ + if (DECL_BUILT_IN (new_decl)) + { + gcc_assert (DECL_BUILT_IN_CLASS (new_decl) == BUILT_IN_NORMAL); + gcc_assert (DECL_FUNCTION_CODE (new_decl) < BEGIN_CHKP_BUILTINS); + DECL_FUNCTION_CODE (new_decl) + = (enum built_in_function)(DECL_FUNCTION_CODE (new_decl) + + BEGIN_CHKP_BUILTINS + 1); + } + return new_decl; } @@ -355,6 +365,30 @@ chkp_add_bounds_params_to_function (tree fndecl) chkp_copy_function_type_adding_bounds (TREE_TYPE (fndecl)); } +tree +chkp_maybe_clone_builtin_fndecl (tree fndecl) +{ + tree clone; + enum built_in_function fcode = DECL_FUNCTION_CODE (fndecl); + + gcc_assert (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL + && fcode < BEGIN_CHKP_BUILTINS); + + fcode = (enum built_in_function) (fcode + BEGIN_CHKP_BUILTINS + 1); + clone = builtin_decl_explicit (fcode); + if (clone) + return clone; + + clone = chkp_build_instrumented_fndecl (fndecl); + chkp_add_bounds_params_to_function (clone); + + gcc_assert (DECL_FUNCTION_CODE (clone) == fcode); + + set_builtin_decl (fcode, clone, false); + + return clone; +} + /* Return clone created for instrumentation of NODE or NULL. */ cgraph_node * @@ -365,6 +399,52 @@ chkp_maybe_create_clone (tree fndecl) gcc_assert (!node->instrumentation_clone); + if (DECL_BUILT_IN (fndecl) + && (DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL + || DECL_FUNCTION_CODE (fndecl) >= BEGIN_CHKP_BUILTINS)) + return NULL; + + clone = node->instrumented_version; + + /* For builtin functions we may loose and recreate + cgraph node. We should check if we already have + instrumented version. */ + if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL + && fndecl == builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)) + && !clone) + { + enum built_in_function fncode = DECL_FUNCTION_CODE (fndecl); + tree new_decl; + + fncode = (enum built_in_function) (fncode + BEGIN_CHKP_BUILTINS + 1); + new_decl = builtin_decl_explicit (fncode); + + /* We've actually already created an instrumented clone once. + Restore it. */ + if (new_decl) + { + clone = cgraph_node::get (new_decl); + + if (!clone) + { + gcc_assert (!gimple_has_body_p (fndecl)); + clone = cgraph_node::get_create (new_decl); + clone->externally_visible = node->externally_visible; + clone->local = node->local; + clone->address_taken = node->address_taken; + clone->thunk = node->thunk; + clone->alias = node->alias; + clone->weakref = node->weakref; + clone->cpp_implicit_alias = node->cpp_implicit_alias; + clone->orig_decl = fndecl; + clone->instrumentation_clone = true; + } + + clone->instrumented_version = node; + node->instrumented_version = clone; + } + } + if (!clone) { tree new_decl = chkp_build_instrumented_fndecl (fndecl); @@ -409,6 +489,15 @@ chkp_maybe_create_clone (tree fndecl) actually copies args list from the original decl. */ chkp_add_bounds_params_to_function (new_decl); + /* Remember builtin fndecl. */ + if (DECL_BUILT_IN_CLASS (clone->decl) == BUILT_IN_NORMAL + && fndecl == builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl))) + { + gcc_assert (!builtin_decl_explicit (DECL_FUNCTION_CODE (clone->decl))); + set_builtin_decl (DECL_FUNCTION_CODE (clone->decl), + clone->decl, false); + } + /* Clones have the same comdat group as originals. */ if (node->same_comdat_group || DECL_ONE_ONLY (node->decl)) diff --git a/gcc/ipa-chkp.h b/gcc/ipa-chkp.h index d4ad113..b2d03ad 100644 --- a/gcc/ipa-chkp.h +++ b/gcc/ipa-chkp.h @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see #define GCC_IPA_CHKP_H extern tree chkp_copy_function_type_adding_bounds (tree orig_type); +extern tree chkp_maybe_clone_builtin_fndecl (tree fndecl); extern cgraph_node *chkp_maybe_create_clone (tree fndecl); #endif /* GCC_IPA_CHKP_H */ diff --git a/gcc/tree-core.h b/gcc/tree-core.h index f8ee8cc..2027bd53 100644 --- a/gcc/tree-core.h +++ b/gcc/tree-core.h @@ -168,6 +168,14 @@ enum built_in_class { enum built_in_function { #include "builtins.def" + BEGIN_CHKP_BUILTINS, + +#undef DEF_BUILTIN +#define DEF_BUILTIN(ENUM, N, C, T, LT, B, F, NA, AT, IM, COND) ENUM##_CHKP, +#include "builtins.def" + + END_CHKP_BUILTINS, + /* Complex division routines in libgcc. These are done via builtins because emit_library_call_value can't handle complex values. */ BUILT_IN_COMPLEX_MUL_MIN, diff --git a/gcc/tree-streamer-in.c b/gcc/tree-streamer-in.c index 371e316..1137029 100644 --- a/gcc/tree-streamer-in.c +++ b/gcc/tree-streamer-in.c @@ -49,6 +49,7 @@ along with GCC; see the file COPYING3. If not see #include "streamer-hooks.h" #include "lto-streamer.h" #include "builtins.h" +#include "ipa-chkp.h" /* Read a STRING_CST from the string table in DATA_IN using input block IB. */ @@ -1128,6 +1129,14 @@ streamer_get_builtin_tree (struct lto_input_block *ib, struct data_in *data_in) if (fcode >= END_BUILTINS) fatal_error ("machine independent builtin code out of range"); result = builtin_decl_explicit (fcode); + if (!result + && fcode > BEGIN_CHKP_BUILTINS + && fcode < END_CHKP_BUILTINS) + { + fcode = (enum built_in_function) (fcode - BEGIN_CHKP_BUILTINS - 1); + result = builtin_decl_explicit (fcode); + result = chkp_maybe_clone_builtin_fndecl (result); + } gcc_assert (result); } else if (fclass == BUILT_IN_MD)