This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, CHKP, PR64363] Don't instrument functions we cannot copy
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Ilya Enkovich <enkovich dot gnu at gmail dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Fri, 16 Jan 2015 12:41:44 +0100
- Subject: Re: [PATCH, CHKP, PR64363] Don't instrument functions we cannot copy
- Authentication-results: sourceware.org; auth=none
- References: <20150115102447 dot GC56209 at msticlxl57 dot ims dot intel dot com> <CAFiYyc382-u7NZs6=oZy=qn7HO+fKSiFRXniJQHKkjpdRNUBdA at mail dot gmail dot com> <CAMbmDYZiXwjxTRopp0kKGvc0++1GOWZeHvAX8ojqTg1r1iCH7w at mail dot gmail dot com> <CAFiYyc2V4apyvrKuAMwrk3cQn1UzLJFgB0OrPfiRYRVsa5J7jQ at mail dot gmail dot com> <20150116094646 dot GA51557 at msticlxl57 dot ims dot intel dot com>
On Fri, Jan 16, 2015 at 11:41 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> On 15 Jan 15:03, Richard Biener wrote:
>> On Thu, Jan 15, 2015 at 2:51 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> > 2015-01-15 16:07 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>> >> On Thu, Jan 15, 2015 at 12:46 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> >>> Hi,
>> >>>
>> >>> This patch is to fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64363. Patch disables instrumentation for functions we cannot clone correctly due to labels.
>> >>>
>> >>> Bootstrapped and checked on x86_64-unknown-linux-gnu. OK for trunk?
>> >>
>> >> {
>> >> + clone->remove_callees ();
>> >> + clone->remove_all_references ();
>> >>
>> >> this looks like a spurious change?
>> >
>> > Seems bogus thunk creation here wasn't triggered before but this test
>> > revealed it in addition to labels issue.
>> >
>> >>
>> >> + std::string msg = "function cannot be instrumented: ";
>> >> + msg += reason;
>> >> + warning (OPT_Wchkp, msg.c_str (), node->decl);
>> >>
>> >> ick - please don't use std::string. SImply do
>> >>
>> >> warning (OPT_Wchkp, "function cannot be instrumented: %s", node->decl, reason);
>> >
>> > reason string has "%q+F" inside and therefore string concatenation is required.
>>
>> Then use
>>
>> if (warning (OPT_Wchkp, "function cannot be instrumented"))
>> inform (reason);
>>
>> Richard.
>>
>> > Thanks,
>> > Ilya
>> >
>> >>
>
> Here is a version with warning + inform. Thanks for review!
Ok.
Thanks,
Richard.
> Ilya
> --
> gcc/
>
> 2015-01-16 Ilya Enkovich <ilya.enkovich@intel.com>
>
> PR target/64363
> * ipa-chkp.h (chkp_instrumentable_p): New.
> * ipa-chkp.c: Include tree-inline.h.
> (chkp_instrumentable_p): New.
> (chkp_maybe_create_clone): Use chkp_instrumentable_p.
> Fix processing of not instrumentable functions.
> (chkp_versioning): Use chkp_instrumentable_p. Warn about
> not instrumentable functions.
> * tree-chkp.c (chkp_add_bounds_to_call_stmt): Use
> chkp_instrumentable_p.
> * tree-inline.h (copy_forbidden): New.
> * tree-inline.c (copy_forbidden): Not static anymore.
>
> gcc/testsuite/
>
> 2015-01-16 Ilya Enkovich <ilya.enkovich@intel.com>
>
> PR target/64363
> * gcc.target/i386/chkp-label-address.c: New.
>
>
> diff --git a/gcc/ipa-chkp.c b/gcc/ipa-chkp.c
> index 30d511d..8e6612e 100644
> --- a/gcc/ipa-chkp.c
> +++ b/gcc/ipa-chkp.c
> @@ -50,6 +50,7 @@ along with GCC; see the file COPYING3. If not see
> #include "lto-streamer.h"
> #include "cgraph.h"
> #include "tree-chkp.h"
> +#include "tree-inline.h"
> #include "ipa-chkp.h"
>
> /* Pointer Bounds Checker has two IPA passes to support code instrumentation.
> @@ -401,6 +402,18 @@ chkp_maybe_clone_builtin_fndecl (tree fndecl)
> return clone;
> }
>
> +/* Return 1 if function FNDECL should be instrumented. */
> +
> +bool
> +chkp_instrumentable_p (tree fndecl)
> +{
> + struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
> + return (!lookup_attribute ("bnd_legacy", DECL_ATTRIBUTES (fndecl))
> + && (!flag_chkp_instrument_marked_only
> + || lookup_attribute ("bnd_instrument", DECL_ATTRIBUTES (fndecl)))
> + && (!fn || !copy_forbidden (fn, fndecl)));
> +}
> +
> /* Return clone created for instrumentation of NODE or NULL. */
>
> cgraph_node *
> @@ -483,10 +496,10 @@ chkp_maybe_create_clone (tree fndecl)
> {
> /* If function will not be instrumented, then it's instrumented
> version is a thunk for the original. */
> - if (lookup_attribute ("bnd_legacy", DECL_ATTRIBUTES (fndecl))
> - || (flag_chkp_instrument_marked_only
> - && !lookup_attribute ("bnd_instrument", DECL_ATTRIBUTES (fndecl))))
> + if (!chkp_instrumentable_p (fndecl))
> {
> + clone->remove_callees ();
> + clone->remove_all_references ();
> clone->thunk.thunk_p = true;
> clone->thunk.add_pointer_bounds_args = true;
> clone->create_edge (node, NULL, 0, CGRAPH_FREQ_BASE);
> @@ -532,7 +545,8 @@ chkp_maybe_create_clone (tree fndecl)
>
> /* Clone all thunks. */
> for (e = node->callers; e; e = e->next_caller)
> - if (e->caller->thunk.thunk_p)
> + if (e->caller->thunk.thunk_p
> + && !e->caller->thunk.add_pointer_bounds_args)
> {
> struct cgraph_node *thunk
> = chkp_maybe_create_clone (e->caller->decl);
> @@ -578,6 +592,7 @@ static unsigned int
> chkp_versioning (void)
> {
> struct cgraph_node *node;
> + const char *reason;
>
> bitmap_obstack_initialize (NULL);
>
> @@ -587,14 +602,20 @@ chkp_versioning (void)
> && !node->instrumented_version
> && !node->alias
> && !node->thunk.thunk_p
> - && !lookup_attribute ("bnd_legacy", DECL_ATTRIBUTES (node->decl))
> - && (!flag_chkp_instrument_marked_only
> - || lookup_attribute ("bnd_instrument",
> - DECL_ATTRIBUTES (node->decl)))
> && (!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);
> + {
> + if (chkp_instrumentable_p (node->decl))
> + chkp_maybe_create_clone (node->decl);
> + else if ((reason = copy_forbidden (DECL_STRUCT_FUNCTION (node->decl),
> + node->decl)))
> + {
> + if (warning_at (DECL_SOURCE_LOCATION (node->decl), OPT_Wchkp,
> + "function cannot be instrumented"))
> + inform (DECL_SOURCE_LOCATION (node->decl), reason, node->decl);
> + }
> + }
> }
>
> /* Mark all aliases and thunks of functions with no instrumented
> diff --git a/gcc/ipa-chkp.h b/gcc/ipa-chkp.h
> index b087227..6708fe9 100644
> --- a/gcc/ipa-chkp.h
> +++ b/gcc/ipa-chkp.h
> @@ -23,5 +23,6 @@ along with GCC; see the file COPYING3. If not see
> 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);
> +extern bool chkp_instrumentable_p (tree fndecl);
>
> #endif /* GCC_IPA_CHKP_H */
> diff --git a/gcc/testsuite/gcc.target/i386/chkp-label-address.c b/gcc/testsuite/gcc.target/i386/chkp-label-address.c
> new file mode 100644
> index 0000000..05963e2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/chkp-label-address.c
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target mpx } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx -O2 -Wchkp" } */
> +
> +#include <stdio.h>
> +
> +static int f1 () /* { dg-warning "function cannot be instrumented" "" } */
> +{
> + static int array = &&label_B - &&label_A;
> +
> + label_A:
> +
> + printf ("%d\n", array);
> +
> + label_B:
> +
> + return 0;
> +}
> +
> +int f2 (int i)
> +{
> + printf ("%d\n", i);
> + return f1 ();
> +}
> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
> index c606040..f2e8c56 100644
> --- a/gcc/tree-chkp.c
> +++ b/gcc/tree-chkp.c
> @@ -1672,9 +1672,8 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi)
> && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_OBJECT_SIZE)
> return;
>
> - /* Do nothing for calls to legacy functions. */
> - if (fndecl
> - && lookup_attribute ("bnd_legacy", DECL_ATTRIBUTES (fndecl)))
> + /* Do nothing for calls to not instrumentable functions. */
> + if (fndecl && !chkp_instrumentable_p (fndecl))
> return;
>
> /* Ignore CHKP_INIT_PTR_BOUNDS, CHKP_NULL_PTR_BOUNDS
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index 4a47fd2..8dbb55f 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -3532,7 +3532,7 @@ has_label_address_in_static_1 (tree *nodep, int *walk_subtrees, void *fnp)
> /* Determine if the function can be copied. If so return NULL. If
> not return a string describng the reason for failure. */
>
> -static const char *
> +const char *
> copy_forbidden (struct function *fun, tree fndecl)
> {
> const char *reason = fun->cannot_be_copied_reason;
> diff --git a/gcc/tree-inline.h b/gcc/tree-inline.h
> index 985d83b..f8b2ebf 100644
> --- a/gcc/tree-inline.h
> +++ b/gcc/tree-inline.h
> @@ -234,6 +234,7 @@ extern tree remap_type (tree type, copy_body_data *id);
> extern gimple_seq copy_gimple_seq_and_replace_locals (gimple_seq seq);
> extern bool debug_find_tree (tree, tree);
> extern tree copy_fn (tree, tree&, tree&);
> +extern const char *copy_forbidden (struct function *fun, tree fndecl);
>
> /* This is in tree-inline.c since the routine uses
> data structures from the inliner. */
- References:
- [PATCH, CHKP, PR64363] Don't instrument functions we cannot copy
- Re: [PATCH, CHKP, PR64363] Don't instrument functions we cannot copy
- Re: [PATCH, CHKP, PR64363] Don't instrument functions we cannot copy
- Re: [PATCH, CHKP, PR64363] Don't instrument functions we cannot copy
- Re: [PATCH, CHKP, PR64363] Don't instrument functions we cannot copy