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
