Another gentle ping?
-Han
On Mon, Jan 9, 2012 at 2:18 PM, Han Shen(沈涵) <shen...@google.com> wrote:
> Hi, all, it's been a long time, I've slightly modified my code -
> TREE_ADDRESSABLE is only applied when TREE_CODE is VAR_DECL, and it's
> combined with test for arrays/struct-union-contain-array, which makes
> the code a little bit cleaner.
>
> Comments, approvals? Thanks!
>
> ===== Patch starts ======
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index 2b2e464..eb48607 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -1509,15 +1509,38 @@ estimated_stack_frame_size (struct cgraph_node *node)
> return size;
> }
>
> +/* Helper routine to check if a record or union contains an array field. */
> +
> +static int
> +record_or_union_type_has_array (const_tree tree_type)
> +{
> + tree fields = TYPE_FIELDS (tree_type);
> + tree f;
> + for (f = fields; f; f = DECL_CHAIN (f))
> + {
> + if (TREE_CODE (f) == FIELD_DECL)
> + {
> + tree field_type = TREE_TYPE (f);
> + if (RECORD_OR_UNION_TYPE_P (field_type))
> + return record_or_union_type_has_array (field_type);
> + if (TREE_CODE (field_type) == ARRAY_TYPE)
> + return 1;
> + }
> + }
> + return 0;
> +}
> +
> /* Expand all variables used in the function. */
>
> static void
> expand_used_vars (void)
> {
> tree var, outer_block = DECL_INITIAL (current_function_decl);
> + referenced_var_iterator rvi;
> VEC(tree,heap) *maybe_local_decls = NULL;
> unsigned i;
> unsigned len;
> + int gen_stack_protect_signal = 0;
>
> /* Compute the phase of the stack frame for this function. */
> {
> @@ -1550,6 +1573,23 @@ expand_used_vars (void)
> }
> }
>
> + FOR_EACH_REFERENCED_VAR (cfun, var, rvi)
> + if (!is_global_var (var))
> + {
> + tree var_type = TREE_TYPE (var);
> + /* Examine local referenced variables that have their addresses taken,
> + contain an array or are arrays. */
> + if (TREE_CODE (var) == VAR_DECL
> + && (TREE_CODE (var_type) == ARRAY_TYPE
> + || TREE_ADDRESSABLE (var)
> + || (RECORD_OR_UNION_TYPE_P (var_type)
> + && record_or_union_type_has_array (var_type))))
> + {
> + ++gen_stack_protect_signal;
> + break;
> + }
> + }
> +
> /* At this point all variables on the local_decls with TREE_USED
> set are not associated with any block scope. Lay them out. */
>
> @@ -1640,12 +1680,17 @@ expand_used_vars (void)
> dump_stack_var_partition ();
> }
>
> - /* There are several conditions under which we should create a
> - stack guard: protect-all, alloca used, protected decls present. */
> + /* There are several conditions under which we should create a stack guard:
> + protect-all, alloca used, protected decls present or a positive
> + gen_stack_protect_signal. */
> if (flag_stack_protect == 2
> - || (flag_stack_protect
> + || (flag_stack_protect == 1
> && (cfun->calls_alloca || has_protected_decls)))
> create_stack_guard ();
> + else if (flag_stack_protect == 3
> + && (gen_stack_protect_signal
> + || cfun->calls_alloca || has_protected_decls))
> + create_stack_guard ();
>
> /* Assign rtl to each variable based on these partitions. */
> if (stack_vars_num > 0)
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 6cfe17a..0680057 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1836,6 +1836,10 @@ fstack-protector-all
> Common Report RejectNegative Var(flag_stack_protect, 2)
> Use a stack protection method for every function
>
> +fstack-protector-strong
> +Common Report RejectNegative Var(flag_stack_protect, 3)
> +Use a smart stack protection method for certain functions
> +
> fstack-usage
> Common RejectNegative Var(flag_stack_usage)
> Output stack usage information on a per-function basis
> diff --git a/gcc/testsuite/g++.dg/fstack-protector-strong.C
> b/gcc/testsuite/g++.dg/fstack-protector-strong.C
> new file mode 100644
> index 0000000..a4f0f81
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/fstack-protector-strong.C
> @@ -0,0 +1,35 @@
> +/* Test that stack protection is done on chosen functions. */
> +
> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-O2 -fstack-protector-strong" } */
> +
> +class A
> +{
> +public:
> + A() {}
> + ~A() {}
> + void method();
> + int state;
> +};
> +
> +/* Frame address exposed to A::method via "this". */
> +int
> +foo1 ()
> +{
> + A a;
> + a.method ();
> + return a.state;
> +}
> +
> +/* Possible destroying foo2's stack via &a. */
> +int
> +global_func (A& a);
> +
> +/* Frame address exposed to global_func. */
> +int foo2 ()
> +{
> + A a;
> + return global_func (a);
> +}
> +
> +/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */
> diff --git a/gcc/testsuite/gcc.dg/fstack-protector-strong.c
> b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
> new file mode 100644
> index 0000000..5a5cf98
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
> @@ -0,0 +1,135 @@
> +/* Test that stack protection is done on chosen functions. */
> +
> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-O2 -fstack-protector-strong" } */
> +
> +#include<string.h>
> +#include<stdlib.h>
> +
> +extern int g0;
> +extern int* pg0;
> +int
> +goo (int *);
> +int
> +hoo (int);
> +
> +/* Function frame address escaped function call. */
> +int
> +foo1 ()
> +{
> + int i;
> + return goo (&i);
> +}
> +
> +struct ArrayStruct
> +{
> + int a;
> + int array[10];
> +};
> +
> +struct AA
> +{
> + int b;
> + struct ArrayStruct as;
> +};
> +
> +/* Function frame contains array. */
> +int
> +foo2 ()
> +{
> + struct AA aa;
> + int i;
> + for (i = 0; i < 10; ++i)
> + {
> + aa.as.array[i] = i * (i-1) + i / 2;
> + }
> + return aa.as.array[5];
> +}
> +
> +/* Address computation based on a function frame address. */
> +int
> +foo3 ()
> +{
> + int a;
> + int *p;
> + p = &a + 5;
> + return goo (p);
> +}
> +
> +/* Address cast based on a function frame address. */
> +int
> +foo4 ()
> +{
> + int a;
> + return goo (g0 << 2 ? (int *)(3 * (long)(void *)(&a)) : 0);
> +}
> +
> +/* Address cast based on a local array. */
> +int
> +foo5 ()
> +{
> + short array[10];
> + return goo ((int *)(array + 5));
> +}
> +
> +struct BB
> +{
> + int one;
> + int two;
> + int three;
> +};
> +
> +/* Address computaton based on a function frame address.*/
> +int
> +foo6 ()
> +{
> + struct BB bb;
> + return goo (&bb.one + sizeof(int));
> +}
> +
> +/* Function frame address escaped via global variable. */
> +int
> +foo7 ()
> +{
> + int a;
> + pg0 = &a;
> + goo (pg0);
> + return *pg0;
> +}
> +
> +/* Check that this covers -fstack-protector. */
> +int
> +foo8 ()
> +{
> + char base[100];
> + memcpy ((void *)base, (const void *)pg0, 105);
> + return (int)(base[32]);
> +}
> +
> +/* Check that this covers -fstack-protector. */
> +int
> +foo9 ()
> +{
> + char* p = alloca (100);
> + return goo ((int *)(p + 50));
> +}
> +
> +int
> +global2 (struct BB* pbb);
> +
> +/* Address taken on struct. */
> +int
> +foo10 ()
> +{
> + struct BB bb;
> + int i;
> + bb.one = global2 (&bb);
> + for (i = 0; i < 10; ++i)
> + {
> + bb.two = bb.one + bb.two;
> + bb.three = bb.one + bb.two + bb.three;
> + }
> + return bb.three;
> +}
> +
> +/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */
>
>
> On Tue, Dec 13, 2011 at 9:23 AM, Han Shen(沈涵) <shen...@google.com> wrote:
>> Hi, further comments? Or ok for submit?
>>
>> And as suggested by Diego, I'd like to make it upstream and google branch.
>>
>> Thanks,
>> -Han
>>
>>
>> On Thu, Dec 8, 2011 at 4:55 PM, Han Shen(沈涵) <shen...@google.com> wrote:
>>> Hi, Jakub, thanks! Fixed!
>>>
>>> Hi, Andrew, it's good suggestion. Done. Also modified foo10.
>>>
>>> A small c++ test case was added also.
>>>
>>> Patches (also on http://codereview.appspot.com/5461043)
>>> ============================================
>>> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>>> index 8684721..0a7a9f7 100644
>>> --- a/gcc/cfgexpand.c
>>> +++ b/gcc/cfgexpand.c
>>> @@ -1507,15 +1507,38 @@ estimated_stack_frame_size (struct cgraph_node
>>> *node)
>>> return size;
>>> }
>>>
>>> +/* Helper routine to check if a record or union contains an array field. */
>>> +
>>> +static int
>>> +record_or_union_type_has_array (const_tree tree_type)
>>> +{
>>> + tree fields = TYPE_FIELDS (tree_type);
>>> + tree f;
>>> + for (f = fields; f; f = DECL_CHAIN (f))
>>> + {
>>> + if (TREE_CODE (f) == FIELD_DECL)
>>> + {
>>> + tree field_type = TREE_TYPE (f);
>>> + if (RECORD_OR_UNION_TYPE_P (field_type))
>>> + return record_or_union_type_has_array (field_type);
>>> + if (TREE_CODE (field_type) == ARRAY_TYPE)
>>> + return 1;
>>> + }
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> /* Expand all variables used in the function. */
>>>
>>> static void
>>> expand_used_vars (void)
>>> {
>>> tree var, outer_block = DECL_INITIAL (current_function_decl);
>>> + referenced_var_iterator rvi;
>>> VEC(tree,heap) *maybe_local_decls = NULL;
>>> unsigned i;
>>> unsigned len;
>>> + int gen_stack_protect_signal = 0;
>>>
>>> /* Compute the phase of the stack frame for this function. */
>>> {
>>> @@ -1548,6 +1571,28 @@ expand_used_vars (void)
>>> }
>>> }
>>>
>>> + FOR_EACH_REFERENCED_VAR (cfun, var, rvi)
>>> + if (!is_global_var (var))
>>> + {
>>> + tree var_type = TREE_TYPE (var);
>>> + /* Examine local variables that have been address taken. */
>>> + if (TREE_ADDRESSABLE (var))
>>> + {
>>> + ++gen_stack_protect_signal;
>>> + break;
>>> + }
>>> + /* Examine local referenced variables that contain an array or are
>>> + arrays. */
>>> + if (TREE_CODE (var) == VAR_DECL
>>> + && (TREE_CODE (var_type) == ARRAY_TYPE
>>> + || (RECORD_OR_UNION_TYPE_P (var_type)
>>> + && record_or_union_type_has_array (var_type))))
>>> + {
>>> + ++gen_stack_protect_signal;
>>> + break;
>>> + }
>>> + }
>>> +
>>> /* At this point all variables on the local_decls with TREE_USED
>>> set are not associated with any block scope. Lay them out. */
>>>
>>> @@ -1638,12 +1683,17 @@ expand_used_vars (void)
>>> dump_stack_var_partition ();
>>> }
>>>
>>> - /* There are several conditions under which we should create a
>>> - stack guard: protect-all, alloca used, protected decls present. */
>>> + /* There are several conditions under which we should create a stack
>>> guard:
>>> + protect-all, alloca used, protected decls present or a positive
>>> + gen_stack_protect_signal. */
>>> if (flag_stack_protect == 2
>>> - || (flag_stack_protect
>>> + || (flag_stack_protect == 1
>>> && (cfun->calls_alloca || has_protected_decls)))
>>> create_stack_guard ();
>>> + else if (flag_stack_protect == 3
>>> + && (gen_stack_protect_signal
>>> + || cfun->calls_alloca || has_protected_decls))
>>> + create_stack_guard ();
>>>
>>> /* Assign rtl to each variable based on these partitions. */
>>> if (stack_vars_num > 0)
>>> diff --git a/gcc/common.opt b/gcc/common.opt
>>> index 55d3f2d..1ad9717 100644
>>> --- a/gcc/common.opt
>>> +++ b/gcc/common.opt
>>> @@ -1848,6 +1848,10 @@ fstack-protector-all
>>> Common Report RejectNegative Var(flag_stack_protect, 2)
>>> Use a stack protection method for every function
>>>
>>> +fstack-protector-strong
>>> +Common Report RejectNegative Var(flag_stack_protect, 3)
>>> +Use a smart stack protection method for certain functions
>>> +
>>> fstack-usage
>>> Common RejectNegative Var(flag_stack_usage)
>>> Output stack usage information on a per-function basis
>>> diff --git a/gcc/testsuite/g++.dg/fstack-protector-strong.C
>>> b/gcc/testsuite/g++.dg/fstack-protector-strong.C
>>> new file mode 100644
>>> index 0000000..a4f0f81
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/fstack-protector-strong.C
>>> @@ -0,0 +1,35 @@
>>> +/* Test that stack protection is done on chosen functions. */
>>> +
>>> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
>>> +/* { dg-options "-O2 -fstack-protector-strong" } */
>>> +
>>> +class A
>>> +{
>>> +public:
>>> + A() {}
>>> + ~A() {}
>>> + void method();
>>> + int state;
>>> +};
>>> +
>>> +/* Frame address exposed to A::method via "this". */
>>> +int
>>> +foo1 ()
>>> +{
>>> + A a;
>>> + a.method ();
>>> + return a.state;
>>> +}
>>> +
>>> +/* Possible destroying foo2's stack via &a. */
>>> +int
>>> +global_func (A& a);
>>> +
>>> +/* Frame address exposed to global_func. */
>>> +int foo2 ()
>>> +{
>>> + A a;
>>> + return global_func (a);
>>> +}
>>> +
>>> +/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */
>>> diff --git a/gcc/testsuite/gcc.dg/fstack-protector-strong.c
>>> b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
>>> new file mode 100644
>>> index 0000000..5a5cf98
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
>>> @@ -0,0 +1,135 @@
>>> +/* Test that stack protection is done on chosen functions. */
>>> +
>>> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
>>> +/* { dg-options "-O2 -fstack-protector-strong" } */
>>> +
>>> +#include<string.h>
>>> +#include<stdlib.h>
>>> +
>>> +extern int g0;
>>> +extern int* pg0;
>>> +int
>>> +goo (int *);
>>> +int
>>> +hoo (int);
>>> +
>>> +/* Function frame address escaped function call. */
>>> +int
>>> +foo1 ()
>>> +{
>>> + int i;
>>> + return goo (&i);
>>> +}
>>> +
>>> +struct ArrayStruct
>>> +{
>>> + int a;
>>> + int array[10];
>>> +};
>>> +
>>> +struct AA
>>> +{
>>> + int b;
>>> + struct ArrayStruct as;
>>> +};
>>> +
>>> +/* Function frame contains array. */
>>> +int
>>> +foo2 ()
>>> +{
>>> + struct AA aa;
>>> + int i;
>>> + for (i = 0; i < 10; ++i)
>>> + {
>>> + aa.as.array[i] = i * (i-1) + i / 2;
>>> + }
>>> + return aa.as.array[5];
>>> +}
>>> +
>>> +/* Address computation based on a function frame address. */
>>> +int
>>> +foo3 ()
>>> +{
>>> + int a;
>>> + int *p;
>>> + p = &a + 5;
>>> + return goo (p);
>>> +}
>>> +
>>> +/* Address cast based on a function frame address. */
>>> +int
>>> +foo4 ()
>>> +{
>>> + int a;
>>> + return goo (g0 << 2 ? (int *)(3 * (long)(void *)(&a)) : 0);
>>> +}
>>> +
>>> +/* Address cast based on a local array. */
>>> +int
>>> +foo5 ()
>>> +{
>>> + short array[10];
>>> + return goo ((int *)(array + 5));
>>> +}
>>> +
>>> +struct BB
>>> +{
>>> + int one;
>>> + int two;
>>> + int three;
>>> +};
>>> +
>>> +/* Address computaton based on a function frame address.*/
>>> +int
>>> +foo6 ()
>>> +{
>>> + struct BB bb;
>>> + return goo (&bb.one + sizeof(int));
>>> +}
>>> +
>>> +/* Function frame address escaped via global variable. */
>>> +int
>>> +foo7 ()
>>> +{
>>> + int a;
>>> + pg0 = &a;
>>> + goo (pg0);
>>> + return *pg0;
>>> +}
>>> +
>>> +/* Check that this covers -fstack-protector. */
>>> +int
>>> +foo8 ()
>>> +{
>>> + char base[100];
>>> + memcpy ((void *)base, (const void *)pg0, 105);
>>> + return (int)(base[32]);
>>> +}
>>> +
>>> +/* Check that this covers -fstack-protector. */
>>> +int
>>> +foo9 ()
>>> +{
>>> + char* p = alloca (100);
>>> + return goo ((int *)(p + 50));
>>> +}
>>> +
>>> +int
>>> +global2 (struct BB* pbb);
>>> +
>>> +/* Address taken on struct. */
>>> +int
>>> +foo10 ()
>>> +{
>>> + struct BB bb;
>>> + int i;
>>> + bb.one = global2 (&bb);
>>> + for (i = 0; i < 10; ++i)
>>> + {
>>> + bb.two = bb.one + bb.two;
>>> + bb.three = bb.one + bb.two + bb.three;
>>> + }
>>> + return bb.three;
>>> +}
>>> +
>>> +/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */
>>> ========================
>>>
>>> -Han
>>>
>>> On Thu, Dec 8, 2011 at 2:32 PM, Andrew Pinski <pins...@gmail.com> wrote:
>>>> On Thu, Dec 8, 2011 at 2:02 PM, Han Shen(沈涵) <shen...@google.com> wrote:
>>>>> +/* Address taken on struct. */
>>>>> +int foo10()
>>>>> +{
>>>>> + struct BB bb;
>>>>> + int i;
>>>>> + memset(&bb, 5, sizeof bb);
>>>>> + for (i = 0; i < 10; ++i)
>>>>> + {
>>>>> + bb.one = i;
>>>>> + bb.two = bb.one + bb.two;
>>>>> + bb.three = bb.one + bb.two + bb.three;
>>>>> + }
>>>>> + return bb.three;
>>>>> +}
>>>>
>>>> The above testcase could be optimized such that it does not need bb
>>>> has its address taken.
>>>>
>>>> Thanks,
>>>> Andrew Pinski
>
> --
> Han Shen | Software Engineer | shen...@google.com | +1-xxx-xxx-xxxx
