[PATCH v9] add -fpatchable-function-entry=N,M option

[PATCH v9] add -fpatchable-function-entry=N,M option

[Torsten Duwe]

Changes since v8:

  * Documentation changes as requested by Sandra
  * 3 functional test cases added

	Torsten


gcc/c-family/ChangeLog
2017-06-13  Torsten Duwe  <duwe@suse.de>

	* c-attribs.c (c_common_attribute_table): Add entry for
	"patchable_function_entry".

gcc/lto/ChangeLog
2017-06-13  Torsten Duwe  <duwe@suse.de>

	* lto-lang.c (lto_attribute_table): Add entry for
	"patchable_function_entry".

gcc/ChangeLog
2017-06-13  Torsten Duwe  <duwe@suse.de>

	* common.opt: Introduce -fpatchable-function-entry
	command line option, and its variables function_entry_patch_area_size
	and function_entry_patch_area_start.
	* opts.c (common_handle_option): Add -fpatchable_function_entry_ case,
	including a two-value parser.
	* target.def (print_patchable_function_entry): New target hook.
	* targhooks.h (default_print_patchable_function_entry): New function.
	* targhooks.c (default_print_patchable_function_entry): Likewise.
	* toplev.c (process_options): Switch off IPA-RA if
	patchable function entries are being generated.
	* varasm.c (assemble_start_function): Look at the
	patchable-function-entry command line switch and current
	function attributes and maybe generate NOP instructions by
	calling the print_patchable_function_entry hook.
	* doc/extend.texi: Document patchable_function_entry attribute.
	* doc/invoke.texi: Document -fpatchable_function_entry
	command line option.
	* doc/tm.texi.in (TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY):
	New target hook.
	* doc/tm.texi: Likewise.

gcc/testsuite/ChangeLog
2017-06-13  Torsten Duwe  <duwe@suse.de>

	* c-c++-common/patchable_function_entry-default.c: New test.
	* c-c++-common/patchable_function_entry-decl.c: Likewise.
	* c-c++-common/patchable_function_entry-definition.c: Likewise.

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index f2a88e147ba..31137ce0433 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -139,6 +139,8 @@ static tree handle_bnd_variable_size_attribute (tree *, tree, tree, int, bool *)
 static tree handle_bnd_legacy (tree *, tree, tree, int, bool *);
 static tree handle_bnd_instrument (tree *, tree, tree, int, bool *);
 static tree handle_fallthrough_attribute (tree *, tree, tree, int, bool *);
+static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
+						       int, bool *);
 
 /* Table of machine-independent attributes common to all C-like languages.
 
@@ -345,6 +347,9 @@ const struct attribute_spec c_common_attribute_table[] =
 			      handle_bnd_instrument, false },
   { "fallthrough",	      0, 0, false, false, false,
 			      handle_fallthrough_attribute, false },
+  { "patchable_function_entry",	1, 2, true, false, false,
+			      handle_patchable_function_entry_attribute,
+			      false },
   { NULL,                     0, 0, false, false, false, NULL, false }
 };
 
@@ -3173,3 +3178,10 @@ handle_fallthrough_attribute (tree *, tree name, tree, int,
   *no_add_attrs = true;
   return NULL_TREE;
 }
+
+static tree
+handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
+{
+  /* Nothing to be done here.  */
+  return NULL_TREE;
+}
diff --git a/gcc/common.opt b/gcc/common.opt
index a5c3aeaa336..f542590650c 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -163,6 +163,13 @@ bool flag_stack_usage_info = false
 Variable
 int flag_debug_asm
 
+; How many NOP insns to place at each function entry by default
+Variable
+HOST_WIDE_INT function_entry_patch_area_size
+
+; And how far the real asm entry point is into this area
+Variable
+HOST_WIDE_INT function_entry_patch_area_start
 
 ; Balance between GNAT encodings and standard DWARF to emit.
 Variable
@@ -2022,6 +2029,10 @@ fprofile-reorder-functions
 Common Report Var(flag_profile_reorder_functions)
 Enable function reordering that improves code placement.
 
+fpatchable-function-entry=
+Common Joined Optimization
+Insert NOP instructions at each function entry.
+
 frandom-seed
 Common Var(common_deferred_options) Defer
 
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 1255995eb78..d09ccd90c42 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3083,6 +3083,25 @@ that affect more than one function.
 This attribute should be used for debugging purposes only.  It is not
 suitable in production code.
 
+@item patchable_function_entry
+@cindex @code{patchable_function_entry} function attribute
+@cindex extra NOP instructions at the function entry point
+In case the target's text segment can be made writable at run time by
+any means, padding the function entry with a number of NOPs can be
+used to provide a universal tool for instrumentation.  Usually,
+patchable function entries are enabled globally using the
+@option{-fpatchable-function-entry=N,M} command-line switch, and
+disabled with attribute @code{patchable_function_entry (0)} for
+functions that are part of the actual instrumentation framework.
+This conveniently avoids an endless recursion.
+The @code{patchable_function_entry} function attribute can be used to
+change the number of NOPs to any desired value.  The two-value syntax
+is the same as for the command-line switch
+@option{-fpatchable-function-entry=N,M}, generating @var{N} NOPs, with
+the function entry point before the @var{M}th NOP instruction.
+@var{M} defaults to 0 if omitted e.g. function entry point is before
+the first NOP.
+
 @item pure
 @cindex @code{pure} function attribute
 @cindex functions that have no side effects
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 68a558e9992..2e08387ad40 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -11384,6 +11384,34 @@ of the function name, it is considered to be a match.  For C99 and C++
 extended identifiers, the function name must be given in UTF-8, not
 using universal character names.
 
+@item -fpatchable-function-entry=@var{N}[,@var{M}]
+@opindex fpatchable-function-entry
+Generate @var{N} NOPs right at the beginning
+of each function, with the function entry point before the @var{M}th NOP.
+If @var{M} is omitted, it defaults to @code{0} so the
+function entry points to the address just at the first NOP.
+The NOP instructions reserve extra space which can be used to patch in
+any desired instrumentation at run time, provided that the code segment
+is writable.  The amount of space is controllable indirectly via
+the number of NOPs; the NOP instruction used corresponds to the instruction
+emitted by the internal GCC back-end interface @code{gen_nop}.  This behavior
+is target-specific and may also depend on the architecture variant and/or
+other compilation options.
+
+For run-time identification, the starting addresses of these areas,
+which correspond to their respective function entries minus @var{M},
+are additionally collected in the @code{__patchable_function_entries}
+section of the resulting binary.
+
+Note that the value of @code{__attribute__ ((patchable_function_entry
+(N,M)))} takes precedence over command-line option
+@option{-fpatchable-function-entry=N,M}.  This can be used to increase
+the area size or to remove it completely on a single function.
+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.
+
 @end table
 
 
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index c4f2c893c8e..0e5e20904af 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -4566,6 +4566,12 @@ will select the smallest suitable mode.
 This section describes the macros that output function entry
 (@dfn{prologue}) and exit (@dfn{epilogue}) code.
 
+@deftypefn {Target Hook} void TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY (FILE *@var{file}, unsigned HOST_WIDE_INT @var{patch_area_size}, bool @var{record_p})
+Generate a patchable area at a function start, consisting of
+@var{patch_area_size} NOP instructions.  Note the current location in
+a dedicated segment if @var{record_p} is true.   
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_ASM_FUNCTION_PROLOGUE (FILE *@var{file}, HOST_WIDE_INT @var{size})
 If defined, a function that outputs the assembler code for entry to a
 function.  The prologue is responsible for setting up the stack frame,
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 1c471d8da35..53696740f48 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -3650,6 +3650,8 @@ will select the smallest suitable mode.
 This section describes the macros that output function entry
 (@dfn{prologue}) and exit (@dfn{epilogue}) code.
 
+@hook TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
+
 @hook TARGET_ASM_FUNCTION_PROLOGUE
 
 @hook TARGET_ASM_FUNCTION_END_PROLOGUE
diff --git a/gcc/lto/lto-lang.c b/gcc/lto/lto-lang.c
index 52ab2a8cb81..2312ada48c6 100644
--- a/gcc/lto/lto-lang.c
+++ b/gcc/lto/lto-lang.c
@@ -48,6 +48,8 @@ static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
 static tree handle_type_generic_attribute (tree *, tree, tree, int, bool *);
 static tree handle_transaction_pure_attribute (tree *, tree, tree, int, bool *);
 static tree handle_returns_twice_attribute (tree *, tree, tree, int, bool *);
+static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
+						       int, bool *);
 static tree ignore_attribute (tree *, tree, tree, int, bool *);
 
 static tree handle_format_attribute (tree *, tree, tree, int, bool *);
@@ -76,6 +78,9 @@ const struct attribute_spec lto_attribute_table[] =
 			      handle_nonnull_attribute, false },
   { "nothrow",                0, 0, true,  false, false,
 			      handle_nothrow_attribute, false },
+  { "patchable_function_entry", 1, 2, true, false, false,
+			      handle_patchable_function_entry_attribute,
+			      false },
   { "returns_twice",          0, 0, true,  false, false,
 			      handle_returns_twice_attribute, false },
   { "sentinel",               0, 1, false, true, true,
@@ -473,6 +478,13 @@ handle_returns_twice_attribute (tree *node, tree ARG_UNUSED (name),
   return NULL_TREE;
 }
 
+static tree
+handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
+{
+  /* Nothing to be done here.  */
+  return NULL_TREE;
+}
+
 /* Ignore the given attribute.  Used when this attribute may be usefully
    overridden by the target, but is not used generically.  */
 
diff --git a/gcc/opts.c b/gcc/opts.c
index ffedb10f18f..f240c01f7d9 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -2155,6 +2155,33 @@ common_handle_option (struct gcc_options *opts,
         opts->x_flag_ipa_reference = false;
       break;
 
+    case OPT_fpatchable_function_entry_:
+      {
+	char *patch_area_arg = xstrdup (arg);
+	char *comma = strchr (patch_area_arg, ',');
+	if (comma)
+	  {
+	    *comma = '\0';
+	    function_entry_patch_area_size = 
+	      integral_argument (patch_area_arg);
+	    function_entry_patch_area_start =
+	      integral_argument (comma + 1);
+	  }
+	else
+	  {
+	    function_entry_patch_area_size =
+	      integral_argument (patch_area_arg);
+	    function_entry_patch_area_start = 0;
+	  }
+	if (function_entry_patch_area_size < 0
+	    || function_entry_patch_area_start < 0
+	    || function_entry_patch_area_size 
+		< function_entry_patch_area_start)
+	  error ("invalid arguments for %<-fpatchable_function_entry%>");
+	free (patch_area_arg);
+      }
+      break;
+
     case OPT_ftree_vectorize:
       if (!opts_set->x_flag_tree_loop_vectorize)
         opts->x_flag_tree_loop_vectorize = value;
diff --git a/gcc/target.def b/gcc/target.def
index 6bebfd5b9d6..53b60f3d276 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -288,6 +288,12 @@ hidden, protected or internal visibility as specified by @var{visibility}.",
  void, (tree decl, int visibility),
  default_assemble_visibility)
 
+DEFHOOK
+(print_patchable_function_entry,
+ "Generate a patchable area at the function start",
+ void, (FILE *file, unsigned HOST_WIDE_INT patch_area_size, bool record_p),
+ default_print_patchable_function_entry)
+
 /* Output the assembler code for entry to a function.  */
 DEFHOOK
 (function_prologue,
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 1cdec068ed8..f3d9edafafe 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1609,6 +1609,54 @@ default_compare_by_pieces_branch_ratio (machine_mode)
   return 1;
 }
 
+/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function
+   entry.  If RECORD_P is true, the location of the NOPs will be
+   recorded in a special object section called "__patchable_function_entries".
+   This routine may be called twice per function to put NOPs before
+   and after the function entry.  */
+
+void
+default_print_patchable_function_entry (FILE *file,
+					unsigned HOST_WIDE_INT patch_area_size,
+					bool record_p)
+{
+  static const char *nop_templ = 0;
+
+  /* We use the template alone, relying on the (currently sane) assumption
+     that the NOP template does not have variable operands.  */
+  if (!nop_templ)
+    {
+      int code_num;
+      rtx_insn *my_nop = make_insn_raw (gen_nop ());
+
+      code_num = recog_memoized (my_nop);
+      nop_templ = get_insn_template (code_num, my_nop);
+    }
+
+  if (record_p)
+    {
+      char buf[256];
+      static int patch_area_number;
+      section *previous_section = in_section;
+
+      patch_area_number++;
+      ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number);
+
+      switch_to_section (get_section ("__patchable_function_entries",
+				      0, NULL));
+      fputs (integer_asm_op (POINTER_SIZE_UNITS, false), file);
+      assemble_name_raw (file, buf);
+      fputc ('\n', file);
+
+      switch_to_section (previous_section);
+      ASM_OUTPUT_LABEL (file, buf);
+    }
+
+  unsigned i;
+  for (i = 0; i < patch_area_size; ++i)
+    fprintf (file, "\t%s\n", nop_templ);
+}
+
 bool
 default_profile_before_prologue (void)
 {
diff --git a/gcc/targhooks.h b/gcc/targhooks.h
index 18070df7839..6206fe20823 100644
--- a/gcc/targhooks.h
+++ b/gcc/targhooks.h
@@ -203,6 +203,9 @@ extern bool default_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT,
 						    bool);
 extern int default_compare_by_pieces_branch_ratio (machine_mode);
 
+extern void default_print_patchable_function_entry (FILE *,
+						    unsigned HOST_WIDE_INT,
+						    bool);
 extern bool default_profile_before_prologue (void);
 extern reg_class_t default_preferred_reload_class (rtx, reg_class_t);
 extern reg_class_t default_preferred_output_reload_class (rtx, reg_class_t);
diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
new file mode 100644
index 00000000000..8514b10e820
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
+/* { dg-final { scan-assembler-times "nop" 2 } } */
+
+extern int a;
+
+/* Respect overriding attributes in the declaration.  */
+int f3 (void) __attribute__((patchable_function_entry(2)));
+
+/* F3 should now get 2 NOPs.  */
+int
+__attribute__((noinline))
+f3 (void)
+{
+  return 5*a;
+}
diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-default.c b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
new file mode 100644
index 00000000000..0dcf1181dde
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
+/* { dg-final { scan-assembler-times "nop" 3 } } */
+
+extern int a;
+
+/* Nothing declared must not mean anything.  */
+int f3 (void);
+
+/* F3 should get a default-sized NOP area.  */
+int
+__attribute__((noinline))
+f3 (void)
+{
+  return 5*a;
+}
diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
new file mode 100644
index 00000000000..a007867dcb0
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
+/* { dg-final { scan-assembler-times "nop" 1 } } */
+
+extern int a;
+
+int f3 (void);
+
+/* F3 should now get 1 NOP.  */
+int
+__attribute__((noinline))
+__attribute__((patchable_function_entry(1)))
+f3 (void)
+{
+  return 5*a;
+}
diff --git a/gcc/toplev.c b/gcc/toplev.c
index f1384fc2fda..84969fb8ef4 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1612,8 +1612,10 @@ process_options (void)
     }
 
  /* Do not use IPA optimizations for register allocation if profiler is active
+    or patchable function entries are inserted for run-time instrumentation
     or port does not emit prologue and epilogue as RTL.  */
-  if (profile_flag || !targetm.have_prologue () || !targetm.have_epilogue ())
+  if (profile_flag || function_entry_patch_area_size
+      || !targetm.have_prologue () || !targetm.have_epilogue ())
     flag_ipa_ra = 0;
 
   /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 05e48a5b894..6c3a56419e8 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -1830,6 +1830,46 @@ 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);
+
+      if (tree_fits_uhwi_p (patchable_function_entry_value1))
+	patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
+      else
+	gcc_unreachable ();
+
+      patch_area_entry = 0;
+      if (list_length (pp_val) > 1)
+	{
+	  tree patchable_function_entry_value2 =
+	    TREE_VALUE (TREE_CHAIN (pp_val));
+
+	  if (tree_fits_uhwi_p (patchable_function_entry_value2))
+	    patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
+	  else
+	    gcc_unreachable ();
+	}
+    }
+
+  if (patch_area_entry > patch_area_size)
+    {
+      if (patch_area_size > 0)
+	warning (OPT_Wattributes, "Patchable function entry > size");
+      patch_area_entry = 0;
+    }
+
+  /* Emit the patching area before the entry label, if any.  */
+  if (patch_area_entry > 0)
+    targetm.asm_out.print_patchable_function_entry (asm_out_file,
+						    patch_area_entry, true);
+
   /* Do any machine/system dependent processing of the function name.  */
 #ifdef ASM_DECLARE_FUNCTION_NAME
   ASM_DECLARE_FUNCTION_NAME (asm_out_file, fnname, current_function_decl);
@@ -1838,6 +1878,12 @@ assemble_start_function (tree decl, const char *fnname)
   ASM_OUTPUT_FUNCTION_LABEL (asm_out_file, fnname, current_function_decl);
 #endif /* ASM_DECLARE_FUNCTION_NAME */
 
+  /* And the area after the label.  Record it if we haven't done so yet.  */
+  if (patch_area_size > patch_area_entry)
+    targetm.asm_out.print_patchable_function_entry (asm_out_file,
+					     patch_area_size-patch_area_entry,
+						    patch_area_entry == 0);
+
   if (lookup_attribute ("no_split_stack", DECL_ATTRIBUTES (decl)))
     saw_no_split_stack = true;
 }

Re: [PATCH v9] add -fpatchable-function-entry=N,M option

[Torsten Duwe]

Richard, Ping?

On Tue, Jun 13, 2017 at 07:00:27PM +0200, Torsten Duwe wrote:
> Changes since v8:
> 
>   * Documentation changes as requested by Sandra
>   * 3 functional test cases added
> 
> 	Torsten
> 
> 
> gcc/c-family/ChangeLog
> 2017-06-13  Torsten Duwe  <duwe@suse.de>
> 
> 	* c-attribs.c (c_common_attribute_table): Add entry for
> 	"patchable_function_entry".
> 
> gcc/lto/ChangeLog
> 2017-06-13  Torsten Duwe  <duwe@suse.de>
> 
> 	* lto-lang.c (lto_attribute_table): Add entry for
> 	"patchable_function_entry".
> 
> gcc/ChangeLog
> 2017-06-13  Torsten Duwe  <duwe@suse.de>
> 
> 	* common.opt: Introduce -fpatchable-function-entry
> 	command line option, and its variables function_entry_patch_area_size
> 	and function_entry_patch_area_start.
> 	* opts.c (common_handle_option): Add -fpatchable_function_entry_ case,
> 	including a two-value parser.
> 	* target.def (print_patchable_function_entry): New target hook.
> 	* targhooks.h (default_print_patchable_function_entry): New function.
> 	* targhooks.c (default_print_patchable_function_entry): Likewise.
> 	* toplev.c (process_options): Switch off IPA-RA if
> 	patchable function entries are being generated.
> 	* varasm.c (assemble_start_function): Look at the
> 	patchable-function-entry command line switch and current
> 	function attributes and maybe generate NOP instructions by
> 	calling the print_patchable_function_entry hook.
> 	* doc/extend.texi: Document patchable_function_entry attribute.
> 	* doc/invoke.texi: Document -fpatchable_function_entry
> 	command line option.
> 	* doc/tm.texi.in (TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY):
> 	New target hook.
> 	* doc/tm.texi: Likewise.
> 
> gcc/testsuite/ChangeLog
> 2017-06-13  Torsten Duwe  <duwe@suse.de>
> 
> 	* c-c++-common/patchable_function_entry-default.c: New test.
> 	* c-c++-common/patchable_function_entry-decl.c: Likewise.
> 	* c-c++-common/patchable_function_entry-definition.c: Likewise.
> 
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index f2a88e147ba..31137ce0433 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -139,6 +139,8 @@ static tree handle_bnd_variable_size_attribute (tree *, tree, tree, int, bool *)
>  static tree handle_bnd_legacy (tree *, tree, tree, int, bool *);
>  static tree handle_bnd_instrument (tree *, tree, tree, int, bool *);
>  static tree handle_fallthrough_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
> +						       int, bool *);
>  
>  /* Table of machine-independent attributes common to all C-like languages.
>  
> @@ -345,6 +347,9 @@ const struct attribute_spec c_common_attribute_table[] =
>  			      handle_bnd_instrument, false },
>    { "fallthrough",	      0, 0, false, false, false,
>  			      handle_fallthrough_attribute, false },
> +  { "patchable_function_entry",	1, 2, true, false, false,
> +			      handle_patchable_function_entry_attribute,
> +			      false },
>    { NULL,                     0, 0, false, false, false, NULL, false }
>  };
>  
> @@ -3173,3 +3178,10 @@ handle_fallthrough_attribute (tree *, tree name, tree, int,
>    *no_add_attrs = true;
>    return NULL_TREE;
>  }
> +
> +static tree
> +handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
> +{
> +  /* Nothing to be done here.  */
> +  return NULL_TREE;
> +}
> diff --git a/gcc/common.opt b/gcc/common.opt
> index a5c3aeaa336..f542590650c 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -163,6 +163,13 @@ bool flag_stack_usage_info = false
>  Variable
>  int flag_debug_asm
>  
> +; How many NOP insns to place at each function entry by default
> +Variable
> +HOST_WIDE_INT function_entry_patch_area_size
> +
> +; And how far the real asm entry point is into this area
> +Variable
> +HOST_WIDE_INT function_entry_patch_area_start
>  
>  ; Balance between GNAT encodings and standard DWARF to emit.
>  Variable
> @@ -2022,6 +2029,10 @@ fprofile-reorder-functions
>  Common Report Var(flag_profile_reorder_functions)
>  Enable function reordering that improves code placement.
>  
> +fpatchable-function-entry=
> +Common Joined Optimization
> +Insert NOP instructions at each function entry.
> +
>  frandom-seed
>  Common Var(common_deferred_options) Defer
>  
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 1255995eb78..d09ccd90c42 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3083,6 +3083,25 @@ that affect more than one function.
>  This attribute should be used for debugging purposes only.  It is not
>  suitable in production code.
>  
> +@item patchable_function_entry
> +@cindex @code{patchable_function_entry} function attribute
> +@cindex extra NOP instructions at the function entry point
> +In case the target's text segment can be made writable at run time by
> +any means, padding the function entry with a number of NOPs can be
> +used to provide a universal tool for instrumentation.  Usually,
> +patchable function entries are enabled globally using the
> +@option{-fpatchable-function-entry=N,M} command-line switch, and
> +disabled with attribute @code{patchable_function_entry (0)} for
> +functions that are part of the actual instrumentation framework.
> +This conveniently avoids an endless recursion.
> +The @code{patchable_function_entry} function attribute can be used to
> +change the number of NOPs to any desired value.  The two-value syntax
> +is the same as for the command-line switch
> +@option{-fpatchable-function-entry=N,M}, generating @var{N} NOPs, with
> +the function entry point before the @var{M}th NOP instruction.
> +@var{M} defaults to 0 if omitted e.g. function entry point is before
> +the first NOP.
> +
>  @item pure
>  @cindex @code{pure} function attribute
>  @cindex functions that have no side effects
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 68a558e9992..2e08387ad40 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -11384,6 +11384,34 @@ of the function name, it is considered to be a match.  For C99 and C++
>  extended identifiers, the function name must be given in UTF-8, not
>  using universal character names.
>  
> +@item -fpatchable-function-entry=@var{N}[,@var{M}]
> +@opindex fpatchable-function-entry
> +Generate @var{N} NOPs right at the beginning
> +of each function, with the function entry point before the @var{M}th NOP.
> +If @var{M} is omitted, it defaults to @code{0} so the
> +function entry points to the address just at the first NOP.
> +The NOP instructions reserve extra space which can be used to patch in
> +any desired instrumentation at run time, provided that the code segment
> +is writable.  The amount of space is controllable indirectly via
> +the number of NOPs; the NOP instruction used corresponds to the instruction
> +emitted by the internal GCC back-end interface @code{gen_nop}.  This behavior
> +is target-specific and may also depend on the architecture variant and/or
> +other compilation options.
> +
> +For run-time identification, the starting addresses of these areas,
> +which correspond to their respective function entries minus @var{M},
> +are additionally collected in the @code{__patchable_function_entries}
> +section of the resulting binary.
> +
> +Note that the value of @code{__attribute__ ((patchable_function_entry
> +(N,M)))} takes precedence over command-line option
> +@option{-fpatchable-function-entry=N,M}.  This can be used to increase
> +the area size or to remove it completely on a single function.
> +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.
> +
>  @end table
>  
>  
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index c4f2c893c8e..0e5e20904af 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -4566,6 +4566,12 @@ will select the smallest suitable mode.
>  This section describes the macros that output function entry
>  (@dfn{prologue}) and exit (@dfn{epilogue}) code.
>  
> +@deftypefn {Target Hook} void TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY (FILE *@var{file}, unsigned HOST_WIDE_INT @var{patch_area_size}, bool @var{record_p})
> +Generate a patchable area at a function start, consisting of
> +@var{patch_area_size} NOP instructions.  Note the current location in
> +a dedicated segment if @var{record_p} is true.   
> +@end deftypefn
> +
>  @deftypefn {Target Hook} void TARGET_ASM_FUNCTION_PROLOGUE (FILE *@var{file}, HOST_WIDE_INT @var{size})
>  If defined, a function that outputs the assembler code for entry to a
>  function.  The prologue is responsible for setting up the stack frame,
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 1c471d8da35..53696740f48 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -3650,6 +3650,8 @@ will select the smallest suitable mode.
>  This section describes the macros that output function entry
>  (@dfn{prologue}) and exit (@dfn{epilogue}) code.
>  
> +@hook TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
> +
>  @hook TARGET_ASM_FUNCTION_PROLOGUE
>  
>  @hook TARGET_ASM_FUNCTION_END_PROLOGUE
> diff --git a/gcc/lto/lto-lang.c b/gcc/lto/lto-lang.c
> index 52ab2a8cb81..2312ada48c6 100644
> --- a/gcc/lto/lto-lang.c
> +++ b/gcc/lto/lto-lang.c
> @@ -48,6 +48,8 @@ static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_type_generic_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_transaction_pure_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_returns_twice_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
> +						       int, bool *);
>  static tree ignore_attribute (tree *, tree, tree, int, bool *);
>  
>  static tree handle_format_attribute (tree *, tree, tree, int, bool *);
> @@ -76,6 +78,9 @@ const struct attribute_spec lto_attribute_table[] =
>  			      handle_nonnull_attribute, false },
>    { "nothrow",                0, 0, true,  false, false,
>  			      handle_nothrow_attribute, false },
> +  { "patchable_function_entry", 1, 2, true, false, false,
> +			      handle_patchable_function_entry_attribute,
> +			      false },
>    { "returns_twice",          0, 0, true,  false, false,
>  			      handle_returns_twice_attribute, false },
>    { "sentinel",               0, 1, false, true, true,
> @@ -473,6 +478,13 @@ handle_returns_twice_attribute (tree *node, tree ARG_UNUSED (name),
>    return NULL_TREE;
>  }
>  
> +static tree
> +handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
> +{
> +  /* Nothing to be done here.  */
> +  return NULL_TREE;
> +}
> +
>  /* Ignore the given attribute.  Used when this attribute may be usefully
>     overridden by the target, but is not used generically.  */
>  
> diff --git a/gcc/opts.c b/gcc/opts.c
> index ffedb10f18f..f240c01f7d9 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -2155,6 +2155,33 @@ common_handle_option (struct gcc_options *opts,
>          opts->x_flag_ipa_reference = false;
>        break;
>  
> +    case OPT_fpatchable_function_entry_:
> +      {
> +	char *patch_area_arg = xstrdup (arg);
> +	char *comma = strchr (patch_area_arg, ',');
> +	if (comma)
> +	  {
> +	    *comma = '\0';
> +	    function_entry_patch_area_size = 
> +	      integral_argument (patch_area_arg);
> +	    function_entry_patch_area_start =
> +	      integral_argument (comma + 1);
> +	  }
> +	else
> +	  {
> +	    function_entry_patch_area_size =
> +	      integral_argument (patch_area_arg);
> +	    function_entry_patch_area_start = 0;
> +	  }
> +	if (function_entry_patch_area_size < 0
> +	    || function_entry_patch_area_start < 0
> +	    || function_entry_patch_area_size 
> +		< function_entry_patch_area_start)
> +	  error ("invalid arguments for %<-fpatchable_function_entry%>");
> +	free (patch_area_arg);
> +      }
> +      break;
> +
>      case OPT_ftree_vectorize:
>        if (!opts_set->x_flag_tree_loop_vectorize)
>          opts->x_flag_tree_loop_vectorize = value;
> diff --git a/gcc/target.def b/gcc/target.def
> index 6bebfd5b9d6..53b60f3d276 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -288,6 +288,12 @@ hidden, protected or internal visibility as specified by @var{visibility}.",
>   void, (tree decl, int visibility),
>   default_assemble_visibility)
>  
> +DEFHOOK
> +(print_patchable_function_entry,
> + "Generate a patchable area at the function start",
> + void, (FILE *file, unsigned HOST_WIDE_INT patch_area_size, bool record_p),
> + default_print_patchable_function_entry)
> +
>  /* Output the assembler code for entry to a function.  */
>  DEFHOOK
>  (function_prologue,
> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index 1cdec068ed8..f3d9edafafe 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -1609,6 +1609,54 @@ default_compare_by_pieces_branch_ratio (machine_mode)
>    return 1;
>  }
>  
> +/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function
> +   entry.  If RECORD_P is true, the location of the NOPs will be
> +   recorded in a special object section called "__patchable_function_entries".
> +   This routine may be called twice per function to put NOPs before
> +   and after the function entry.  */
> +
> +void
> +default_print_patchable_function_entry (FILE *file,
> +					unsigned HOST_WIDE_INT patch_area_size,
> +					bool record_p)
> +{
> +  static const char *nop_templ = 0;
> +
> +  /* We use the template alone, relying on the (currently sane) assumption
> +     that the NOP template does not have variable operands.  */
> +  if (!nop_templ)
> +    {
> +      int code_num;
> +      rtx_insn *my_nop = make_insn_raw (gen_nop ());
> +
> +      code_num = recog_memoized (my_nop);
> +      nop_templ = get_insn_template (code_num, my_nop);
> +    }
> +
> +  if (record_p)
> +    {
> +      char buf[256];
> +      static int patch_area_number;
> +      section *previous_section = in_section;
> +
> +      patch_area_number++;
> +      ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number);
> +
> +      switch_to_section (get_section ("__patchable_function_entries",
> +				      0, NULL));
> +      fputs (integer_asm_op (POINTER_SIZE_UNITS, false), file);
> +      assemble_name_raw (file, buf);
> +      fputc ('\n', file);
> +
> +      switch_to_section (previous_section);
> +      ASM_OUTPUT_LABEL (file, buf);
> +    }
> +
> +  unsigned i;
> +  for (i = 0; i < patch_area_size; ++i)
> +    fprintf (file, "\t%s\n", nop_templ);
> +}
> +
>  bool
>  default_profile_before_prologue (void)
>  {
> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> index 18070df7839..6206fe20823 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -203,6 +203,9 @@ extern bool default_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT,
>  						    bool);
>  extern int default_compare_by_pieces_branch_ratio (machine_mode);
>  
> +extern void default_print_patchable_function_entry (FILE *,
> +						    unsigned HOST_WIDE_INT,
> +						    bool);
>  extern bool default_profile_before_prologue (void);
>  extern reg_class_t default_preferred_reload_class (rtx, reg_class_t);
>  extern reg_class_t default_preferred_output_reload_class (rtx, reg_class_t);
> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
> new file mode 100644
> index 00000000000..8514b10e820
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
> +/* { dg-final { scan-assembler-times "nop" 2 } } */
> +
> +extern int a;
> +
> +/* Respect overriding attributes in the declaration.  */
> +int f3 (void) __attribute__((patchable_function_entry(2)));
> +
> +/* F3 should now get 2 NOPs.  */
> +int
> +__attribute__((noinline))
> +f3 (void)
> +{
> +  return 5*a;
> +}
> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-default.c b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
> new file mode 100644
> index 00000000000..0dcf1181dde
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
> +/* { dg-final { scan-assembler-times "nop" 3 } } */
> +
> +extern int a;
> +
> +/* Nothing declared must not mean anything.  */
> +int f3 (void);
> +
> +/* F3 should get a default-sized NOP area.  */
> +int
> +__attribute__((noinline))
> +f3 (void)
> +{
> +  return 5*a;
> +}
> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
> new file mode 100644
> index 00000000000..a007867dcb0
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
> +/* { dg-final { scan-assembler-times "nop" 1 } } */
> +
> +extern int a;
> +
> +int f3 (void);
> +
> +/* F3 should now get 1 NOP.  */
> +int
> +__attribute__((noinline))
> +__attribute__((patchable_function_entry(1)))
> +f3 (void)
> +{
> +  return 5*a;
> +}
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index f1384fc2fda..84969fb8ef4 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -1612,8 +1612,10 @@ process_options (void)
>      }
>  
>   /* Do not use IPA optimizations for register allocation if profiler is active
> +    or patchable function entries are inserted for run-time instrumentation
>      or port does not emit prologue and epilogue as RTL.  */
> -  if (profile_flag || !targetm.have_prologue () || !targetm.have_epilogue ())
> +  if (profile_flag || function_entry_patch_area_size
> +      || !targetm.have_prologue () || !targetm.have_epilogue ())
>      flag_ipa_ra = 0;
>  
>    /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index 05e48a5b894..6c3a56419e8 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -1830,6 +1830,46 @@ 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);
> +
> +      if (tree_fits_uhwi_p (patchable_function_entry_value1))
> +	patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
> +      else
> +	gcc_unreachable ();
> +
> +      patch_area_entry = 0;
> +      if (list_length (pp_val) > 1)
> +	{
> +	  tree patchable_function_entry_value2 =
> +	    TREE_VALUE (TREE_CHAIN (pp_val));
> +
> +	  if (tree_fits_uhwi_p (patchable_function_entry_value2))
> +	    patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
> +	  else
> +	    gcc_unreachable ();
> +	}
> +    }
> +
> +  if (patch_area_entry > patch_area_size)
> +    {
> +      if (patch_area_size > 0)
> +	warning (OPT_Wattributes, "Patchable function entry > size");
> +      patch_area_entry = 0;
> +    }
> +
> +  /* Emit the patching area before the entry label, if any.  */
> +  if (patch_area_entry > 0)
> +    targetm.asm_out.print_patchable_function_entry (asm_out_file,
> +						    patch_area_entry, true);
> +
>    /* Do any machine/system dependent processing of the function name.  */
>  #ifdef ASM_DECLARE_FUNCTION_NAME
>    ASM_DECLARE_FUNCTION_NAME (asm_out_file, fnname, current_function_decl);
> @@ -1838,6 +1878,12 @@ assemble_start_function (tree decl, const char *fnname)
>    ASM_OUTPUT_FUNCTION_LABEL (asm_out_file, fnname, current_function_decl);
>  #endif /* ASM_DECLARE_FUNCTION_NAME */
>  
> +  /* And the area after the label.  Record it if we haven't done so yet.  */
> +  if (patch_area_size > patch_area_entry)
> +    targetm.asm_out.print_patchable_function_entry (asm_out_file,
> +					     patch_area_size-patch_area_entry,
> +						    patch_area_entry == 0);
> +
>    if (lookup_attribute ("no_split_stack", DECL_ATTRIBUTES (decl)))
>      saw_no_split_stack = true;
>  }

Re: [PATCH v9] add -fpatchable-function-entry=N,M option

[Richard Earnshaw (lists)]

On 13/06/17 18:00, Torsten Duwe wrote:
> Changes since v8:
> 
>   * Documentation changes as requested by Sandra
>   * 3 functional test cases added
> 
> 	Torsten
> 
> 
> gcc/c-family/ChangeLog
> 2017-06-13  Torsten Duwe  <duwe@suse.de>
> 
> 	* c-attribs.c (c_common_attribute_table): Add entry for
> 	"patchable_function_entry".
> 
> gcc/lto/ChangeLog
> 2017-06-13  Torsten Duwe  <duwe@suse.de>
> 
> 	* lto-lang.c (lto_attribute_table): Add entry for
> 	"patchable_function_entry".
> 
> gcc/ChangeLog
> 2017-06-13  Torsten Duwe  <duwe@suse.de>
> 
> 	* common.opt: Introduce -fpatchable-function-entry
> 	command line option, and its variables function_entry_patch_area_size
> 	and function_entry_patch_area_start.
> 	* opts.c (common_handle_option): Add -fpatchable_function_entry_ case,
> 	including a two-value parser.
> 	* target.def (print_patchable_function_entry): New target hook.
> 	* targhooks.h (default_print_patchable_function_entry): New function.
> 	* targhooks.c (default_print_patchable_function_entry): Likewise.
> 	* toplev.c (process_options): Switch off IPA-RA if
> 	patchable function entries are being generated.
> 	* varasm.c (assemble_start_function): Look at the
> 	patchable-function-entry command line switch and current
> 	function attributes and maybe generate NOP instructions by
> 	calling the print_patchable_function_entry hook.
> 	* doc/extend.texi: Document patchable_function_entry attribute.
> 	* doc/invoke.texi: Document -fpatchable_function_entry
> 	command line option.
> 	* doc/tm.texi.in (TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY):
> 	New target hook.
> 	* doc/tm.texi: Likewise.
> 
> gcc/testsuite/ChangeLog
> 2017-06-13  Torsten Duwe  <duwe@suse.de>
> 
> 	* c-c++-common/patchable_function_entry-default.c: New test.
> 	* c-c++-common/patchable_function_entry-decl.c: Likewise.
> 	* c-c++-common/patchable_function_entry-definition.c: Likewise.

I think we're nearly there with this one, but there are a couple of nits
still to sort out.

I can't see anything in the patch to deal with targets that set
TARGET_HAVE_NAMED_SECTIONS to false.  We'll probably ICE in that case
since the compiler will be unable to record the location of the patch
region in the explicitly named section.  I think the correct thing to do
there is to reject the option during option validation when we can't
support it properly.

The template NOP in default_print_patchable_function_entry needs to be
added as a GC root to prevent it being discarded during garbage collection.

There are a couple of documentation tweaks, which I think help make the
text a little more comprehensible.  See inline.

R.

> 
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index f2a88e147ba..31137ce0433 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -139,6 +139,8 @@ static tree handle_bnd_variable_size_attribute (tree *, tree, tree, int, bool *)
>  static tree handle_bnd_legacy (tree *, tree, tree, int, bool *);
>  static tree handle_bnd_instrument (tree *, tree, tree, int, bool *);
>  static tree handle_fallthrough_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
> +						       int, bool *);
>  
>  /* Table of machine-independent attributes common to all C-like languages.
>  
> @@ -345,6 +347,9 @@ const struct attribute_spec c_common_attribute_table[] =
>  			      handle_bnd_instrument, false },
>    { "fallthrough",	      0, 0, false, false, false,
>  			      handle_fallthrough_attribute, false },
> +  { "patchable_function_entry",	1, 2, true, false, false,
> +			      handle_patchable_function_entry_attribute,
> +			      false },
>    { NULL,                     0, 0, false, false, false, NULL, false }
>  };
>  
> @@ -3173,3 +3178,10 @@ handle_fallthrough_attribute (tree *, tree name, tree, int,
>    *no_add_attrs = true;
>    return NULL_TREE;
>  }
> +
> +static tree
> +handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
> +{
> +  /* Nothing to be done here.  */
> +  return NULL_TREE;
> +}
> diff --git a/gcc/common.opt b/gcc/common.opt
> index a5c3aeaa336..f542590650c 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -163,6 +163,13 @@ bool flag_stack_usage_info = false
>  Variable
>  int flag_debug_asm
>  
> +; How many NOP insns to place at each function entry by default
> +Variable
> +HOST_WIDE_INT function_entry_patch_area_size
> +
> +; And how far the real asm entry point is into this area
> +Variable
> +HOST_WIDE_INT function_entry_patch_area_start
>  
>  ; Balance between GNAT encodings and standard DWARF to emit.
>  Variable
> @@ -2022,6 +2029,10 @@ fprofile-reorder-functions
>  Common Report Var(flag_profile_reorder_functions)
>  Enable function reordering that improves code placement.
>  
> +fpatchable-function-entry=
> +Common Joined Optimization
> +Insert NOP instructions at each function entry.
> +
>  frandom-seed
>  Common Var(common_deferred_options) Defer
>  
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 1255995eb78..d09ccd90c42 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3083,6 +3083,25 @@ that affect more than one function.
>  This attribute should be used for debugging purposes only.  It is not
>  suitable in production code.
>  
> +@item patchable_function_entry
> +@cindex @code{patchable_function_entry} function attribute
> +@cindex extra NOP instructions at the function entry point
> +In case the target's text segment can be made writable at run time by
> +any means, padding the function entry with a number of NOPs can be
> +used to provide a universal tool for instrumentation.  


> +Usually,
> +patchable function entries are enabled globally using the
> +@option{-fpatchable-function-entry=N,M} command-line switch, and
> +disabled with attribute @code{patchable_function_entry (0)} for
> +functions that are part of the actual instrumentation framework.
> +This conveniently avoids an endless recursion.

I think it would be better to use the imperative here.  Perhaps
something like:

If patchable function entries are enabled globally using the command
line option @option{-fpatchable-function-entry=N,M}, then all functions
that are part of the instrumentation framework must disable
instrumentation with the attribute @code{patchable_function_entry (0)}
to prevent recursion.

However, I'd be inclined to put that in a separate paragraph below
following text.

> +The @code{patchable_function_entry} function attribute can be used to
> +change the number of NOPs to any desired value.  The two-value syntax
> +is the same as for the command-line switch
> +@option{-fpatchable-function-entry=N,M}, generating @var{N} NOPs, with
> +the function entry point before the @var{M}th NOP instruction.
> +@var{M} defaults to 0 if omitted e.g. function entry point is before
> +the first NOP.
> +
>  @item pure
>  @cindex @code{pure} function attribute
>  @cindex functions that have no side effects
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 68a558e9992..2e08387ad40 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -11384,6 +11384,34 @@ of the function name, it is considered to be a match.  For C99 and C++
>  extended identifiers, the function name must be given in UTF-8, not
>  using universal character names.
>  
> +@item -fpatchable-function-entry=@var{N}[,@var{M}]
> +@opindex fpatchable-function-entry
> +Generate @var{N} NOPs right at the beginning
> +of each function, with the function entry point before the @var{M}th NOP.
> +If @var{M} is omitted, it defaults to @code{0} so the
> +function entry points to the address just at the first NOP.
> +The NOP instructions reserve extra space which can be used to patch in
> +any desired instrumentation at run time, provided that the code segment
> +is writable.  The amount of space is controllable indirectly via
> +the number of NOPs; the NOP instruction used corresponds to the instruction
> +emitted by the internal GCC back-end interface @code{gen_nop}.  This behavior
> +is target-specific and may also depend on the architecture variant and/or
> +other compilation options.
> +
> +For run-time identification, the starting addresses of these areas,
> +which correspond to their respective function entries minus @var{M},
> +are additionally collected in the @code{__patchable_function_entries}
> +section of the resulting binary.
> +
> +Note that the value of @code{__attribute__ ((patchable_function_entry
> +(N,M)))} takes precedence over command-line option
> +@option{-fpatchable-function-entry=N,M}.  This can be used to increase
> +the area size or to remove it completely on a single function.
> +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.
> +
>  @end table
>  
>  
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index c4f2c893c8e..0e5e20904af 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -4566,6 +4566,12 @@ will select the smallest suitable mode.
>  This section describes the macros that output function entry
>  (@dfn{prologue}) and exit (@dfn{epilogue}) code.
>  
> +@deftypefn {Target Hook} void TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY (FILE *@var{file}, unsigned HOST_WIDE_INT @var{patch_area_size}, bool @var{record_p})
> +Generate a patchable area at a function start, consisting of
> +@var{patch_area_size} NOP instructions.  Note the current location in
> +a dedicated segment if @var{record_p} is true.   

I had to parse that final sentence twice to understand what you meant.
The problem is that in documentation 'Note' is often used as something
being pointed out to the reader to be of special importance.  Here, I
think you'd be better off with

If @var{record_p} is true insert a pointer the current location in the
table of patchable functions.

You then need to describe where that 'table' goes.

> +@end deftypefn
> +
>  @deftypefn {Target Hook} void TARGET_ASM_FUNCTION_PROLOGUE (FILE *@var{file}, HOST_WIDE_INT @var{size})
>  If defined, a function that outputs the assembler code for entry to a
>  function.  The prologue is responsible for setting up the stack frame,
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 1c471d8da35..53696740f48 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -3650,6 +3650,8 @@ will select the smallest suitable mode.
>  This section describes the macros that output function entry
>  (@dfn{prologue}) and exit (@dfn{epilogue}) code.
>  
> +@hook TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
> +
>  @hook TARGET_ASM_FUNCTION_PROLOGUE
>  
>  @hook TARGET_ASM_FUNCTION_END_PROLOGUE
> diff --git a/gcc/lto/lto-lang.c b/gcc/lto/lto-lang.c
> index 52ab2a8cb81..2312ada48c6 100644
> --- a/gcc/lto/lto-lang.c
> +++ b/gcc/lto/lto-lang.c
> @@ -48,6 +48,8 @@ static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_type_generic_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_transaction_pure_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_returns_twice_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
> +						       int, bool *);
>  static tree ignore_attribute (tree *, tree, tree, int, bool *);
>  
>  static tree handle_format_attribute (tree *, tree, tree, int, bool *);
> @@ -76,6 +78,9 @@ const struct attribute_spec lto_attribute_table[] =
>  			      handle_nonnull_attribute, false },
>    { "nothrow",                0, 0, true,  false, false,
>  			      handle_nothrow_attribute, false },
> +  { "patchable_function_entry", 1, 2, true, false, false,
> +			      handle_patchable_function_entry_attribute,
> +			      false },
>    { "returns_twice",          0, 0, true,  false, false,
>  			      handle_returns_twice_attribute, false },
>    { "sentinel",               0, 1, false, true, true,
> @@ -473,6 +478,13 @@ handle_returns_twice_attribute (tree *node, tree ARG_UNUSED (name),
>    return NULL_TREE;
>  }
>  
> +static tree
> +handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
> +{
> +  /* Nothing to be done here.  */
> +  return NULL_TREE;
> +}
> +
>  /* Ignore the given attribute.  Used when this attribute may be usefully
>     overridden by the target, but is not used generically.  */
>  
> diff --git a/gcc/opts.c b/gcc/opts.c
> index ffedb10f18f..f240c01f7d9 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -2155,6 +2155,33 @@ common_handle_option (struct gcc_options *opts,
>          opts->x_flag_ipa_reference = false;
>        break;
>  
> +    case OPT_fpatchable_function_entry_:
> +      {
> +	char *patch_area_arg = xstrdup (arg);
> +	char *comma = strchr (patch_area_arg, ',');
> +	if (comma)
> +	  {
> +	    *comma = '\0';
> +	    function_entry_patch_area_size = 
> +	      integral_argument (patch_area_arg);
> +	    function_entry_patch_area_start =
> +	      integral_argument (comma + 1);
> +	  }
> +	else
> +	  {
> +	    function_entry_patch_area_size =
> +	      integral_argument (patch_area_arg);
> +	    function_entry_patch_area_start = 0;
> +	  }
> +	if (function_entry_patch_area_size < 0
> +	    || function_entry_patch_area_start < 0
> +	    || function_entry_patch_area_size 
> +		< function_entry_patch_area_start)
> +	  error ("invalid arguments for %<-fpatchable_function_entry%>");
> +	free (patch_area_arg);
> +      }
> +      break;
> +
>      case OPT_ftree_vectorize:
>        if (!opts_set->x_flag_tree_loop_vectorize)
>          opts->x_flag_tree_loop_vectorize = value;
> diff --git a/gcc/target.def b/gcc/target.def
> index 6bebfd5b9d6..53b60f3d276 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -288,6 +288,12 @@ hidden, protected or internal visibility as specified by @var{visibility}.",
>   void, (tree decl, int visibility),
>   default_assemble_visibility)
>  
> +DEFHOOK
> +(print_patchable_function_entry,
> + "Generate a patchable area at the function start",
> + void, (FILE *file, unsigned HOST_WIDE_INT patch_area_size, bool record_p),
> + default_print_patchable_function_entry)
> +
>  /* Output the assembler code for entry to a function.  */
>  DEFHOOK
>  (function_prologue,
> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index 1cdec068ed8..f3d9edafafe 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -1609,6 +1609,54 @@ default_compare_by_pieces_branch_ratio (machine_mode)
>    return 1;
>  }
>  
> +/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function
> +   entry.  If RECORD_P is true, the location of the NOPs will be
> +   recorded in a special object section called "__patchable_function_entries".
> +   This routine may be called twice per function to put NOPs before
> +   and after the function entry.  */
> +
> +void
> +default_print_patchable_function_entry (FILE *file,
> +					unsigned HOST_WIDE_INT patch_area_size,
> +					bool record_p)
> +{
> +  static const char *nop_templ = 0;

You need to record this pointer as a GC root.  Otherwise garbage
collection might destroy your template NOP.

> +
> +  /* We use the template alone, relying on the (currently sane) assumption
> +     that the NOP template does not have variable operands.  */
> +  if (!nop_templ)
> +    {
> +      int code_num;
> +      rtx_insn *my_nop = make_insn_raw (gen_nop ());
> +
> +      code_num = recog_memoized (my_nop);
> +      nop_templ = get_insn_template (code_num, my_nop);
> +    }
> +
> +  if (record_p)
> +    {
> +      char buf[256];
> +      static int patch_area_number;
> +      section *previous_section = in_section;
> +
> +      patch_area_number++;
> +      ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number);
> +
> +      switch_to_section (get_section ("__patchable_function_entries",
> +				      0, NULL));
> +      fputs (integer_asm_op (POINTER_SIZE_UNITS, false), file);
> +      assemble_name_raw (file, buf);
> +      fputc ('\n', file);
> +
> +      switch_to_section (previous_section);
> +      ASM_OUTPUT_LABEL (file, buf);
> +    }
> +
> +  unsigned i;
> +  for (i = 0; i < patch_area_size; ++i)
> +    fprintf (file, "\t%s\n", nop_templ);
> +}
> +
>  bool
>  default_profile_before_prologue (void)
>  {
> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> index 18070df7839..6206fe20823 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -203,6 +203,9 @@ extern bool default_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT,
>  						    bool);
>  extern int default_compare_by_pieces_branch_ratio (machine_mode);
>  
> +extern void default_print_patchable_function_entry (FILE *,
> +						    unsigned HOST_WIDE_INT,
> +						    bool);
>  extern bool default_profile_before_prologue (void);
>  extern reg_class_t default_preferred_reload_class (rtx, reg_class_t);
>  extern reg_class_t default_preferred_output_reload_class (rtx, reg_class_t);
> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
> new file mode 100644
> index 00000000000..8514b10e820
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
> +/* { dg-final { scan-assembler-times "nop" 2 } } */
> +
> +extern int a;
> +
> +/* Respect overriding attributes in the declaration.  */
> +int f3 (void) __attribute__((patchable_function_entry(2)));
> +
> +/* F3 should now get 2 NOPs.  */
> +int
> +__attribute__((noinline))
> +f3 (void)
> +{
> +  return 5*a;
> +}
> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-default.c b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
> new file mode 100644
> index 00000000000..0dcf1181dde
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
> +/* { dg-final { scan-assembler-times "nop" 3 } } */
> +
> +extern int a;
> +
> +/* Nothing declared must not mean anything.  */
> +int f3 (void);
> +
> +/* F3 should get a default-sized NOP area.  */
> +int
> +__attribute__((noinline))
> +f3 (void)
> +{
> +  return 5*a;
> +}
> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
> new file mode 100644
> index 00000000000..a007867dcb0
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
> +/* { dg-final { scan-assembler-times "nop" 1 } } */
> +
> +extern int a;
> +
> +int f3 (void);
> +
> +/* F3 should now get 1 NOP.  */
> +int
> +__attribute__((noinline))
> +__attribute__((patchable_function_entry(1)))
> +f3 (void)
> +{
> +  return 5*a;
> +}
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index f1384fc2fda..84969fb8ef4 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -1612,8 +1612,10 @@ process_options (void)
>      }
>  
>   /* Do not use IPA optimizations for register allocation if profiler is active
> +    or patchable function entries are inserted for run-time instrumentation
>      or port does not emit prologue and epilogue as RTL.  */
> -  if (profile_flag || !targetm.have_prologue () || !targetm.have_epilogue ())
> +  if (profile_flag || function_entry_patch_area_size
> +      || !targetm.have_prologue () || !targetm.have_epilogue ())
>      flag_ipa_ra = 0;
>  
>    /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index 05e48a5b894..6c3a56419e8 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -1830,6 +1830,46 @@ 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);
> +
> +      if (tree_fits_uhwi_p (patchable_function_entry_value1))
> +	patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
> +      else
> +	gcc_unreachable ();
> +
> +      patch_area_entry = 0;
> +      if (list_length (pp_val) > 1)
> +	{
> +	  tree patchable_function_entry_value2 =
> +	    TREE_VALUE (TREE_CHAIN (pp_val));
> +
> +	  if (tree_fits_uhwi_p (patchable_function_entry_value2))
> +	    patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
> +	  else
> +	    gcc_unreachable ();
> +	}
> +    }
> +
> +  if (patch_area_entry > patch_area_size)
> +    {
> +      if (patch_area_size > 0)
> +	warning (OPT_Wattributes, "Patchable function entry > size");
> +      patch_area_entry = 0;
> +    }
> +
> +  /* Emit the patching area before the entry label, if any.  */
> +  if (patch_area_entry > 0)
> +    targetm.asm_out.print_patchable_function_entry (asm_out_file,
> +						    patch_area_entry, true);
> +
>    /* Do any machine/system dependent processing of the function name.  */
>  #ifdef ASM_DECLARE_FUNCTION_NAME
>    ASM_DECLARE_FUNCTION_NAME (asm_out_file, fnname, current_function_decl);
> @@ -1838,6 +1878,12 @@ assemble_start_function (tree decl, const char *fnname)
>    ASM_OUTPUT_FUNCTION_LABEL (asm_out_file, fnname, current_function_decl);
>  #endif /* ASM_DECLARE_FUNCTION_NAME */
>  
> +  /* And the area after the label.  Record it if we haven't done so yet.  */
> +  if (patch_area_size > patch_area_entry)
> +    targetm.asm_out.print_patchable_function_entry (asm_out_file,
> +					     patch_area_size-patch_area_entry,
> +						    patch_area_entry == 0);
> +
>    if (lookup_attribute ("no_split_stack", DECL_ATTRIBUTES (decl)))
>      saw_no_split_stack = true;
>  }
> 

Re: [PATCH v9] add -fpatchable-function-entry=N,M option

[Torsten Duwe]

On Tue, Jul 04, 2017 at 12:02:47PM +0100, Richard Earnshaw (lists) wrote:
> On 13/06/17 18:00, Torsten Duwe wrote:
> > Changes since v8:
> > 
> >   * Documentation changes as requested by Sandra
> >   * 3 functional test cases added
> > 
> > 	Torsten
> > 
> > 
> > gcc/c-family/ChangeLog
> > 2017-06-13  Torsten Duwe  <duwe@suse.de>
> > 
> > 	* c-attribs.c (c_common_attribute_table): Add entry for
> > 	"patchable_function_entry".
> > 
> > gcc/lto/ChangeLog
> > 2017-06-13  Torsten Duwe  <duwe@suse.de>
> > 
> > 	* lto-lang.c (lto_attribute_table): Add entry for
> > 	"patchable_function_entry".
> > 
> > gcc/ChangeLog
> > 2017-06-13  Torsten Duwe  <duwe@suse.de>
> > 
> > 	* common.opt: Introduce -fpatchable-function-entry
> > 	command line option, and its variables function_entry_patch_area_size
> > 	and function_entry_patch_area_start.
> > 	* opts.c (common_handle_option): Add -fpatchable_function_entry_ case,
> > 	including a two-value parser.
> > 	* target.def (print_patchable_function_entry): New target hook.
> > 	* targhooks.h (default_print_patchable_function_entry): New function.
> > 	* targhooks.c (default_print_patchable_function_entry): Likewise.
> > 	* toplev.c (process_options): Switch off IPA-RA if
> > 	patchable function entries are being generated.
> > 	* varasm.c (assemble_start_function): Look at the
> > 	patchable-function-entry command line switch and current
> > 	function attributes and maybe generate NOP instructions by
> > 	calling the print_patchable_function_entry hook.
> > 	* doc/extend.texi: Document patchable_function_entry attribute.
> > 	* doc/invoke.texi: Document -fpatchable_function_entry
> > 	command line option.
> > 	* doc/tm.texi.in (TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY):
> > 	New target hook.
> > 	* doc/tm.texi: Likewise.
> > 
> > gcc/testsuite/ChangeLog
> > 2017-06-13  Torsten Duwe  <duwe@suse.de>
> > 
> > 	* c-c++-common/patchable_function_entry-default.c: New test.
> > 	* c-c++-common/patchable_function_entry-decl.c: Likewise.
> > 	* c-c++-common/patchable_function_entry-definition.c: Likewise.
> 
> I think we're nearly there with this one, but there are a couple of nits
> still to sort out.
> 
> I can't see anything in the patch to deal with targets that set
> TARGET_HAVE_NAMED_SECTIONS to false.  We'll probably ICE in that case
> since the compiler will be unable to record the location of the patch
> region in the explicitly named section.  I think the correct thing to do
> there is to reject the option during option validation when we can't
> support it properly.

How about omitting the recording step and document that? This way the
instrumentation can still be useful on e.g. a.out format; the framework
would then have to check around each function entry whether the NOPs are
there.

> The template NOP in default_print_patchable_function_entry needs to be
> added as a GC root to prevent it being discarded during garbage collection.

Are you sure? For the nop_templ I'm only digging my way to the platform-
dependent constant string, which I find amongst the constant strings in
.rodata (or .text, for platforms without named sections ;-).
Is the memory region pointed to by nop_templ really endangered?

> There are a couple of documentation tweaks, which I think help make the
> text a little more comprehensible.  See inline.

Ack for those, in toto. I wrote this stuff, and am actively using it.
I find it sort of hard to imagine to have 0 knowledge about it, and phrase
documentation accordingly. I'd add the half-sentence for non named sections,
see below.

> R.
> 
> > 
> > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> > index f2a88e147ba..31137ce0433 100644
> > --- a/gcc/c-family/c-attribs.c
> > +++ b/gcc/c-family/c-attribs.c
> > @@ -139,6 +139,8 @@ static tree handle_bnd_variable_size_attribute (tree *, tree, tree, int, bool *)
> >  static tree handle_bnd_legacy (tree *, tree, tree, int, bool *);
> >  static tree handle_bnd_instrument (tree *, tree, tree, int, bool *);
> >  static tree handle_fallthrough_attribute (tree *, tree, tree, int, bool *);
> > +static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
> > +						       int, bool *);
> >  
> >  /* Table of machine-independent attributes common to all C-like languages.
> >  
> > @@ -345,6 +347,9 @@ const struct attribute_spec c_common_attribute_table[] =
> >  			      handle_bnd_instrument, false },
> >    { "fallthrough",	      0, 0, false, false, false,
> >  			      handle_fallthrough_attribute, false },
> > +  { "patchable_function_entry",	1, 2, true, false, false,
> > +			      handle_patchable_function_entry_attribute,
> > +			      false },
> >    { NULL,                     0, 0, false, false, false, NULL, false }
> >  };
> >  
> > @@ -3173,3 +3178,10 @@ handle_fallthrough_attribute (tree *, tree name, tree, int,
> >    *no_add_attrs = true;
> >    return NULL_TREE;
> >  }
> > +
> > +static tree
> > +handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
> > +{
> > +  /* Nothing to be done here.  */
> > +  return NULL_TREE;
> > +}
> > diff --git a/gcc/common.opt b/gcc/common.opt
> > index a5c3aeaa336..f542590650c 100644
> > --- a/gcc/common.opt
> > +++ b/gcc/common.opt
> > @@ -163,6 +163,13 @@ bool flag_stack_usage_info = false
> >  Variable
> >  int flag_debug_asm
> >  
> > +; How many NOP insns to place at each function entry by default
> > +Variable
> > +HOST_WIDE_INT function_entry_patch_area_size
> > +
> > +; And how far the real asm entry point is into this area
> > +Variable
> > +HOST_WIDE_INT function_entry_patch_area_start
> >  
> >  ; Balance between GNAT encodings and standard DWARF to emit.
> >  Variable
> > @@ -2022,6 +2029,10 @@ fprofile-reorder-functions
> >  Common Report Var(flag_profile_reorder_functions)
> >  Enable function reordering that improves code placement.
> >  
> > +fpatchable-function-entry=
> > +Common Joined Optimization
> > +Insert NOP instructions at each function entry.
> > +
> >  frandom-seed
> >  Common Var(common_deferred_options) Defer
> >  
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> > index 1255995eb78..d09ccd90c42 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -3083,6 +3083,25 @@ that affect more than one function.
> >  This attribute should be used for debugging purposes only.  It is not
> >  suitable in production code.
> >  
> > +@item patchable_function_entry
> > +@cindex @code{patchable_function_entry} function attribute
> > +@cindex extra NOP instructions at the function entry point
> > +In case the target's text segment can be made writable at run time by
> > +any means, padding the function entry with a number of NOPs can be
> > +used to provide a universal tool for instrumentation.  
> 
> 
> > +Usually,
> > +patchable function entries are enabled globally using the
> > +@option{-fpatchable-function-entry=N,M} command-line switch, and
> > +disabled with attribute @code{patchable_function_entry (0)} for
> > +functions that are part of the actual instrumentation framework.
> > +This conveniently avoids an endless recursion.
> 
> I think it would be better to use the imperative here.  Perhaps
> something like:
> 
> If patchable function entries are enabled globally using the command
> line option @option{-fpatchable-function-entry=N,M}, then all functions
> that are part of the instrumentation framework must disable
> instrumentation with the attribute @code{patchable_function_entry (0)}
> to prevent recursion.
> 
> However, I'd be inclined to put that in a separate paragraph below
> following text.
> 
> > +The @code{patchable_function_entry} function attribute can be used to
> > +change the number of NOPs to any desired value.  The two-value syntax
> > +is the same as for the command-line switch
> > +@option{-fpatchable-function-entry=N,M}, generating @var{N} NOPs, with
> > +the function entry point before the @var{M}th NOP instruction.
> > +@var{M} defaults to 0 if omitted e.g. function entry point is before
> > +the first NOP.
> > +
> >  @item pure
> >  @cindex @code{pure} function attribute
> >  @cindex functions that have no side effects
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index 68a558e9992..2e08387ad40 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -11384,6 +11384,34 @@ of the function name, it is considered to be a match.  For C99 and C++
> >  extended identifiers, the function name must be given in UTF-8, not
> >  using universal character names.
> >  
> > +@item -fpatchable-function-entry=@var{N}[,@var{M}]
> > +@opindex fpatchable-function-entry
> > +Generate @var{N} NOPs right at the beginning
> > +of each function, with the function entry point before the @var{M}th NOP.
> > +If @var{M} is omitted, it defaults to @code{0} so the
> > +function entry points to the address just at the first NOP.
> > +The NOP instructions reserve extra space which can be used to patch in
> > +any desired instrumentation at run time, provided that the code segment
> > +is writable.  The amount of space is controllable indirectly via
> > +the number of NOPs; the NOP instruction used corresponds to the instruction
> > +emitted by the internal GCC back-end interface @code{gen_nop}.  This behavior
> > +is target-specific and may also depend on the architecture variant and/or
> > +other compilation options.
> > +
> > +For run-time identification, the starting addresses of these areas,
> > +which correspond to their respective function entries minus @var{M},
> > +are additionally collected in the @code{__patchable_function_entries}
> > +section of the resulting binary

, if the target platform supports named sections.
(or the other way, for clarity: If the target..., ... the starting addrs are
collected...)

> > +
> > +Note that the value of @code{__attribute__ ((patchable_function_entry
> > +(N,M)))} takes precedence over command-line option
> > +@option{-fpatchable-function-entry=N,M}.  This can be used to increase
> > +the area size or to remove it completely on a single function.
> > +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.
> > +
> >  @end table
> >  
> >  
> > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> > index c4f2c893c8e..0e5e20904af 100644
> > --- a/gcc/doc/tm.texi
> > +++ b/gcc/doc/tm.texi
> > @@ -4566,6 +4566,12 @@ will select the smallest suitable mode.
> >  This section describes the macros that output function entry
> >  (@dfn{prologue}) and exit (@dfn{epilogue}) code.
> >  
> > +@deftypefn {Target Hook} void TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY (FILE *@var{file}, unsigned HOST_WIDE_INT @var{patch_area_size}, bool @var{record_p})
> > +Generate a patchable area at a function start, consisting of
> > +@var{patch_area_size} NOP instructions.  Note the current location in
> > +a dedicated segment if @var{record_p} is true.   
> 
> I had to parse that final sentence twice to understand what you meant.
> The problem is that in documentation 'Note' is often used as something
> being pointed out to the reader to be of special importance.  Here, I
> think you'd be better off with
> 
> If @var{record_p} is true insert a pointer the current location in the
> table of patchable functions.
> 
> You then need to describe where that 'table' goes.
> 
> > +@end deftypefn
> > +
> >  @deftypefn {Target Hook} void TARGET_ASM_FUNCTION_PROLOGUE (FILE *@var{file}, HOST_WIDE_INT @var{size})
> >  If defined, a function that outputs the assembler code for entry to a
> >  function.  The prologue is responsible for setting up the stack frame,
> > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> > index 1c471d8da35..53696740f48 100644
> > --- a/gcc/doc/tm.texi.in
> > +++ b/gcc/doc/tm.texi.in
> > @@ -3650,6 +3650,8 @@ will select the smallest suitable mode.
> >  This section describes the macros that output function entry
> >  (@dfn{prologue}) and exit (@dfn{epilogue}) code.
> >  
> > +@hook TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
> > +
> >  @hook TARGET_ASM_FUNCTION_PROLOGUE
> >  
> >  @hook TARGET_ASM_FUNCTION_END_PROLOGUE
> > diff --git a/gcc/lto/lto-lang.c b/gcc/lto/lto-lang.c
> > index 52ab2a8cb81..2312ada48c6 100644
> > --- a/gcc/lto/lto-lang.c
> > +++ b/gcc/lto/lto-lang.c
> > @@ -48,6 +48,8 @@ static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
> >  static tree handle_type_generic_attribute (tree *, tree, tree, int, bool *);
> >  static tree handle_transaction_pure_attribute (tree *, tree, tree, int, bool *);
> >  static tree handle_returns_twice_attribute (tree *, tree, tree, int, bool *);
> > +static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
> > +						       int, bool *);
> >  static tree ignore_attribute (tree *, tree, tree, int, bool *);
> >  
> >  static tree handle_format_attribute (tree *, tree, tree, int, bool *);
> > @@ -76,6 +78,9 @@ const struct attribute_spec lto_attribute_table[] =
> >  			      handle_nonnull_attribute, false },
> >    { "nothrow",                0, 0, true,  false, false,
> >  			      handle_nothrow_attribute, false },
> > +  { "patchable_function_entry", 1, 2, true, false, false,
> > +			      handle_patchable_function_entry_attribute,
> > +			      false },
> >    { "returns_twice",          0, 0, true,  false, false,
> >  			      handle_returns_twice_attribute, false },
> >    { "sentinel",               0, 1, false, true, true,
> > @@ -473,6 +478,13 @@ handle_returns_twice_attribute (tree *node, tree ARG_UNUSED (name),
> >    return NULL_TREE;
> >  }
> >  
> > +static tree
> > +handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
> > +{
> > +  /* Nothing to be done here.  */
> > +  return NULL_TREE;
> > +}
> > +
> >  /* Ignore the given attribute.  Used when this attribute may be usefully
> >     overridden by the target, but is not used generically.  */
> >  
> > diff --git a/gcc/opts.c b/gcc/opts.c
> > index ffedb10f18f..f240c01f7d9 100644
> > --- a/gcc/opts.c
> > +++ b/gcc/opts.c
> > @@ -2155,6 +2155,33 @@ common_handle_option (struct gcc_options *opts,
> >          opts->x_flag_ipa_reference = false;
> >        break;
> >  
> > +    case OPT_fpatchable_function_entry_:
> > +      {
> > +	char *patch_area_arg = xstrdup (arg);
> > +	char *comma = strchr (patch_area_arg, ',');
> > +	if (comma)
> > +	  {
> > +	    *comma = '\0';
> > +	    function_entry_patch_area_size = 
> > +	      integral_argument (patch_area_arg);
> > +	    function_entry_patch_area_start =
> > +	      integral_argument (comma + 1);
> > +	  }
> > +	else
> > +	  {
> > +	    function_entry_patch_area_size =
> > +	      integral_argument (patch_area_arg);
> > +	    function_entry_patch_area_start = 0;
> > +	  }
> > +	if (function_entry_patch_area_size < 0
> > +	    || function_entry_patch_area_start < 0
> > +	    || function_entry_patch_area_size 
> > +		< function_entry_patch_area_start)
> > +	  error ("invalid arguments for %<-fpatchable_function_entry%>");
> > +	free (patch_area_arg);
> > +      }
> > +      break;
> > +
> >      case OPT_ftree_vectorize:
> >        if (!opts_set->x_flag_tree_loop_vectorize)
> >          opts->x_flag_tree_loop_vectorize = value;
> > diff --git a/gcc/target.def b/gcc/target.def
> > index 6bebfd5b9d6..53b60f3d276 100644
> > --- a/gcc/target.def
> > +++ b/gcc/target.def
> > @@ -288,6 +288,12 @@ hidden, protected or internal visibility as specified by @var{visibility}.",
> >   void, (tree decl, int visibility),
> >   default_assemble_visibility)
> >  
> > +DEFHOOK
> > +(print_patchable_function_entry,
> > + "Generate a patchable area at the function start",
> > + void, (FILE *file, unsigned HOST_WIDE_INT patch_area_size, bool record_p),
> > + default_print_patchable_function_entry)
> > +
> >  /* Output the assembler code for entry to a function.  */
> >  DEFHOOK
> >  (function_prologue,
> > diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> > index 1cdec068ed8..f3d9edafafe 100644
> > --- a/gcc/targhooks.c
> > +++ b/gcc/targhooks.c
> > @@ -1609,6 +1609,54 @@ default_compare_by_pieces_branch_ratio (machine_mode)
> >    return 1;
> >  }
> >  
> > +/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function
> > +   entry.  If RECORD_P is true, the location of the NOPs will be
> > +   recorded in a special object section called "__patchable_function_entries".

, if the target...

> > +   This routine may be called twice per function to put NOPs before
> > +   and after the function entry.  */
> > +
> > +void
> > +default_print_patchable_function_entry (FILE *file,
> > +					unsigned HOST_WIDE_INT patch_area_size,
> > +					bool record_p)
> > +{
> > +  static const char *nop_templ = 0;
> 
> You need to record this pointer as a GC root.  Otherwise garbage
> collection might destroy your template NOP.
> 
> > +
> > +  /* We use the template alone, relying on the (currently sane) assumption
> > +     that the NOP template does not have variable operands.  */
> > +  if (!nop_templ)
> > +    {
> > +      int code_num;
> > +      rtx_insn *my_nop = make_insn_raw (gen_nop ());
> > +
> > +      code_num = recog_memoized (my_nop);
> > +      nop_templ = get_insn_template (code_num, my_nop);
> > +    }
> > +

#if TARGET_HAVE_NAMED_SECTIONS
> > +  if (record_p)
> > +    {
> > +      char buf[256];
> > +      static int patch_area_number;
> > +      section *previous_section = in_section;
> > +
> > +      patch_area_number++;
> > +      ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number);
> > +
> > +      switch_to_section (get_section ("__patchable_function_entries",
> > +				      0, NULL));
> > +      fputs (integer_asm_op (POINTER_SIZE_UNITS, false), file);
> > +      assemble_name_raw (file, buf);
> > +      fputc ('\n', file);
> > +
> > +      switch_to_section (previous_section);
> > +      ASM_OUTPUT_LABEL (file, buf);
> > +    }
#endif

	?

> > +
> > +  unsigned i;
> > +  for (i = 0; i < patch_area_size; ++i)
> > +    fprintf (file, "\t%s\n", nop_templ);
> > +}
> > +
> >  bool
> >  default_profile_before_prologue (void)
> >  {
> > diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> > index 18070df7839..6206fe20823 100644
> > --- a/gcc/targhooks.h
> > +++ b/gcc/targhooks.h
> > @@ -203,6 +203,9 @@ extern bool default_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT,
> >  						    bool);
> >  extern int default_compare_by_pieces_branch_ratio (machine_mode);
> >  
> > +extern void default_print_patchable_function_entry (FILE *,
> > +						    unsigned HOST_WIDE_INT,
> > +						    bool);
> >  extern bool default_profile_before_prologue (void);
> >  extern reg_class_t default_preferred_reload_class (rtx, reg_class_t);
> >  extern reg_class_t default_preferred_output_reload_class (rtx, reg_class_t);
> > diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
> > new file mode 100644
> > index 00000000000..8514b10e820
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
> > @@ -0,0 +1,16 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
> > +/* { dg-final { scan-assembler-times "nop" 2 } } */
> > +
> > +extern int a;
> > +
> > +/* Respect overriding attributes in the declaration.  */
> > +int f3 (void) __attribute__((patchable_function_entry(2)));
> > +
> > +/* F3 should now get 2 NOPs.  */
> > +int
> > +__attribute__((noinline))
> > +f3 (void)
> > +{
> > +  return 5*a;
> > +}
> > diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-default.c b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
> > new file mode 100644
> > index 00000000000..0dcf1181dde
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
> > @@ -0,0 +1,16 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
> > +/* { dg-final { scan-assembler-times "nop" 3 } } */
> > +
> > +extern int a;
> > +
> > +/* Nothing declared must not mean anything.  */
> > +int f3 (void);
> > +
> > +/* F3 should get a default-sized NOP area.  */
> > +int
> > +__attribute__((noinline))
> > +f3 (void)
> > +{
> > +  return 5*a;
> > +}
> > diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
> > new file mode 100644
> > index 00000000000..a007867dcb0
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
> > @@ -0,0 +1,16 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
> > +/* { dg-final { scan-assembler-times "nop" 1 } } */
> > +
> > +extern int a;
> > +
> > +int f3 (void);
> > +
> > +/* F3 should now get 1 NOP.  */
> > +int
> > +__attribute__((noinline))
> > +__attribute__((patchable_function_entry(1)))
> > +f3 (void)
> > +{
> > +  return 5*a;
> > +}
> > diff --git a/gcc/toplev.c b/gcc/toplev.c
> > index f1384fc2fda..84969fb8ef4 100644
> > --- a/gcc/toplev.c
> > +++ b/gcc/toplev.c
> > @@ -1612,8 +1612,10 @@ process_options (void)
> >      }
> >  
> >   /* Do not use IPA optimizations for register allocation if profiler is active
> > +    or patchable function entries are inserted for run-time instrumentation
> >      or port does not emit prologue and epilogue as RTL.  */
> > -  if (profile_flag || !targetm.have_prologue () || !targetm.have_epilogue ())
> > +  if (profile_flag || function_entry_patch_area_size
> > +      || !targetm.have_prologue () || !targetm.have_epilogue ())
> >      flag_ipa_ra = 0;
> >  
> >    /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
> > diff --git a/gcc/varasm.c b/gcc/varasm.c
> > index 05e48a5b894..6c3a56419e8 100644
> > --- a/gcc/varasm.c
> > +++ b/gcc/varasm.c
> > @@ -1830,6 +1830,46 @@ 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);
> > +
> > +      if (tree_fits_uhwi_p (patchable_function_entry_value1))
> > +	patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
> > +      else
> > +	gcc_unreachable ();
> > +
> > +      patch_area_entry = 0;
> > +      if (list_length (pp_val) > 1)
> > +	{
> > +	  tree patchable_function_entry_value2 =
> > +	    TREE_VALUE (TREE_CHAIN (pp_val));
> > +
> > +	  if (tree_fits_uhwi_p (patchable_function_entry_value2))
> > +	    patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
> > +	  else
> > +	    gcc_unreachable ();
> > +	}
> > +    }
> > +
> > +  if (patch_area_entry > patch_area_size)
> > +    {
> > +      if (patch_area_size > 0)
> > +	warning (OPT_Wattributes, "Patchable function entry > size");
> > +      patch_area_entry = 0;
> > +    }
> > +
> > +  /* Emit the patching area before the entry label, if any.  */
> > +  if (patch_area_entry > 0)
> > +    targetm.asm_out.print_patchable_function_entry (asm_out_file,
> > +						    patch_area_entry, true);
> > +
> >    /* Do any machine/system dependent processing of the function name.  */
> >  #ifdef ASM_DECLARE_FUNCTION_NAME
> >    ASM_DECLARE_FUNCTION_NAME (asm_out_file, fnname, current_function_decl);
> > @@ -1838,6 +1878,12 @@ assemble_start_function (tree decl, const char *fnname)
> >    ASM_OUTPUT_FUNCTION_LABEL (asm_out_file, fnname, current_function_decl);
> >  #endif /* ASM_DECLARE_FUNCTION_NAME */
> >  
> > +  /* And the area after the label.  Record it if we haven't done so yet.  */
> > +  if (patch_area_size > patch_area_entry)
> > +    targetm.asm_out.print_patchable_function_entry (asm_out_file,
> > +					     patch_area_size-patch_area_entry,
> > +						    patch_area_entry == 0);
> > +
> >    if (lookup_attribute ("no_split_stack", DECL_ATTRIBUTES (decl)))
> >      saw_no_split_stack = true;
> >  }
> > 
> 

Re: [PATCH v9] add -fpatchable-function-entry=N,M option

[Richard Earnshaw (lists)]

On 04/07/17 14:14, Torsten Duwe wrote:
> On Tue, Jul 04, 2017 at 12:02:47PM +0100, Richard Earnshaw (lists) wrote:
>> On 13/06/17 18:00, Torsten Duwe wrote:
>>> Changes since v8:
>>>
>>>   * Documentation changes as requested by Sandra
>>>   * 3 functional test cases added
>>>
>>> 	Torsten
>>>
>>>
>>> gcc/c-family/ChangeLog
>>> 2017-06-13  Torsten Duwe  <duwe@suse.de>
>>>
>>> 	* c-attribs.c (c_common_attribute_table): Add entry for
>>> 	"patchable_function_entry".
>>>
>>> gcc/lto/ChangeLog
>>> 2017-06-13  Torsten Duwe  <duwe@suse.de>
>>>
>>> 	* lto-lang.c (lto_attribute_table): Add entry for
>>> 	"patchable_function_entry".
>>>
>>> gcc/ChangeLog
>>> 2017-06-13  Torsten Duwe  <duwe@suse.de>
>>>
>>> 	* common.opt: Introduce -fpatchable-function-entry
>>> 	command line option, and its variables function_entry_patch_area_size
>>> 	and function_entry_patch_area_start.
>>> 	* opts.c (common_handle_option): Add -fpatchable_function_entry_ case,
>>> 	including a two-value parser.
>>> 	* target.def (print_patchable_function_entry): New target hook.
>>> 	* targhooks.h (default_print_patchable_function_entry): New function.
>>> 	* targhooks.c (default_print_patchable_function_entry): Likewise.
>>> 	* toplev.c (process_options): Switch off IPA-RA if
>>> 	patchable function entries are being generated.
>>> 	* varasm.c (assemble_start_function): Look at the
>>> 	patchable-function-entry command line switch and current
>>> 	function attributes and maybe generate NOP instructions by
>>> 	calling the print_patchable_function_entry hook.
>>> 	* doc/extend.texi: Document patchable_function_entry attribute.
>>> 	* doc/invoke.texi: Document -fpatchable_function_entry
>>> 	command line option.
>>> 	* doc/tm.texi.in (TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY):
>>> 	New target hook.
>>> 	* doc/tm.texi: Likewise.
>>>
>>> gcc/testsuite/ChangeLog
>>> 2017-06-13  Torsten Duwe  <duwe@suse.de>
>>>
>>> 	* c-c++-common/patchable_function_entry-default.c: New test.
>>> 	* c-c++-common/patchable_function_entry-decl.c: Likewise.
>>> 	* c-c++-common/patchable_function_entry-definition.c: Likewise.
>>
>> I think we're nearly there with this one, but there are a couple of nits
>> still to sort out.
>>
>> I can't see anything in the patch to deal with targets that set
>> TARGET_HAVE_NAMED_SECTIONS to false.  We'll probably ICE in that case
>> since the compiler will be unable to record the location of the patch
>> region in the explicitly named section.  I think the correct thing to do
>> there is to reject the option during option validation when we can't
>> support it properly.
> 
> How about omitting the recording step and document that? This way the
> instrumentation can still be useful on e.g. a.out format; the framework
> would then have to check around each function entry whether the NOPs are
> there.
> 

I'm happy with that if the result is still something useful.  Frankly, I
don't think GCC has many targets now that don't support named sections,
so I wouldn't loose too much sleep over being unable to support this
feature on such targets.  However, if the implementation is straight
forward then doing what you suggest is fine.


>> The template NOP in default_print_patchable_function_entry needs to be
>> added as a GC root to prevent it being discarded during garbage collection.
> 
> Are you sure? For the nop_templ I'm only digging my way to the platform-
> dependent constant string, which I find amongst the constant strings in
> .rodata (or .text, for platforms without named sections ;-).
> Is the memory region pointed to by nop_templ really endangered?
> 

I'd missed that this was trying to recover a string.  That's probably OK
from a GC perspective then, but it isn't safe to hold this between
functions since the nop used for one function might not be the same when
we come to the next.  Consider, for example, the ARM port where we can
switch from generating ARM code to generating Thumb code: it might work
with the current implementation, but this isn't guaranteed to be the case.

>> There are a couple of documentation tweaks, which I think help make the
>> text a little more comprehensible.  See inline.
> 
> Ack for those, in toto. I wrote this stuff, and am actively using it.
> I find it sort of hard to imagine to have 0 knowledge about it, and phrase
> documentation accordingly. I'd add the half-sentence for non named sections,
> see below.
> 
>> R.
>>
>>>
>>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>>> index f2a88e147ba..31137ce0433 100644
>>> --- a/gcc/c-family/c-attribs.c
>>> +++ b/gcc/c-family/c-attribs.c
>>> @@ -139,6 +139,8 @@ static tree handle_bnd_variable_size_attribute (tree *, tree, tree, int, bool *)
>>>  static tree handle_bnd_legacy (tree *, tree, tree, int, bool *);
>>>  static tree handle_bnd_instrument (tree *, tree, tree, int, bool *);
>>>  static tree handle_fallthrough_attribute (tree *, tree, tree, int, bool *);
>>> +static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
>>> +						       int, bool *);
>>>  
>>>  /* Table of machine-independent attributes common to all C-like languages.
>>>  
>>> @@ -345,6 +347,9 @@ const struct attribute_spec c_common_attribute_table[] =
>>>  			      handle_bnd_instrument, false },
>>>    { "fallthrough",	      0, 0, false, false, false,
>>>  			      handle_fallthrough_attribute, false },
>>> +  { "patchable_function_entry",	1, 2, true, false, false,
>>> +			      handle_patchable_function_entry_attribute,
>>> +			      false },
>>>    { NULL,                     0, 0, false, false, false, NULL, false }
>>>  };
>>>  
>>> @@ -3173,3 +3178,10 @@ handle_fallthrough_attribute (tree *, tree name, tree, int,
>>>    *no_add_attrs = true;
>>>    return NULL_TREE;
>>>  }
>>> +
>>> +static tree
>>> +handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
>>> +{
>>> +  /* Nothing to be done here.  */
>>> +  return NULL_TREE;
>>> +}
>>> diff --git a/gcc/common.opt b/gcc/common.opt
>>> index a5c3aeaa336..f542590650c 100644
>>> --- a/gcc/common.opt
>>> +++ b/gcc/common.opt
>>> @@ -163,6 +163,13 @@ bool flag_stack_usage_info = false
>>>  Variable
>>>  int flag_debug_asm
>>>  
>>> +; How many NOP insns to place at each function entry by default
>>> +Variable
>>> +HOST_WIDE_INT function_entry_patch_area_size
>>> +
>>> +; And how far the real asm entry point is into this area
>>> +Variable
>>> +HOST_WIDE_INT function_entry_patch_area_start
>>>  
>>>  ; Balance between GNAT encodings and standard DWARF to emit.
>>>  Variable
>>> @@ -2022,6 +2029,10 @@ fprofile-reorder-functions
>>>  Common Report Var(flag_profile_reorder_functions)
>>>  Enable function reordering that improves code placement.
>>>  
>>> +fpatchable-function-entry=
>>> +Common Joined Optimization
>>> +Insert NOP instructions at each function entry.
>>> +
>>>  frandom-seed
>>>  Common Var(common_deferred_options) Defer
>>>  
>>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>>> index 1255995eb78..d09ccd90c42 100644
>>> --- a/gcc/doc/extend.texi
>>> +++ b/gcc/doc/extend.texi
>>> @@ -3083,6 +3083,25 @@ that affect more than one function.
>>>  This attribute should be used for debugging purposes only.  It is not
>>>  suitable in production code.
>>>  
>>> +@item patchable_function_entry
>>> +@cindex @code{patchable_function_entry} function attribute
>>> +@cindex extra NOP instructions at the function entry point
>>> +In case the target's text segment can be made writable at run time by
>>> +any means, padding the function entry with a number of NOPs can be
>>> +used to provide a universal tool for instrumentation.  
>>
>>
>>> +Usually,
>>> +patchable function entries are enabled globally using the
>>> +@option{-fpatchable-function-entry=N,M} command-line switch, and
>>> +disabled with attribute @code{patchable_function_entry (0)} for
>>> +functions that are part of the actual instrumentation framework.
>>> +This conveniently avoids an endless recursion.
>>
>> I think it would be better to use the imperative here.  Perhaps
>> something like:
>>
>> If patchable function entries are enabled globally using the command
>> line option @option{-fpatchable-function-entry=N,M}, then all functions
>> that are part of the instrumentation framework must disable
>> instrumentation with the attribute @code{patchable_function_entry (0)}
>> to prevent recursion.
>>
>> However, I'd be inclined to put that in a separate paragraph below
>> following text.
>>
>>> +The @code{patchable_function_entry} function attribute can be used to
>>> +change the number of NOPs to any desired value.  The two-value syntax
>>> +is the same as for the command-line switch
>>> +@option{-fpatchable-function-entry=N,M}, generating @var{N} NOPs, with
>>> +the function entry point before the @var{M}th NOP instruction.
>>> +@var{M} defaults to 0 if omitted e.g. function entry point is before
>>> +the first NOP.
>>> +
>>>  @item pure
>>>  @cindex @code{pure} function attribute
>>>  @cindex functions that have no side effects
>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>>> index 68a558e9992..2e08387ad40 100644
>>> --- a/gcc/doc/invoke.texi
>>> +++ b/gcc/doc/invoke.texi
>>> @@ -11384,6 +11384,34 @@ of the function name, it is considered to be a match.  For C99 and C++
>>>  extended identifiers, the function name must be given in UTF-8, not
>>>  using universal character names.
>>>  
>>> +@item -fpatchable-function-entry=@var{N}[,@var{M}]
>>> +@opindex fpatchable-function-entry
>>> +Generate @var{N} NOPs right at the beginning
>>> +of each function, with the function entry point before the @var{M}th NOP.
>>> +If @var{M} is omitted, it defaults to @code{0} so the
>>> +function entry points to the address just at the first NOP.
>>> +The NOP instructions reserve extra space which can be used to patch in
>>> +any desired instrumentation at run time, provided that the code segment
>>> +is writable.  The amount of space is controllable indirectly via
>>> +the number of NOPs; the NOP instruction used corresponds to the instruction
>>> +emitted by the internal GCC back-end interface @code{gen_nop}.  This behavior
>>> +is target-specific and may also depend on the architecture variant and/or
>>> +other compilation options.
>>> +
>>> +For run-time identification, the starting addresses of these areas,
>>> +which correspond to their respective function entries minus @var{M},
>>> +are additionally collected in the @code{__patchable_function_entries}
>>> +section of the resulting binary
> 
> , if the target platform supports named sections.
> (or the other way, for clarity: If the target..., ... the starting addrs are
> collected...)
> 

Obviously this will need some tweaking if you change things to support
object formats with different capabilities.

>>> +
>>> +Note that the value of @code{__attribute__ ((patchable_function_entry
>>> +(N,M)))} takes precedence over command-line option
>>> +@option{-fpatchable-function-entry=N,M}.  This can be used to increase
>>> +the area size or to remove it completely on a single function.
>>> +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.
>>> +
>>>  @end table
>>>  
>>>  
>>> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>>> index c4f2c893c8e..0e5e20904af 100644
>>> --- a/gcc/doc/tm.texi
>>> +++ b/gcc/doc/tm.texi
>>> @@ -4566,6 +4566,12 @@ will select the smallest suitable mode.
>>>  This section describes the macros that output function entry
>>>  (@dfn{prologue}) and exit (@dfn{epilogue}) code.
>>>  
>>> +@deftypefn {Target Hook} void TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY (FILE *@var{file}, unsigned HOST_WIDE_INT @var{patch_area_size}, bool @var{record_p})
>>> +Generate a patchable area at a function start, consisting of
>>> +@var{patch_area_size} NOP instructions.  Note the current location in
>>> +a dedicated segment if @var{record_p} is true.   
>>
>> I had to parse that final sentence twice to understand what you meant.
>> The problem is that in documentation 'Note' is often used as something
>> being pointed out to the reader to be of special importance.  Here, I
>> think you'd be better off with
>>
>> If @var{record_p} is true insert a pointer the current location in the
>> table of patchable functions.
>>
>> You then need to describe where that 'table' goes.
>>
>>> +@end deftypefn
>>> +
>>>  @deftypefn {Target Hook} void TARGET_ASM_FUNCTION_PROLOGUE (FILE *@var{file}, HOST_WIDE_INT @var{size})
>>>  If defined, a function that outputs the assembler code for entry to a
>>>  function.  The prologue is responsible for setting up the stack frame,
>>> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
>>> index 1c471d8da35..53696740f48 100644
>>> --- a/gcc/doc/tm.texi.in
>>> +++ b/gcc/doc/tm.texi.in
>>> @@ -3650,6 +3650,8 @@ will select the smallest suitable mode.
>>>  This section describes the macros that output function entry
>>>  (@dfn{prologue}) and exit (@dfn{epilogue}) code.
>>>  
>>> +@hook TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
>>> +
>>>  @hook TARGET_ASM_FUNCTION_PROLOGUE
>>>  
>>>  @hook TARGET_ASM_FUNCTION_END_PROLOGUE
>>> diff --git a/gcc/lto/lto-lang.c b/gcc/lto/lto-lang.c
>>> index 52ab2a8cb81..2312ada48c6 100644
>>> --- a/gcc/lto/lto-lang.c
>>> +++ b/gcc/lto/lto-lang.c
>>> @@ -48,6 +48,8 @@ static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
>>>  static tree handle_type_generic_attribute (tree *, tree, tree, int, bool *);
>>>  static tree handle_transaction_pure_attribute (tree *, tree, tree, int, bool *);
>>>  static tree handle_returns_twice_attribute (tree *, tree, tree, int, bool *);
>>> +static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
>>> +						       int, bool *);
>>>  static tree ignore_attribute (tree *, tree, tree, int, bool *);
>>>  
>>>  static tree handle_format_attribute (tree *, tree, tree, int, bool *);
>>> @@ -76,6 +78,9 @@ const struct attribute_spec lto_attribute_table[] =
>>>  			      handle_nonnull_attribute, false },
>>>    { "nothrow",                0, 0, true,  false, false,
>>>  			      handle_nothrow_attribute, false },
>>> +  { "patchable_function_entry", 1, 2, true, false, false,
>>> +			      handle_patchable_function_entry_attribute,
>>> +			      false },
>>>    { "returns_twice",          0, 0, true,  false, false,
>>>  			      handle_returns_twice_attribute, false },
>>>    { "sentinel",               0, 1, false, true, true,
>>> @@ -473,6 +478,13 @@ handle_returns_twice_attribute (tree *node, tree ARG_UNUSED (name),
>>>    return NULL_TREE;
>>>  }
>>>  
>>> +static tree
>>> +handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
>>> +{
>>> +  /* Nothing to be done here.  */
>>> +  return NULL_TREE;
>>> +}
>>> +
>>>  /* Ignore the given attribute.  Used when this attribute may be usefully
>>>     overridden by the target, but is not used generically.  */
>>>  
>>> diff --git a/gcc/opts.c b/gcc/opts.c
>>> index ffedb10f18f..f240c01f7d9 100644
>>> --- a/gcc/opts.c
>>> +++ b/gcc/opts.c
>>> @@ -2155,6 +2155,33 @@ common_handle_option (struct gcc_options *opts,
>>>          opts->x_flag_ipa_reference = false;
>>>        break;
>>>  
>>> +    case OPT_fpatchable_function_entry_:
>>> +      {
>>> +	char *patch_area_arg = xstrdup (arg);
>>> +	char *comma = strchr (patch_area_arg, ',');
>>> +	if (comma)
>>> +	  {
>>> +	    *comma = '\0';
>>> +	    function_entry_patch_area_size = 
>>> +	      integral_argument (patch_area_arg);
>>> +	    function_entry_patch_area_start =
>>> +	      integral_argument (comma + 1);
>>> +	  }
>>> +	else
>>> +	  {
>>> +	    function_entry_patch_area_size =
>>> +	      integral_argument (patch_area_arg);
>>> +	    function_entry_patch_area_start = 0;
>>> +	  }
>>> +	if (function_entry_patch_area_size < 0
>>> +	    || function_entry_patch_area_start < 0
>>> +	    || function_entry_patch_area_size 
>>> +		< function_entry_patch_area_start)
>>> +	  error ("invalid arguments for %<-fpatchable_function_entry%>");
>>> +	free (patch_area_arg);
>>> +      }
>>> +      break;
>>> +
>>>      case OPT_ftree_vectorize:
>>>        if (!opts_set->x_flag_tree_loop_vectorize)
>>>          opts->x_flag_tree_loop_vectorize = value;
>>> diff --git a/gcc/target.def b/gcc/target.def
>>> index 6bebfd5b9d6..53b60f3d276 100644
>>> --- a/gcc/target.def
>>> +++ b/gcc/target.def
>>> @@ -288,6 +288,12 @@ hidden, protected or internal visibility as specified by @var{visibility}.",
>>>   void, (tree decl, int visibility),
>>>   default_assemble_visibility)
>>>  
>>> +DEFHOOK
>>> +(print_patchable_function_entry,
>>> + "Generate a patchable area at the function start",
>>> + void, (FILE *file, unsigned HOST_WIDE_INT patch_area_size, bool record_p),
>>> + default_print_patchable_function_entry)
>>> +
>>>  /* Output the assembler code for entry to a function.  */
>>>  DEFHOOK
>>>  (function_prologue,
>>> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
>>> index 1cdec068ed8..f3d9edafafe 100644
>>> --- a/gcc/targhooks.c
>>> +++ b/gcc/targhooks.c
>>> @@ -1609,6 +1609,54 @@ default_compare_by_pieces_branch_ratio (machine_mode)
>>>    return 1;
>>>  }
>>>  
>>> +/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function
>>> +   entry.  If RECORD_P is true, the location of the NOPs will be
>>> +   recorded in a special object section called "__patchable_function_entries".
> 
> , if the target...
> 
>>> +   This routine may be called twice per function to put NOPs before
>>> +   and after the function entry.  */
>>> +
>>> +void
>>> +default_print_patchable_function_entry (FILE *file,
>>> +					unsigned HOST_WIDE_INT patch_area_size,
>>> +					bool record_p)
>>> +{
>>> +  static const char *nop_templ = 0;
>>
>> You need to record this pointer as a GC root.  Otherwise garbage
>> collection might destroy your template NOP.
>>
>>> +
>>> +  /* We use the template alone, relying on the (currently sane) assumption
>>> +     that the NOP template does not have variable operands.  */
>>> +  if (!nop_templ)
>>> +    {
>>> +      int code_num;
>>> +      rtx_insn *my_nop = make_insn_raw (gen_nop ());
>>> +
>>> +      code_num = recog_memoized (my_nop);
>>> +      nop_templ = get_insn_template (code_num, my_nop);
>>> +    }
>>> +
> 
> #if TARGET_HAVE_NAMED_SECTIONS
>>> +  if (record_p)
>>> +    {
>>> +      char buf[256];
>>> +      static int patch_area_number;
>>> +      section *previous_section = in_section;
>>> +
>>> +      patch_area_number++;
>>> +      ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number);
>>> +
>>> +      switch_to_section (get_section ("__patchable_function_entries",
>>> +				      0, NULL));
>>> +      fputs (integer_asm_op (POINTER_SIZE_UNITS, false), file);
>>> +      assemble_name_raw (file, buf);
>>> +      fputc ('\n', file);
>>> +
>>> +      switch_to_section (previous_section);
>>> +      ASM_OUTPUT_LABEL (file, buf);
>>> +    }
> #endif
> 
> 	?
> 
>>> +
>>> +  unsigned i;
>>> +  for (i = 0; i < patch_area_size; ++i)
>>> +    fprintf (file, "\t%s\n", nop_templ);
>>> +}
>>> +
>>>  bool
>>>  default_profile_before_prologue (void)
>>>  {
>>> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
>>> index 18070df7839..6206fe20823 100644
>>> --- a/gcc/targhooks.h
>>> +++ b/gcc/targhooks.h
>>> @@ -203,6 +203,9 @@ extern bool default_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT,
>>>  						    bool);
>>>  extern int default_compare_by_pieces_branch_ratio (machine_mode);
>>>  
>>> +extern void default_print_patchable_function_entry (FILE *,
>>> +						    unsigned HOST_WIDE_INT,
>>> +						    bool);
>>>  extern bool default_profile_before_prologue (void);
>>>  extern reg_class_t default_preferred_reload_class (rtx, reg_class_t);
>>>  extern reg_class_t default_preferred_output_reload_class (rtx, reg_class_t);
>>> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
>>> new file mode 100644
>>> index 00000000000..8514b10e820
>>> --- /dev/null
>>> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-decl.c
>>> @@ -0,0 +1,16 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
>>> +/* { dg-final { scan-assembler-times "nop" 2 } } */
>>> +
>>> +extern int a;
>>> +
>>> +/* Respect overriding attributes in the declaration.  */
>>> +int f3 (void) __attribute__((patchable_function_entry(2)));
>>> +
>>> +/* F3 should now get 2 NOPs.  */
>>> +int
>>> +__attribute__((noinline))
>>> +f3 (void)
>>> +{
>>> +  return 5*a;
>>> +}
>>> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-default.c b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
>>> new file mode 100644
>>> index 00000000000..0dcf1181dde
>>> --- /dev/null
>>> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c
>>> @@ -0,0 +1,16 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
>>> +/* { dg-final { scan-assembler-times "nop" 3 } } */
>>> +
>>> +extern int a;
>>> +
>>> +/* Nothing declared must not mean anything.  */
>>> +int f3 (void);
>>> +
>>> +/* F3 should get a default-sized NOP area.  */
>>> +int
>>> +__attribute__((noinline))
>>> +f3 (void)
>>> +{
>>> +  return 5*a;
>>> +}
>>> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
>>> new file mode 100644
>>> index 00000000000..a007867dcb0
>>> --- /dev/null
>>> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-definition.c
>>> @@ -0,0 +1,16 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O2 -fpatchable-function-entry=3,1" } */
>>> +/* { dg-final { scan-assembler-times "nop" 1 } } */
>>> +
>>> +extern int a;
>>> +
>>> +int f3 (void);
>>> +
>>> +/* F3 should now get 1 NOP.  */
>>> +int
>>> +__attribute__((noinline))
>>> +__attribute__((patchable_function_entry(1)))
>>> +f3 (void)
>>> +{
>>> +  return 5*a;
>>> +}
>>> diff --git a/gcc/toplev.c b/gcc/toplev.c
>>> index f1384fc2fda..84969fb8ef4 100644
>>> --- a/gcc/toplev.c
>>> +++ b/gcc/toplev.c
>>> @@ -1612,8 +1612,10 @@ process_options (void)
>>>      }
>>>  
>>>   /* Do not use IPA optimizations for register allocation if profiler is active
>>> +    or patchable function entries are inserted for run-time instrumentation
>>>      or port does not emit prologue and epilogue as RTL.  */
>>> -  if (profile_flag || !targetm.have_prologue () || !targetm.have_epilogue ())
>>> +  if (profile_flag || function_entry_patch_area_size
>>> +      || !targetm.have_prologue () || !targetm.have_epilogue ())
>>>      flag_ipa_ra = 0;
>>>  
>>>    /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
>>> diff --git a/gcc/varasm.c b/gcc/varasm.c
>>> index 05e48a5b894..6c3a56419e8 100644
>>> --- a/gcc/varasm.c
>>> +++ b/gcc/varasm.c
>>> @@ -1830,6 +1830,46 @@ 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);
>>> +
>>> +      if (tree_fits_uhwi_p (patchable_function_entry_value1))
>>> +	patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
>>> +      else
>>> +	gcc_unreachable ();
>>> +
>>> +      patch_area_entry = 0;
>>> +      if (list_length (pp_val) > 1)
>>> +	{
>>> +	  tree patchable_function_entry_value2 =
>>> +	    TREE_VALUE (TREE_CHAIN (pp_val));
>>> +
>>> +	  if (tree_fits_uhwi_p (patchable_function_entry_value2))
>>> +	    patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
>>> +	  else
>>> +	    gcc_unreachable ();
>>> +	}
>>> +    }
>>> +
>>> +  if (patch_area_entry > patch_area_size)
>>> +    {
>>> +      if (patch_area_size > 0)
>>> +	warning (OPT_Wattributes, "Patchable function entry > size");
>>> +      patch_area_entry = 0;
>>> +    }
>>> +
>>> +  /* Emit the patching area before the entry label, if any.  */
>>> +  if (patch_area_entry > 0)
>>> +    targetm.asm_out.print_patchable_function_entry (asm_out_file,
>>> +						    patch_area_entry, true);
>>> +
>>>    /* Do any machine/system dependent processing of the function name.  */
>>>  #ifdef ASM_DECLARE_FUNCTION_NAME
>>>    ASM_DECLARE_FUNCTION_NAME (asm_out_file, fnname, current_function_decl);
>>> @@ -1838,6 +1878,12 @@ assemble_start_function (tree decl, const char *fnname)
>>>    ASM_OUTPUT_FUNCTION_LABEL (asm_out_file, fnname, current_function_decl);
>>>  #endif /* ASM_DECLARE_FUNCTION_NAME */
>>>  
>>> +  /* And the area after the label.  Record it if we haven't done so yet.  */
>>> +  if (patch_area_size > patch_area_entry)
>>> +    targetm.asm_out.print_patchable_function_entry (asm_out_file,
>>> +					     patch_area_size-patch_area_entry,
>>> +						    patch_area_entry == 0);
>>> +
>>>    if (lookup_attribute ("no_split_stack", DECL_ATTRIBUTES (decl)))
>>>      saw_no_split_stack = true;
>>>  }
>>>
>>

Re: [PATCH v9] add -fpatchable-function-entry=N,M option

[Michael Matz]

Hello Richard,

On Tue, 4 Jul 2017, Richard Earnshaw (lists) wrote:

> > +void
> > +default_print_patchable_function_entry (FILE *file,
> > +					unsigned HOST_WIDE_INT patch_area_size,
> > +					bool record_p)
> > +{
> > +  static const char *nop_templ = 0;
> 
> You need to record this pointer as a GC root.  Otherwise garbage
> collection might destroy your template NOP.

I don't think so: get_insn_template() should always return strings in 
.rodata, even for output statements, and should never point into GC 
memory.

> > +  /* We use the template alone, relying on the (currently sane) assumption
> > +     that the NOP template does not have variable operands.  */
> > +  if (!nop_templ)
> > +    {
> > +      int code_num;
> > +      rtx_insn *my_nop = make_insn_raw (gen_nop ());
> > +
> > +      code_num = recog_memoized (my_nop);
> > +      nop_templ = get_insn_template (code_num, my_nop);
> > +    }


Ciao,
Michael.

Re: [PATCH v9] add -fpatchable-function-entry=N,M option

[Michael Matz]

Hi,

On Tue, 4 Jul 2017, Michael Matz wrote:

> I don't think so: get_insn_template() should always return strings in 
> .rodata, even for output statements, and should never point into GC 
> memory.

Bah, ignore that.  I should refetch mail before answering mid-thread ;)


Ciao,
Michael.

Re: [PATCH v9] add -fpatchable-function-entry=N,M option

[Torsten Duwe]

On Tue, Jul 04, 2017 at 02:27:00PM +0100, Richard Earnshaw (lists) wrote:
> > 
> > How about omitting the recording step and document that? This way the
> > instrumentation can still be useful on e.g. a.out format; the framework
> > would then have to check around each function entry whether the NOPs are
> > there.
> > 
> 
> I'm happy with that if the result is still something useful.  Frankly, I
> don't think GCC has many targets now that don't support named sections,
> so I wouldn't loose too much sleep over being unable to support this
> feature on such targets.  However, if the implementation is straight
> forward then doing what you suggest is fine.

Ok, will do it this way then.

> 
> >> The template NOP in default_print_patchable_function_entry needs to be
> >> added as a GC root to prevent it being discarded during garbage collection.
> > 
> > Are you sure? For the nop_templ I'm only digging my way to the platform-
> > dependent constant string, which I find amongst the constant strings in
> > .rodata (or .text, for platforms without named sections ;-).
> > Is the memory region pointed to by nop_templ really endangered?
> > 
> 
> I'd missed that this was trying to recover a string.  That's probably OK
> from a GC perspective then, but it isn't safe to hold this between
> functions since the nop used for one function might not be the same when
> we come to the next.  Consider, for example, the ARM port where we can
> switch from generating ARM code to generating Thumb code: it might work
> with the current implementation, but this isn't guaranteed to be the case.

I'm "caching" the string, currently. Now that you say it...
It will slow things down a bit, but you're right.