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
