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
