On 13/01/17 12:19, Torsten Duwe wrote:
> Changes since v4: hopefully addressed all of Sandra's requests
> and suggestions concerning the documentation snippets, thanks
> for the feedback. If it still isn't clear, feel free to rephrase
> -- I'm a programmer, not a technical writer.
>
Generally, I find 'pad' somewhat confusing. It's OK for the option name
itself, but more generally, we should be talking about either space or
number of instructions according to context.
> 2017-01-13 Torsten Duwe <d...@suse.de> :
Two spaces between date and name, two more between name and email, no
colon at the end.
>
> * c-family/c-attribs.c : introduce prolog_pad attribute and create
> a handler for it.
>
Don't leave blank lines between files mentioned here. All lines should
be indented by /exactly/ one tab; don't add extra indentation for
continuation lines. No space before colon. Capital letter at start of
sentences, full stop at the end of each one. Which function, or data
structure did you change (put the name in brackets before the colon)?
Document each function or variable changed.
The above entry should read:
* c-family/c-attribs.c (c_common_attribute_table): Add entry
for "prolog_pad".
(handle_prolog_pad_attribute): New function.
> * lto/lto-lang.c : Likewise.
You still need to name the objects affected, even if you can then use
likewise to refer to the immediately preceding entry.
>
> * common.opt : introduce -fprolog_pad command line option
> and its variables prolog_nop_pad_size and prolog_nop_pad_entry.
>
> * doc/extend.texi : document prolog_pad attribute.
>
> * doc/invoke.texi : document -fprolog_pad command line option.
>
> * opts.c (OPT_fprolog_pad_): add parser.
>
> * doc/tm.texi.in (TARGET_ASM_PRINT_PROLOG_PAD): new target hook
>
> * doc/tm.texi : Likewise.
>
> * target.def (print_prolog_pad): Likewise.
>
> * targhooks.h (default_print_prolog_pad): new function.
>
> * targhooks.c (default_print_prolog_pad): Likewise.
>
> * testsuite/c-c++-common/attribute-prolog_pad-1.c : New test.
>
> * toplev.c (process_options): Switch off IPA-RA if
> prolog pads are being generated.
>
> * varasm.c (assemble_start_function): look at prolog-pad command
> line switch and function attributes and maybe generate NOP
> pads by calling print_prolog_pad.
>
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index ce7fcaa..5c6cf1c 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -139,6 +139,7 @@ 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_prolog_pad_attribute (tree *, tree, tree, int, bool *);
>
> /* Table of machine-independent attributes common to all C-like languages.
>
> @@ -345,6 +346,8 @@ const struct attribute_spec c_common_attribute_table[] =
> handle_bnd_instrument, false },
> { "fallthrough", 0, 0, false, false, false,
> handle_fallthrough_attribute, false },
> + { "prolog_pad", 1, 2, true, false, false,
> + handle_prolog_pad_attribute, false },
> { NULL, 0, 0, false, false, false, NULL, false }
> };
>
> @@ -3173,3 +3176,10 @@ handle_fallthrough_attribute (tree *, tree name, tree,
> int,
> *no_add_attrs = true;
> return NULL_TREE;
> }
> +
> +static tree
> +handle_prolog_pad_attribute (tree *, tree, tree, int,
> + bool *)
> +{
> + return NULL_TREE;
> +}
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 8ad5b77..37d4009 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -163,6 +163,13 @@ bool flag_stack_usage_info = false
> Variable
> int flag_debug_asm
>
> +; If we should generate NOP pads before each function prologue
'If' suggests this is a boolean, but the declaration is for a count, so
this should be "Number of NOP instructions to insert before each
function prologue."
> +Variable
> +HOST_WIDE_INT prolog_nop_pad_size
> +
> +; And how far the asm entry point is into this pad
> +Variable
> +HOST_WIDE_INT prolog_nop_pad_entry
>
> ; Balance between GNAT encodings and standard DWARF to emit.
> Variable
> @@ -2019,6 +2026,10 @@ fprofile-reorder-functions
> Common Report Var(flag_profile_reorder_functions)
> Enable function reordering that improves code placement.
>
> +fprolog-pad=
> +Common Joined Optimization
> +Pad NOPs before each function prologue.
> +
Insert NOP instructions before ...
> frandom-seed
> Common Var(common_deferred_options) Defer
>
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 6be113c..fb7d62d 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3076,6 +3076,23 @@ that affect more than one function.
> This attribute should be used for debugging purposes only. It is not
> suitable in production code.
>
> +@item prolog_pad
> +@cindex @code{prolog_pad} function attribute
> +@cindex function entry padded with NOPs
> +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,
> +prolog padding is enabled globally using the @option{-fprolog-pad=N,M}
> +command-line switch, and disabled with attribute @code{prolog_pad (0)}
> +for functions that are part of the actual instrumentation framework.
> +This conveniently avoids an endless recursion.
> +Of course the @code{prolog_pad} function attribute can be used to
s/Of course the/The/
> +change the pad size to any desired value. The two-value syntax is
> +the same as for the command-line switch @option{-fprolog-pad=N,M},
> +generating a NOP pad of size @var{N}, with the function entry point
> +@var{M} NOP instructions into the pad. @var{M} defaults to 0
> +if omitted e.g. function entry point is the beginning of the pad.
> +
> @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 69f0adb..d2e11bc 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -11341,6 +11341,27 @@ 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 -fprolog-pad=@var{N},@var{M}
This needs to make it clear that M is optional. Then below state that
if omitted, M defaults to zero.
> +@opindex fprolog-pad
> +Generate a pad of @var{N} NOPs right at the beginning
> +of each function, with the function entry point @var{M} NOPs into
> +the pad, which can be used to patch in any desired
> +instrumentation at run time, provided that the code segment
> +is writable.
> +For run-time identification, the starting addresses
> +of these pads, which correspond to their respective function entries
> +minus @var{M}, are additionally collected in the @code{__prolog_pads_loc}
> +section of the resulting binary.
> +
> +Note that value of @code{__attribute__ ((prolog_pad (N,M)))} takes
> +precedence over command-line option @option{-fprolog-pad=N,M}.
> +This can be used to increase the pad size or to remove the pad completely
> +on a single function. If @code{N=0}, no pad location is recorded.
> +
> +The NOP pad is inserted at (and maybe before) the function entry address,
> +even before the prologue. If @var{M} is omitted, it defaults to 0 e.g. the
> +pad starts exactly at the function entry point and not before.
> +
This is the only text most users will see, it also needs to make it
clear that the size of a NOP in a variable size instruction set is the
size of the smallest NOP on that machine.
> @end table
>
>
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index 4b62e05..eb29f71 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -4564,6 +4564,16 @@ 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_PROLOG_PAD (FILE *@var{file},
> unsigned HOST_WIDE_INT @var{pad_size}, bool @var{record_p})
> +Generate an array of @var{pad_size} NOP instructions in the
> +output @var{file}. If the target supports NOPs of different sizes,
> +the instruction with the smallest code size should be used, in order
> +to achieve the finest granularity for @var{pad_size}. If @var{record_p}
> +is true, the current location should be recorded in the
> +@code{__prolog_pads_loc} section or a target-specific equivalent.
> +
> +@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 ea74d37..7b01026 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -3648,6 +3648,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_PROLOG_PAD
> +
> @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 fccb8c6..d4955c0 100644
> --- a/gcc/lto/lto-lang.c
> +++ b/gcc/lto/lto-lang.c
> @@ -50,6 +50,7 @@ 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_prolog_pad_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 *);
> @@ -78,6 +79,8 @@ const struct attribute_spec lto_attribute_table[] =
> handle_nonnull_attribute, false },
> { "nothrow", 0, 0, true, false, false,
> handle_nothrow_attribute, false },
> + { "prolog_pad", 1, 2, true, false, false,
> + handle_prolog_pad_attribute, false },
> { "returns_twice", 0, 0, true, false, false,
> handle_returns_twice_attribute, false },
> { "sentinel", 0, 1, false, true, true,
> @@ -475,6 +478,14 @@ handle_returns_twice_attribute (tree *node, tree
> ARG_UNUSED (name),
> return NULL_TREE;
> }
>
> +static tree
> +handle_prolog_pad_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 5f573a1..60d80aa 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -2157,6 +2157,26 @@ common_handle_option (struct gcc_options *opts,
> opts->x_flag_ipa_reference = false;
> break;
>
> + case OPT_fprolog_pad_:
> + {
> + const char *comma = strchr (arg, ',');
> + if (comma)
> + {
> + prolog_nop_pad_size = atoi (arg);
> + prolog_nop_pad_entry = atoi (comma + 1);
> + }
> + else
> + {
> + prolog_nop_pad_size = atoi (arg);
> + prolog_nop_pad_entry = 0;
> + }
Where's the error checking? If I write gibberish after the option name
then atoi will silently fail and return zero. I'm not overly familiar
with the option handling code, but I'm sure we have routines to do the
heavy lifting here.
> + if (prolog_nop_pad_size < 0
> + || prolog_nop_pad_entry < 0
> + || prolog_nop_pad_size < prolog_nop_pad_entry)
> + error ("invalid arguments for %<-fprolog_pad%>");
> + }
> + 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 0443390..60e56cd 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_prolog_pad,
> + "Generate prologue pad",
> + void, (FILE *file, unsigned HOST_WIDE_INT pad_size, bool record_p),
> + default_print_prolog_pad)
> +
> /* Output the assembler code for entry to a function. */
> DEFHOOK
> (function_prologue,
> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index 2f2abd3..2ee5e38 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -1617,6 +1617,31 @@ default_compare_by_pieces_branch_ratio (machine_mode)
> return 1;
> }
>
> +/* Write pad_size NOPs into the asm outfile before a function
> + prologue. If record_p is true, the location of the pad will be
> + recorded in a special object section called "__prolog_pads_loc".
> + This routine may be called twice per function to put NOPs before
> + and after the function entry. */
> +
The convention when referring to formal parameters in comments is to
write the name in upper case, so PAD_SIZE and RECORD_P.
> +void
> +default_print_prolog_pad (FILE *file, unsigned HOST_WIDE_INT pad_size,
> + bool record_p)
> +{
> + if (record_p)
> + fprintf (file, "1:");
> +
> + unsigned i;
> + for (i = 0; i < pad_size; ++i)
> + fprintf (file, "\tnop\n");
> +
> + if (record_p)
> + {
> + fprintf (file, "\t.section __prolog_pads_loc, \"a\",@progbits\n");
> + fprintf (file, "\t.quad 1b\n");
> + fprintf (file, "\t.previous\n");
> + }
> +}
NO! Almost everything in this function is wrong, it needs to be done
through suitable hooks that call into the machine back-ends that
understand assembly flavours supported.
What are you doing to do if you have an object format that does not
support named sections?
You're assuming a label syntax.
You're assuming that the assembler supports .previous
You're assuming that '@' is not a comment character (on ARM it is).
You're assuming we support relative labels.
You're assuming the size of the reference to a pad should be a '.quad'.
You're assuming that "nop" is a valid mnemonic in the assembler and
produces a minimally sized NOP.
There are hooks in the compiler to support all this sort of stuff; use them.
It perhaps wouldn't be so bad if you made it clear that this was
explicitly for ELF format (the comment issue still applies, as does the
use of .quad).
> +
> bool
> default_profile_before_prologue (void)
> {
> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> index a5565f5..e302e8d 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -203,6 +203,7 @@ 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_prolog_pad (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-prolog_pad-1.c
> b/gcc/testsuite/c-c++-common/attribute-prolog_pad-1.c
> new file mode 100644
> index 0000000..2236aa8
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/attribute-prolog_pad-1.c
> @@ -0,0 +1,34 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fprolog-pad=3,1" } */
> +
> +void f1 (void) __attribute__((prolog_pad(2,1)));
> +void f2 (void) __attribute__((prolog_pad(3)));
> +int f3 (void);
> +
> +void
> +f1 (void)
> +{
> + f2 ();
> +}
> +
> +void
> +f2 (void)
> +{
> + f1 ();
> +}
> +
> +/* F3 should never have a NOP pad. */
> +int
> +__attribute__((prolog_pad(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 c0f1a2d..649c4bd 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -1595,8 +1595,10 @@ process_options (void)
> }
>
> /* Do not use IPA optimizations for register allocation if profiler is
> active
> + or prolog pads 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 || prolog_nop_pad_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 11a8ac4..6c40bce 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -1830,6 +1830,44 @@ assemble_start_function (tree decl, const char *fnname)
> if (DECL_PRESERVE_P (decl))
> targetm.asm_out.mark_decl_preserved (fnname);
>
> + unsigned HOST_WIDE_INT pad_size = prolog_nop_pad_size;
> + unsigned HOST_WIDE_INT pad_entry = prolog_nop_pad_entry;
> +
> + tree prolog_pad_attr
> + = lookup_attribute ("prolog_pad", DECL_ATTRIBUTES (decl));
> + if (prolog_pad_attr)
> + {
> + tree pp_val = TREE_VALUE (prolog_pad_attr);
> + tree prolog_pad_value1 = TREE_VALUE (pp_val);
> +
> + if (tree_fits_uhwi_p (prolog_pad_value1))
> + pad_size = tree_to_uhwi (prolog_pad_value1);
> + else
> + gcc_unreachable ();
> +
> + pad_entry = 0;
> + if (list_length (pp_val) > 1)
> + {
> + tree prolog_pad_value2 = TREE_VALUE (TREE_CHAIN (pp_val));
> +
> + if (tree_fits_uhwi_p (prolog_pad_value2))
> + pad_entry = tree_to_uhwi (prolog_pad_value2);
> + else
> + gcc_unreachable ();
> + }
> + }
> +
> + if (pad_entry > pad_size)
> + {
> + if (pad_size > 0)
> + warning (OPT_Wattributes, "Prolog nop pad entry > size");
> + pad_entry = 0;
> + }
> +
> + /* Emit the prolog padding before the entry label, if any. */
> + if (pad_entry > 0)
> + targetm.asm_out.print_prolog_pad (asm_out_file, pad_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 +1876,11 @@ 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 padding after the label. Record it if we haven't done so yet.
> */
> + if (pad_size > pad_entry)
> + targetm.asm_out.print_prolog_pad (asm_out_file, pad_size-pad_entry,
> + (pad_entry == 0));
> +
> if (lookup_attribute ("no_split_stack", DECL_ATTRIBUTES (decl)))
> saw_no_split_stack = true;
> }
>
