On Thu, Jan 15, 2015 at 2:51 PM, Ilya Enkovich <[email protected]> wrote:
> 2015-01-15 16:07 GMT+03:00 Richard Biener <[email protected]>:
>> On Thu, Jan 15, 2015 at 12:46 PM, Ilya Enkovich <[email protected]>
>> 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
>
>>
>> Otherwise ok.
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> Ilya
>>> --
>>> gcc/
>>>
>>> 2015-01-14 Ilya Enkovich <[email protected]>
>>>
>>> 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-14 Ilya Enkovich <[email protected]>
>>>
>>> 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..4fb60f0 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)))
>>> + {
>>> + std::string msg = "function cannot be instrumented: ";
>>> + msg += reason;
>>> + warning (OPT_Wchkp, msg.c_str (), 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. */
