Re: [PATCH] Add patch_area_size and patch_area_entry to crtl

["H.J. Lu" <hjl.tools@gmail.com>]

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

On Wed, Feb 5, 2020 at 2:51 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Wed, Feb 5, 2020 at 2:37 PM Marek Polacek <polacek@redhat.com> wrote:
> >
> > On Wed, Feb 05, 2020 at 02:24:48PM -0800, H.J. Lu wrote:
> > > On Wed, Feb 5, 2020 at 12:20 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Wed, Feb 5, 2020 at 9:00 AM Richard Sandiford
> > > > <richard.sandiford@arm.com> wrote:
> > > > >
> > > > > "H.J. Lu" <hjl.tools@gmail.com> writes:
> > > > > > Currently patchable area is at the wrong place.
> > > > >
> > > > > Agreed :-)
> > > > >
> > > > > > It is placed immediately
> > > > > > after function label and before .cfi_startproc.  A backend should be able
> > > > > > to add a pseudo patchable area instruction durectly into RTL.  This patch
> > > > > > adds patch_area_size and patch_area_entry to cfun so that the patchable
> > > > > > area info is available in RTL passes.
> > > > >
> > > > > It might be better to add it to crtl, since it should only be needed
> > > > > during rtl generation.
> > > > >
> > > > > > It also limits patch_area_size and patch_area_entry to 65535, which is
> > > > > > a reasonable maximum size for patchable area.
> > > > > >
> > > > > > gcc/
> > > > > >
> > > > > >       PR target/93492
> > > > > >       * function.c (expand_function_start): Set cfun->patch_area_size
> > > > > >       and cfun->patch_area_entry.
> > > > > >       * function.h (function): Add patch_area_size and patch_area_entry.
> > > > > >       * opts.c (common_handle_option): Limit
> > > > > >       function_entry_patch_area_size and function_entry_patch_area_start
> > > > > >       to USHRT_MAX.  Fix a typo in error message.
> > > > > >       * varasm.c (assemble_start_function): Use cfun->patch_area_size
> > > > > >       and cfun->patch_area_entry.
> > > > > >       * doc/invoke.texi: Document the maximum value for
> > > > > >       -fpatchable-function-entry.
> > > > > >
> > > > > > gcc/testsuite/
> > > > > >
> > > > > >       PR target/93492
> > > > > >       * c-c++-common/patchable_function_entry-error-1.c: New test.
> > > > > >       * c-c++-common/patchable_function_entry-error-2.c: Likewise.
> > > > > >       * c-c++-common/patchable_function_entry-error-3.c: Likewise.
> > > > > > ---
> > > > > >  gcc/doc/invoke.texi                           |  1 +
> > > > > >  gcc/function.c                                | 35 +++++++++++++++++++
> > > > > >  gcc/function.h                                |  6 ++++
> > > > > >  gcc/opts.c                                    |  4 ++-
> > > > > >  .../patchable_function_entry-error-1.c        |  9 +++++
> > > > > >  .../patchable_function_entry-error-2.c        |  9 +++++
> > > > > >  .../patchable_function_entry-error-3.c        | 20 +++++++++++
> > > > > >  gcc/varasm.c                                  | 30 ++--------------
> > > > > >  8 files changed, 85 insertions(+), 29 deletions(-)
> > > > > >  create mode 100644 gcc/testsuite/c-c++-common/patchable_function_entry-error-1.c
> > > > > >  create mode 100644 gcc/testsuite/c-c++-common/patchable_function_entry-error-2.c
> > > > > >  create mode 100644 gcc/testsuite/c-c++-common/patchable_function_entry-error-3.c
> > > > > >
> > > > > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > > > > > index 35b341e759f..dd4835199b0 100644
> > > > > > --- a/gcc/doc/invoke.texi
> > > > > > +++ b/gcc/doc/invoke.texi
> > > > > > @@ -13966,6 +13966,7 @@ If @code{N=0}, no pad location is recorded.
> > > > > >  The NOP instructions are inserted at---and maybe before, depending on
> > > > > >  @var{M}---the function entry address, even before the prologue.
> > > > > >
> > > > > > +The maximum value of @var{N} and @var{M} is 65535.
> > > > > >  @end table
> > > > > >
> > > > > >
> > > > > > diff --git a/gcc/function.c b/gcc/function.c
> > > > > > index d8008f60422..badbf538eec 100644
> > > > > > --- a/gcc/function.c
> > > > > > +++ b/gcc/function.c
> > > > > > @@ -5202,6 +5202,41 @@ expand_function_start (tree subr)
> > > > > >    /* If we are doing generic stack checking, the probe should go here.  */
> > > > > >    if (flag_stack_check == GENERIC_STACK_CHECK)
> > > > > >      stack_check_probe_note = emit_note (NOTE_INSN_DELETED);
> > > > > > +
> > > > > > +  unsigned HOST_WIDE_INT patch_area_size = function_entry_patch_area_size;
> > > > > > +  unsigned HOST_WIDE_INT patch_area_entry = function_entry_patch_area_start;
> > > > > > +
> > > > > > +  tree patchable_function_entry_attr
> > > > > > +    = lookup_attribute ("patchable_function_entry",
> > > > > > +                     DECL_ATTRIBUTES (cfun->decl));
> > > > > > +  if (patchable_function_entry_attr)
> > > > > > +    {
> > > > > > +      tree pp_val = TREE_VALUE (patchable_function_entry_attr);
> > > > > > +      tree patchable_function_entry_value1 = TREE_VALUE (pp_val);
> > > > > > +
> > > > > > +      patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
> > > > > > +      patch_area_entry = 0;
> > > > > > +      if (TREE_CHAIN (pp_val) != NULL_TREE)
> > > > > > +     {
> > > > > > +       tree patchable_function_entry_value2
> > > > > > +         = TREE_VALUE (TREE_CHAIN (pp_val));
> > > > > > +       patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
> > > > > > +     }
> > > > > > +      if (patch_area_size > USHRT_MAX || patch_area_size > USHRT_MAX)
> > > > > > +     error ("invalid values for %<patchable_function_entry%> attribute");
> > > > >
> > > > > This should probably go in handle_patchable_function_entry_attribute
> > > > > instead.  It doesn't look like we have a clear policy about whether
> > > > > errors or warnings are right for unrecognised arguments to known
> > > > > attribute names, but handle_patchable_function_entry_attribute reports
> > > > > an OPT_Wattributes warning for arguments that aren't integers.  Doing the
> > > > > same for out-of-range integers would be more consistent and means that
> > > > > we wouldn't break existing code if we relaxed/changed the rules in future.
> > > >
> > > > Like this?  OK for master if there is no regression?
> > > >
> > >
> > > There is a small problem.  Warnings from C and C++ frond-ends are different:
> > >
> > > [hjl@gnu-skx-1 gcc]$ cat x.c
> > > void
> > >  __attribute__((patchable_function_entry(65536)))
> > > foo1 (void)
> > > { /* { dg-warning "'patchable_function_entry' attribute argument
> > > '65536' is out of range" } */
> > > }
> > > [hjl@gnu-skx-1 gcc]$ ./xgcc -B./ -S x.c
> > > x.c:4:1: warning: ‘patchable_function_entry’ attribute argument
> > > ‘65536’ is out of range (> 65535) [-Wattributes]
> > >     4 | { /* { dg-warning "'patchable_function_entry' attribute
> > > argument '65536' is out of range" } */
> > >       | ^
> > > [hjl@gnu-skx-1 gcc]$ ./xg++ -B./ -S x.c
> > > x.c:3:11: warning: ‘patchable_function_entry’ attribute argument
> > > ‘65536’ is out of range (> 65535) [-Wattributes]
> > >     3 | foo1 (void)
> > >       |           ^
> > > [hjl@gnu-skx-1 gcc]$
> > >
> > > C warns at line 4 and C++ warns at line 3.   Do I need separate tests
> > > for C and C++?
> >
> > I think better would be
> >
> > /* { dg-error "foo" "" { target c } } */
> > /* { dg-error "bar" "" { target c++ } .-1 } */
> >
> > Marek
> >
>
> It worked.

Here is the patch with updated tests.   There are no regressions on
Linux/x86-64.
OK for master?

Thanks.


-- 
H.J.

[-- Attachment #2: 0001-Add-patch_area_size-and-patch_area_entry-to-crtl.patch --]
[-- Type: text/x-patch, Size: 10156 bytes --]

From 8320bbb83e53f9e135fe1eca50840baacf157881 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 5 Feb 2020 04:01:59 -0800
Subject: [PATCH] Add patch_area_size and patch_area_entry to crtl

Currently patchable area is at the wrong place.  It is placed immediately
after function label and before .cfi_startproc.  A backend should be able
to add a pseudo patchable area instruction durectly into RTL.  This patch
adds patch_area_size and patch_area_entry to crtl so that the patchable
area info is available in RTL passes.

It also limits patch_area_size and patch_area_entry to 65535, which is
a reasonable maximum size for patchable area.

gcc/c-family/

	PR target/93492
	* c-attribs.c (handle_patchable_function_entry_attribute): Limit
	value to USHRT_MAX (65535).

gcc/

	PR target/93492
	* cfgexpand.c (pass_expand::execute): Set crtl->patch_area_size
	and crtl->patch_area_entry.
	* emit-rtl.h (rtl_data): Add patch_area_size and patch_area_entry.
	* opts.c (common_handle_option): Limit
	function_entry_patch_area_size and function_entry_patch_area_start
	to USHRT_MAX.  Fix a typo in error message.
	* varasm.c (assemble_start_function): Use crtl->patch_area_size
	and crtl->patch_area_entry.
	* doc/invoke.texi: Document the maximum value for
	-fpatchable-function-entry.

gcc/testsuite/

	PR target/93492
	* c-c++-common/patchable_function_entry-error-1.c: New test.
	* c-c++-common/patchable_function_entry-error-2.c: Likewise.
	* c-c++-common/patchable_function_entry-error-3.c: Likewise.
---
 gcc/c-family/c-attribs.c                      |  9 +++++
 gcc/cfgexpand.c                               | 33 +++++++++++++++++++
 gcc/doc/invoke.texi                           |  1 +
 gcc/emit-rtl.h                                |  6 ++++
 gcc/opts.c                                    |  4 ++-
 .../patchable_function_entry-error-1.c        |  9 +++++
 .../patchable_function_entry-error-2.c        |  9 +++++
 .../patchable_function_entry-error-3.c        | 20 +++++++++++
 gcc/varasm.c                                  | 30 ++---------------
 9 files changed, 92 insertions(+), 29 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/patchable_function_entry-error-1.c
 create mode 100644 gcc/testsuite/c-c++-common/patchable_function_entry-error-2.c
 create mode 100644 gcc/testsuite/c-c++-common/patchable_function_entry-error-3.c

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 7ec6fc848ac..15dbda1eff7 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -4539,6 +4539,15 @@ handle_patchable_function_entry_attribute (tree *, tree name, tree args,
 	  *no_add_attrs = true;
 	  return NULL_TREE;
 	}
+
+      if (tree_to_uhwi (val) > USHRT_MAX)
+	{
+	  warning (OPT_Wattributes,
+		   "%qE attribute argument %qE is out of range (> 65535)",
+		   name, val);
+	  *no_add_attrs = true;
+	  return NULL_TREE;
+	}
     }
   return NULL_TREE;
 }
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 9864e4344d2..f063f50c263 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -6656,6 +6656,39 @@ pass_expand::execute (function *fun)
   if (crtl->tail_call_emit)
     fixup_tail_calls ();
 
+  unsigned HOST_WIDE_INT patch_area_size = function_entry_patch_area_size;
+  unsigned HOST_WIDE_INT patch_area_entry = function_entry_patch_area_start;
+
+  tree patchable_function_entry_attr
+    = lookup_attribute ("patchable_function_entry",
+			DECL_ATTRIBUTES (cfun->decl));
+  if (patchable_function_entry_attr)
+    {
+      tree pp_val = TREE_VALUE (patchable_function_entry_attr);
+      tree patchable_function_entry_value1 = TREE_VALUE (pp_val);
+
+      patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
+      patch_area_entry = 0;
+      if (TREE_CHAIN (pp_val) != NULL_TREE)
+	{
+	  tree patchable_function_entry_value2
+	    = TREE_VALUE (TREE_CHAIN (pp_val));
+	  patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
+	}
+    }
+
+  if (patch_area_entry > patch_area_size)
+    {
+      if (patch_area_size > 0)
+	warning (OPT_Wattributes,
+		 "patchable function entry %wu exceeds size %wu",
+		 patch_area_entry, patch_area_size);
+      patch_area_entry = 0;
+    }
+
+  crtl->patch_area_size = patch_area_size;
+  crtl->patch_area_entry = patch_area_entry;
+
   /* BB subdivision may have created basic blocks that are are only reachable
      from unlikely bbs but not marked as such in the profile.  */
   if (optimize)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 35b341e759f..dd4835199b0 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -13966,6 +13966,7 @@ If @code{N=0}, no pad location is recorded.
 The NOP instructions are inserted at---and maybe before, depending on
 @var{M}---the function entry address, even before the prologue.
 
+The maximum value of @var{N} and @var{M} is 65535.
 @end table
 
 
diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h
index a878efe3cf7..3d6565c8a30 100644
--- a/gcc/emit-rtl.h
+++ b/gcc/emit-rtl.h
@@ -173,6 +173,12 @@ struct GTY(()) rtl_data {
         local stack.  */
   unsigned int stack_alignment_estimated;
 
+  /* How many NOP insns to place at each function entry by default.  */
+  unsigned short patch_area_size;
+
+  /* How far the real asm entry point is into this area.  */
+  unsigned short patch_area_entry;
+
   /* For reorg.  */
 
   /* Nonzero if function being compiled called builtin_return_addr or
diff --git a/gcc/opts.c b/gcc/opts.c
index 7affeb41a96..c6011f1f9b7 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -2598,10 +2598,12 @@ common_handle_option (struct gcc_options *opts,
 	    function_entry_patch_area_start = 0;
 	  }
 	if (function_entry_patch_area_size < 0
+	    || function_entry_patch_area_size > USHRT_MAX
 	    || function_entry_patch_area_start < 0
+	    || function_entry_patch_area_start > USHRT_MAX
 	    || function_entry_patch_area_size 
 		< function_entry_patch_area_start)
-	  error ("invalid arguments for %<-fpatchable_function_entry%>");
+	  error ("invalid arguments for %<-fpatchable-function-entry%>");
 	free (patch_area_arg);
       }
       break;
diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-error-1.c b/gcc/testsuite/c-c++-common/patchable_function_entry-error-1.c
new file mode 100644
index 00000000000..f60bf46cfe3
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/patchable_function_entry-error-1.c
@@ -0,0 +1,9 @@
+/* { dg-do compile { target { ! { nvptx*-*-* visium-*-* } } } } */
+/* { dg-options "-O2 -fpatchable-function-entry=65536,1" } */
+/* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */
+/* { dg-error "invalid arguments for '-fpatchable-function-entry'" "" { target *-*-* } 0 } */
+
+void
+foo (void)
+{
+}
diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-error-2.c b/gcc/testsuite/c-c++-common/patchable_function_entry-error-2.c
new file mode 100644
index 00000000000..90f88c78be7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/patchable_function_entry-error-2.c
@@ -0,0 +1,9 @@
+/* { dg-do compile { target { ! { nvptx*-*-* visium-*-* } } } } */
+/* { dg-options "-O2 -fpatchable-function-entry=1,65536" } */
+/* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */
+/* { dg-error "invalid arguments for '-fpatchable-function-entry'" "" { target *-*-* } 0 } */
+
+void
+foo (void)
+{
+}
diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-error-3.c b/gcc/testsuite/c-c++-common/patchable_function_entry-error-3.c
new file mode 100644
index 00000000000..b3daeabcbb6
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/patchable_function_entry-error-3.c
@@ -0,0 +1,20 @@
+/* { dg-do compile { target { ! { nvptx*-*-* visium-*-* } } } } */
+/* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */
+
+void
+ __attribute__((patchable_function_entry(65536)))
+foo1 (void) /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" "" { target c++ } } */
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" "" { target c } } */
+}
+
+void
+ __attribute__((patchable_function_entry(65536,1)))
+foo2 (void) /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" "" { target c++ } } */
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" "" { target c } } */
+}
+
+void
+ __attribute__((patchable_function_entry(65536,65536)))
+foo3 (void) /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" "" { target c++ } } */
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" "" { target c } } */
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index dc6da6c0b5b..9179fecdf85 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -1857,34 +1857,8 @@ assemble_start_function (tree decl, const char *fnname)
   if (DECL_PRESERVE_P (decl))
     targetm.asm_out.mark_decl_preserved (fnname);
 
-  unsigned HOST_WIDE_INT patch_area_size = function_entry_patch_area_size;
-  unsigned HOST_WIDE_INT patch_area_entry = function_entry_patch_area_start;
-
-  tree patchable_function_entry_attr
-    = lookup_attribute ("patchable_function_entry", DECL_ATTRIBUTES (decl));
-  if (patchable_function_entry_attr)
-    {
-      tree pp_val = TREE_VALUE (patchable_function_entry_attr);
-      tree patchable_function_entry_value1 = TREE_VALUE (pp_val);
-
-      patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
-      patch_area_entry = 0;
-      if (TREE_CHAIN (pp_val) != NULL_TREE)
-	{
-	  tree patchable_function_entry_value2
-	    = TREE_VALUE (TREE_CHAIN (pp_val));
-	  patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
-	}
-    }
-
-  if (patch_area_entry > patch_area_size)
-    {
-      if (patch_area_size > 0)
-	warning (OPT_Wattributes,
-		 "patchable function entry %wu exceeds size %wu",
-		 patch_area_entry, patch_area_size);
-      patch_area_entry = 0;
-    }
+  unsigned short patch_area_size = crtl->patch_area_size;
+  unsigned short patch_area_entry = crtl->patch_area_entry;
 
   /* Emit the patching area before the entry label, if any.  */
   if (patch_area_entry > 0)
-- 
2.24.1