[PATCH] Implement no_stack_protect attribute.

[PATCH] Implement no_stack_protect attribute.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

Hi.

The patch adds new no_stack_protect attribute. The change is requested
from kernel folks and is direct equivalent of Clang's no_stack_protector.
Unlike Clang, I chose to name it no_stack_protect because we already
have stack_protect attribute (used with -fstack-protector-explicit).

First part of the patch contains a small refactoring of an enum, second
implements the functionality.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

[-- Attachment #2: 0002-Implement-no_stack_protect-attribute.patch --]
[-- Type: text/x-patch, Size: 13581 bytes --]

From 6b3e9d32150fe17acb271b7bedee7dc95cad7dc8 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Fri, 15 May 2020 14:42:12 +0200
Subject: [PATCH 2/2] Implement no_stack_protect attribute.

gcc/ChangeLog:

2020-05-18  Martin Liska  <mliska@suse.cz>

	PR c/94722
	* cfgexpand.c (stack_protect_decl_phase):
	Guard with lookup_attribute("no_stack_protect") at
	various places.
	(expand_used_vars): Likewise here.
	* doc/extend.texi: Document no_stack_protect attribute.

gcc/ada/ChangeLog:

2020-05-18  Martin Liska  <mliska@suse.cz>

	PR c/94722
	* gcc-interface/utils.c (handle_no_stack_protect_attribute):
	New.
	(handle_stack_protect_attribute): Add error message for a
	no_stack_protect function.

gcc/c-family/ChangeLog:

2020-05-18  Martin Liska  <mliska@suse.cz>

	PR c/94722
	* c-attribs.c (handle_no_stack_protect_function_attribute): New.
	(handle_stack_protect_attribute): Add error message for a
	no_stack_protect function.

gcc/testsuite/ChangeLog:

2020-05-18  Martin Liska  <mliska@suse.cz>

	PR c/94722
	* g++.dg/no-stack-protect-attr-2.C: New test.
	* g++.dg/no-stack-protect-attr-3.C: New test.
	* g++.dg/no-stack-protect-attr.C: New test.
---
 gcc/ada/gcc-interface/utils.c                 | 32 ++++++++
 gcc/c-family/c-attribs.c                      | 33 ++++++++
 gcc/cfgexpand.c                               | 82 ++++++++++---------
 gcc/doc/extend.texi                           |  4 +
 .../g++.dg/no-stack-protect-attr-2.C          |  7 ++
 .../g++.dg/no-stack-protect-attr-3.C          | 23 ++++++
 gcc/testsuite/g++.dg/no-stack-protect-attr.C  | 16 ++++
 7 files changed, 158 insertions(+), 39 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-2.C
 create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-3.C
 create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr.C

diff --git a/gcc/ada/gcc-interface/utils.c b/gcc/ada/gcc-interface/utils.c
index 1527be4f6d1..144afe37d78 100644
--- a/gcc/ada/gcc-interface/utils.c
+++ b/gcc/ada/gcc-interface/utils.c
@@ -91,6 +91,7 @@ static tree handle_nonnull_attribute (tree *, tree, tree, int, bool *);
 static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noreturn_attribute (tree *, tree, tree, int, bool *);
 static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *);
+static tree handle_no_stack_protect_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noinline_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noclone_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noicf_attribute (tree *, tree, tree, int, bool *);
@@ -141,6 +142,8 @@ const struct attribute_spec gnat_internal_attribute_table[] =
     handle_noreturn_attribute, NULL },
   { "stack_protect",0, 0, true,  false, false, false,
     handle_stack_protect_attribute, NULL },
+  { "no_stack_protect",0, 0, true,  false, false, false,
+    handle_no_stack_protect_attribute, NULL },
   { "noinline",     0, 0,  true,  false, false, false,
     handle_noinline_attribute, NULL },
   { "noclone",      0, 0,  true,  false, false, false,
@@ -6523,10 +6526,39 @@ handle_stack_protect_attribute (tree *node, tree name, tree, int,
       warning (OPT_Wattributes, "%qE attribute ignored", name);
       *no_add_attrs = true;
     }
+  else if (lookup_attribute ("no_stack_protect", DECL_ATTRIBUTES (*node)))
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
+	       "with %qs attribute", name, "no_stack_protect");
+      *no_add_attrs = true;
+    }
+
+  return NULL_TREE;
+}
+
+/* Handle a "no_stack_protect" attribute; arguments as in
+   struct attribute_spec.handler.  */
+
+static tree
+handle_no_stack_protect_attribute (tree *node, tree name, tree, int,
+				   bool *no_add_attrs)
+{
+  if (TREE_CODE (*node) != FUNCTION_DECL)
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored", name);
+      *no_add_attrs = true;
+    }
+  else if (lookup_attribute ("stack_protect", DECL_ATTRIBUTES (*node)))
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
+	       "with %qs attribute", name, "stack_protect");
+      *no_add_attrs = true;
+    }
 
   return NULL_TREE;
 }
 
+
 /* Handle a "noinline" attribute; arguments as in
    struct attribute_spec.handler.  */
 
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 7a6fb9af6da..3c22576035b 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -63,6 +63,8 @@ static tree handle_no_sanitize_undefined_attribute (tree *, tree, tree, int,
 static tree handle_asan_odr_indicator_attribute (tree *, tree, tree, int,
 						 bool *);
 static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *);
+static tree handle_no_stack_protect_function_attribute (tree *, tree,
+							tree, int, bool *);
 static tree handle_noinline_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noclone_attribute (tree *, tree, tree, int, bool *);
 static tree handle_nocf_check_attribute (tree *, tree, tree, int, bool *);
@@ -273,6 +275,9 @@ const struct attribute_spec c_common_attribute_table[] =
 			      handle_noreturn_attribute, NULL },
   { "stack_protect",          0, 0, true,  false, false, false,
 			      handle_stack_protect_attribute, NULL },
+  { "no_stack_protect",	      0, 0, true, false, false, false,
+			      handle_no_stack_protect_function_attribute,
+			      NULL },
   { "noinline",               0, 0, true,  false, false, false,
 			      handle_noinline_attribute,
 	                      attr_noinline_exclusions },
@@ -1017,6 +1022,34 @@ handle_stack_protect_attribute (tree *node, tree name, tree, int,
       warning (OPT_Wattributes, "%qE attribute ignored", name);
       *no_add_attrs = true;
     }
+  else if (lookup_attribute ("no_stack_protect", DECL_ATTRIBUTES (*node)))
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
+	       "with %qs attribute", name, "no_stack_protect");
+      *no_add_attrs = true;
+    }
+
+  return NULL_TREE;
+}
+
+/* Handle a "no_stack_protect" attribute; arguments as in
+   struct attribute_spec.handler.  */
+
+static tree
+handle_no_stack_protect_function_attribute (tree *node, tree name, tree,
+					    int, bool *no_add_attrs)
+{
+  if (TREE_CODE (*node) != FUNCTION_DECL)
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored", name);
+      *no_add_attrs = true;
+    }
+  else if (lookup_attribute ("stack_protect", DECL_ATTRIBUTES (*node)))
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
+	       "with %qs attribute", name, "stack_protect");
+      *no_add_attrs = true;
+    }
 
   return NULL_TREE;
 }
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index b66c3e5f7af..6e8687c392c 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1831,11 +1831,12 @@ stack_protect_decl_phase (tree decl)
   if (bits & SPCT_HAS_SMALL_CHAR_ARRAY)
     has_short_buffer = true;
 
-  if (flag_stack_protect == SPCT_FLAG_ALL
-      || flag_stack_protect == SPCT_FLAG_STRONG
-      || (flag_stack_protect == SPCT_FLAG_EXPLICIT
-	  && lookup_attribute ("stack_protect",
-			       DECL_ATTRIBUTES (current_function_decl))))
+  tree attribs = DECL_ATTRIBUTES (current_function_decl);
+  if (!lookup_attribute ("no_stack_protect", attribs)
+      && (flag_stack_protect == SPCT_FLAG_ALL
+	  || flag_stack_protect == SPCT_FLAG_STRONG
+	  || (flag_stack_protect == SPCT_FLAG_EXPLICIT
+	      && lookup_attribute ("stack_protect", attribs))))
     {
       if ((bits & (SPCT_HAS_SMALL_CHAR_ARRAY | SPCT_HAS_LARGE_CHAR_ARRAY))
 	  && !(bits & SPCT_HAS_AGGREGATE))
@@ -2136,6 +2137,7 @@ expand_used_vars (void)
      set are actually used by the optimized function.  Lay them out.  */
   expand_used_vars_for_block (outer_block, true);
 
+  tree attribs = DECL_ATTRIBUTES (current_function_decl);
   if (stack_vars_num > 0)
     {
       bool has_addressable_vars = false;
@@ -2145,10 +2147,10 @@ expand_used_vars (void)
       /* If stack protection is enabled, we don't share space between
 	 vulnerable data and non-vulnerable data.  */
       if (flag_stack_protect != 0
+	  && !lookup_attribute ("no_stack_protect", attribs)
 	  && (flag_stack_protect != SPCT_FLAG_EXPLICIT
 	      || (flag_stack_protect == SPCT_FLAG_EXPLICIT
-		  && lookup_attribute ("stack_protect",
-				       DECL_ATTRIBUTES (current_function_decl)))))
+		  && lookup_attribute ("stack_protect", attribs))))
 	has_addressable_vars = add_stack_protection_conflicts ();
 
       if (flag_stack_protect == SPCT_FLAG_STRONG && has_addressable_vars)
@@ -2161,38 +2163,40 @@ expand_used_vars (void)
 	dump_stack_var_partition ();
     }
 
-  switch (flag_stack_protect)
-    {
-    case SPCT_FLAG_ALL:
-      create_stack_guard ();
-      break;
 
-    case SPCT_FLAG_STRONG:
-      if (gen_stack_protect_signal
-	  || cfun->calls_alloca
-	  || has_protected_decls
-	  || lookup_attribute ("stack_protect",
-			       DECL_ATTRIBUTES (current_function_decl)))
+  if (!lookup_attribute ("no_stack_protect", attribs))
+    switch (flag_stack_protect)
+      {
+      case SPCT_FLAG_ALL:
 	create_stack_guard ();
-      break;
+	break;
 
-    case SPCT_FLAG_DEFAULT:
-      if (cfun->calls_alloca
-	  || has_protected_decls
-	  || lookup_attribute ("stack_protect",
-			       DECL_ATTRIBUTES (current_function_decl)))
-	create_stack_guard ();
-      break;
+      case SPCT_FLAG_STRONG:
+	if (gen_stack_protect_signal
+	    || cfun->calls_alloca
+	    || has_protected_decls
+	    || lookup_attribute ("stack_protect",
+				 DECL_ATTRIBUTES (current_function_decl)))
+	  create_stack_guard ();
+	break;
 
-    case SPCT_FLAG_EXPLICIT:
-      if (lookup_attribute ("stack_protect",
-			    DECL_ATTRIBUTES (current_function_decl)))
-	create_stack_guard ();
-      break;
+      case SPCT_FLAG_DEFAULT:
+	if (cfun->calls_alloca
+	    || has_protected_decls
+	    || lookup_attribute ("stack_protect",
+				 DECL_ATTRIBUTES (current_function_decl)))
+	  create_stack_guard ();
+	break;
 
-    default:
-      break;
-    }
+      case SPCT_FLAG_EXPLICIT:
+	if (lookup_attribute ("stack_protect",
+			      DECL_ATTRIBUTES (current_function_decl)))
+	  create_stack_guard ();
+	break;
+
+      default:
+	break;
+      }
 
   /* Assign rtl to each variable based on these partitions.  */
   if (stack_vars_num > 0)
@@ -2213,11 +2217,11 @@ expand_used_vars (void)
 	  expand_stack_vars (stack_protect_decl_phase_1, &data);
 
 	  /* Phase 2 contains other kinds of arrays.  */
-	  if (flag_stack_protect == SPCT_FLAG_ALL
-	      || flag_stack_protect == SPCT_FLAG_STRONG
-	      || (flag_stack_protect == SPCT_FLAG_EXPLICIT
-		  && lookup_attribute ("stack_protect",
-				       DECL_ATTRIBUTES (current_function_decl))))
+	  if (!lookup_attribute ("no_stack_protect", attribs)
+	      && (flag_stack_protect == SPCT_FLAG_ALL
+		  || flag_stack_protect == SPCT_FLAG_STRONG
+		  || (flag_stack_protect == SPCT_FLAG_EXPLICIT
+		      && lookup_attribute ("stack_protect", attribs))))
 	    expand_stack_vars (stack_protect_decl_phase_2, &data);
 	}
 
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index c80848e9061..604bf37e916 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3671,6 +3671,10 @@ This attribute adds stack protection code to the function if
 flags @option{-fstack-protector}, @option{-fstack-protector-strong}
 or @option{-fstack-protector-explicit} are set.
 
+@item no_stack_protect
+@cindex @code{no_stack_protect} function attribute
+This attribute prevents stack protection code for the function.
+
 @item target (@var{string}, @dots{})
 @cindex @code{target} function attribute
 Multiple target back ends implement the @code{target} attribute
diff --git a/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C b/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C
new file mode 100644
index 00000000000..dcac6f9d389
--- /dev/null
+++ b/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C
@@ -0,0 +1,7 @@
+/* PR c/94722 */
+/* { dg-do compile } */
+
+int __attribute__((no_stack_protect, stack_protect)) c() /* { dg-warning "'stack_protect' attribute ignored due to conflict with 'no_stack_protect' attribute" } */
+{
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/no-stack-protect-attr-3.C b/gcc/testsuite/g++.dg/no-stack-protect-attr-3.C
new file mode 100644
index 00000000000..a6b702ba5ea
--- /dev/null
+++ b/gcc/testsuite/g++.dg/no-stack-protect-attr-3.C
@@ -0,0 +1,23 @@
+/* PR c/94722 */
+/* Test that stack protection is disabled via no_stack_protect attribute. */
+
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fstack-protector-explicit" } */
+
+/* { dg-do compile } */
+
+int __attribute__((no_stack_protect)) foo()
+{
+  int a;
+  char b[34];
+  return 0;
+}
+
+int __attribute__((stack_protect)) bar()
+{
+  int a;
+  char b[34];
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 1 } } */
diff --git a/gcc/testsuite/g++.dg/no-stack-protect-attr.C b/gcc/testsuite/g++.dg/no-stack-protect-attr.C
new file mode 100644
index 00000000000..8332e5b276d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/no-stack-protect-attr.C
@@ -0,0 +1,16 @@
+/* PR c/94722 */
+/* Test that stack protection is disabled via no_stack_protect attribute. */
+
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fstack-protector-all" } */
+
+/* { dg-do compile } */
+
+int __attribute__((no_stack_protect)) c()
+{
+  int a;
+  char b[34];
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not "stack_chk_fail" } } */
-- 
2.26.2


[-- Attachment #3: 0001-Come-up-with-stack_protector-enum.patch --]
[-- Type: text/x-patch, Size: 3244 bytes --]

From 2465dd4f502824548472a4c303cde25c157215ef Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Fri, 15 May 2020 14:51:24 +0200
Subject: [PATCH 1/2] Come up with stack_protector enum.

gcc/ChangeLog:

2020-05-15  Martin Liska  <mliska@suse.cz>

	* cfgexpand.c: Move the enum to ...
	* coretypes.h (enum stack_protector): ... here.
	* function.c (assign_parm_adjust_stack_rtl): Use the stack_protector
	enum.

gcc/c-family/ChangeLog:

2020-05-15  Martin Liska  <mliska@suse.cz>

	* c-cppbuiltin.c (c_cpp_builtins): Use the stack_protector enum.
---
 gcc/c-family/c-cppbuiltin.c | 8 ++++----
 gcc/cfgexpand.c             | 7 -------
 gcc/coretypes.h             | 8 ++++++++
 gcc/function.c              | 2 +-
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
index a7d65d63934..9164400a046 100644
--- a/gcc/c-family/c-cppbuiltin.c
+++ b/gcc/c-family/c-cppbuiltin.c
@@ -1425,13 +1425,13 @@ c_cpp_builtins (cpp_reader *pfile)
   /* Make the choice of the stack protector runtime visible to source code.
      The macro names and values here were chosen for compatibility with an
      earlier implementation, i.e. ProPolice.  */
-  if (flag_stack_protect == 4)
+  if (flag_stack_protect == SPCT_FLAG_EXPLICIT)
     cpp_define (pfile, "__SSP_EXPLICIT__=4");
-  if (flag_stack_protect == 3)
+  if (flag_stack_protect == SPCT_FLAG_STRONG)
     cpp_define (pfile, "__SSP_STRONG__=3");
-  if (flag_stack_protect == 2)
+  if (flag_stack_protect == SPCT_FLAG_ALL)
     cpp_define (pfile, "__SSP_ALL__=2");
-  else if (flag_stack_protect == 1)
+  else if (flag_stack_protect == SPCT_FLAG_DEFAULT)
     cpp_define (pfile, "__SSP__=1");
 
   if (flag_openacc)
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 2f6ec97ed04..b66c3e5f7af 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1762,13 +1762,6 @@ clear_tree_used (tree block)
     clear_tree_used (t);
 }
 
-enum {
-  SPCT_FLAG_DEFAULT = 1,
-  SPCT_FLAG_ALL = 2,
-  SPCT_FLAG_STRONG = 3,
-  SPCT_FLAG_EXPLICIT = 4
-};
-
 /* Examine TYPE and determine a bit mask of the following features.  */
 
 #define SPCT_HAS_LARGE_CHAR_ARRAY	1
diff --git a/gcc/coretypes.h b/gcc/coretypes.h
index cda22697cc3..323b1cf0f1e 100644
--- a/gcc/coretypes.h
+++ b/gcc/coretypes.h
@@ -219,6 +219,14 @@ enum profile_reproducibility {
     PROFILE_REPRODUCIBILITY_MULTITHREADED
 };
 
+/* Type of -fstack-protector-*.  */
+enum stack_protector {
+  SPCT_FLAG_DEFAULT = 1,
+  SPCT_FLAG_ALL = 2,
+  SPCT_FLAG_STRONG = 3,
+  SPCT_FLAG_EXPLICIT = 4
+};
+
 /* Types of unwind/exception handling info that can be generated.  */
 
 enum unwind_info_type
diff --git a/gcc/function.c b/gcc/function.c
index 9eee9b59bfd..fe5c91809f3 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -2841,7 +2841,7 @@ assign_parm_adjust_stack_rtl (struct assign_parm_data_one *data)
   /* If stack protection is in effect for this function, don't leave any
      pointers in their passed stack slots.  */
   else if (crtl->stack_protect_guard
-	   && (flag_stack_protect == 2
+	   && (flag_stack_protect == SPCT_FLAG_ALL
 	       || data->arg.pass_by_reference
 	       || POINTER_TYPE_P (data->nominal_type)))
     stack_parm = NULL;
-- 
2.26.2

Re: [PATCH] Implement no_stack_protect attribute.

[Jakub Jelinek]

On Mon, May 18, 2020 at 12:37:58PM +0200, Martin Liška wrote:
> The patch adds new no_stack_protect attribute. The change is requested
> from kernel folks and is direct equivalent of Clang's no_stack_protector.
> Unlike Clang, I chose to name it no_stack_protect because we already
> have stack_protect attribute (used with -fstack-protector-explicit).

Wouldn't it be better to look at the optimize attribute to see if it really
does what has been said in the thread and if we don't want to change it,
such that a function with optimize attribute xyz stands for the option
defaults + command line options + xyz, rather than option defaults + xyz
only?

	Jakub

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

On 5/18/20 12:41 PM, Jakub Jelinek wrote:
> On Mon, May 18, 2020 at 12:37:58PM +0200, Martin Liška wrote:
>> The patch adds new no_stack_protect attribute. The change is requested
>> from kernel folks and is direct equivalent of Clang's no_stack_protector.
>> Unlike Clang, I chose to name it no_stack_protect because we already
>> have stack_protect attribute (used with -fstack-protector-explicit).
> 
> Wouldn't it be better to look at the optimize attribute to see if it really
> does what has been said in the thread and if we don't want to change it,
> such that a function with optimize attribute xyz stands for the option
> defaults + command line options + xyz, rather than option defaults + xyz
> only?

It's documented behavior what we do:

```
The optimize attribute is used to specify that a function is to be compiled
with different optimization options than specified on the command line.
```

It's pretty clear about the command line arguments (that are ignored).

Martin

> 
> 	Jakub
> 

Re: [PATCH] Implement no_stack_protect attribute.

[Jakub Jelinek]

On Mon, May 18, 2020 at 01:00:01PM +0200, Martin Liška wrote:
> On 5/18/20 12:41 PM, Jakub Jelinek wrote:
> > On Mon, May 18, 2020 at 12:37:58PM +0200, Martin Liška wrote:
> > > The patch adds new no_stack_protect attribute. The change is requested
> > > from kernel folks and is direct equivalent of Clang's no_stack_protector.
> > > Unlike Clang, I chose to name it no_stack_protect because we already
> > > have stack_protect attribute (used with -fstack-protector-explicit).
> > 
> > Wouldn't it be better to look at the optimize attribute to see if it really
> > does what has been said in the thread and if we don't want to change it,
> > such that a function with optimize attribute xyz stands for the option
> > defaults + command line options + xyz, rather than option defaults + xyz
> > only?
> 
> It's documented behavior what we do:
> 
> ```
> The optimize attribute is used to specify that a function is to be compiled
> with different optimization options than specified on the command line.
> ```
> 
> It's pretty clear about the command line arguments (that are ignored).

That is not clear at all.  The difference is primarily in what the option
string says in there.

	Jakub

Re: [PATCH] Implement no_stack_protect attribute.

[Jakub Jelinek]

On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote:
> > The optimize attribute is used to specify that a function is to be compiled
> > with different optimization options than specified on the command line.
> > ```
> > 
> > It's pretty clear about the command line arguments (that are ignored).
> 
> That is not clear at all.  The difference is primarily in what the option
> string says in there.

And if we decide we want to keep existing behavior (haven't checked if we
actually behave that way yet), we should add some syntax that will allow
ammending command line options rather than replacing it.
Could be say start the optimize attribute string with + or something
similar.
Does target attribute behave that way too?

	Jakub

Re: [PATCH] Implement no_stack_protect attribute.

[Richard Biener]

On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote:
> > > The optimize attribute is used to specify that a function is to be compiled
> > > with different optimization options than specified on the command line.
> > > ```
> > >
> > > It's pretty clear about the command line arguments (that are ignored).
> >
> > That is not clear at all.  The difference is primarily in what the option
> > string says in there.
>
> And if we decide we want to keep existing behavior (haven't checked if we
> actually behave that way yet), we should add some syntax that will allow
> ammending command line options rather than replacing it.

We do behave this way - while we're running against the current
gcc_options we call decode_options which first does
default_options_optimization.  So it behaves inconsistently with
respect to options not explicitly listed in default_options_table.

But I also think doing the whole processing as in decode_options
is the only thing that's really tested.  And a fix would be to not
start from the current set of options but from a clean slate...

The code clearly thinks we're doing incremental changes:

      /* Save current options.  */
      cl_optimization_save (&cur_opts, &global_options);

      /* If we previously had some optimization options, use them as the
         default.  */
      if (old_opts)
        cl_optimization_restore (&global_options,
                                 TREE_OPTIMIZATION (old_opts));

      /* Parse options, and update the vector.  */
      parse_optimize_options (args, true);
      DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
        = build_optimization_node (&global_options);

      /* Restore current options.  */
      cl_optimization_restore (&global_options, &cur_opts);

so for example you should see -fipa-pta preserved from the
command-line while -fno-tree-pta would be overridden even
if not mentioned explicitely in the optimize attribute.

IMHO the current situation is far from useful...

Richard.

> Could be say start the optimize attribute string with + or something
> similar.
> Does target attribute behave that way too?
>
>         Jakub
>

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2778 bytes --]

On 5/18/20 1:49 PM, Richard Biener wrote:
> On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote:
>>>> The optimize attribute is used to specify that a function is to be compiled
>>>> with different optimization options than specified on the command line.
>>>> ```
>>>>
>>>> It's pretty clear about the command line arguments (that are ignored).
>>>
>>> That is not clear at all.  The difference is primarily in what the option
>>> string says in there.
>>
>> And if we decide we want to keep existing behavior (haven't checked if we
>> actually behave that way yet), we should add some syntax that will allow
>> ammending command line options rather than replacing it.

Hello.

Back to this, I must say that option handling is a complex thing in the GCC.

> 
> We do behave this way - while we're running against the current
> gcc_options we call decode_options which first does
> default_options_optimization.  So it behaves inconsistently with
> respect to options not explicitly listed in default_options_table.

Yes, we basically build on top of the currently selection flags.
We use default_options_table because any optimization level selection
in an optimization attribute should efficiently enforce call of default_options_table.

What about using them only in case one changes optimization level (patch)?

Martin

> 
> But I also think doing the whole processing as in decode_options
> is the only thing that's really tested.  And a fix would be to not
> start from the current set of options but from a clean slate...
> 
> The code clearly thinks we're doing incremental changes:
> 
>        /* Save current options.  */
>        cl_optimization_save (&cur_opts, &global_options);
> 
>        /* If we previously had some optimization options, use them as the
>           default.  */
>        if (old_opts)
>          cl_optimization_restore (&global_options,
>                                   TREE_OPTIMIZATION (old_opts));
> 
>        /* Parse options, and update the vector.  */
>        parse_optimize_options (args, true);
>        DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
>          = build_optimization_node (&global_options);
> 
>        /* Restore current options.  */
>        cl_optimization_restore (&global_options, &cur_opts);
> 
> so for example you should see -fipa-pta preserved from the
> command-line while -fno-tree-pta would be overridden even
> if not mentioned explicitely in the optimize attribute.
> 
> IMHO the current situation is far from useful...
> 
> Richard.
> 
>> Could be say start the optimize attribute string with + or something
>> similar.
>> Does target attribute behave that way too?
>>
>>          Jakub
>>


[-- Attachment #2: 0001-optimize-attribute-fix.patch --]
[-- Type: text/x-patch, Size: 1858 bytes --]

diff --git a/gcc/opts.c b/gcc/opts.c
index ec3ca0720f9..8c39562c697 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -571,6 +571,7 @@ default_options_optimization (struct gcc_options *opts,
   unsigned int i;
   int opt2;
   bool openacc_mode = false;
+  bool opt_level_changed = false;
 
   /* Scan to see what optimization level has been specified.  That will
      determine the default value of many flags.  */
@@ -603,6 +604,7 @@ default_options_optimization (struct gcc_options *opts,
 		  opts->x_optimize_debug = 0;
 		}
 	    }
+	  opt_level_changed = true;
 	  break;
 
 	case OPT_Os:
@@ -612,6 +614,7 @@ default_options_optimization (struct gcc_options *opts,
 	  opts->x_optimize = 2;
 	  opts->x_optimize_fast = 0;
 	  opts->x_optimize_debug = 0;
+	  opt_level_changed = true;
 	  break;
 
 	case OPT_Ofast:
@@ -620,6 +623,7 @@ default_options_optimization (struct gcc_options *opts,
 	  opts->x_optimize = 3;
 	  opts->x_optimize_fast = 1;
 	  opts->x_optimize_debug = 0;
+	  opt_level_changed = true;
 	  break;
 
 	case OPT_Og:
@@ -628,6 +632,7 @@ default_options_optimization (struct gcc_options *opts,
 	  opts->x_optimize = 1;
 	  opts->x_optimize_fast = 0;
 	  opts->x_optimize_debug = 1;
+	  opt_level_changed = true;
 	  break;
 
 	case OPT_fopenacc:
@@ -641,10 +646,11 @@ default_options_optimization (struct gcc_options *opts,
 	}
     }
 
-  maybe_default_options (opts, opts_set, default_options_table,
-			 opts->x_optimize, opts->x_optimize_size,
-			 opts->x_optimize_fast, opts->x_optimize_debug,
-			 lang_mask, handlers, loc, dc);
+  if (opt_level_changed)
+    maybe_default_options (opts, opts_set, default_options_table,
+			   opts->x_optimize, opts->x_optimize_size,
+			   opts->x_optimize_fast, opts->x_optimize_debug,
+			   lang_mask, handlers, loc, dc);
 
   /* -O2 param settings.  */
   opt2 = (opts->x_optimize >= 2);

Re: [PATCH] Implement no_stack_protect attribute.

[Richard Biener]

On Thu, May 21, 2020 at 12:09 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/18/20 1:49 PM, Richard Biener wrote:
> > On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote:
> >>>> The optimize attribute is used to specify that a function is to be compiled
> >>>> with different optimization options than specified on the command line.
> >>>> ```
> >>>>
> >>>> It's pretty clear about the command line arguments (that are ignored).
> >>>
> >>> That is not clear at all.  The difference is primarily in what the option
> >>> string says in there.
> >>
> >> And if we decide we want to keep existing behavior (haven't checked if we
> >> actually behave that way yet), we should add some syntax that will allow
> >> ammending command line options rather than replacing it.
>
> Hello.
>
> Back to this, I must say that option handling is a complex thing in the GCC.
>
> >
> > We do behave this way - while we're running against the current
> > gcc_options we call decode_options which first does
> > default_options_optimization.  So it behaves inconsistently with
> > respect to options not explicitly listed in default_options_table.
>
> Yes, we basically build on top of the currently selection flags.
> We use default_options_table because any optimization level selection
> in an optimization attribute should efficiently enforce call of default_options_table.
>
> What about using them only in case one changes optimization level (patch)?

I'm not sure if that works, no -On means -O0, so one might interpret
optimize("omit-frame-pointer") as -O0 -fomit-frame-pointer.   Also -On
are not treated in position dependent way, thus -fno-tree-pre -O2 is
the same as -O2 -fno-tree-pre rather than re-enabling PRE over -fno-tree-pre.
So your patch would turn a -O2 -fno-tree-pre invocation, when
optimize("O3") is encountered, to one with PRE enabled, not matching
behavior of -O2 -fno-tree-pre -O3.

I think the only sensible way is to save the original decoded options
from the gcc invocation (and have #pragma optimize push/pop append
accordingly) and append the current attribute/pragmas optimization
string to them and run that whole thing on a default option state.

That makes semantics equivalent to appending more options to the
command-line.  Well, hopefully.  Interaction with the target attribute
might be interesting (that likely also needs to append to that
"current decoded options" set).

As you say, option handling is difficult.

Richard.

> Martin
>
> >
> > But I also think doing the whole processing as in decode_options
> > is the only thing that's really tested.  And a fix would be to not
> > start from the current set of options but from a clean slate...
> >
> > The code clearly thinks we're doing incremental changes:
> >
> >        /* Save current options.  */
> >        cl_optimization_save (&cur_opts, &global_options);
> >
> >        /* If we previously had some optimization options, use them as the
> >           default.  */
> >        if (old_opts)
> >          cl_optimization_restore (&global_options,
> >                                   TREE_OPTIMIZATION (old_opts));
> >
> >        /* Parse options, and update the vector.  */
> >        parse_optimize_options (args, true);
> >        DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
> >          = build_optimization_node (&global_options);
> >
> >        /* Restore current options.  */
> >        cl_optimization_restore (&global_options, &cur_opts);
> >
> > so for example you should see -fipa-pta preserved from the
> > command-line while -fno-tree-pta would be overridden even
> > if not mentioned explicitely in the optimize attribute.
> >
> > IMHO the current situation is far from useful...
> >
> > Richard.
> >
> >> Could be say start the optimize attribute string with + or something
> >> similar.
> >> Does target attribute behave that way too?
> >>
> >>          Jakub
> >>
>

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

On 5/22/20 12:51 PM, Richard Biener wrote:
> On Thu, May 21, 2020 at 12:09 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 5/18/20 1:49 PM, Richard Biener wrote:
>>> On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote:
>>>>>> The optimize attribute is used to specify that a function is to be compiled
>>>>>> with different optimization options than specified on the command line.
>>>>>> ```
>>>>>>
>>>>>> It's pretty clear about the command line arguments (that are ignored).
>>>>>
>>>>> That is not clear at all.  The difference is primarily in what the option
>>>>> string says in there.
>>>>
>>>> And if we decide we want to keep existing behavior (haven't checked if we
>>>> actually behave that way yet), we should add some syntax that will allow
>>>> ammending command line options rather than replacing it.
>>
>> Hello.
>>
>> Back to this, I must say that option handling is a complex thing in the GCC.
>>
>>>
>>> We do behave this way - while we're running against the current
>>> gcc_options we call decode_options which first does
>>> default_options_optimization.  So it behaves inconsistently with
>>> respect to options not explicitly listed in default_options_table.
>>
>> Yes, we basically build on top of the currently selection flags.
>> We use default_options_table because any optimization level selection
>> in an optimization attribute should efficiently enforce call of default_options_table.
>>
>> What about using them only in case one changes optimization level (patch)?
> 
> I'm not sure if that works, no -On means -O0, so one might interpret
> optimize("omit-frame-pointer") as -O0 -fomit-frame-pointer.   Also -On
> are not treated in position dependent way, thus -fno-tree-pre -O2 is
> the same as -O2 -fno-tree-pre rather than re-enabling PRE over -fno-tree-pre.
> So your patch would turn a -O2 -fno-tree-pre invocation, when
> optimize("O3") is encountered, to one with PRE enabled, not matching
> behavior of -O2 -fno-tree-pre -O3.
> 
> I think the only sensible way is to save the original decoded options
> from the gcc invocation (and have #pragma optimize push/pop append
> accordingly) and append the current attribute/pragmas optimization
> string to them and run that whole thing on a default option state.

That should work. Anyway, that's a task with unclear finish.
Would it be possible to add the no_stack_protect attribute for the time being?
The limitation is there for ages and we have another "optimize" attributes
that can be theoretically replaced with proper optimize handling.

> 
> That makes semantics equivalent to appending more options to the
> command-line.  Well, hopefully.  Interaction with the target attribute
> might be interesting (that likely also needs to append to that
> "current decoded options" set).

I fear from the interaction of optimization and target attribute.

> 
> As you say, option handling is difficult.

Anyway, I'm planning to work on the options in stage1. It's quite close
relative to PR92860.

Martin

> 
> Richard.
> 
>> Martin
>>
>>>
>>> But I also think doing the whole processing as in decode_options
>>> is the only thing that's really tested.  And a fix would be to not
>>> start from the current set of options but from a clean slate...
>>>
>>> The code clearly thinks we're doing incremental changes:
>>>
>>>         /* Save current options.  */
>>>         cl_optimization_save (&cur_opts, &global_options);
>>>
>>>         /* If we previously had some optimization options, use them as the
>>>            default.  */
>>>         if (old_opts)
>>>           cl_optimization_restore (&global_options,
>>>                                    TREE_OPTIMIZATION (old_opts));
>>>
>>>         /* Parse options, and update the vector.  */
>>>         parse_optimize_options (args, true);
>>>         DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
>>>           = build_optimization_node (&global_options);
>>>
>>>         /* Restore current options.  */
>>>         cl_optimization_restore (&global_options, &cur_opts);
>>>
>>> so for example you should see -fipa-pta preserved from the
>>> command-line while -fno-tree-pta would be overridden even
>>> if not mentioned explicitely in the optimize attribute.
>>>
>>> IMHO the current situation is far from useful...
>>>
>>> Richard.
>>>
>>>> Could be say start the optimize attribute string with + or something
>>>> similar.
>>>> Does target attribute behave that way too?
>>>>
>>>>           Jakub
>>>>
>>

Re: [PATCH] Implement no_stack_protect attribute.

[Richard Biener]

On Mon, May 25, 2020 at 11:27 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/22/20 12:51 PM, Richard Biener wrote:
> > On Thu, May 21, 2020 at 12:09 PM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 5/18/20 1:49 PM, Richard Biener wrote:
> >>> On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches
> >>> <gcc-patches@gcc.gnu.org> wrote:
> >>>>
> >>>> On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote:
> >>>>>> The optimize attribute is used to specify that a function is to be compiled
> >>>>>> with different optimization options than specified on the command line.
> >>>>>> ```
> >>>>>>
> >>>>>> It's pretty clear about the command line arguments (that are ignored).
> >>>>>
> >>>>> That is not clear at all.  The difference is primarily in what the option
> >>>>> string says in there.
> >>>>
> >>>> And if we decide we want to keep existing behavior (haven't checked if we
> >>>> actually behave that way yet), we should add some syntax that will allow
> >>>> ammending command line options rather than replacing it.
> >>
> >> Hello.
> >>
> >> Back to this, I must say that option handling is a complex thing in the GCC.
> >>
> >>>
> >>> We do behave this way - while we're running against the current
> >>> gcc_options we call decode_options which first does
> >>> default_options_optimization.  So it behaves inconsistently with
> >>> respect to options not explicitly listed in default_options_table.
> >>
> >> Yes, we basically build on top of the currently selection flags.
> >> We use default_options_table because any optimization level selection
> >> in an optimization attribute should efficiently enforce call of default_options_table.
> >>
> >> What about using them only in case one changes optimization level (patch)?
> >
> > I'm not sure if that works, no -On means -O0, so one might interpret
> > optimize("omit-frame-pointer") as -O0 -fomit-frame-pointer.   Also -On
> > are not treated in position dependent way, thus -fno-tree-pre -O2 is
> > the same as -O2 -fno-tree-pre rather than re-enabling PRE over -fno-tree-pre.
> > So your patch would turn a -O2 -fno-tree-pre invocation, when
> > optimize("O3") is encountered, to one with PRE enabled, not matching
> > behavior of -O2 -fno-tree-pre -O3.
> >
> > I think the only sensible way is to save the original decoded options
> > from the gcc invocation (and have #pragma optimize push/pop append
> > accordingly) and append the current attribute/pragmas optimization
> > string to them and run that whole thing on a default option state.
>
> That should work. Anyway, that's a task with unclear finish.
> Would it be possible to add the no_stack_protect attribute for the time being?

I think it's a reasonable request, yes.

Richard.

> The limitation is there for ages and we have another "optimize" attributes
> that can be theoretically replaced with proper optimize handling.
>
> >
> > That makes semantics equivalent to appending more options to the
> > command-line.  Well, hopefully.  Interaction with the target attribute
> > might be interesting (that likely also needs to append to that
> > "current decoded options" set).
>
> I fear from the interaction of optimization and target attribute.
>
> >
> > As you say, option handling is difficult.
>
> Anyway, I'm planning to work on the options in stage1. It's quite close
> relative to PR92860.
>
> Martin
>
> >
> > Richard.
> >
> >> Martin
> >>
> >>>
> >>> But I also think doing the whole processing as in decode_options
> >>> is the only thing that's really tested.  And a fix would be to not
> >>> start from the current set of options but from a clean slate...
> >>>
> >>> The code clearly thinks we're doing incremental changes:
> >>>
> >>>         /* Save current options.  */
> >>>         cl_optimization_save (&cur_opts, &global_options);
> >>>
> >>>         /* If we previously had some optimization options, use them as the
> >>>            default.  */
> >>>         if (old_opts)
> >>>           cl_optimization_restore (&global_options,
> >>>                                    TREE_OPTIMIZATION (old_opts));
> >>>
> >>>         /* Parse options, and update the vector.  */
> >>>         parse_optimize_options (args, true);
> >>>         DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
> >>>           = build_optimization_node (&global_options);
> >>>
> >>>         /* Restore current options.  */
> >>>         cl_optimization_restore (&global_options, &cur_opts);
> >>>
> >>> so for example you should see -fipa-pta preserved from the
> >>> command-line while -fno-tree-pta would be overridden even
> >>> if not mentioned explicitely in the optimize attribute.
> >>>
> >>> IMHO the current situation is far from useful...
> >>>
> >>> Richard.
> >>>
> >>>> Could be say start the optimize attribute string with + or something
> >>>> similar.
> >>>> Does target attribute behave that way too?
> >>>>
> >>>>           Jakub
> >>>>
> >>
>

Re: [PATCH] Implement no_stack_protect attribute.

[Florian Weimer]

* Martin Liška:

> The patch adds new no_stack_protect attribute. The change is requested
> from kernel folks and is direct equivalent of Clang's no_stack_protector.
> Unlike Clang, I chose to name it no_stack_protect because we already
> have stack_protect attribute (used with -fstack-protector-explicit).
>
> First part of the patch contains a small refactoring of an enum, second
> implements the functionality.

In glibc, we already have this:

/* Used to disable stack protection in sensitive places, like ifunc
   resolvers and early static TLS init.  */
#ifdef HAVE_CC_NO_STACK_PROTECTOR
# define inhibit_stack_protector \
    __attribute__ ((__optimize__ ("-fno-stack-protector")))
#else
# define inhibit_stack_protector
#endif

Is it broken?

Thanks,
Florian

Re: [PATCH] Implement no_stack_protect attribute.

[Michael Matz]

Hello,

On Mon, 18 May 2020, Florian Weimer via Gcc-patches wrote:

> > The patch adds new no_stack_protect attribute. The change is requested
> > from kernel folks and is direct equivalent of Clang's no_stack_protector.
> > Unlike Clang, I chose to name it no_stack_protect because we already
> > have stack_protect attribute (used with -fstack-protector-explicit).
> >
> > First part of the patch contains a small refactoring of an enum, second
> > implements the functionality.
> 
> In glibc, we already have this:
> 
> /* Used to disable stack protection in sensitive places, like ifunc
>    resolvers and early static TLS init.  */
> #ifdef HAVE_CC_NO_STACK_PROTECTOR
> # define inhibit_stack_protector \
>     __attribute__ ((__optimize__ ("-fno-stack-protector")))
> #else
> # define inhibit_stack_protector
> #endif
> 
> Is it broken?

Depends on what your expectations are.  It completely overrides all 
options given on the command line (including things like 
fno-omit-frame-pointer and -O2!).  At least I was very surprised by that 
even though the current docu can be read that way; if you're similarly 
surprised, then yes, the above is broken, it does not only disable stack 
protection (but also e.g. all optimizations, despite the attributes name 
:-) ).

And given that there's no possibility to express "and please only _add_ to 
the cmdline options" (which implies also being able to override values 
from the cmdline with -fno-xxx or other -fyyy options) I consider our 
current GCC behaviour for the optimize attribute basically unusable.

(One work-around would be to define a macro containing all cmdline options 
in string form in the build system.  Obviously that's a silly solution.)

So, yeah, IMHO the semantics of that attribute should be revised to be 
more useful by default (with the possibility to express the current 
behaviour, for whomever would like to have that (but who?)).


Ciao,
Michael.

Re: [PATCH] Implement no_stack_protect attribute.

[Florian Weimer]

* Michael Matz:

> Hello,
>
> On Mon, 18 May 2020, Florian Weimer via Gcc-patches wrote:
>
>> > The patch adds new no_stack_protect attribute. The change is requested
>> > from kernel folks and is direct equivalent of Clang's no_stack_protector.
>> > Unlike Clang, I chose to name it no_stack_protect because we already
>> > have stack_protect attribute (used with -fstack-protector-explicit).
>> >
>> > First part of the patch contains a small refactoring of an enum, second
>> > implements the functionality.
>> 
>> In glibc, we already have this:
>> 
>> /* Used to disable stack protection in sensitive places, like ifunc
>>    resolvers and early static TLS init.  */
>> #ifdef HAVE_CC_NO_STACK_PROTECTOR
>> # define inhibit_stack_protector \
>>     __attribute__ ((__optimize__ ("-fno-stack-protector")))
>> #else
>> # define inhibit_stack_protector
>> #endif
>> 
>> Is it broken?
>
> Depends on what your expectations are.  It completely overrides all 
> options given on the command line (including things like 
> fno-omit-frame-pointer and -O2!).  At least I was very surprised by that 
> even though the current docu can be read that way; if you're similarly 
> surprised, then yes, the above is broken, it does not only disable stack 
> protection (but also e.g. all optimizations, despite the attributes name 
> :-) ).

Yes, that would qualify as broken.

This is not what I observe with gcc-9.3.1-2.fc31.x86_64 from Fedora.
-O2 still has an effect.  So does -fcf-protection.
-fno-omit-frame-pointer does not work for me at all for some reason, the
frame pointer is always missing?

Not sure what is going on here …

Thanks,
Florian

Re: [PATCH] Implement no_stack_protect attribute.

[Michael Matz]

Hello,

On Mon, 18 May 2020, Florian Weimer wrote:

> >> In glibc, we already have this:
> >> 
> >> /* Used to disable stack protection in sensitive places, like ifunc
> >>    resolvers and early static TLS init.  */
> >> #ifdef HAVE_CC_NO_STACK_PROTECTOR
> >> # define inhibit_stack_protector \
> >>     __attribute__ ((__optimize__ ("-fno-stack-protector")))
> >> #else
> >> # define inhibit_stack_protector
> >> #endif
> >> 
> >> Is it broken?
> >
> > Depends on what your expectations are.  It completely overrides all 
> > options given on the command line (including things like 
> > fno-omit-frame-pointer and -O2!).  At least I was very surprised by that 
> > even though the current docu can be read that way; if you're similarly 
> > surprised, then yes, the above is broken, it does not only disable stack 
> > protection (but also e.g. all optimizations, despite the attributes name 
> > :-) ).
> 
> Yes, that would qualify as broken.
> 
> This is not what I observe with gcc-9.3.1-2.fc31.x86_64 from Fedora.
> -O2 still has an effect.

Indeed, I definitely remember an interaction with the attribute and -O{,2} 
(or something that I interpreted as such) but it obviously isn't as simple 
as plain disabling it, and right now I can't recreate the situation :-/
(It might be disabling of some further cmdline flags that I conflated in 
my brain with "effect of -O2")

> So does -fcf-protection. -fno-omit-frame-pointer does not work for me at 
> all for some reason, the frame pointer is always missing?

Not for me:

% cat simple.c
extern int bla(int *);
int
#ifdef ATTR
__attribute__((__optimize__ ("-fno-stack-protector")))
#endif
foo(int a, int b)
{
  int c = b;
  return a * 42 + bla(&c);
}
% gcc-9 -fno-omit-frame-pointer -O -S -o - tryme.c | grep bp
        pushq   %rbp
        movq    %rsp, %rbp
        movl    %esi, -20(%rbp)
        leaq    -20(%rbp), %rdi
        popq    %rbp
% gcc-9 -fstack-protector-all -fno-omit-frame-pointer -O -S -o - tryme.c | grep bp
        pushq   %rbp
        movq    %rsp, %rbp
        movq    %rax, -24(%rbp)
        movl    %esi, -28(%rbp)
        leaq    -28(%rbp), %rdi
        movq    -24(%rbp), %rdx
        popq    %rbp

But using the attr:

% gcc-9 -DATTR -fstack-protector-all -fno-omit-frame-pointer -O -S -o - tryme.c | grep bp
% 

(gcc9 is gcc9-9.2.1+r275327-1.1.x86_64 on opensuse)


Ciao,
Michael.

Re: [PATCH] Implement no_stack_protect attribute.

[Florian Weimer]

* Michael Matz:

>> So does -fcf-protection. -fno-omit-frame-pointer does not work for me at 
>> all for some reason, the frame pointer is always missing?
>
> Not for me:

I see.  I didn't know that -fno-omit-frame-pointer only applies to
non-leaf functions.  For them, the behavior I see matches yours.

Thanks,
Florian

Re: [PATCH] Implement no_stack_protect attribute.

[Michael Matz]

Hello,

On Mon, 18 May 2020, Florian Weimer wrote:

> * Michael Matz:
> 
> >> So does -fcf-protection. -fno-omit-frame-pointer does not work for me at 
> >> all for some reason, the frame pointer is always missing?
> >
> > Not for me:
> 
> I see.  I didn't know that -fno-omit-frame-pointer only applies to
> non-leaf functions.

Yeah, I actually consider this a bug as well (unrelated though).  The user 
of that flag quite surely has good reasons to want to have a frame 
pointer, and those good reasons usually extend to all functions, not just 
to leaf ones.  (E.g. ensuring that backtraces from async signal handlers 
are meaningful, for instance for poor mans profiling).


Ciao,
Michael.

Re: [PATCH] Implement no_stack_protect attribute.

[Andreas Schwab]

On Mai 18 2020, Florian Weimer via Gcc-patches wrote:

> * Michael Matz:
>
>>> So does -fcf-protection. -fno-omit-frame-pointer does not work for me at 
>>> all for some reason, the frame pointer is always missing?
>>
>> Not for me:
>
> I see.  I didn't know that -fno-omit-frame-pointer only applies to
> non-leaf functions.  For them, the behavior I see matches yours.

There is also -momit-leaf-frame-pointer.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

On 5/18/20 5:56 PM, Florian Weimer via Gcc-patches wrote:
> * Michael Matz:
> 
>>> So does -fcf-protection. -fno-omit-frame-pointer does not work for me at
>>> all for some reason, the frame pointer is always missing?
>>
>> Not for me:
> 
> I see.  I didn't know that -fno-omit-frame-pointer only applies to
> non-leaf functions.  For them, the behavior I see matches yours.
> 
> Thanks,
> Florian
> 

I bet it's related to this issue https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92860#c4.
The issue is about update of target options from optimization option and there's proper
restore to the original state.

Martin

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Sebor]

On 5/18/20 4:37 AM, Martin Liška wrote:
> Hi.
> 
> The patch adds new no_stack_protect attribute. The change is requested
> from kernel folks and is direct equivalent of Clang's no_stack_protector.
> Unlike Clang, I chose to name it no_stack_protect because we already
> have stack_protect attribute (used with -fstack-protector-explicit).
> 
> First part of the patch contains a small refactoring of an enum, second
> implements the functionality.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> 0002-Implement-no_stack_protect-attribute.patch
> 
>  From 6b3e9d32150fe17acb271b7bedee7dc95cad7dc8 Mon Sep 17 00:00:00 2001
> From: Martin Liska<mliska@suse.cz>
> Date: Fri, 15 May 2020 14:42:12 +0200
> Subject: [PATCH 2/2] Implement no_stack_protect attribute.
> 
> gcc/ChangeLog:
> 
> 2020-05-18  Martin Liska<mliska@suse.cz>
> 
> 	PR c/94722
> 	* cfgexpand.c (stack_protect_decl_phase):
> 	Guard with lookup_attribute("no_stack_protect") at
> 	various places.
> 	(expand_used_vars): Likewise here.
> 	* doc/extend.texi: Document no_stack_protect attribute.
> 
> gcc/ada/ChangeLog:
> 
> 2020-05-18  Martin Liska<mliska@suse.cz>
> 
> 	PR c/94722
> 	* gcc-interface/utils.c (handle_no_stack_protect_attribute):
> 	New.
> 	(handle_stack_protect_attribute): Add error message for a
> 	no_stack_protect function.
> 
> gcc/c-family/ChangeLog:
> 
> 2020-05-18  Martin Liska<mliska@suse.cz>
> 
> 	PR c/94722
> 	* c-attribs.c (handle_no_stack_protect_function_attribute): New.
> 	(handle_stack_protect_attribute): Add error message for a
> 	no_stack_protect function.
> 
> gcc/testsuite/ChangeLog:
> 
> 2020-05-18  Martin Liska<mliska@suse.cz>
> 
> 	PR c/94722
> 	* g++.dg/no-stack-protect-attr-2.C: New test.
> 	* g++.dg/no-stack-protect-attr-3.C: New test.
> 	* g++.dg/no-stack-protect-attr.C: New test.
> ---
>   gcc/ada/gcc-interface/utils.c                 | 32 ++++++++
>   gcc/c-family/c-attribs.c                      | 33 ++++++++
>   gcc/cfgexpand.c                               | 82 ++++++++++---------
>   gcc/doc/extend.texi                           |  4 +
>   .../g++.dg/no-stack-protect-attr-2.C          |  7 ++
>   .../g++.dg/no-stack-protect-attr-3.C          | 23 ++++++
>   gcc/testsuite/g++.dg/no-stack-protect-attr.C  | 16 ++++
>   7 files changed, 158 insertions(+), 39 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-2.C
>   create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-3.C
>   create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr.C
> 
> diff --git a/gcc/ada/gcc-interface/utils.c b/gcc/ada/gcc-interface/utils.c
> index 1527be4f6d1..144afe37d78 100644
> --- a/gcc/ada/gcc-interface/utils.c
> +++ b/gcc/ada/gcc-interface/utils.c
> @@ -91,6 +91,7 @@ static tree handle_nonnull_attribute (tree *, tree, tree, int, bool *);
>   static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
>   static tree handle_noreturn_attribute (tree *, tree, tree, int, bool *);
>   static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_no_stack_protect_attribute (tree *, tree, tree, int, bool *);
>   static tree handle_noinline_attribute (tree *, tree, tree, int, bool *);
>   static tree handle_noclone_attribute (tree *, tree, tree, int, bool *);
>   static tree handle_noicf_attribute (tree *, tree, tree, int, bool *);
> @@ -141,6 +142,8 @@ const struct attribute_spec gnat_internal_attribute_table[] =
>       handle_noreturn_attribute, NULL },
>     { "stack_protect",0, 0, true,  false, false, false,
>       handle_stack_protect_attribute, NULL },
> +  { "no_stack_protect",0, 0, true,  false, false, false,
> +    handle_no_stack_protect_attribute, NULL },
>     { "noinline",     0, 0,  true,  false, false, false,
>       handle_noinline_attribute, NULL },
>     { "noclone",      0, 0,  true,  false, false, false,
> @@ -6523,10 +6526,39 @@ handle_stack_protect_attribute (tree *node, tree name, tree, int,
>         warning (OPT_Wattributes, "%qE attribute ignored", name);
>         *no_add_attrs = true;
>       }
> +  else if (lookup_attribute ("no_stack_protect", DECL_ATTRIBUTES (*node)))
> +    {
> +      warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
> +	       "with %qs attribute", name, "no_stack_protect");
> +      *no_add_attrs = true;

I know there are some somewhat complex cases the attribute exclusion
mechanism isn't general enough to handle but this seems simple enough
that it should work.  Unless I'm missing something that makes it not
feasible I would suggest to use it.

> diff --git a/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C b/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C
> new file mode 100644
> index 00000000000..dcac6f9d389
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C
> @@ -0,0 +1,7 @@
> +/* PR c/94722 */
> +/* { dg-do compile } */
> +
> +int __attribute__((no_stack_protect, stack_protect)) c() /* { dg-warning "'stack_protect' attribute ignored due to conflict with 'no_stack_protect' attribute" } */

The more likely misuse than applying incompatible attributes to
the same declaration is to redeclare the same function, once with
one attribute and then again with the other.  One of the main
benefits of the attribute exclusion mechanism is to detect both
and point at the two declarations to make correcting one or
the other easier.

Martin

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

On 5/18/20 10:37 PM, Martin Sebor wrote:
> I know there are some somewhat complex cases the attribute exclusion
> mechanism isn't general enough to handle but this seems simple enough
> that it should work.  Unless I'm missing something that makes it not
> feasible I would suggest to use it.

Hi Martin.

Do we have a better place where we check for attribute collision?

Martin

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Sebor]

On 5/21/20 5:28 AM, Martin Liška wrote:
> On 5/18/20 10:37 PM, Martin Sebor wrote:
>> I know there are some somewhat complex cases the attribute exclusion
>> mechanism isn't general enough to handle but this seems simple enough
>> that it should work.  Unless I'm missing something that makes it not
>> feasible I would suggest to use it.
> 
> Hi Martin.
> 
> Do we have a better place where we check for attribute collision?

If by collision you mean the same thing as the mutual exclusion I was
talking about then that's done by creating an attribute_spec::exclusions
array like for instance attr_cold_hot_exclusions in c-attribs.c and
pointing to it from the attribute_spec entries for each of
the mutually exclusive attributes in the attribute table.  Everything
else is handled automatically by decl_attributes.

Martin

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

On 5/21/20 4:53 PM, Martin Sebor wrote:
> On 5/21/20 5:28 AM, Martin Liška wrote:
>> On 5/18/20 10:37 PM, Martin Sebor wrote:
>>> I know there are some somewhat complex cases the attribute exclusion
>>> mechanism isn't general enough to handle but this seems simple enough
>>> that it should work.  Unless I'm missing something that makes it not
>>> feasible I would suggest to use it.
>>
>> Hi Martin.
>>
>> Do we have a better place where we check for attribute collision?
> 
> If by collision you mean the same thing as the mutual exclusion I was
> talking about then that's done by creating an attribute_spec::exclusions
> array like for instance attr_cold_hot_exclusions in c-attribs.c and
> pointing to it from the attribute_spec entries for each of
> the mutually exclusive attributes in the attribute table.  Everything
> else is handled automatically by decl_attributes.
> 
> Martin

Thanks, I'm sending updated version of the patch that utilizes the conflict
detection.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

[-- Attachment #2: 0001-Implement-no_stack_protect-attribute.patch --]
[-- Type: text/x-patch, Size: 13914 bytes --]

From b026a8684a0a82394ce2faf010257fa3f0978ca0 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Fri, 15 May 2020 14:42:12 +0200
Subject: [PATCH] Implement no_stack_protect attribute.

gcc/ChangeLog:

2020-05-18  Martin Liska  <mliska@suse.cz>

	PR c/94722
	* cfgexpand.c (stack_protect_decl_phase):
	Guard with lookup_attribute("no_stack_protect") at
	various places.
	(expand_used_vars): Likewise here.
	* doc/extend.texi: Document no_stack_protect attribute.

gcc/ada/ChangeLog:

2020-05-18  Martin Liska  <mliska@suse.cz>

	PR c/94722
	* gcc-interface/utils.c (handle_no_stack_protect_attribute):
	New.
	(handle_stack_protect_attribute): Add error message for a
	no_stack_protect function.

gcc/c-family/ChangeLog:

2020-05-18  Martin Liska  <mliska@suse.cz>

	PR c/94722
	* c-attribs.c (handle_no_stack_protect_function_attribute): New.
	(handle_stack_protect_attribute): Add error message for a
	no_stack_protect function.

gcc/testsuite/ChangeLog:

2020-05-18  Martin Liska  <mliska@suse.cz>

	PR c/94722
	* g++.dg/no-stack-protect-attr-2.C: New test.
	* g++.dg/no-stack-protect-attr-3.C: New test.
	* g++.dg/no-stack-protect-attr.C: New test.
---
 gcc/ada/gcc-interface/utils.c                 | 31 ++++++-
 gcc/c-family/c-attribs.c                      | 32 +++++++-
 gcc/cfgexpand.c                               | 82 ++++++++++---------
 gcc/doc/extend.texi                           |  4 +
 .../g++.dg/no-stack-protect-attr-2.C          |  7 ++
 .../g++.dg/no-stack-protect-attr-3.C          | 23 ++++++
 gcc/testsuite/g++.dg/no-stack-protect-attr.C  | 16 ++++
 7 files changed, 154 insertions(+), 41 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-2.C
 create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-3.C
 create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr.C

diff --git a/gcc/ada/gcc-interface/utils.c b/gcc/ada/gcc-interface/utils.c
index fb08b6c90ed..b6a60eb1b11 100644
--- a/gcc/ada/gcc-interface/utils.c
+++ b/gcc/ada/gcc-interface/utils.c
@@ -91,6 +91,7 @@ static tree handle_nonnull_attribute (tree *, tree, tree, int, bool *);
 static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noreturn_attribute (tree *, tree, tree, int, bool *);
 static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *);
+static tree handle_no_stack_protect_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noinline_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noclone_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noicf_attribute (tree *, tree, tree, int, bool *);
@@ -115,6 +116,13 @@ static const struct attribute_spec::exclusions attr_cold_hot_exclusions[] =
   { NULL  , false, false, false }
 };
 
+static const struct attribute_spec::exclusions attr_stack_protect_exclusions[] =
+{
+  { "stack_protect", true, false, false },
+  { "no_stack_protect", true, false, false },
+  { NULL, false, false, false },
+};
+
 /* Fake handler for attributes we don't properly support, typically because
    they'd require dragging a lot of the common-c front-end circuitry.  */
 static tree fake_attribute_handler (tree *, tree, tree, int, bool *);
@@ -140,7 +148,11 @@ const struct attribute_spec gnat_internal_attribute_table[] =
   { "noreturn",     0, 0,  true,  false, false, false,
     handle_noreturn_attribute, NULL },
   { "stack_protect",0, 0, true,  false, false, false,
-    handle_stack_protect_attribute, NULL },
+    handle_stack_protect_attribute,
+    attr_stack_protect_exclusions },
+  { "no_stack_protect",0, 0, true,  false, false, false,
+    handle_no_stack_protect_attribute,
+    attr_stack_protect_exclusions },
   { "noinline",     0, 0,  true,  false, false, false,
     handle_noinline_attribute, NULL },
   { "noclone",      0, 0,  true,  false, false, false,
@@ -6524,6 +6536,23 @@ handle_stack_protect_attribute (tree *node, tree name, tree, int,
   return NULL_TREE;
 }
 
+/* Handle a "no_stack_protect" attribute; arguments as in
+   struct attribute_spec.handler.  */
+
+static tree
+handle_no_stack_protect_attribute (tree *node, tree name, tree, int,
+				   bool *no_add_attrs)
+{
+  if (TREE_CODE (*node) != FUNCTION_DECL)
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored", name);
+      *no_add_attrs = true;
+    }
+
+  return NULL_TREE;
+}
+
+
 /* Handle a "noinline" attribute; arguments as in
    struct attribute_spec.handler.  */
 
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 7a6fb9af6da..84bdb4fca04 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -63,6 +63,8 @@ static tree handle_no_sanitize_undefined_attribute (tree *, tree, tree, int,
 static tree handle_asan_odr_indicator_attribute (tree *, tree, tree, int,
 						 bool *);
 static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *);
+static tree handle_no_stack_protect_function_attribute (tree *, tree,
+							tree, int, bool *);
 static tree handle_noinline_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noclone_attribute (tree *, tree, tree, int, bool *);
 static tree handle_nocf_check_attribute (tree *, tree, tree, int, bool *);
@@ -245,6 +247,14 @@ static const struct attribute_spec::exclusions attr_noinit_exclusions[] =
   ATTR_EXCL (NULL, false, false, false),
 };
 
+static const struct attribute_spec::exclusions attr_stack_protect_exclusions[] =
+{
+  ATTR_EXCL ("stack_protect", true, false, false),
+  ATTR_EXCL ("no_stack_protect", true, false, false),
+  ATTR_EXCL (NULL, false, false, false),
+};
+
+
 /* Table of machine-independent attributes common to all C-like languages.
 
    Current list of processed common attributes: nonnull.  */
@@ -272,7 +282,11 @@ const struct attribute_spec c_common_attribute_table[] =
   { "volatile",               0, 0, true,  false, false, false,
 			      handle_noreturn_attribute, NULL },
   { "stack_protect",          0, 0, true,  false, false, false,
-			      handle_stack_protect_attribute, NULL },
+			      handle_stack_protect_attribute,
+			      attr_stack_protect_exclusions },
+  { "no_stack_protect",	      0, 0, true, false, false, false,
+			      handle_no_stack_protect_function_attribute,
+			      attr_stack_protect_exclusions },
   { "noinline",               0, 0, true,  false, false, false,
 			      handle_noinline_attribute,
 	                      attr_noinline_exclusions },
@@ -1021,6 +1035,22 @@ handle_stack_protect_attribute (tree *node, tree name, tree, int,
   return NULL_TREE;
 }
 
+/* Handle a "no_stack_protect" attribute; arguments as in
+   struct attribute_spec.handler.  */
+
+static tree
+handle_no_stack_protect_function_attribute (tree *node, tree name, tree,
+					    int, bool *no_add_attrs)
+{
+  if (TREE_CODE (*node) != FUNCTION_DECL)
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored", name);
+      *no_add_attrs = true;
+    }
+
+  return NULL_TREE;
+}
+
 /* Handle a "noipa" attribute; arguments as in
    struct attribute_spec.handler.  */
 
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index b66c3e5f7af..6e8687c392c 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1831,11 +1831,12 @@ stack_protect_decl_phase (tree decl)
   if (bits & SPCT_HAS_SMALL_CHAR_ARRAY)
     has_short_buffer = true;
 
-  if (flag_stack_protect == SPCT_FLAG_ALL
-      || flag_stack_protect == SPCT_FLAG_STRONG
-      || (flag_stack_protect == SPCT_FLAG_EXPLICIT
-	  && lookup_attribute ("stack_protect",
-			       DECL_ATTRIBUTES (current_function_decl))))
+  tree attribs = DECL_ATTRIBUTES (current_function_decl);
+  if (!lookup_attribute ("no_stack_protect", attribs)
+      && (flag_stack_protect == SPCT_FLAG_ALL
+	  || flag_stack_protect == SPCT_FLAG_STRONG
+	  || (flag_stack_protect == SPCT_FLAG_EXPLICIT
+	      && lookup_attribute ("stack_protect", attribs))))
     {
       if ((bits & (SPCT_HAS_SMALL_CHAR_ARRAY | SPCT_HAS_LARGE_CHAR_ARRAY))
 	  && !(bits & SPCT_HAS_AGGREGATE))
@@ -2136,6 +2137,7 @@ expand_used_vars (void)
      set are actually used by the optimized function.  Lay them out.  */
   expand_used_vars_for_block (outer_block, true);
 
+  tree attribs = DECL_ATTRIBUTES (current_function_decl);
   if (stack_vars_num > 0)
     {
       bool has_addressable_vars = false;
@@ -2145,10 +2147,10 @@ expand_used_vars (void)
       /* If stack protection is enabled, we don't share space between
 	 vulnerable data and non-vulnerable data.  */
       if (flag_stack_protect != 0
+	  && !lookup_attribute ("no_stack_protect", attribs)
 	  && (flag_stack_protect != SPCT_FLAG_EXPLICIT
 	      || (flag_stack_protect == SPCT_FLAG_EXPLICIT
-		  && lookup_attribute ("stack_protect",
-				       DECL_ATTRIBUTES (current_function_decl)))))
+		  && lookup_attribute ("stack_protect", attribs))))
 	has_addressable_vars = add_stack_protection_conflicts ();
 
       if (flag_stack_protect == SPCT_FLAG_STRONG && has_addressable_vars)
@@ -2161,38 +2163,40 @@ expand_used_vars (void)
 	dump_stack_var_partition ();
     }
 
-  switch (flag_stack_protect)
-    {
-    case SPCT_FLAG_ALL:
-      create_stack_guard ();
-      break;
 
-    case SPCT_FLAG_STRONG:
-      if (gen_stack_protect_signal
-	  || cfun->calls_alloca
-	  || has_protected_decls
-	  || lookup_attribute ("stack_protect",
-			       DECL_ATTRIBUTES (current_function_decl)))
+  if (!lookup_attribute ("no_stack_protect", attribs))
+    switch (flag_stack_protect)
+      {
+      case SPCT_FLAG_ALL:
 	create_stack_guard ();
-      break;
+	break;
 
-    case SPCT_FLAG_DEFAULT:
-      if (cfun->calls_alloca
-	  || has_protected_decls
-	  || lookup_attribute ("stack_protect",
-			       DECL_ATTRIBUTES (current_function_decl)))
-	create_stack_guard ();
-      break;
+      case SPCT_FLAG_STRONG:
+	if (gen_stack_protect_signal
+	    || cfun->calls_alloca
+	    || has_protected_decls
+	    || lookup_attribute ("stack_protect",
+				 DECL_ATTRIBUTES (current_function_decl)))
+	  create_stack_guard ();
+	break;
 
-    case SPCT_FLAG_EXPLICIT:
-      if (lookup_attribute ("stack_protect",
-			    DECL_ATTRIBUTES (current_function_decl)))
-	create_stack_guard ();
-      break;
+      case SPCT_FLAG_DEFAULT:
+	if (cfun->calls_alloca
+	    || has_protected_decls
+	    || lookup_attribute ("stack_protect",
+				 DECL_ATTRIBUTES (current_function_decl)))
+	  create_stack_guard ();
+	break;
 
-    default:
-      break;
-    }
+      case SPCT_FLAG_EXPLICIT:
+	if (lookup_attribute ("stack_protect",
+			      DECL_ATTRIBUTES (current_function_decl)))
+	  create_stack_guard ();
+	break;
+
+      default:
+	break;
+      }
 
   /* Assign rtl to each variable based on these partitions.  */
   if (stack_vars_num > 0)
@@ -2213,11 +2217,11 @@ expand_used_vars (void)
 	  expand_stack_vars (stack_protect_decl_phase_1, &data);
 
 	  /* Phase 2 contains other kinds of arrays.  */
-	  if (flag_stack_protect == SPCT_FLAG_ALL
-	      || flag_stack_protect == SPCT_FLAG_STRONG
-	      || (flag_stack_protect == SPCT_FLAG_EXPLICIT
-		  && lookup_attribute ("stack_protect",
-				       DECL_ATTRIBUTES (current_function_decl))))
+	  if (!lookup_attribute ("no_stack_protect", attribs)
+	      && (flag_stack_protect == SPCT_FLAG_ALL
+		  || flag_stack_protect == SPCT_FLAG_STRONG
+		  || (flag_stack_protect == SPCT_FLAG_EXPLICIT
+		      && lookup_attribute ("stack_protect", attribs))))
 	    expand_stack_vars (stack_protect_decl_phase_2, &data);
 	}
 
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index a2ebef8cf8c..446f6e7faf4 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3671,6 +3671,10 @@ This attribute adds stack protection code to the function if
 flags @option{-fstack-protector}, @option{-fstack-protector-strong}
 or @option{-fstack-protector-explicit} are set.
 
+@item no_stack_protect
+@cindex @code{no_stack_protect} function attribute
+This attribute prevents stack protection code for the function.
+
 @item target (@var{string}, @dots{})
 @cindex @code{target} function attribute
 Multiple target back ends implement the @code{target} attribute
diff --git a/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C b/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C
new file mode 100644
index 00000000000..b9a0439c23c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C
@@ -0,0 +1,7 @@
+/* PR c/94722 */
+/* { dg-do compile } */
+
+int __attribute__((no_stack_protect, stack_protect)) c() /* { dg-warning "ignoring attribute 'stack_protect' because it conflicts with attribute 'no_stack_protect'" } */
+{
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/no-stack-protect-attr-3.C b/gcc/testsuite/g++.dg/no-stack-protect-attr-3.C
new file mode 100644
index 00000000000..a6b702ba5ea
--- /dev/null
+++ b/gcc/testsuite/g++.dg/no-stack-protect-attr-3.C
@@ -0,0 +1,23 @@
+/* PR c/94722 */
+/* Test that stack protection is disabled via no_stack_protect attribute. */
+
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fstack-protector-explicit" } */
+
+/* { dg-do compile } */
+
+int __attribute__((no_stack_protect)) foo()
+{
+  int a;
+  char b[34];
+  return 0;
+}
+
+int __attribute__((stack_protect)) bar()
+{
+  int a;
+  char b[34];
+  return 0;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 1 } } */
diff --git a/gcc/testsuite/g++.dg/no-stack-protect-attr.C b/gcc/testsuite/g++.dg/no-stack-protect-attr.C
new file mode 100644
index 00000000000..8332e5b276d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/no-stack-protect-attr.C
@@ -0,0 +1,16 @@
+/* PR c/94722 */
+/* Test that stack protection is disabled via no_stack_protect attribute. */
+
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fstack-protector-all" } */
+
+/* { dg-do compile } */
+
+int __attribute__((no_stack_protect)) c()
+{
+  int a;
+  char b[34];
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not "stack_chk_fail" } } */
-- 
2.26.2

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

PING^1

On 5/25/20 3:10 PM, Martin Liška wrote:
> On 5/21/20 4:53 PM, Martin Sebor wrote:
>> On 5/21/20 5:28 AM, Martin Liška wrote:
>>> On 5/18/20 10:37 PM, Martin Sebor wrote:
>>>> I know there are some somewhat complex cases the attribute exclusion
>>>> mechanism isn't general enough to handle but this seems simple enough
>>>> that it should work.  Unless I'm missing something that makes it not
>>>> feasible I would suggest to use it.
>>>
>>> Hi Martin.
>>>
>>> Do we have a better place where we check for attribute collision?
>>
>> If by collision you mean the same thing as the mutual exclusion I was
>> talking about then that's done by creating an attribute_spec::exclusions
>> array like for instance attr_cold_hot_exclusions in c-attribs.c and
>> pointing to it from the attribute_spec entries for each of
>> the mutually exclusive attributes in the attribute table.  Everything
>> else is handled automatically by decl_attributes.
>>
>> Martin
> 
> Thanks, I'm sending updated version of the patch that utilizes the conflict
> detection.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Sebor]

On 6/10/20 2:12 AM, Martin Liška wrote:
> PING^1

The exclusion changes are just what I was suggesting, thanks.

Martin

> 
> On 5/25/20 3:10 PM, Martin Liška wrote:
>> On 5/21/20 4:53 PM, Martin Sebor wrote:
>>> On 5/21/20 5:28 AM, Martin Liška wrote:
>>>> On 5/18/20 10:37 PM, Martin Sebor wrote:
>>>>> I know there are some somewhat complex cases the attribute exclusion
>>>>> mechanism isn't general enough to handle but this seems simple enough
>>>>> that it should work.  Unless I'm missing something that makes it not
>>>>> feasible I would suggest to use it.
>>>>
>>>> Hi Martin.
>>>>
>>>> Do we have a better place where we check for attribute collision?
>>>
>>> If by collision you mean the same thing as the mutual exclusion I was
>>> talking about then that's done by creating an attribute_spec::exclusions
>>> array like for instance attr_cold_hot_exclusions in c-attribs.c and
>>> pointing to it from the attribute_spec entries for each of
>>> the mutually exclusive attributes in the attribute table.  Everything
>>> else is handled automatically by decl_attributes.
>>>
>>> Martin
>>
>> Thanks, I'm sending updated version of the patch that utilizes the 
>> conflict
>> detection.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
>> Thanks,
>> Martin
> 

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

PING^2

On 6/10/20 10:12 AM, Martin Liška wrote:
> PING^1
> 
> On 5/25/20 3:10 PM, Martin Liška wrote:
>> On 5/21/20 4:53 PM, Martin Sebor wrote:
>>> On 5/21/20 5:28 AM, Martin Liška wrote:
>>>> On 5/18/20 10:37 PM, Martin Sebor wrote:
>>>>> I know there are some somewhat complex cases the attribute exclusion
>>>>> mechanism isn't general enough to handle but this seems simple enough
>>>>> that it should work.  Unless I'm missing something that makes it not
>>>>> feasible I would suggest to use it.
>>>>
>>>> Hi Martin.
>>>>
>>>> Do we have a better place where we check for attribute collision?
>>>
>>> If by collision you mean the same thing as the mutual exclusion I was
>>> talking about then that's done by creating an attribute_spec::exclusions
>>> array like for instance attr_cold_hot_exclusions in c-attribs.c and
>>> pointing to it from the attribute_spec entries for each of
>>> the mutually exclusive attributes in the attribute table.  Everything
>>> else is handled automatically by decl_attributes.
>>>
>>> Martin
>>
>> Thanks, I'm sending updated version of the patch that utilizes the conflict
>> detection.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
>> Thanks,
>> Martin
> 

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

PING^3

On 6/24/20 11:09 AM, Martin Liška wrote:
> PING^2
> 
> On 6/10/20 10:12 AM, Martin Liška wrote:
>> PING^1
>>
>> On 5/25/20 3:10 PM, Martin Liška wrote:
>>> On 5/21/20 4:53 PM, Martin Sebor wrote:
>>>> On 5/21/20 5:28 AM, Martin Liška wrote:
>>>>> On 5/18/20 10:37 PM, Martin Sebor wrote:
>>>>>> I know there are some somewhat complex cases the attribute exclusion
>>>>>> mechanism isn't general enough to handle but this seems simple enough
>>>>>> that it should work.  Unless I'm missing something that makes it not
>>>>>> feasible I would suggest to use it.
>>>>>
>>>>> Hi Martin.
>>>>>
>>>>> Do we have a better place where we check for attribute collision?
>>>>
>>>> If by collision you mean the same thing as the mutual exclusion I was
>>>> talking about then that's done by creating an attribute_spec::exclusions
>>>> array like for instance attr_cold_hot_exclusions in c-attribs.c and
>>>> pointing to it from the attribute_spec entries for each of
>>>> the mutually exclusive attributes in the attribute table.  Everything
>>>> else is handled automatically by decl_attributes.
>>>>
>>>> Martin
>>>
>>> Thanks, I'm sending updated version of the patch that utilizes the conflict
>>> detection.
>>>
>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>
>>> Ready to be installed?
>>> Thanks,
>>> Martin
>>
> 

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

PING^4

On 7/23/20 1:10 PM, Martin Liška wrote:
> PING^3
> 
> On 6/24/20 11:09 AM, Martin Liška wrote:
>> PING^2
>>
>> On 6/10/20 10:12 AM, Martin Liška wrote:
>>> PING^1
>>>
>>> On 5/25/20 3:10 PM, Martin Liška wrote:
>>>> On 5/21/20 4:53 PM, Martin Sebor wrote:
>>>>> On 5/21/20 5:28 AM, Martin Liška wrote:
>>>>>> On 5/18/20 10:37 PM, Martin Sebor wrote:
>>>>>>> I know there are some somewhat complex cases the attribute exclusion
>>>>>>> mechanism isn't general enough to handle but this seems simple enough
>>>>>>> that it should work.  Unless I'm missing something that makes it not
>>>>>>> feasible I would suggest to use it.
>>>>>>
>>>>>> Hi Martin.
>>>>>>
>>>>>> Do we have a better place where we check for attribute collision?
>>>>>
>>>>> If by collision you mean the same thing as the mutual exclusion I was
>>>>> talking about then that's done by creating an attribute_spec::exclusions
>>>>> array like for instance attr_cold_hot_exclusions in c-attribs.c and
>>>>> pointing to it from the attribute_spec entries for each of
>>>>> the mutually exclusive attributes in the attribute table.  Everything
>>>>> else is handled automatically by decl_attributes.
>>>>>
>>>>> Martin
>>>>
>>>> Thanks, I'm sending updated version of the patch that utilizes the conflict
>>>> detection.
>>>>
>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>
>>>> Ready to be installed?
>>>> Thanks,
>>>> Martin
>>>
>>
> 

Re: [PATCH] Implement no_stack_protect attribute.

[Nick Desaulniers]

This would solve a common pattern in the kernel where folks are using
`extern inline` with `gnu_inline` semantics or worse (empty `asm("");`
statements) in certain places where it would be much more preferable
to have this attribute.  Thank you very much Martin for writing it.

> is direct equivalent of Clang's no_stack_protector.
> Unlike Clang, I chose to name it no_stack_protect because we already
> have stack_protect attribute (used with -fstack-protector-explicit).

That's pretty easy for us to work around the differences in the
kernel, but one final plea for the users; it would simplify users'
codebases not to have to shim this for differences between compilers.
If I had a dollar for every time I had to implement something in LLVM
where a different identifier or flag would be more consistent with
another part of the codebase...I'm sure there's many examples of this
on LLVM's side too, but I would prefer to stop the proliferation of
subtle differences like this that harm toolchain portability when
possible and when we can proactively address.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

On 8/25/20 4:46 PM, Nick Desaulniers wrote:
> This would solve a common pattern in the kernel where folks are using
> `extern inline` with `gnu_inline` semantics or worse (empty `asm("");`
> statements) in certain places where it would be much more preferable
> to have this attribute.  Thank you very much Martin for writing it.
> 
>> is direct equivalent of Clang's no_stack_protector.
>> Unlike Clang, I chose to name it no_stack_protect because we already
>> have stack_protect attribute (used with -fstack-protector-explicit).
> 
> That's pretty easy for us to work around the differences in the
> kernel, but one final plea for the users; it would simplify users'
> codebases not to have to shim this for differences between compilers.
> If I had a dollar for every time I had to implement something in LLVM
> where a different identifier or flag would be more consistent with
> another part of the codebase...I'm sure there's many examples of this
> on LLVM's side too, but I would prefer to stop the proliferation of
> subtle differences like this that harm toolchain portability when
> possible and when we can proactively address.
> 

All right, I'm open to unify the flag name to what LLVM has ;)

Martin

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

PING^5

On 8/17/20 2:35 PM, Martin Liška wrote:
> PING^4
> 
> On 7/23/20 1:10 PM, Martin Liška wrote:
>> PING^3
>>
>> On 6/24/20 11:09 AM, Martin Liška wrote:
>>> PING^2
>>>
>>> On 6/10/20 10:12 AM, Martin Liška wrote:
>>>> PING^1
>>>>
>>>> On 5/25/20 3:10 PM, Martin Liška wrote:
>>>>> On 5/21/20 4:53 PM, Martin Sebor wrote:
>>>>>> On 5/21/20 5:28 AM, Martin Liška wrote:
>>>>>>> On 5/18/20 10:37 PM, Martin Sebor wrote:
>>>>>>>> I know there are some somewhat complex cases the attribute exclusion
>>>>>>>> mechanism isn't general enough to handle but this seems simple enough
>>>>>>>> that it should work.  Unless I'm missing something that makes it not
>>>>>>>> feasible I would suggest to use it.
>>>>>>>
>>>>>>> Hi Martin.
>>>>>>>
>>>>>>> Do we have a better place where we check for attribute collision?
>>>>>>
>>>>>> If by collision you mean the same thing as the mutual exclusion I was
>>>>>> talking about then that's done by creating an attribute_spec::exclusions
>>>>>> array like for instance attr_cold_hot_exclusions in c-attribs.c and
>>>>>> pointing to it from the attribute_spec entries for each of
>>>>>> the mutually exclusive attributes in the attribute table.  Everything
>>>>>> else is handled automatically by decl_attributes.
>>>>>>
>>>>>> Martin
>>>>>
>>>>> Thanks, I'm sending updated version of the patch that utilizes the conflict
>>>>> detection.
>>>>>
>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>
>>>>> Ready to be installed?
>>>>> Thanks,
>>>>> Martin
>>>>
>>>
>>
> 

Re: [PATCH] Implement no_stack_protect attribute.

[Richard Biener]

On Tue, Oct 20, 2020 at 1:24 PM Martin Liška <mliska@suse.cz> wrote:
>
> PING^5

So can we use the same identifier as clang here as Nick
requests?  Thus, OK with re-naming everything alongside
no_stack_protector.  It isn't really the opposite of the
stack_protect attribute since that only protects when
-fstack-protector-explicit is enabled.

Thanks,
Richard.

> On 8/17/20 2:35 PM, Martin Liška wrote:
> > PING^4
> >
> > On 7/23/20 1:10 PM, Martin Liška wrote:
> >> PING^3
> >>
> >> On 6/24/20 11:09 AM, Martin Liška wrote:
> >>> PING^2
> >>>
> >>> On 6/10/20 10:12 AM, Martin Liška wrote:
> >>>> PING^1
> >>>>
> >>>> On 5/25/20 3:10 PM, Martin Liška wrote:
> >>>>> On 5/21/20 4:53 PM, Martin Sebor wrote:
> >>>>>> On 5/21/20 5:28 AM, Martin Liška wrote:
> >>>>>>> On 5/18/20 10:37 PM, Martin Sebor wrote:
> >>>>>>>> I know there are some somewhat complex cases the attribute exclusion
> >>>>>>>> mechanism isn't general enough to handle but this seems simple enough
> >>>>>>>> that it should work.  Unless I'm missing something that makes it not
> >>>>>>>> feasible I would suggest to use it.
> >>>>>>>
> >>>>>>> Hi Martin.
> >>>>>>>
> >>>>>>> Do we have a better place where we check for attribute collision?
> >>>>>>
> >>>>>> If by collision you mean the same thing as the mutual exclusion I was
> >>>>>> talking about then that's done by creating an attribute_spec::exclusions
> >>>>>> array like for instance attr_cold_hot_exclusions in c-attribs.c and
> >>>>>> pointing to it from the attribute_spec entries for each of
> >>>>>> the mutually exclusive attributes in the attribute table.  Everything
> >>>>>> else is handled automatically by decl_attributes.
> >>>>>>
> >>>>>> Martin
> >>>>>
> >>>>> Thanks, I'm sending updated version of the patch that utilizes the conflict
> >>>>> detection.
> >>>>>
> >>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>>>>
> >>>>> Ready to be installed?
> >>>>> Thanks,
> >>>>> Martin
> >>>>
> >>>
> >>
> >
>

Re: [PATCH] Implement no_stack_protect attribute.

[Nick Desaulniers]

On Tue, Oct 20, 2020 at 5:19 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Tue, Oct 20, 2020 at 1:24 PM Martin Liška <mliska@suse.cz> wrote:
> >
> > PING^5
>
> So can we use the same identifier as clang here as Nick
> requests?  Thus, OK with re-naming everything alongside
> no_stack_protector.  It isn't really the opposite of the
> stack_protect attribute since that only protects when
> -fstack-protector-explicit is enabled.

I'll be happy to help test and review with an updated/rebased patch.

Tangentially related question:
We're running into a bug related to LTO for the kernel when code
compiled with -fno-stack-protector is called from and inlined into
code that is compiled with -fstack-protector.  Specifically, stack
canaries get checked before they're restored post suspend/resume which
leads to spooky bugs.

Once we have more fine grain function level attribute to explicitly
disable stack protectors on a per function basis, I'm considering
making this function attribute a barrier to inlining in LLVM so that
callers with stack protectors don't inline callees that explicitly
should not have a stack protector and vice versa (more concretely,
when they don't match).  I think this would maximize which functions
are still covered by stack protectors, and be the most straightforward
to implement.

The next question then is what happens when the callee is marked
__attribute__((always_inline))?  My answer for LLVM currently is
"still disallow inline substitution" which is more surprising than I'd
like, but we already have precedent for "always inline" not meaning
"always."  Warning from the frontend when mixing no_stack_protector
and always_inline is possible if the callers are visible and don't
match, but I don't think that works for cross translation unit calls.

I guess I was curious if others have ideas for solutions to this
particular problem?  Otherwise I plan to implement the above logic in
LLVM.  We'd eventually need matching logic in GCC to support LTO
kernels not having the same bug.

https://reviews.llvm.org/D87956
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] Implement no_stack_protect attribute.

[Jakub Jelinek]

On Wed, Oct 21, 2020 at 02:04:15PM -0700, Nick Desaulniers via Gcc-patches wrote:
> Tangentially related question:
> We're running into a bug related to LTO for the kernel when code
> compiled with -fno-stack-protector is called from and inlined into
> code that is compiled with -fstack-protector.  Specifically, stack
> canaries get checked before they're restored post suspend/resume which
> leads to spooky bugs.
> 
> Once we have more fine grain function level attribute to explicitly
> disable stack protectors on a per function basis, I'm considering
> making this function attribute a barrier to inlining in LLVM so that
> callers with stack protectors don't inline callees that explicitly
> should not have a stack protector and vice versa (more concretely,
> when they don't match).  I think this would maximize which functions
> are still covered by stack protectors, and be the most straightforward
> to implement.

That doesn't make sense to me.
Stack protector doesn't affect in any way inlined code, the stack protection
is always solely in the prologue and epilogue of out of line functions.
So, if the (non-inlined) caller is -fstack-protector and inlined callee
is -fno-stack-protector, there should be ssp store in the prologue of the
resulting function and test in the epilogue.  The effect will be exactly
like that if the function wouldn't be inlined.
Similarly, if the non-inlined caller is -fno-stack-protector and inlined
callee is -fstack-protector, there will be no stack protection.  This isn't
exactly the effect one would get without the inlining (as in that case
the callee would check it), but matches the general behavior that we allow
inlining functions with -fstack-protector* at all (and only check it in the
prologue/epilogue, not somewhere in the middle).

	Jakub

Re: [PATCH] Implement no_stack_protect attribute.

[Nick Desaulniers]

Thanks for the quick feedback!

On Wed, Oct 21, 2020 at 2:13 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Oct 21, 2020 at 02:04:15PM -0700, Nick Desaulniers via Gcc-patches wrote:
> > Tangentially related question:
> > We're running into a bug related to LTO for the kernel when code
> > compiled with -fno-stack-protector is called from and inlined into
> > code that is compiled with -fstack-protector.  Specifically, stack
> > canaries get checked before they're restored post suspend/resume which
> > leads to spooky bugs.
> >
> > Once we have more fine grain function level attribute to explicitly
> > disable stack protectors on a per function basis, I'm considering
> > making this function attribute a barrier to inlining in LLVM so that
> > callers with stack protectors don't inline callees that explicitly
> > should not have a stack protector and vice versa (more concretely,
> > when they don't match).  I think this would maximize which functions
> > are still covered by stack protectors, and be the most straightforward
> > to implement.
>
> That doesn't make sense to me.
> Stack protector doesn't affect in any way inlined code, the stack protection
> is always solely in the prologue and epilogue of out of line functions.
> So, if the (non-inlined) caller is -fstack-protector and inlined callee
> is -fno-stack-protector, there should be ssp store in the prologue of the
> resulting function and test in the epilogue.

That is the case today, and I'm arguing that leads to bugs in the
Linux kernel when built with LTO.

> The effect will be exactly
> like that if the function wouldn't be inlined.

I don't follow.  If the -fno-stack-protector callee was not inlined,
the caller would have a stack protector, while the callee would not.
I think today there's not a strong enough distinction between the
level of stack protection being specified vs explicit
annotations/flags that stack protectors MUST NOT be inserted.

> Similarly, if the non-inlined caller is -fno-stack-protector and inlined
> callee is -fstack-protector, there will be no stack protection.  This isn't

And I'd argue that now we may have stripped off stack protection in
the pursuit of inlining.  Consider for example the case where that
stack protected callee contained a large alloca; post inlining into a
-fno-stack-protected caller suddenly now it doesn't.  Oops?

Wouldn't it be safer to just prevent inlining, then the callee retains
the stack protector, regardless of caller stack protection?

> exactly the effect one would get without the inlining (as in that case
> the callee would check it), but matches the general behavior that we allow
> inlining functions with -fstack-protector* at all (and only check it in the
> prologue/epilogue, not somewhere in the middle).
>
>         Jakub
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] Implement no_stack_protect attribute.

[Nick Desaulniers]

+ correct kernel mailing list this time.

On Wed, Oct 21, 2020 at 2:33 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Thanks for the quick feedback!
>
> On Wed, Oct 21, 2020 at 2:13 PM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Wed, Oct 21, 2020 at 02:04:15PM -0700, Nick Desaulniers via Gcc-patches wrote:
> > > Tangentially related question:
> > > We're running into a bug related to LTO for the kernel when code
> > > compiled with -fno-stack-protector is called from and inlined into
> > > code that is compiled with -fstack-protector.  Specifically, stack
> > > canaries get checked before they're restored post suspend/resume which
> > > leads to spooky bugs.
> > >
> > > Once we have more fine grain function level attribute to explicitly
> > > disable stack protectors on a per function basis, I'm considering
> > > making this function attribute a barrier to inlining in LLVM so that
> > > callers with stack protectors don't inline callees that explicitly
> > > should not have a stack protector and vice versa (more concretely,
> > > when they don't match).  I think this would maximize which functions
> > > are still covered by stack protectors, and be the most straightforward
> > > to implement.
> >
> > That doesn't make sense to me.
> > Stack protector doesn't affect in any way inlined code, the stack protection
> > is always solely in the prologue and epilogue of out of line functions.
> > So, if the (non-inlined) caller is -fstack-protector and inlined callee
> > is -fno-stack-protector, there should be ssp store in the prologue of the
> > resulting function and test in the epilogue.
>
> That is the case today, and I'm arguing that leads to bugs in the
> Linux kernel when built with LTO.
>
> > The effect will be exactly
> > like that if the function wouldn't be inlined.
>
> I don't follow.  If the -fno-stack-protector callee was not inlined,
> the caller would have a stack protector, while the callee would not.
> I think today there's not a strong enough distinction between the
> level of stack protection being specified vs explicit
> annotations/flags that stack protectors MUST NOT be inserted.
>
> > Similarly, if the non-inlined caller is -fno-stack-protector and inlined
> > callee is -fstack-protector, there will be no stack protection.  This isn't
>
> And I'd argue that now we may have stripped off stack protection in
> the pursuit of inlining.  Consider for example the case where that
> stack protected callee contained a large alloca; post inlining into a
> -fno-stack-protected caller suddenly now it doesn't.  Oops?
>
> Wouldn't it be safer to just prevent inlining, then the callee retains
> the stack protector, regardless of caller stack protection?
>
> > exactly the effect one would get without the inlining (as in that case
> > the callee would check it), but matches the general behavior that we allow
> > inlining functions with -fstack-protector* at all (and only check it in the
> > prologue/epilogue, not somewhere in the middle).
> >
> >         Jakub
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] Implement no_stack_protect attribute.

[Martin Liška]

On 10/20/20 2:19 PM, Richard Biener wrote:
> So can we use the same identifier as clang here as Nick
> requests?  Thus, OK with re-naming everything alongside
> no_stack_protector.  It isn't really the opposite of the
> stack_protect attribute since that only protects when
> -fstack-protector-explicit is enabled.

Done that and installed the patch.

Thanks for review,
Martin