[4.7][google] Adding a new option "-fstack-protector-strong".

[shenhan, 2011-12-08 01:07:53]

Hi, this patch provides a new stack protection option -
"fstack-protector-strong".

Background - some times stack-protector is too-simple while stack-protector-all
over-kills, for example, to build one of our core systems, we forcibly add
"-fstack-protector-all" to all compile commands, which brings big performance
penalty (due to extra stack guard/check insns on function prologue and epilogue)
on both atom and arm. To use "-fstack-protector" is just regarded as not secure
enough (only "protects" <2% functions) by the system secure team. So I'd like to
add the option "-fstack-protector-strong", that hits the balance between
"-fstack-protector" and "-fstack-protector-all".

Benefit - gain big performance while sacrificing little security (for scenarios
using -fstack-protector-all)

Status - implemented internally, to be up-streamed or merged to google branch
only.

Detail -
https://docs.google.com/a/google.com/document/d/1xXBH6rRZue4f296vGt9YQcuLVQHe...

Tested - manually, built chrome browser using the modified compiler with
"-fstack-protector-strong".

=========================
gcc/ChangeLog:
	* cfgexpand.c (expand_used_vars): Add logic handling stack-protector-strong.
        (is_local_address_taken): Internal function that returns true when
gimple contains
        an address taken on function local variables.
        (record_or_union_type_has_array): New, tests if a record or union type
contains an array.
	* common.opt (fstack-protector-all): New option.

gcc/testsuite/ChangeLog
	* gcc.dg/fstack-protector-strong.c: New.

==========================

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 8684721..1d9df87 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1507,6 +1507,50 @@ estimated_stack_frame_size (struct cgraph_node *node)
   return size;
 }
 
+static int is_local_address_taken(tree t) {
+  if (t && TREE_CODE(t) == ADDR_EXPR) {
+    int i;
+    tree local_var;
+    tree v = TREE_OPERAND(t, 0);
+    switch (TREE_CODE(v)) {
+    case MEM_REF:
+      for (i = 0; i < TREE_OPERAND_LENGTH(v); ++i)
+        if (is_local_address_taken(TREE_OPERAND(v, i)))
+          return 1;
+      return 0;
+    case COMPONENT_REF:
+      while (v && TREE_CODE(v) == COMPONENT_REF)
+        v = TREE_OPERAND(v, 0);
+      break;
+    case VAR_DECL:
+    default:
+      ;
+    }
+    if (v && TREE_CODE(v) == VAR_DECL && !is_global_var(v)) {
+      FOR_EACH_VEC_ELT(tree, cfun->local_decls, i, local_var) {
+        if (local_var == v)
+          return 1;
+      }
+    }
+  }
+  return 0;
+}
+
+static int record_or_union_type_has_array(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
@@ -1516,6 +1560,8 @@ expand_used_vars (void)
   VEC(tree,heap) *maybe_local_decls = NULL;
   unsigned i;
   unsigned len;
+  int gen_stack_protect_signal = 0;
+  basic_block bb;
 
   /* Compute the phase of the stack frame for this function.  */
   {
@@ -1548,6 +1594,63 @@ expand_used_vars (void)
 	}
     }
 
+  /* Examine each basic block for address taking of local variables. */
+  FOR_EACH_BB(bb) {
+    gimple_stmt_iterator si;
+    /* Scanning phis. */
+    for (si = gsi_start_phis(bb); !gsi_end_p(si); gsi_next(&si)) {
+      gimple phi_stmt = gsi_stmt(si);
+      unsigned int i;
+      for (i = 0; i < gimple_phi_num_args(phi_stmt); ++i)
+        if (is_local_address_taken(gimple_phi_arg(phi_stmt, i)->def))
+          ++gen_stack_protect_signal;
+    }
+    /* Scanning assignments and calls. */
+    for (si = gsi_start_bb(bb); !gen_stack_protect_signal && !gsi_end_p(si);
+         gsi_next(&si)) {
+      gimple stmt = gsi_stmt (si);
+      if (is_gimple_assign(stmt)) {
+        switch(gimple_assign_rhs_class(stmt)) {
+        case GIMPLE_TERNARY_RHS:
+          if (is_local_address_taken(gimple_assign_rhs3(stmt))) {
+            ++gen_stack_protect_signal;
+            break;
+          }
+          /* Otherwise, fall through. */
+        case GIMPLE_BINARY_RHS:
+          if (is_local_address_taken(gimple_assign_rhs2(stmt))) {
+            ++gen_stack_protect_signal;
+            break;
+          }
+        case GIMPLE_SINGLE_RHS:
+        case GIMPLE_UNARY_RHS:
+          if (is_local_address_taken(gimple_assign_rhs1(stmt)))
+            ++gen_stack_protect_signal;
+          break;
+        case GIMPLE_INVALID_RHS:
+          break;
+        }
+      }
+
+      if (!gen_stack_protect_signal && is_gimple_call(stmt)) {
+        int ii, num_arg = gimple_call_num_args(stmt);
+        for (ii = 0; !gen_stack_protect_signal && ii < num_arg; ++ii)
+          if (is_local_address_taken(gimple_call_arg(stmt, ii)))
+            ++gen_stack_protect_signal;
+      }
+    }
+  }
+
+  /* Examine local variable declaration. */
+  if (!gen_stack_protect_signal)
+    FOR_EACH_LOCAL_DECL (cfun, i, var)
+      if (TREE_CODE(var) == VAR_DECL && !is_global_var(var)) {
+        tree var_type = TREE_TYPE(var);
+        gen_stack_protect_signal += (TREE_CODE(var_type) == ARRAY_TYPE) ||
+          (RECORD_OR_UNION_TYPE_P(var_type) &&
+           record_or_union_type_has_array(var_type));
+      }
+
   /* At this point all variables on the local_decls with TREE_USED
      set are not associated with any block scope.  Lay them out.  */
 
@@ -1640,9 +1743,13 @@ expand_used_vars (void)
 
   /* There are several conditions under which we should create a
      stack guard: protect-all, alloca used, protected decls present.  */
-  if (flag_stack_protect == 2
-      || (flag_stack_protect
-	  && (cfun->calls_alloca || has_protected_decls)))
+  if (flag_stack_protect == 2 /* -fstack-protector-all */
+      || (flag_stack_protect == 1 /* -fstack-protector */
+	  && (cfun->calls_alloca || has_protected_decls))) {
+    create_stack_guard ();
+  } else if (flag_stack_protect == 3 && /* -fstack-protector-strong */
+             (gen_stack_protect_signal ||
+              cfun->calls_alloca || has_protected_decls))
     create_stack_guard ();
 
   /* Assign rtl to each variable based on these partitions.  */
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/gcc.dg/fstack-protector-strong.c
b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
new file mode 100644
index 0000000..44225f5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
@@ -0,0 +1,110 @@
+/* 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));
+}
+
+/* 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;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */
+
+

--
This patch is available for review at http://codereview.appspot.com/5461043

[pinskia_gmail.com, 2011-12-08 01:19:55]

On Wed, Dec 7, 2011 at 5:07 PM, Han Shen <shenhan@google.com> wrote:
> +  /* Examine each basic block for address taking of local variables. */

I don't think you need to look at the basic blocks to figure out if a
local variable has its address taken.  You can look through referenced
variables and see if it is a local variable and TREE_ADDRESSABLE is
set.  This should speed up the code a lot.  Though that might has some
false positives with arrays but that is ok because you are checking if
there are any arrays already anyways.

Thanks,
Andrew Pinski

[richard.guenther_gmail.com, 2011-12-08 09:07:06]

On Thu, Dec 8, 2011 at 2:19 AM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Wed, Dec 7, 2011 at 5:07 PM, Han Shen <shenhan@google.com> wrote:
>> +  /* Examine each basic block for address taking of local variables. */
>
> I don't think you need to look at the basic blocks to figure out if a
> local variable has its address taken.  You can look through referenced
> variables and see if it is a local variable and TREE_ADDRESSABLE is
> set.  This should speed up the code a lot.  Though that might has some
> false positives with arrays but that is ok because you are checking if
> there are any arrays already anyways.

Indeed.

Also your patch does not adhere to the GCC coding standards.
For example all functions need toplevel comments documenting
their purpose and their parameters.

Richard.

> Thanks,
> Andrew Pinski

[Diego Novillo, 2011-12-08 13:18:27]

On Wed, Dec 7, 2011 at 20:07, Han Shen <shenhan@google.com> wrote:

> Status - implemented internally, to be up-streamed or merged to google branch
only.

Why would you not consider sending this for trunk at the next stage 1?

(patch review in progress)

Diego.

[shenhan, 2011-12-08 19:05:43]

Hi, Andrew and Richard, check via referenced vars is much easier, thanks!

Updated patches attached at EOM, also uploaded to
http://codereview.appspot.com/5461043

Hi, Diego, that's good suggestion. I'm glad to send this for trunk at
the next stage 1.

-Han

Updated patches ================
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 8684721..ae76441 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1507,15 +1507,34 @@ 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(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 +1567,20 @@ expand_used_vars (void)
 	}
     }

+  FOR_EACH_REFERENCED_VAR(DECL_STRUCT_FUNCTION(current_function_decl),
var, rvi)
+    if (!is_global_var(var) && TREE_ADDRESSABLE(var))
+      ++gen_stack_protect_signal;
+
+  /* Examine local variable declaration. */
+  if (!gen_stack_protect_signal)
+    FOR_EACH_LOCAL_DECL (cfun, i, var)
+      if (TREE_CODE(var) == VAR_DECL && !is_global_var(var)) {
+        tree var_type = TREE_TYPE(var);
+        gen_stack_protect_signal += (TREE_CODE(var_type) == ARRAY_TYPE) ||
+          (RECORD_OR_UNION_TYPE_P(var_type) &&
+           record_or_union_type_has_array(var_type));
+      }
+
   /* At this point all variables on the local_decls with TREE_USED
      set are not associated with any block scope.  Lay them out.  */

@@ -1640,9 +1673,13 @@ expand_used_vars (void)

   /* There are several conditions under which we should create a
      stack guard: protect-all, alloca used, protected decls present.  */
-  if (flag_stack_protect == 2
-      || (flag_stack_protect
-	  && (cfun->calls_alloca || has_protected_decls)))
+  if (flag_stack_protect == 2 /* -fstack-protector-all */
+      || (flag_stack_protect == 1 /* -fstack-protector */
+	  && (cfun->calls_alloca || has_protected_decls))) {
+    create_stack_guard ();
+  } else if (flag_stack_protect == 3 && /* -fstack-protector-strong */
+             (gen_stack_protect_signal ||
+              cfun->calls_alloca || has_protected_decls))
     create_stack_guard ();

   /* Assign rtl to each variable based on these partitions.  */
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/gcc.dg/fstack-protector-strong.c
b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
new file mode 100644
index 0000000..44225f5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
@@ -0,0 +1,110 @@
+/* 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));
+}
+
+/* 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;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */
+
+
================================

On Thu, Dec 8, 2011 at 5:18 AM, Diego Novillo <dnovillo@google.com> wrote:
> On Wed, Dec 7, 2011 at 20:07, Han Shen <shenhan@google.com> wrote:
>
>> Status - implemented internally, to be up-streamed or merged to google branch
only.
>
> Why would you not consider sending this for trunk at the next stage 1?
>
> (patch review in progress)
>
> Diego.

[jakub_redhat.com, 2011-12-08 19:13:13]

On Thu, Dec 08, 2011 at 11:05:42AM -0800, Han Shen(沈涵) wrote:
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -1507,15 +1507,34 @@ 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(tree tree_type) {
> +  tree fields = TYPE_FIELDS(tree_type);
> +  tree f;

The formatting is wrong in the whole patch, please read the guidelines and
fix it up.

> +  FOR_EACH_REFERENCED_VAR(DECL_STRUCT_FUNCTION(current_function_decl),
> var, rvi)

Use cfun instead of DECL_STRUCT_FUNCTION (current_function_decl) here.

	Jakub

[shenhan, 2011-12-08 22:02:23]

Hi, Jakub, thanks! Patches modified according to gnu coding standards. -Han

Patches (also on http://codereview.appspot.com/5461043)
============================================
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 8684721..e584cae 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1507,15 +1507,37 @@ 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(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 +1570,20 @@ expand_used_vars (void)
 	}
     }

+  FOR_EACH_REFERENCED_VAR(cfun, var, rvi)
+    if (!is_global_var(var) && TREE_ADDRESSABLE(var))
+      ++gen_stack_protect_signal;
+
+  /* Examine local variable declaration. */
+  if (!gen_stack_protect_signal)
+    FOR_EACH_LOCAL_DECL (cfun, i, var)
+      if (TREE_CODE(var) == VAR_DECL && !is_global_var(var)) {
+	tree var_type = TREE_TYPE(var);
+	gen_stack_protect_signal += (TREE_CODE(var_type) == ARRAY_TYPE) ||
+	  (RECORD_OR_UNION_TYPE_P(var_type) &&
+	   record_or_union_type_has_array(var_type));
+      }
+
   /* 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 +1674,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/gcc.dg/fstack-protector-strong.c
b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
new file mode 100644
index 0000000..3f9d3e7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
@@ -0,0 +1,121 @@
+/* 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));
+}
+
+/* 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;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */
=====================

On Thu, Dec 8, 2011 at 11:13 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Dec 08, 2011 at 11:05:42AM -0800, Han Shen(沈涵) wrote:
>> --- a/gcc/cfgexpand.c
>> +++ b/gcc/cfgexpand.c
>> @@ -1507,15 +1507,34 @@ 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(tree tree_type) {
>> +  tree fields = TYPE_FIELDS(tree_type);
>> +  tree f;
>
> The formatting is wrong in the whole patch, please read the guidelines and
> fix it up.
>
>> +  FOR_EACH_REFERENCED_VAR(DECL_STRUCT_FUNCTION(current_function_decl),
>> var, rvi)
>
> Use cfun instead of DECL_STRUCT_FUNCTION (current_function_decl) here.
>
>        Jakub



-- 
Han Shen |  Software Engineer |  shenhan@google.com |  +1-650-440-3330

[jakub_redhat.com, 2011-12-08 22:22:45]

On Thu, Dec 08, 2011 at 02:02:23PM -0800, Han Shen(沈涵) wrote:
> Hi, Jakub, thanks! Patches modified according to gnu coding standards. -Han

Not completely:
> +/* Helper routine to check if a record or union contains an array field. */
> +
> +static int record_or_union_type_has_array(tree tree_type)

This needs to be
static int
record_or_union_type_has_array (tree tree_type)

(function name at column 1, space before ( ).

> +{
> +  tree fields = TYPE_FIELDS(tree_type);

Space before ( (in many places through the whole patch).

> +      if (TREE_CODE(var) == VAR_DECL && !is_global_var(var)) {

      if (...)
	{
	  tree

> +	tree var_type = TREE_TYPE(var);
> +	gen_stack_protect_signal += (TREE_CODE(var_type) == ARRAY_TYPE) ||
> +	  (RECORD_OR_UNION_TYPE_P(var_type) &&
> +	   record_or_union_type_has_array(var_type));

|| and && go on the next line, you probably want
	gen_stack_protect_signal
	  += (TREE_CODE (var_type) == ARRAY_TYPE
	      || (RECORD_OR_UNION_TYPE_P (var_type)
		  && record_or_union_type_has_array (var_type)));

> +  else if (flag_stack_protect == 3 &&
> +	   (gen_stack_protect_signal ||
> +	    cfun->calls_alloca || has_protected_decls))

Again.

	Jakub

[pinskia_gmail.com, 2011-12-08 22:30:22]

On Thu, Dec 8, 2011 at 2:02 PM, Han Shen(沈涵) <shenhan@google.com> wrote:
> +  FOR_EACH_REFERENCED_VAR(cfun, var, rvi)
> +    if (!is_global_var(var) && TREE_ADDRESSABLE(var))
> +      ++gen_stack_protect_signal;
> +
> +  /* Examine local variable declaration. */
> +  if (!gen_stack_protect_signal)
> +    FOR_EACH_LOCAL_DECL (cfun, i, var)
> +      if (TREE_CODE(var) == VAR_DECL && !is_global_var(var)) {
> +       tree var_type = TREE_TYPE(var);
> +       gen_stack_protect_signal += (TREE_CODE(var_type) == ARRAY_TYPE) ||
> +         (RECORD_OR_UNION_TYPE_P(var_type) &&
> +          record_or_union_type_has_array(var_type));
> +      }

How about merging the above two loops and break if you set
gen_stack_protect_signal?  Also I think it is better if you only look
at referenced variables rather than just all local decls.

Something like:
 FOR_EACH_REFERENCED_VAR(cfun, var, rvi)
  if (!is_global_var(var))
    {
      tree var_type = TREE_TYPE(var);
      if (TREE_ADDRESSABLE(var))
        {
          gen_stack_protect_signal = true;
          break;
        }
      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 = true;
          break;
        }
    }

Also I think you should use const_tree in some places like:
>static int record_or_union_type_has_array(tree tree_type)
static int
record_or_union_type_has_array (const_tree tree_type)

Thanks,
Andrew Pinski

[pinskia_gmail.com, 2011-12-08 22:32:54]

On Thu, Dec 8, 2011 at 2:02 PM, Han Shen(沈涵) <shenhan@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

[shenhan, 2011-12-09 00:55:46]

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 <pinskia@gmail.com> wrote:
> On Thu, Dec 8, 2011 at 2:02 PM, Han Shen(沈涵) <shenhan@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

[shenhan, 2011-12-13 17:23:53]

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(沈涵) <shenhan@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 <pinskia@gmail.com> wrote:
>> On Thu, Dec 8, 2011 at 2:02 PM, Han Shen(沈涵) <shenhan@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

[shenhan, 2012-01-09 22:18:43]

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(沈涵) <shenhan@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(沈涵) <shenhan@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 <pinskia@gmail.com> wrote:
>>> On Thu, Dec 8, 2011 at 2:02 PM, Han Shen(沈涵) <shenhan@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 |  shenhan@google.com |  +1-xxx-xxx-xxxx

[shenhan, 2012-01-23 19:22:14]

Another gentle ping?
-Han

On Mon, Jan 9, 2012 at 2:18 PM, Han Shen(沈涵) <shenhan@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(沈涵) <shenhan@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(沈涵) <shenhan@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 <pinskia@gmail.com> wrote:
>>>> On Thu, Dec 8, 2011 at 2:02 PM, Han Shen(沈涵) <shenhan@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 |  shenhan@google.com |  +1-xxx-xxx-xxxx

[xur, 2012-01-24 21:56:58]

OK for google branches.

http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c
File gcc/cfgexpand.c (right):

http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1597
gcc/cfgexpand.c:1597: contain an array or are arrays. */
"," before "or", and
two spaces before "*/".

http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1709
gcc/cfgexpand.c:1709: create_stack_guard ();
it's better to merge to the earlier if statement. something like:
  if (flag_stack_protect == 2
      || (flag_stack_protect == 3 && gen_stack_protect_signal)
      || (flag_stack_protect
 	  && (cfun->calls_alloca || has_protected_decls)))
     create_stack_guard ();

[davidxl, 2012-01-24 22:16:22]

Also need to update doc/invoke.texi file for the new option.

http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c
File gcc/cfgexpand.c (right):

http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1531
gcc/cfgexpand.c:1531: record_or_union_type_has_array (const_tree tree_type)
Better add '_p' suffix to the predicate function name.

http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1535
gcc/cfgexpand.c:1535: for (f = fields; f; f = DECL_CHAIN (f))
Add an empty line after declarations.

http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1702
gcc/cfgexpand.c:1702: if (flag_stack_protect == 2
Add more descriptions. Better yet, fix the flag value mapping -- protect_all->
3, protect --> 2, and protect_strong-->1

[shenhan, 2012-01-26 04:09:14]

Hi, David and Rong, thanks a lot! Modified code uploaded as patch 8
and is also included at the end of email body.

Ref - http://codereview.appspot.com/5461043

Regards,
-Han

====== Patch start
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 6d31e90..131c1b9 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1524,15 +1524,39 @@ 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_p (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_p (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.  */
   {
@@ -1565,6 +1589,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_p (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.  */

@@ -1655,11 +1696,18 @@ 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.  */
-  if (flag_stack_protect == 2
-      || (flag_stack_protect
-	  && (cfun->calls_alloca || has_protected_decls)))
+  /* Create stack guard, if
+     a) "-fstack-protector-all" - always;
+     b) "-fstack-protector-strong" - if there are arrays, memory
+     references to local variables, alloca used, or protected decls present;
+     c) "-fstack-protector" - if alloca used, or protected decls present  */
+  if (flag_stack_protect == 3  /* -fstack-protector-all  */
+      || (flag_stack_protect == 2  /* -fstack-protector-strong  */
+	  && (gen_stack_protect_signal || cfun->calls_alloca
+	      || has_protected_decls))
+      || (flag_stack_protector == 1  /* -fstack-protector  */
+	  && (cfun->calls_alloca
+	      || has_protected_decls)))
     create_stack_guard ();

   /* Assign rtl to each variable based on these partitions.  */
diff --git a/gcc/common.opt b/gcc/common.opt
index ec1dbd1..b79b8cc 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1835,8 +1835,12 @@ fstack-protector
 Common Report Var(flag_stack_protect, 1)
 Use propolice as a stack protection method

-fstack-protector-all
+fstack-protector-strong
 Common Report RejectNegative Var(flag_stack_protect, 2)
+Use a smart stack protection method for certain functions
+
+fstack-protector-all
+Common Report RejectNegative Var(flag_stack_protect, 3)
 Use a stack protection method for every function

 fstack-usage
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index e3d3789..607a7a5 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -400,8 +400,8 @@ Objective-C and Objective-C++ Dialects}.
 -fsel-sched-pipelining -fsel-sched-pipelining-outer-loops @gol
 -fshrink-wrap -fsignaling-nans -fsingle-precision-constant @gol
 -fsplit-ivs-in-unroller -fsplit-wide-types -fstack-protector @gol
--fstack-protector-all -fstrict-aliasing -fstrict-overflow @gol
--fthread-jumps -ftracer -ftree-bit-ccp @gol
+-fstack-protector-strong -fstack-protector-all -fstrict-aliasing @gol
+-fstrict-overflow -fthread-jumps -ftracer -ftree-bit-ccp @gol
 -ftree-builtin-call-dce -ftree-ccp -ftree-ch -ftree-copy-prop @gol
 -ftree-copyrename -ftree-dce -ftree-dominator-opts -ftree-dse @gol
 -ftree-forwprop -ftree-fre -ftree-loop-if-convert @gol
@@ -8443,6 +8443,12 @@ functions with buffers larger than 8 bytes.
The guards are initialized
 when a function is entered and then checked when the function exits.
 If a guard check fails, an error message is printed and the program exits.

+@item -fstack-protector-strong
+@opindex fstack-protector-strong
+Like @option{-fstack-protector} but includes additional functions to be
+protected - those that have local array definitions, or have references to
+local frame addresses.
+
 @item -fstack-protector-all
 @opindex fstack-protector-all
 Like @option{-fstack-protector} except that all functions are protected.
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 } } */

===== patch end


On Tue, Jan 24, 2012 at 2:16 PM,  <davidxl@google.com> wrote:
>
> Also need to update doc/invoke.texi file for the new option.
>
>
>
>
>
> http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c
> File gcc/cfgexpand.c (right):
>
> http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1531
> gcc/cfgexpand.c:1531: record_or_union_type_has_array (const_tree
> tree_type)
> Better add '_p' suffix to the predicate function name.
>
> http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1535
> gcc/cfgexpand.c:1535: for (f = fields; f; f = DECL_CHAIN (f))
> Add an empty line after declarations.
>
>
http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1702diff
--git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 6d31e90..131c1b9 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1524,15 +1524,39 @@ 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_p (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_p (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.  */
   {
@@ -1565,6 +1589,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_p (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.  */

@@ -1655,11 +1696,18 @@ 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.  */
-  if (flag_stack_protect == 2
-      || (flag_stack_protect
-	  && (cfun->calls_alloca || has_protected_decls)))
+  /* Create stack guard, if
+     a) "-fstack-protector-all" - always;
+     b) "-fstack-protector-strong" - if there are arrays, memory
+     references to local variables, alloca used, or protected decls present;
+     c) "-fstack-protector" - if alloca used, or protected decls present  */
+  if (flag_stack_protect == 3  /* -fstack-protector-all  */
+      || (flag_stack_protect == 2  /* -fstack-protector-strong  */
+	  && (gen_stack_protect_signal || cfun->calls_alloca
+	      || has_protected_decls))
+      || (flag_stack_protector == 1  /* -fstack-protector  */
+	  && (cfun->calls_alloca
+	      || has_protected_decls)))
     create_stack_guard ();

   /* Assign rtl to each variable based on these partitions.  */
diff --git a/gcc/common.opt b/gcc/common.opt
index ec1dbd1..b79b8cc 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1835,8 +1835,12 @@ fstack-protector
 Common Report Var(flag_stack_protect, 1)
 Use propolice as a stack protection method

-fstack-protector-all
+fstack-protector-strong
 Common Report RejectNegative Var(flag_stack_protect, 2)
+Use a smart stack protection method for certain functions
+
+fstack-protector-all
+Common Report RejectNegative Var(flag_stack_protect, 3)
 Use a stack protection method for every function

 fstack-usage
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index e3d3789..607a7a5 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -400,8 +400,8 @@ Objective-C and Objective-C++ Dialects}.
 -fsel-sched-pipelining -fsel-sched-pipelining-outer-loops @gol
 -fshrink-wrap -fsignaling-nans -fsingle-precision-constant @gol
 -fsplit-ivs-in-unroller -fsplit-wide-types -fstack-protector @gol
--fstack-protector-all -fstrict-aliasing -fstrict-overflow @gol
--fthread-jumps -ftracer -ftree-bit-ccp @gol
+-fstack-protector-strong -fstack-protector-all -fstrict-aliasing @gol
+-fstrict-overflow -fthread-jumps -ftracer -ftree-bit-ccp @gol
 -ftree-builtin-call-dce -ftree-ccp -ftree-ch -ftree-copy-prop @gol
 -ftree-copyrename -ftree-dce -ftree-dominator-opts -ftree-dse @gol
 -ftree-forwprop -ftree-fre -ftree-loop-if-convert @gol
@@ -8443,6 +8443,12 @@ functions with buffers larger than 8 bytes.
The guards are initialized
 when a function is entered and then checked when the function exits.
 If a guard check fails, an error message is printed and the program exits.

+@item -fstack-protector-strong
+@opindex fstack-protector-strong
+Like @option{-fstack-protector} but includes additional functions to be
+protected - those that have local array definitions, or have references to
+local frame addresses.
+
 @item -fstack-protector-all
 @opindex fstack-protector-all
 Like @option{-fstack-protector} except that all functions are protected.
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 } } */

> gcc/cfgexpand.c:1702: if (flag_stack_protect == 2
> Add more descriptions. Better yet, fix the flag value mapping --
> protect_all-> 3, protect --> 2, and protect_strong-->1
>
> http://codereview.appspot.com/5461043/

[davidxl, 2012-01-26 05:41:45]

ok for google branches with the above changes. Please continue to seek upstream
approval.

David

http://codereview.appspot.com/5461043/diff/19001/gcc/doc/invoke.texi
File gcc/doc/invoke.texi (right):

http://codereview.appspot.com/5461043/diff/19001/gcc/doc/invoke.texi#newcode403
gcc/doc/invoke.texi:403: +-fstack-protector-strong -fstack-protector-all
-fstrict-aliasing @gol
Switch the order of -fstack-protector-all and -fstack-proctor-strong (in
alphabetic order)

http://codereview.appspot.com/5461043/diff/19001/gcc/doc/invoke.texi#newcode8446
gcc/doc/invoke.texi:8446: +@item -fstack-protector-strong
Move this item after -fstack-protector-all

[shenhan, 2012-01-30 22:24:57]

Hi, ping?

Could someone take a look at this patch, it has already been reviewed
several rounds. I'm to submit it to gcc trunk.

Thanks,
-Han

On Wed, Jan 25, 2012 at 9:41 PM, <davidxl@google.com> wrote:
>
> ok for google branches with the above changes. Please continue to seek
> upstream approval.
>
> David
>
>
> http://codereview.appspot.com/5461043/diff/19001/gcc/doc/invoke.texi
> File gcc/doc/invoke.texi (right):
>
>
http://codereview.appspot.com/5461043/diff/19001/gcc/doc/invoke.texi#newcode403
> gcc/doc/invoke.texi:403: +-fstack-protector-strong -fstack-protector-all
> -fstrict-aliasing @gol
> Switch the order of -fstack-protector-all and -fstack-proctor-strong (in
> alphabetic order)
>
>
http://codereview.appspot.com/5461043/diff/19001/gcc/doc/invoke.texi#newcode8446
> gcc/doc/invoke.texi:8446: +@item -fstack-protector-strong
> Move this item after -fstack-protector-all
>
> http://codereview.appspot.com/5461043/

[love1060224, 2017-02-20 22:05:01]

https://www.google.com/usability/index.html?reserved=0&pType=0&productTag=and...

fstack-protector-strong