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

[Sandra Loosemore <sandra at codesourcery dot com>]

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