[PATCH 3/3] x86: Simplify UNSPECV_PATCHABLE

public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed

* [PATCH 3/3] x86: Simplify UNSPECV_PATCHABLE_AREA generation
  2020-02-05 14:33 [PATCH 0/3] Update -fpatchable-function-entry implementation H.J. Lu
  2020-02-05 14:33 ` [PATCH 1/3] x86: Add UNSPECV_PATCHABLE_AREA H.J. Lu
@ 2020-02-05 14:33 ` H.J. Lu
  2020-02-05 14:33 ` [PATCH 2/3] Add patch_area_size and patch_area_entry to cfun H.J. Lu
  2 siblings, 0 replies; 12+ messages in thread
From: H.J. Lu @ 2020-02-05 14:33 UTC (permalink / raw)
  To: gcc-patches
  Cc: Uros Bizjak, Jeff Law, Richard Biener, Richard Earnshaw,
	Jakub Jelinek, Richard Sandiford, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou

Since patch_area_size and patch_area_entry have been added to cfun,
we can use them to directly insert the pseudo UNSPECV_PATCHABLE_AREA
instruction.

	PR target/93492
	* config/i386/i386-features.c
	(rest_of_insert_endbr_and_patchable_area): Change
	need_patchable_area argument to patchable_area_size.  Generate
	UNSPECV_PATCHABLE_AREA instruction with proper arguments.
	(pass_insert_endbr_and_patchable_area::gate): Set and check
	patchable_area_size instead of need_patchable_area.
	(pass_insert_endbr_and_patchable_area::execute): Pass
	patchable_area_size to rest_of_insert_endbr_and_patchable_area.
	(pass_insert_endbr_and_patchable_area): Replace
	need_patchable_area with patchable_area_size.
	* config/i386/i386.c (ix86_print_patchable_function_entry):
	Just return if function table has been emitted.
	(x86_function_profiler): Use cfun->patch_area_size and
	cfun->patch_area_entry.
	* config/i386/i386.h (machine_function): Remove patch_area_size
	and record_patch_area.
	* config/i386/i386.md (patchable_area): Set length attribute.
---
 gcc/config/i386/i386-features.c | 22 +++++-------
 gcc/config/i386/i386.c          | 60 ++++++---------------------------
 gcc/config/i386/i386.h          |  6 ----
 gcc/config/i386/i386.md         | 10 +++---
 4 files changed, 25 insertions(+), 73 deletions(-)

diff --git a/gcc/config/i386/i386-features.c b/gcc/config/i386/i386-features.c
index be46f036126..d358abe7064 100644
--- a/gcc/config/i386/i386-features.c
+++ b/gcc/config/i386/i386-features.c
@@ -1941,7 +1941,7 @@ make_pass_stv (gcc::context *ctxt)
 
 static void
 rest_of_insert_endbr_and_patchable_area (bool need_endbr,
-					 bool need_patchable_area)
+					 unsigned int patchable_area_size)
 {
   rtx endbr;
   rtx_insn *insn;
@@ -1980,7 +1980,7 @@ rest_of_insert_endbr_and_patchable_area (bool need_endbr,
 	}
     }
 
-  if (need_patchable_area)
+  if (patchable_area_size)
     {
       if (crtl->profile && flag_fentry)
 	{
@@ -1992,10 +1992,9 @@ rest_of_insert_endbr_and_patchable_area (bool need_endbr,
 	}
       else
 	{
-	  /* ix86_print_patchable_function_entry will provide actual
-	     size.  */
-	  rtx patchable_area = gen_patchable_area (GEN_INT (0),
-						   GEN_INT (0));
+	  rtx patchable_area
+	    = gen_patchable_area (GEN_INT (patchable_area_size),
+				  GEN_INT (cfun->patch_area_entry == 0));
 	  if (endbr_insn)
 	    emit_insn_after (patchable_area, endbr_insn);
 	  else
@@ -2123,25 +2122,22 @@ public:
   virtual bool gate (function *fun)
     {
       need_endbr = (flag_cf_protection & CF_BRANCH) != 0;
-      need_patchable_area
-	= (function_entry_patch_area_size
-	   || lookup_attribute ("patchable_function_entry",
-				DECL_ATTRIBUTES (fun->decl)));
-      return need_endbr || need_patchable_area;
+      patchable_area_size = fun->patch_area_size - fun->patch_area_entry;
+      return need_endbr || patchable_area_size;
     }
 
   virtual unsigned int execute (function *)
     {
       timevar_push (TV_MACH_DEP);
       rest_of_insert_endbr_and_patchable_area (need_endbr,
-					       need_patchable_area);
+					       patchable_area_size);
       timevar_pop (TV_MACH_DEP);
       return 0;
     }
 
 private:
   bool need_endbr;
-  bool need_patchable_area;
+  unsigned int patchable_area_size;
 }; // class pass_insert_endbr_and_patchable_area
 
 } // anon namespace
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 051a1fcbdc2..79a8823f61e 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -9130,53 +9130,11 @@ ix86_print_patchable_function_entry (FILE *file,
 {
   if (cfun->machine->function_label_emitted)
     {
-      /* The insert_endbr_and_patchable_area pass inserted a dummy
-	 UNSPECV_PATCHABLE_AREA with 0 patchable area size.  If the
-	 patchable area is placed after the function label, we replace
-	 0 patchable area size with the real one.  Otherwise, the
-	 dummy UNSPECV_PATCHABLE_AREA will be ignored.  */
-      if (cfun->machine->insn_queued_at_entrance)
-	{
-	  /* Record the patchable area.  Both ENDBR and patchable area
-	     will be inserted by x86_function_profiler later.  */
-	  cfun->machine->patch_area_size = patch_area_size;
-	  cfun->machine->record_patch_area = record_p;
-	  return;
-	}
-
-      /* We can have
-
-	 UNSPECV_NOP_ENDBR
-	 UNSPECV_PATCHABLE_AREA
-
-	 or just
-
-	 UNSPECV_PATCHABLE_AREA
-       */
-      rtx_insn *patchable_insn;
-      rtx_insn *insn = next_real_nondebug_insn (get_insns ());
-      if (insn
-	  && INSN_P (insn)
-	  && GET_CODE (PATTERN (insn)) == UNSPEC_VOLATILE
-	  && XINT (PATTERN (insn), 1) == UNSPECV_NOP_ENDBR)
-	patchable_insn = next_real_nondebug_insn (insn);
-      else
-	patchable_insn = insn;
-
-      if (patchable_insn && INSN_P (patchable_insn))
-	{
-	  /* Replace the dummy patchable area size with the real one.  */
-	  rtx pattern = PATTERN (patchable_insn);
-	  if (GET_CODE (pattern) == UNSPEC_VOLATILE
-	      && XINT (pattern, 1) == UNSPECV_PATCHABLE_AREA)
-	    {
-	      XVECEXP (pattern, 0, 0) = GEN_INT (patch_area_size);
-	      XVECEXP (pattern, 0, 1) = GEN_INT (record_p);
-	    }
-	  return;
-	}
-
-      gcc_unreachable ();
+      /* NB: When ix86_print_patchable_function_entry is called after
+	 function table has been emitted, we have inserted or queued
+	 a pseudo UNSPECV_PATCHABLE_AREA instruction at the proper
+	 place.  There is nothing to do here.  */
+      return;
     }
 
   default_print_patchable_function_entry (file, patch_area_size,
@@ -20232,9 +20190,11 @@ x86_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
     {
       if (cfun->machine->insn_queued_at_entrance == TYPE_ENDBR)
 	fprintf (file, "\t%s\n", TARGET_64BIT ? "endbr64" : "endbr32");
-      if (cfun->machine->patch_area_size)
-	ix86_output_patchable_area (cfun->machine->patch_area_size,
-				    cfun->machine->record_patch_area);
+      unsigned int patch_area_size
+	= cfun->patch_area_size - cfun->patch_area_entry;
+      if (patch_area_size)
+	ix86_output_patchable_area (patch_area_size,
+				    cfun->patch_area_entry == 0);
     }
 
   const char *mcount_name = MCOUNT_NAME;
diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
index 7c8f0cd4c70..e63edf16bcc 100644
--- a/gcc/config/i386/i386.h
+++ b/gcc/config/i386/i386.h
@@ -2774,12 +2774,6 @@ struct GTY(()) machine_function {
      structure.  */
   rtx split_stack_varargs_pointer;
 
-  /* The size of the patchable area at function entry.  */
-  unsigned int patch_area_size;
-
-  /* If true, record patchable area at function entry.  */
-  BOOL_BITFIELD record_patch_area : 1;
-
   /* This value is used for amd64 targets and specifies the current abi
      to be used. MS_ABI means ms abi. Otherwise SYSV_ABI means sysv abi.  */
   ENUM_BITFIELD(calling_abi) call_abi : 8;
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 78c38d5704a..3b243048d81 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -21289,11 +21289,13 @@ (define_insn "patchable_area"
 		    UNSPECV_PATCHABLE_AREA)]
   ""
 {
-  if (INTVAL (operands[0]))
-    ix86_output_patchable_area (INTVAL (operands[0]),
-				INTVAL (operands[1]) != 0);
+  ix86_output_patchable_area (INTVAL (operands[0]),
+			      INTVAL (operands[1]) != 0);
   return "";
-})
+}
+  [(set (attr "length") (symbol_ref "INTVAL (operands[0])"))
+   (set_attr "length_immediate" "0")
+   (set_attr "modrm" "0")])
 
 (include "mmx.md")
 (include "sse.md")
-- 
2.24.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/3] Add patch_area_size and patch_area_entry to cfun
  2020-02-05 14:33 [PATCH 0/3] Update -fpatchable-function-entry implementation H.J. Lu
  2020-02-05 14:33 ` [PATCH 1/3] x86: Add UNSPECV_PATCHABLE_AREA H.J. Lu
  2020-02-05 14:33 ` [PATCH 3/3] x86: Simplify UNSPECV_PATCHABLE_AREA generation H.J. Lu
@ 2020-02-05 14:33 ` H.J. Lu
  2020-02-05 17:00   ` Richard Sandiford
  2 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2020-02-05 14:33 UTC (permalink / raw)
  To: gcc-patches
  Cc: Uros Bizjak, Jeff Law, Richard Biener, Richard Earnshaw,
	Jakub Jelinek, Richard Sandiford, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou

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 cfun 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/

	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");
+    }
+
+  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;
+    }
+
+  cfun->patch_area_size = patch_area_size;
+  cfun->patch_area_entry = patch_area_entry;
 }
 \f
 void
diff --git a/gcc/function.h b/gcc/function.h
index 1ee8ed3de53..1ed7c400f23 100644
--- a/gcc/function.h
+++ b/gcc/function.h
@@ -332,6 +332,12 @@ struct GTY(()) function {
   /* Last assigned dependence info clique.  */
   unsigned short last_clique;
 
+  /* 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;
+
   /* Collected bit flags.  */
 
   /* Number of units of general registers that need saving in stdarg
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..b6e449425d2
--- /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-error "invalid values for 'patchable_function_entry'" } */
+{
+}
+
+void
+ __attribute__((patchable_function_entry(65536,1)))
+foo2 (void) /* { dg-error "invalid values for 'patchable_function_entry'" } */
+{
+}
+
+void
+ __attribute__((patchable_function_entry(65536,65536)))
+foo3 (void) /* { dg-error "invalid values for 'patchable_function_entry'" } */
+{
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index dc6da6c0b5b..93251f07d6e 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 = cfun->patch_area_size;
+  unsigned short patch_area_entry = cfun->patch_area_entry;
 
   /* Emit the patching area before the entry label, if any.  */
   if (patch_area_entry > 0)
-- 
2.24.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 0/3] Update -fpatchable-function-entry implementation
@ 2020-02-05 14:33 H.J. Lu
  2020-02-05 14:33 ` [PATCH 1/3] x86: Add UNSPECV_PATCHABLE_AREA H.J. Lu
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: H.J. Lu @ 2020-02-05 14:33 UTC (permalink / raw)
  To: gcc-patches
  Cc: Uros Bizjak, Jeff Law, Richard Biener, Richard Earnshaw,
	Jakub Jelinek, Richard Sandiford, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou

The current -fpatchable-function-entry implementation is done almost
behind the backend back.  Backend doesn't know if and where patchable
area will be generated during RTL passes.

TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY is only used to print out
assembly codes for patchable area.   This leads to wrong codes with
-fpatchable-function-entry:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93492

Also .cfi_startproc and DWARF info are at the wrong places when
-fpatchable-function-entry is used.

This patch set has 3 parts:

1. Add a pseudo UNSPECV_PATCHABLE_AREA instruction and define
TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY to work around the
-fpatchable-function-entry implementation deficiency.
2. Add patch_area_size and patch_area_entry to cfun to make the 
patchable area info is available in RTL passes so that backend
can handle patchable area properly.  It also limits patch_area_size
and patch_area_entry to 65535, which is a reasonable maximum size
for patchable area.  Other backends can also use it properly generate
patchable area.
3. Remove workaround in UNSPECV_PATCHABLE_AREA generation. 

If the patch set is acceptable, I can combine patch 1 and patch 3
into a single patch.

H.J. Lu (3):
  x86: Add UNSPECV_PATCHABLE_AREA
  Add patch_area_size and patch_area_entry to cfun
  x86: Simplify UNSPECV_PATCHABLE_AREA generation

 gcc/config/i386/i386-features.c               | 130 ++++++++++++------
 gcc/config/i386/i386-passes.def               |   2 +-
 gcc/config/i386/i386-protos.h                 |   5 +-
 gcc/config/i386/i386.c                        |  51 ++++++-
 gcc/config/i386/i386.h                        |  14 +-
 gcc/config/i386/i386.md                       |  17 +++
 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/testsuite/gcc.target/i386/pr93492-1.c     |  73 ++++++++++
 gcc/testsuite/gcc.target/i386/pr93492-2.c     |  12 ++
 gcc/testsuite/gcc.target/i386/pr93492-3.c     |  13 ++
 gcc/testsuite/gcc.target/i386/pr93492-4.c     |  11 ++
 gcc/testsuite/gcc.target/i386/pr93492-5.c     |  12 ++
 gcc/varasm.c                                  |  30 +---
 19 files changed, 375 insertions(+), 79 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
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-2.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-3.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-4.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-5.c

-- 
2.24.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] x86: Add UNSPECV_PATCHABLE_AREA
  2020-02-05 14:33 [PATCH 0/3] Update -fpatchable-function-entry implementation H.J. Lu
@ 2020-02-05 14:33 ` H.J. Lu
  2020-02-05 14:33 ` [PATCH 3/3] x86: Simplify UNSPECV_PATCHABLE_AREA generation H.J. Lu
  2020-02-05 14:33 ` [PATCH 2/3] Add patch_area_size and patch_area_entry to cfun H.J. Lu
  2 siblings, 0 replies; 12+ messages in thread
From: H.J. Lu @ 2020-02-05 14:33 UTC (permalink / raw)
  To: gcc-patches
  Cc: Uros Bizjak, Jeff Law, Richard Biener, Richard Earnshaw,
	Jakub Jelinek, Richard Sandiford, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou

Currently patchable area is at the wrong place.  It is placed immediately
after function label, before both .cfi_startproc and ENDBR.  This patch
adds UNSPECV_PATCHABLE_AREA for pseudo patchable area instruction and
changes ENDBR insertion pass to also insert a dummy patchable area.
TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY is defined to provide the
actual size for patchable area.  It places patchable area immediately
after .cfi_startproc and ENDBR.

gcc/

	PR target/93492
	* config/i386/i386-features.c (rest_of_insert_endbranch):
	Renamed to ...
	(rest_of_insert_endbr_and_patchable_area): Change return type
	to void.  Don't call timevar_push nor timevar_pop.  Replace
	endbr_queued_at_entrance with insn_queued_at_entrance.  Insert
	UNSPECV_PATCHABLE_AREA for patchable area.
	(pass_data_insert_endbranch): Renamed to ...
	(pass_data_insert_endbr_and_patchable_area): This.  Change
	pass name to endbr_and_patchable_area.
	(pass_insert_endbranch): Renamed to ...
	(pass_insert_endbr_and_patchable_area): This.  Add need_endbr
	and need_patchable_area.
	(pass_insert_endbr_and_patchable_area::gate): Set and check
	need_endbr/need_patchable_area.
	(pass_insert_endbr_and_patchable_area::execute): Call
	timevar_push and timevar_pop.  Pass need_endbr amd
	need_patchable_area to rest_of_insert_endbr_and_patchable_area.
	(make_pass_insert_endbranch): Renamed to ...
	(make_pass_insert_endbr_and_patchable_area): This.
	* config/i386/i386-passes.def: Replace pass_insert_endbranch
	with pass_insert_endbr_and_patchable_area.
	* config/i386/i386-protos.h (ix86_output_patchable_area): New.
	(make_pass_insert_endbranch): Renamed to ...
	(make_pass_insert_endbr_and_patchable_area): This.
	* config/i386/i386.c (ix86_asm_output_function_label): Set
	function_label_emitted to true.
	(ix86_print_patchable_function_entry): New function.
	(ix86_output_patchable_area): Likewise.
	(x86_function_profiler): Replace endbr_queued_at_entrance with
	insn_queued_at_entrance.  Generate ENDBR only for TYPE_ENDBR.
	Call ix86_output_patchable_area to generate patchable area.
	(TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY): New.
	* i386.h (queued_insn_type): New.
	(machine_function): Add patch_area_size, record_patch_area and
	function_label_emitted.  Replace endbr_queued_at_entrance with
	insn_queued_at_entrance.
	* config/i386/i386.md (UNSPECV_PATCHABLE_AREA): New.
	(patchable_area): New.

gcc/testsuite/

	PR target/93492
	* gcc.target/i386/pr93492-1.c: New test.
	* gcc.target/i386/pr93492-2.c: Likewise.
	* gcc.target/i386/pr93492-3.c: Likewise.
	* gcc.target/i386/pr93492-4.c: Likewise.
	* gcc.target/i386/pr93492-5.c: Likewise.
---
 gcc/config/i386/i386-features.c           | 136 +++++++++++++++-------
 gcc/config/i386/i386-passes.def           |   2 +-
 gcc/config/i386/i386-protos.h             |   5 +-
 gcc/config/i386/i386.c                    |  91 ++++++++++++++-
 gcc/config/i386/i386.h                    |  20 +++-
 gcc/config/i386/i386.md                   |  15 +++
 gcc/testsuite/gcc.target/i386/pr93492-1.c |  73 ++++++++++++
 gcc/testsuite/gcc.target/i386/pr93492-2.c |  12 ++
 gcc/testsuite/gcc.target/i386/pr93492-3.c |  13 +++
 gcc/testsuite/gcc.target/i386/pr93492-4.c |  11 ++
 gcc/testsuite/gcc.target/i386/pr93492-5.c |  12 ++
 11 files changed, 339 insertions(+), 51 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-2.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-3.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-4.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr93492-5.c

diff --git a/gcc/config/i386/i386-features.c b/gcc/config/i386/i386-features.c
index b49e6f8d408..be46f036126 100644
--- a/gcc/config/i386/i386-features.c
+++ b/gcc/config/i386/i386-features.c
@@ -1937,43 +1937,79 @@ make_pass_stv (gcc::context *ctxt)
   return new pass_stv (ctxt);
 }
 
-/* Inserting ENDBRANCH instructions.  */
+/* Inserting ENDBR and pseudo patchable-area instructions.  */
 
-static unsigned int
-rest_of_insert_endbranch (void)
+static void
+rest_of_insert_endbr_and_patchable_area (bool need_endbr,
+					 bool need_patchable_area)
 {
-  timevar_push (TV_MACH_DEP);
-
-  rtx cet_eb;
+  rtx endbr;
   rtx_insn *insn;
+  rtx_insn *endbr_insn = NULL;
   basic_block bb;
 
-  /* Currently emit EB if it's a tracking function, i.e. 'nocf_check' is
-     absent among function attributes.  Later an optimization will be
-     introduced to make analysis if an address of a static function is
-     taken.  A static function whose address is not taken will get a
-     nocf_check attribute.  This will allow to reduce the number of EB.  */
+  if (need_endbr)
+    {
+      /* Currently emit EB if it's a tracking function, i.e. 'nocf_check'
+	 is absent among function attributes.  Later an optimization will
+	 be introduced to make analysis if an address of a static function
+	 is taken.  A static function whose address is not taken will get
+	 a nocf_check attribute.  This will allow to reduce the number of
+	 EB.  */
+      if (!lookup_attribute ("nocf_check",
+			     TYPE_ATTRIBUTES (TREE_TYPE (cfun->decl)))
+	  && (!flag_manual_endbr
+	      || lookup_attribute ("cf_check",
+				   DECL_ATTRIBUTES (cfun->decl)))
+	  && !cgraph_node::get (cfun->decl)->only_called_directly_p ())
+	{
+	  if (crtl->profile && flag_fentry)
+	    {
+	      /* Queue ENDBR insertion to x86_function_profiler.
+		 NB: Any patchable-area insn will be inserted after
+		 ENDBR.  */
+	      cfun->machine->insn_queued_at_entrance = TYPE_ENDBR;
+	    }
+	  else
+	    {
+	      endbr = gen_nop_endbr ();
+	      bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb;
+	      rtx_insn *insn = BB_HEAD (bb);
+	      endbr_insn = emit_insn_before (endbr, insn);
+	    }
+	}
+    }
 
-  if (!lookup_attribute ("nocf_check",
-			 TYPE_ATTRIBUTES (TREE_TYPE (cfun->decl)))
-      && (!flag_manual_endbr
-	  || lookup_attribute ("cf_check",
-			       DECL_ATTRIBUTES (cfun->decl)))
-      && !cgraph_node::get (cfun->decl)->only_called_directly_p ())
+  if (need_patchable_area)
     {
-      /* Queue ENDBR insertion to x86_function_profiler.  */
       if (crtl->profile && flag_fentry)
-	cfun->machine->endbr_queued_at_entrance = true;
+	{
+	  /* Queue patchable-area insertion to x86_function_profiler.
+	     NB: If there is a queued ENDBR, x86_function_profiler
+	     will also handle patchable-area.  */
+	  if (!cfun->machine->insn_queued_at_entrance)
+	    cfun->machine->insn_queued_at_entrance = TYPE_PATCHABLE_AREA;
+	}
       else
 	{
-	  cet_eb = gen_nop_endbr ();
-
-	  bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb;
-	  insn = BB_HEAD (bb);
-	  emit_insn_before (cet_eb, insn);
+	  /* ix86_print_patchable_function_entry will provide actual
+	     size.  */
+	  rtx patchable_area = gen_patchable_area (GEN_INT (0),
+						   GEN_INT (0));
+	  if (endbr_insn)
+	    emit_insn_after (patchable_area, endbr_insn);
+	  else
+	    {
+	      bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb;
+	      insn = BB_HEAD (bb);
+	      emit_insn_before (patchable_area, insn);
+	    }
 	}
     }
 
+  if (!need_endbr)
+    return;
+
   bb = 0;
   FOR_EACH_BB_FN (bb, cfun)
     {
@@ -1982,7 +2018,6 @@ rest_of_insert_endbranch (void)
 	{
 	  if (CALL_P (insn))
 	    {
-	      bool need_endbr;
 	      need_endbr = find_reg_note (insn, REG_SETJMP, NULL) != NULL;
 	      if (!need_endbr && !SIBLING_CALL_P (insn))
 		{
@@ -2013,8 +2048,8 @@ rest_of_insert_endbranch (void)
 	      /* Generate ENDBRANCH after CALL, which can return more than
 		 twice, setjmp-like functions.  */
 
-	      cet_eb = gen_nop_endbr ();
-	      emit_insn_after_setloc (cet_eb, insn, INSN_LOCATION (insn));
+	      endbr = gen_nop_endbr ();
+	      emit_insn_after_setloc (endbr, insn, INSN_LOCATION (insn));
 	      continue;
 	    }
 
@@ -2044,31 +2079,30 @@ rest_of_insert_endbranch (void)
 		  dest_blk = e->dest;
 		  insn = BB_HEAD (dest_blk);
 		  gcc_assert (LABEL_P (insn));
-		  cet_eb = gen_nop_endbr ();
-		  emit_insn_after (cet_eb, insn);
+		  endbr = gen_nop_endbr ();
+		  emit_insn_after (endbr, insn);
 		}
 	      continue;
 	    }
 
 	  if (LABEL_P (insn) && LABEL_PRESERVE_P (insn))
 	    {
-	      cet_eb = gen_nop_endbr ();
-	      emit_insn_after (cet_eb, insn);
+	      endbr = gen_nop_endbr ();
+	      emit_insn_after (endbr, insn);
 	      continue;
 	    }
 	}
     }
 
-  timevar_pop (TV_MACH_DEP);
-  return 0;
+  return;
 }
 
 namespace {
 
-const pass_data pass_data_insert_endbranch =
+const pass_data pass_data_insert_endbr_and_patchable_area =
 {
   RTL_PASS, /* type.  */
-  "cet", /* name.  */
+  "endbr_and_patchable_area", /* name.  */
   OPTGROUP_NONE, /* optinfo_flags.  */
   TV_MACH_DEP, /* tv_id.  */
   0, /* properties_required.  */
@@ -2078,32 +2112,44 @@ const pass_data pass_data_insert_endbranch =
   0, /* todo_flags_finish.  */
 };
 
-class pass_insert_endbranch : public rtl_opt_pass
+class pass_insert_endbr_and_patchable_area : public rtl_opt_pass
 {
 public:
-  pass_insert_endbranch (gcc::context *ctxt)
-    : rtl_opt_pass (pass_data_insert_endbranch, ctxt)
+  pass_insert_endbr_and_patchable_area (gcc::context *ctxt)
+    : rtl_opt_pass (pass_data_insert_endbr_and_patchable_area, ctxt)
   {}
 
   /* opt_pass methods: */
-  virtual bool gate (function *)
-    {
-      return ((flag_cf_protection & CF_BRANCH));
+  virtual bool gate (function *fun)
+    {
+      need_endbr = (flag_cf_protection & CF_BRANCH) != 0;
+      need_patchable_area
+	= (function_entry_patch_area_size
+	   || lookup_attribute ("patchable_function_entry",
+				DECL_ATTRIBUTES (fun->decl)));
+      return need_endbr || need_patchable_area;
     }
 
   virtual unsigned int execute (function *)
     {
-      return rest_of_insert_endbranch ();
+      timevar_push (TV_MACH_DEP);
+      rest_of_insert_endbr_and_patchable_area (need_endbr,
+					       need_patchable_area);
+      timevar_pop (TV_MACH_DEP);
+      return 0;
     }
 
-}; // class pass_insert_endbranch
+private:
+  bool need_endbr;
+  bool need_patchable_area;
+}; // class pass_insert_endbr_and_patchable_area
 
 } // anon namespace
 
 rtl_opt_pass *
-make_pass_insert_endbranch (gcc::context *ctxt)
+make_pass_insert_endbr_and_patchable_area (gcc::context *ctxt)
 {
-  return new pass_insert_endbranch (ctxt);
+  return new pass_insert_endbr_and_patchable_area (ctxt);
 }
 
 /* At entry of the nearest common dominator for basic blocks with
diff --git a/gcc/config/i386/i386-passes.def b/gcc/config/i386/i386-passes.def
index 41386a13d88..d83c7b956b1 100644
--- a/gcc/config/i386/i386-passes.def
+++ b/gcc/config/i386/i386-passes.def
@@ -30,6 +30,6 @@ along with GCC; see the file COPYING3.  If not see
      CONSTM1_RTX generated by the STV pass can be CSEed.  */
   INSERT_PASS_BEFORE (pass_cse2, 1, pass_stv, true /* timode_p */);
 
-  INSERT_PASS_BEFORE (pass_shorten_branches, 1, pass_insert_endbranch);
+  INSERT_PASS_BEFORE (pass_shorten_branches, 1, pass_insert_endbr_and_patchable_area);
 
   INSERT_PASS_AFTER (pass_combine, 1, pass_remove_partial_avx_dependency);
diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h
index 266381ca5a6..cbcc08ebfe8 100644
--- a/gcc/config/i386/i386-protos.h
+++ b/gcc/config/i386/i386-protos.h
@@ -87,6 +87,8 @@ extern const char *output_fp_compare (rtx_insn *, rtx*, bool, bool);
 extern const char *output_adjust_stack_and_probe (rtx);
 extern const char *output_probe_stack_range (rtx, rtx);
 
+extern void ix86_output_patchable_area (unsigned int, bool);
+
 extern void ix86_expand_clear (rtx);
 extern void ix86_expand_move (machine_mode, rtx[]);
 extern void ix86_expand_vector_move (machine_mode, rtx[]);
@@ -376,6 +378,7 @@ class rtl_opt_pass;
 
 extern rtl_opt_pass *make_pass_insert_vzeroupper (gcc::context *);
 extern rtl_opt_pass *make_pass_stv (gcc::context *);
-extern rtl_opt_pass *make_pass_insert_endbranch (gcc::context *);
+extern rtl_opt_pass *make_pass_insert_endbr_and_patchable_area
+  (gcc::context *);
 extern rtl_opt_pass *make_pass_remove_partial_avx_dependency
   (gcc::context *);
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index ffda3e8fd21..051a1fcbdc2 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -1563,6 +1563,9 @@ ix86_asm_output_function_label (FILE *asm_out_file, const char *fname,
 {
   bool is_ms_hook = ix86_function_ms_hook_prologue (decl);
 
+  if (cfun)
+    cfun->machine->function_label_emitted = true;
+
   if (is_ms_hook)
     {
       int i, filler_count = (TARGET_64BIT ? 32 : 16);
@@ -9118,6 +9121,80 @@ ix86_output_function_epilogue (FILE *file ATTRIBUTE_UNUSED)
     }
 }
 
+/* Implement TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY.  */
+
+void
+ix86_print_patchable_function_entry (FILE *file,
+				     unsigned HOST_WIDE_INT patch_area_size,
+				     bool record_p)
+{
+  if (cfun->machine->function_label_emitted)
+    {
+      /* The insert_endbr_and_patchable_area pass inserted a dummy
+	 UNSPECV_PATCHABLE_AREA with 0 patchable area size.  If the
+	 patchable area is placed after the function label, we replace
+	 0 patchable area size with the real one.  Otherwise, the
+	 dummy UNSPECV_PATCHABLE_AREA will be ignored.  */
+      if (cfun->machine->insn_queued_at_entrance)
+	{
+	  /* Record the patchable area.  Both ENDBR and patchable area
+	     will be inserted by x86_function_profiler later.  */
+	  cfun->machine->patch_area_size = patch_area_size;
+	  cfun->machine->record_patch_area = record_p;
+	  return;
+	}
+
+      /* We can have
+
+	 UNSPECV_NOP_ENDBR
+	 UNSPECV_PATCHABLE_AREA
+
+	 or just
+
+	 UNSPECV_PATCHABLE_AREA
+       */
+      rtx_insn *patchable_insn;
+      rtx_insn *insn = next_real_nondebug_insn (get_insns ());
+      if (insn
+	  && INSN_P (insn)
+	  && GET_CODE (PATTERN (insn)) == UNSPEC_VOLATILE
+	  && XINT (PATTERN (insn), 1) == UNSPECV_NOP_ENDBR)
+	patchable_insn = next_real_nondebug_insn (insn);
+      else
+	patchable_insn = insn;
+
+      if (patchable_insn && INSN_P (patchable_insn))
+	{
+	  /* Replace the dummy patchable area size with the real one.  */
+	  rtx pattern = PATTERN (patchable_insn);
+	  if (GET_CODE (pattern) == UNSPEC_VOLATILE
+	      && XINT (pattern, 1) == UNSPECV_PATCHABLE_AREA)
+	    {
+	      XVECEXP (pattern, 0, 0) = GEN_INT (patch_area_size);
+	      XVECEXP (pattern, 0, 1) = GEN_INT (record_p);
+	    }
+	  return;
+	}
+
+      gcc_unreachable ();
+    }
+
+  default_print_patchable_function_entry (file, patch_area_size,
+					  record_p);
+}
+
+/* Output patchable area.  NB: default_print_patchable_function_entry
+   isn't available in i386.md.  */
+
+void
+ix86_output_patchable_area (unsigned int patch_area_size,
+			    bool record_p)
+{
+  default_print_patchable_function_entry (asm_out_file,
+					  patch_area_size,
+					  record_p);
+}
+
 /* Return a scratch register to use in the split stack prologue.  The
    split stack prologue is used for -fsplit-stack.  It is the first
    instructions in the function, even before the regular prologue.
@@ -20151,8 +20228,14 @@ current_fentry_section (const char **name)
 void
 x86_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
 {
-  if (cfun->machine->endbr_queued_at_entrance)
-    fprintf (file, "\t%s\n", TARGET_64BIT ? "endbr64" : "endbr32");
+  if (cfun->machine->insn_queued_at_entrance)
+    {
+      if (cfun->machine->insn_queued_at_entrance == TYPE_ENDBR)
+	fprintf (file, "\t%s\n", TARGET_64BIT ? "endbr64" : "endbr32");
+      if (cfun->machine->patch_area_size)
+	ix86_output_patchable_area (cfun->machine->patch_area_size,
+				    cfun->machine->record_patch_area);
+    }
 
   const char *mcount_name = MCOUNT_NAME;
 
@@ -22744,6 +22827,10 @@ ix86_run_selftests (void)
 #undef TARGET_ASM_FUNCTION_EPILOGUE
 #define TARGET_ASM_FUNCTION_EPILOGUE ix86_output_function_epilogue
 
+#undef TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
+#define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY \
+  ix86_print_patchable_function_entry
+
 #undef TARGET_ENCODE_SECTION_INFO
 #ifndef SUBTARGET_ENCODE_SECTION_INFO
 #define TARGET_ENCODE_SECTION_INFO ix86_encode_section_info
diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
index 943e9a5c783..7c8f0cd4c70 100644
--- a/gcc/config/i386/i386.h
+++ b/gcc/config/i386/i386.h
@@ -2751,6 +2751,13 @@ enum function_type
   TYPE_EXCEPTION
 };
 
+enum queued_insn_type
+{
+  TYPE_NONE = 0,
+  TYPE_ENDBR,
+  TYPE_PATCHABLE_AREA
+};
+
 struct GTY(()) machine_function {
   struct stack_local_entry *stack_locals;
   int varargs_gpr_size;
@@ -2767,6 +2774,12 @@ struct GTY(()) machine_function {
      structure.  */
   rtx split_stack_varargs_pointer;
 
+  /* The size of the patchable area at function entry.  */
+  unsigned int patch_area_size;
+
+  /* If true, record patchable area at function entry.  */
+  BOOL_BITFIELD record_patch_area : 1;
+
   /* This value is used for amd64 targets and specifies the current abi
      to be used. MS_ABI means ms abi. Otherwise SYSV_ABI means sysv abi.  */
   ENUM_BITFIELD(calling_abi) call_abi : 8;
@@ -2841,8 +2854,11 @@ struct GTY(()) machine_function {
   /* Nonzero if the function places outgoing arguments on stack.  */
   BOOL_BITFIELD outgoing_args_on_stack : 1;
 
-  /* If true, ENDBR is queued at function entrance.  */
-  BOOL_BITFIELD endbr_queued_at_entrance : 1;
+  /* If true, ENDBR or patchable area is queued at function entrance.  */
+  ENUM_BITFIELD(queued_insn_type) insn_queued_at_entrance : 2;
+
+  /* If true, the function label has been emitted.  */
+  BOOL_BITFIELD function_label_emitted : 1;
 
   /* True if the function needs a stack frame.  */
   BOOL_BITFIELD stack_frame_required : 1;
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 46b442dae51..78c38d5704a 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -300,6 +300,9 @@ (define_c_enum "unspecv" [
   ;; For ENQCMD and ENQCMDS support
   UNSPECV_ENQCMD
   UNSPECV_ENQCMDS
+
+  ;; For patchable area support
+  UNSPECV_PATCHABLE_AREA
 ])
 
 ;; Constants to represent rounding modes in the ROUND instruction
@@ -21280,6 +21283,18 @@ (define_insn "speculation_barrier"
   [(set_attr "type" "other")
    (set_attr "length" "3")])
 
+(define_insn "patchable_area"
+  [(unspec_volatile [(match_operand 0 "const_int_operand")
+		     (match_operand 1 "const_int_operand")]
+		    UNSPECV_PATCHABLE_AREA)]
+  ""
+{
+  if (INTVAL (operands[0]))
+    ix86_output_patchable_area (INTVAL (operands[0]),
+				INTVAL (operands[1]) != 0);
+  return "";
+})
+
 (include "mmx.md")
 (include "sse.md")
 (include "sync.md")
diff --git a/gcc/testsuite/gcc.target/i386/pr93492-1.c b/gcc/testsuite/gcc.target/i386/pr93492-1.c
new file mode 100644
index 00000000000..f978d2e5220
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr93492-1.c
@@ -0,0 +1,73 @@
+/* { dg-do "compile" } */
+/* { dg-options "-O1 -fcf-protection -mmanual-endbr" } */
+/* { dg-final { check-function-bodies "**" "" } } */
+
+/* Note: this test only checks the instructions in the function bodies,
+   not the placement of the patch label or nops before the function.  */
+
+/*
+**f10_none:
+**	nop
+**	ret
+*/
+void
+__attribute__ ((nocf_check,patchable_function_entry (1, 0)))
+f10_none (void)
+{
+}
+
+/*
+**f10_endbr:
+**	endbr(32|64)
+**	nop
+**	ret
+*/
+void
+__attribute__ ((cf_check,patchable_function_entry (1, 0)))
+f10_endbr (void)
+{
+}
+
+/*
+**f11_none:
+**	ret
+*/
+void
+__attribute__ ((nocf_check,patchable_function_entry (1, 1)))
+f11_none (void)
+{
+}
+
+/*
+**f11_endbr:
+**	endbr(32|64)
+**	ret
+*/
+void
+__attribute__ ((cf_check,patchable_function_entry (1, 1)))
+f11_endbr (void)
+{
+}
+
+/*
+**f21_none:
+**	nop
+**	ret
+*/
+void
+__attribute__ ((nocf_check,patchable_function_entry (2, 1)))
+f21_none (void)
+{
+}
+
+/*
+**f21_endbr:
+**	endbr(32|64)
+**	nop
+**	ret
+*/
+void
+__attribute__ ((cf_check,patchable_function_entry (2, 1)))
+f21_endbr (void)
+{
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr93492-2.c b/gcc/testsuite/gcc.target/i386/pr93492-2.c
new file mode 100644
index 00000000000..ec26d4cc367
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr93492-2.c
@@ -0,0 +1,12 @@
+/* { dg-do "compile" } */
+/* { dg-options "-O1 -fcf-protection -mmanual-endbr -fasynchronous-unwind-tables" } */
+
+/* Test the placement of the .LPFE1 label.  */
+
+void
+__attribute__ ((cf_check,patchable_function_entry (1, 0)))
+f10_endbr (void)
+{
+}
+
+/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n\tret\n" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr93492-3.c b/gcc/testsuite/gcc.target/i386/pr93492-3.c
new file mode 100644
index 00000000000..1f03c627120
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr93492-3.c
@@ -0,0 +1,13 @@
+/* { dg-do "compile" } */
+/* { dg-require-effective-target mfentry } */
+/* { dg-options "-O1 -fcf-protection -mmanual-endbr -mfentry -pg -fasynchronous-unwind-tables" } */
+
+/* Test the placement of the .LPFE1 label.  */
+
+void
+__attribute__ ((cf_check,patchable_function_entry (1, 0)))
+f10_endbr (void)
+{
+}
+
+/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n1:\tcall\t__fentry__\n\tret\n" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr93492-4.c b/gcc/testsuite/gcc.target/i386/pr93492-4.c
new file mode 100644
index 00000000000..d193df8e66d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr93492-4.c
@@ -0,0 +1,11 @@
+/* { dg-do "compile" } */
+/* { dg-options "-O1 -fpatchable-function-entry=1 -fasynchronous-unwind-tables" } */
+
+/* Test the placement of the .LPFE1 label.  */
+
+void
+foo (void)
+{
+}
+
+/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n\tret\n" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr93492-5.c b/gcc/testsuite/gcc.target/i386/pr93492-5.c
new file mode 100644
index 00000000000..d04077c6007
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr93492-5.c
@@ -0,0 +1,12 @@
+/* { dg-do "compile" } */
+/* { dg-require-effective-target mfentry } */
+/* { dg-options "-O1 -fpatchable-function-entry=1 -mfentry -pg -fasynchronous-unwind-tables" } */
+
+/* Test the placement of the .LPFE1 label.  */
+
+void
+foo (void)
+{
+}
+
+/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n1:\tcall\t__fentry__\n\tret\n" } } */
-- 
2.24.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] Add patch_area_size and patch_area_entry to cfun
  2020-02-05 14:33 ` [PATCH 2/3] Add patch_area_size and patch_area_entry to cfun H.J. Lu
@ 2020-02-05 17:00   ` Richard Sandiford
  2020-02-05 20:21     ` [PATCH] Add patch_area_size and patch_area_entry to crtl H.J. Lu
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Sandiford @ 2020-02-05 17:00 UTC (permalink / raw)
  To: H.J. Lu
  Cc: gcc-patches, Uros Bizjak, Jeff Law, Richard Biener,
	Richard Earnshaw, Jakub Jelinek, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou

"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.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] Add patch_area_size and patch_area_entry to crtl
  2020-02-05 17:00   ` Richard Sandiford
@ 2020-02-05 20:21     ` H.J. Lu
  2020-02-05 22:25       ` H.J. Lu
  0 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2020-02-05 20:21 UTC (permalink / raw)
  To: GCC Patches, Uros Bizjak, Jeff Law, Richard Biener,
	Richard Earnshaw, Jakub Jelinek, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou, Richard Sandiford

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

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?

Thanks.

-- 
H.J.

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

From 8a56c3424d4194dfc0290eaa666962c6e75f9ce8 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..45e93988886
--- /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" } */
+}
+
+void
+ __attribute__((patchable_function_entry(65536,1)))
+foo2 (void)
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" } */
+}
+
+void
+ __attribute__((patchable_function_entry(65536,65536)))
+foo3 (void)
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" } */
+}
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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Add patch_area_size and patch_area_entry to crtl
  2020-02-05 20:21     ` [PATCH] Add patch_area_size and patch_area_entry to crtl H.J. Lu
@ 2020-02-05 22:25       ` H.J. Lu
  2020-02-05 22:37         ` Marek Polacek
  0 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2020-02-05 22:25 UTC (permalink / raw)
  To: GCC Patches, Uros Bizjak, Jeff Law, Richard Biener,
	Richard Earnshaw, Jakub Jelinek, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou, Richard Sandiford

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++?

-- 
H.J.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Add patch_area_size and patch_area_entry to crtl
  2020-02-05 22:25       ` H.J. Lu
@ 2020-02-05 22:37         ` Marek Polacek
  2020-02-05 22:52           ` H.J. Lu
  0 siblings, 1 reply; 12+ messages in thread
From: Marek Polacek @ 2020-02-05 22:37 UTC (permalink / raw)
  To: H.J. Lu
  Cc: GCC Patches, Uros Bizjak, Jeff Law, Richard Biener,
	Richard Earnshaw, Jakub Jelinek, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou, Richard Sandiford

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Add patch_area_size and patch_area_entry to crtl
  2020-02-05 22:37         ` Marek Polacek
@ 2020-02-05 22:52           ` H.J. Lu
  2020-02-05 22:55             ` H.J. Lu
  0 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2020-02-05 22:52 UTC (permalink / raw)
  To: Marek Polacek
  Cc: GCC Patches, Uros Bizjak, Jeff Law, Richard Biener,
	Richard Earnshaw, Jakub Jelinek, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou, Richard Sandiford

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.

Thanks.

-- 
H.J.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Add patch_area_size and patch_area_entry to crtl
  2020-02-05 22:52           ` H.J. Lu
@ 2020-02-05 22:55             ` H.J. Lu
  2020-02-06  8:01               ` Richard Sandiford
  0 siblings, 1 reply; 12+ messages in thread
From: H.J. Lu @ 2020-02-05 22:55 UTC (permalink / raw)
  To: Marek Polacek
  Cc: GCC Patches, Uros Bizjak, Jeff Law, Richard Biener,
	Richard Earnshaw, Jakub Jelinek, Torsten Duwe, Szabolcs Nagy,
	Eric Botcazou, Richard Sandiford

[-- 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Add patch_area_size and patch_area_entry to crtl
  2020-02-05 22:55             ` H.J. Lu
@ 2020-02-06  8:01               ` Richard Sandiford
  2020-05-02  1:23                 ` H.J. Lu
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Sandiford @ 2020-02-06  8:01 UTC (permalink / raw)
  To: H.J. Lu
  Cc: Marek Polacek, GCC Patches, Uros Bizjak, Jeff Law,
	Richard Biener, Richard Earnshaw, Jakub Jelinek, Torsten Duwe,
	Szabolcs Nagy, Eric Botcazou

"H.J. Lu" <hjl.tools@gmail.com> writes:
> 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.
>
> 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.

LGTM, but an RM should have the final say on whether this is
suitable for stage 4.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Add patch_area_size and patch_area_entry to crtl
  2020-02-06  8:01               ` Richard Sandiford
@ 2020-05-02  1:23                 ` H.J. Lu
  0 siblings, 0 replies; 12+ messages in thread
From: H.J. Lu @ 2020-05-02  1:23 UTC (permalink / raw)
  To: Marek Polacek, GCC Patches, Uros Bizjak, Jeff Law,
	Richard Biener, Richard Earnshaw, Jakub Jelinek, Torsten Duwe,
	Szabolcs Nagy, Eric Botcazou, Richard Sandiford

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

On Thu, Feb 6, 2020 at 12:01 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> "H.J. Lu" <hjl.tools@gmail.com> writes:
> > 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.
> >
> > 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.
>
> LGTM, but an RM should have the final say on whether this is
> suitable for stage 4.
>

This is the patch I am checking in.

Thanks.

-- 
H.J.

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

From f4a89f5c9beb31f707c970db0320e4687e962c88 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 ac936d5bbbb..a101312c581 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -4553,6 +4553,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 a7ec77d5c85..86efa22bf60 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 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 527d362533a..767d1f07801 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -14112,6 +14112,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 c212a1a57dc..3dccef39701 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -2615,10 +2615,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..45e93988886
--- /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" } */
+}
+
+void
+ __attribute__((patchable_function_entry(65536,1)))
+foo2 (void)
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" } */
+}
+
+void
+ __attribute__((patchable_function_entry(65536,65536)))
+foo3 (void)
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" } */
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 271a67abf56..f062e48071f 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.26.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-05-02  1:24 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-05 14:33 [PATCH 0/3] Update -fpatchable-function-entry implementation H.J. Lu
2020-02-05 14:33 ` [PATCH 1/3] x86: Add UNSPECV_PATCHABLE_AREA H.J. Lu
2020-02-05 14:33 ` [PATCH 3/3] x86: Simplify UNSPECV_PATCHABLE_AREA generation H.J. Lu
2020-02-05 14:33 ` [PATCH 2/3] Add patch_area_size and patch_area_entry to cfun H.J. Lu
2020-02-05 17:00   ` Richard Sandiford
2020-02-05 20:21     ` [PATCH] Add patch_area_size and patch_area_entry to crtl H.J. Lu
2020-02-05 22:25       ` H.J. Lu
2020-02-05 22:37         ` Marek Polacek
2020-02-05 22:52           ` H.J. Lu
2020-02-05 22:55             ` H.J. Lu
2020-02-06  8:01               ` Richard Sandiford
2020-05-02  1:23                 ` H.J. Lu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).