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

On Wed, Mar 01, 2017 at 01:35:52PM +0000, Richard Earnshaw (lists) wrote:
> >>
> >> How about --fpatchable-function-entry=<size-spec>?
> > 
> I haven't reviewed it yet.  I'm not really planning to spend any more
> time on this until stage1 re-opens.

So I guess this is about now? Here is version 8, which is functionally identical
to v6 (v7 tried to guard the gen_nop call, which you wrote isn't neccessary).
The longer names required some reformatting.

	Torsten

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

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

gcc/lto/ChangeLog
2017-05-03  Torsten Duwe  <duwe@suse.de>

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

gcc/ChangeLog
2017-05-03  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-05-03  Torsten Duwe  <duwe@suse.de>

	* c-c++-common/attribute-patchable_function_entry-1.c: New test.

Ping?

Richard E., this feature will immediately benefit AArch64 backend and
Linux kernel.  Do you plan to review it?

Thank you.

On Wed, May 3, 2017 at 5:48 PM, Torsten Duwe <duwe@suse.de> wrote:
> On Wed, Mar 01, 2017 at 01:35:52PM +0000, Richard Earnshaw (lists) wrote:
>> >>
>> >> How about --fpatchable-function-entry=<size-spec>?
>> >
>> I haven't reviewed it yet.  I'm not really planning to spend any more
>> time on this until stage1 re-opens.
>
> So I guess this is about now? Here is version 8, which is functionally identical
> to v6 (v7 tried to guard the gen_nop call, which you wrote isn't neccessary).
> The longer names required some reformatting.
>
>         Torsten
>
> gcc/c-family/ChangeLog
> 2017-05-03  Torsten Duwe  <duwe@suse.de>
>
>         * c-attribs.c (c_common_attribute_table): Add entry for
>         "patchable_function_entry".
>
> gcc/lto/ChangeLog
> 2017-05-03  Torsten Duwe  <duwe@suse.de>
>
>         * lto-lang.c (lto_attribute_table): Add entry for
>         "patchable_function_entry".
>
> gcc/ChangeLog
> 2017-05-03  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-05-03  Torsten Duwe  <duwe@suse.de>
>
>         * c-c++-common/attribute-patchable_function_entry-1.c: New test.
>
> 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 b7ece0c73e1..1b698ef4fc5 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 65308c9d933..6cbb77a8dc4 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -11382,6 +11382,32 @@ 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 is by default the one created
> +by @code{gen_nop ()}.
> +
> +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..4a5317d73bb 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -4566,6 +4566,10 @@ 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 the function start
> +@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/attribute-patchable_function_entry-1.c b/gcc/testsuite/c-c++-common/attribute-patchable_function_entry-1.c
> new file mode 100644
> index 00000000000..5578759bc36
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/attribute-patchable_function_entry-1.c
> @@ -0,0 +1,36 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fpatchable-function-entry=3,1" } */
> +
> +/* Respect overriding attributes in the declaration.  */
> +void f1 (void) __attribute__((patchable_function_entry(2,1)));
> +void f2 (void) __attribute__((patchable_function_entry(3)));
> +/* But nothing declared must not mean anything.  */
> +int f3 (void);
> +
> +void
> +f1 (void)
> +{
> +  f2 ();
> +}
> +
> +void
> +f2 (void)
> +{
> +  f1 ();
> +}
> +
> +/* F3 should never get a NOP area.  */
> +int
> +__attribute__((patchable_function_entry(0)))
> +__attribute__((noinline))
> +f3 (void)
> +{
> +  return 5;
> +}
> +
> +/* F4 should receive the command line default setting.  */
> +int
> +f4 (void)
> +{
> +  return 3*f3 ()+1;
> +}
> 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;
>  }

On 05/29/2017 04:29 AM, Maxim Kuvyrkov wrote:

>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index 65308c9d933..6cbb77a8dc4 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -11382,6 +11382,32 @@ 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 is by default the one created
>> +by @code{gen_nop ()}.

This is too implementor-speaky.  We shouldn't expect GCC users to read 
the source code to figure out what gen_nop is, or what it might do on 
any particular target.

>> +The NOP instructions are inserted at -- and maybe before, depending on
>> +@var{M} -- the function entry address, even before the prologue.

Texinfo nit: s / -- /---/g

>> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>> index c4f2c893c8e..4a5317d73bb 100644
>> --- a/gcc/doc/tm.texi
>> +++ b/gcc/doc/tm.texi
>> @@ -4566,6 +4566,10 @@ 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 the function start
>> +@end deftypefn

This surely needs explanation of the parameters, and some punctuation.

-Sandra

On Sun, Jun 04, 2017 at 08:12:49PM -0600, Sandra Loosemore wrote:
> On 05/29/2017 04:29 AM, Maxim Kuvyrkov wrote:
> 
> >>diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> >>index 65308c9d933..6cbb77a8dc4 100644
> >>--- a/gcc/doc/invoke.texi
> >>+++ b/gcc/doc/invoke.texi
> >>@@ -11382,6 +11382,32 @@ 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 is by default the one created
> >>+by @code{gen_nop ()}.
> 
> This is too implementor-speaky.  We shouldn't expect GCC users to read the
> source code to figure out what gen_nop is, or what it might do on any
> particular target.

In previous versions of the patch, some people had complained it was unclear
which NOP instruction would be used. Please make up your mind and suggest
a phrase, I'm open for suggestion, as you may already have noticed.
"by default, the default nop is used" sounds bad, IMHO.
Sandra, gen_nop picks the pattern the machine description provides for the
named instruction "nop". Richard wrote that every target is required to have
one.

> >>+The NOP instructions are inserted at -- and maybe before, depending on
> >>+@var{M} -- the function entry address, even before the prologue.
> 
> Texinfo nit: s / -- /---/g

Ok, trivial.

> >>diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> >>index c4f2c893c8e..4a5317d73bb 100644
> >>--- a/gcc/doc/tm.texi
> >>+++ b/gcc/doc/tm.texi
> >>@@ -4566,6 +4566,10 @@ 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 the function start
> >>+@end deftypefn
> 
> This surely needs explanation of the parameters, and some punctuation.

Like

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.

?

Richard E.: we haven't heard a word from you yet about the actual code.

	Torsten

On 06/06/2017 03:49 AM, Torsten Duwe wrote:
> On Sun, Jun 04, 2017 at 08:12:49PM -0600, Sandra Loosemore wrote:
>> On 05/29/2017 04:29 AM, Maxim Kuvyrkov wrote:
>>
>>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>>>> index 65308c9d933..6cbb77a8dc4 100644
>>>> --- a/gcc/doc/invoke.texi
>>>> +++ b/gcc/doc/invoke.texi
>>>> @@ -11382,6 +11382,32 @@ 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 is by default the one created
>>>> +by @code{gen_nop ()}.
>>
>> This is too implementor-speaky.  We shouldn't expect GCC users to read the
>> source code to figure out what gen_nop is, or what it might do on any
>> particular target.
>
> In previous versions of the patch, some people had complained it was unclear
> which NOP instruction would be used. Please make up your mind and suggest
> a phrase, I'm open for suggestion, as you may already have noticed.
> "by default, the default nop is used" sounds bad, IMHO.
> Sandra, gen_nop picks the pattern the machine description provides for the
> named instruction "nop". Richard wrote that every target is required to have
> one.

I'm aware of what gen_nop is and what it does.  But a GCC *user*, as 
opposed to a GCC *implementor*, would not be familiar with gen_nop.  If 
there is no better way to explain the behavior of this option, you at 
least need to better explain what gen_nop is.  E.g.

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.

>>>> +The NOP instructions are inserted at -- and maybe before, depending on
>>>> +@var{M} -- the function entry address, even before the prologue.
>>
>> Texinfo nit: s / -- /---/g
>
> Ok, trivial.
>
>>>> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>>>> index c4f2c893c8e..4a5317d73bb 100644
>>>> --- a/gcc/doc/tm.texi
>>>> +++ b/gcc/doc/tm.texi
>>>> @@ -4566,6 +4566,10 @@ 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 the function start
>>>> +@end deftypefn
>>
>> This surely needs explanation of the parameters, and some punctuation.
>
> Like
>
> 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.
>
> ?

Yes, that's better.  At least it's complete sentences.  :-)

-Sandra