> On Oct 5, 2016, at 12:45 AM, Maxim Kuvyrkov <maxim.kuvyr...@linaro.org> wrote:
>
>> On Sep 29, 2016, at 11:14 AM, Torsten Duwe <d...@suse.de> wrote:
>>
>> In case anybody missed it, the Linux kernel side to make use
>> of this has also been finished meanwhile. Of course it can not
>> be accepted without compiler support; and this feature patch
>> is much more versatile than just Linux kernel live patching
>> on a single architecture.
>
> Hi Torsten,
>
> Good job moving -fprolog-pad= forward! I've reviewed your patch, and it
> looks OK with the minor comments inline. I've CC'ed Richard E. since you
> will need a global maintainer approval for this change.
Ping?
--
Maxim Kuvyrkov
www.linaro.org
>
> Ideally, I want to improve support for -fprolog-pad=N and
> __attribute__((prolog_pad(N))) to provide functionality to also output pad
> before the function label to address use-cases for s390, sparc, etc (what
> Jose E. Marchesi was referring to). I.e., -fprolog-pad= option would accept
> both -fprolog-pad=N and -fprolog-pad=M,N forms -- issue M nops before
> function label and N nops after function label. Similarly for
> __attribute__((prolog_pad(N,M))). I (or you :-) ) can attempt to implement
> this functionality before stage1 closes, but it should not block this initial
> patch.
>
> Comments on the patch below.
>
>>
>> Changes since the previous version
>> (which in turn was based on Maxim's suggestion):
>>
>> * Document the feature in *.texi
>>
>> * Automatically disable IPA-RA, like normal profiling does.
>> You never know in advance what the code patched in at run time will do.
>> Any optimisation here is potentially wrong.
>>
>> * record a prolog_nop_pad_size value specified on the command line
>> in each function's attributes, so that it survives an LTO pipe.
>>
>> Signed-off-by: Torsten Duwe <d...@suse.de>
>>
>> diff --git a/gcc/attribs.c b/gcc/attribs.c
>> index 16996e9..a5cf2aa 100644
>> --- a/gcc/attribs.c
>> +++ b/gcc/attribs.c
>> @@ -365,6 +365,21 @@ decl_attributes (tree *node, tree attributes, int flags)
>> if (!attributes_initialized)
>> init_attributes ();
>>
>> + /* If we're building NOP pads because of a command line arg, note the size
>> + for LTO builds, unless the attribute has already been overridden. */
>> + if (TREE_CODE (*node) == FUNCTION_DECL && prolog_nop_pad_size > 0)
>> + {
>> + tree pp_attr = lookup_attribute ("prolog_pad", attributes);
>> + if (! pp_attr )
>
> Coding style: should be "if (!pp_attr)"
>
> Also, this clause implies that attribute takes precedence over command-line
> option, and the two are not attempted to be merged. I agree that this is a
> reasonable approach, but consider adding a warning if prolog_nop_pad_size is
> bigger than the value of the existing attribute. Something like ...
>
>> + {
>> + tree pp_size = build_int_cstu (integer_type_node,
>> prolog_nop_pad_size);
>> +
>> + attributes = tree_cons (get_identifier ("prolog_pad"),
>> + tree_cons ( NULL_TREE, pp_size, NULL_TREE),
>> + attributes);
>> + }
>
> ... here:
>
> else if (ATTRIBUTE_VALUE (pp_attr) > prolog_nop_pad_size)
> warning (OPT_Wattributes, "Prologue pad might be truncated:
> <FUNCTION_NAME>", node);
>
> Also, I suggest to issue a warning here if ipa-ra is not disabled, and adding
> a comment to the documentation about issues with interoperability of
> -fprolog-pad/__attribute__((prolog_pad)) and IPA RA..
>
>> + }
>> +
>> /* If this is a function and the user used #pragma GCC optimize, add the
>> options to the attribute((optimize(...))) list. */
>> if (TREE_CODE (*node) == FUNCTION_DECL && current_optimize_pragma)
>> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
>> index f2846bb..278a99e 100644
>> --- a/gcc/c-family/c-common.c
>> +++ b/gcc/c-family/c-common.c
>> @@ -393,6 +393,7 @@ static tree handle_designated_init_attribute (tree *,
>> tree, tree, int, bool *);
>> 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_prolog_pad_attribute (tree *, tree, tree, int, bool *);
>>
>> static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
>> static bool nonnull_check_p (tree, unsigned HOST_WIDE_INT);
>> @@ -834,6 +835,8 @@ const struct attribute_spec c_common_attribute_table[] =
>> handle_bnd_legacy, false },
>> { "bnd_instrument", 0, 0, true, false, false,
>> handle_bnd_instrument, false },
>> + { "prolog_pad", 1, 1, false, true, true,
>> + handle_prolog_pad_attribute, false },
>
> The patch also needs to document new attribute in doc/extend.texi
>
>> { NULL, 0, 0, false, false, false, NULL, false }
>> };
>>
>> @@ -9687,6 +9690,14 @@ handle_designated_init_attribute (tree *node, tree
>> name, tree, int,
>> return NULL_TREE;
>> }
>>
>> +static tree
>> +handle_prolog_pad_attribute (tree *, tree, tree, int,
>> + bool *)
>> +{
>> + /* Nothing to be done here. */
>> + return NULL_TREE;
>> +}
>> +
>>
>> /* Check for valid arguments being passed to a function with FNTYPE.
>> There are NARGS arguments in the array ARGARRAY. LOC should be used for
>> diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
>> index fec58bc..0220faa 100644
>> --- a/gcc/c-family/c-opts.c
>> +++ b/gcc/c-family/c-opts.c
>> @@ -536,6 +536,10 @@ c_common_handle_option (size_t scode, const char *arg,
>> int value,
>> cpp_opts->ext_numeric_literals = value;
>> break;
>>
>> + case OPT_fprolog_pad_:
>> + prolog_nop_pad_size = value;
>> + break;
>> +
>> case OPT_idirafter:
>> add_path (xstrdup (arg), AFTER, 0, true);
>> break;
>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
>> index 88038a0..804c654 100644
>> --- a/gcc/c-family/c.opt
>> +++ b/gcc/c-family/c.opt
>> @@ -1427,6 +1427,10 @@ fpreprocessed
>> C ObjC C++ ObjC++
>> Treat the input file as already preprocessed.
>>
>> +fprolog-pad=
>> +C ObjC C++ ObjC++ RejectNegative Joined UInteger Var(prolog_nop_pad_size)
>> Init(0)
>> +Pad NOPs before each function prolog
>> +
>> ftrack-macro-expansion
>> C ObjC C++ ObjC++ JoinedOrMissing RejectNegative UInteger
>> ; converted into ftrack-macro-expansion=
>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index 2ed9285..463a5a5 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -10387,6 +10387,16 @@ 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=N
>> +@opindex fprolog-pad
>> +Generate a pad of N nops right at the beginning
>> +of each function, which can be used to patch in any desired
>> +instrumentation at run time, provided that the code segment
>> +is writeable. For run time identification, the starting addresses
>> +of these pads, which correspond to their respective functions,
>> +are additionally collected in the @code{__prolog_pads_loc} section
>> +of the resulting binary.
>
> ... Note that value of @code{__attribute__((prolog_pad(N)))} takes precedence
> over command-line option -fprolog_pad=N.
>
>> +
>> @end table
>>
>>
>> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>> index 745910f..f6f9d59 100644
>> --- a/gcc/doc/tm.texi
>> +++ b/gcc/doc/tm.texi
>> @@ -4553,6 +4553,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_PROLOG_PAD (FILE
>> *@var{file}, unsigned HOST_WIDE_INT @var{pad_size}, bool @var{record_p})
>> +Generate prologue pad
>> +@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 f31c763..b9585da 100644
>> --- a/gcc/doc/tm.texi.in
>> +++ b/gcc/doc/tm.texi.in
>> @@ -3662,6 +3662,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/final.c b/gcc/final.c
>> index 55cf509..94bbf8f 100644
>> --- a/gcc/final.c
>> +++ b/gcc/final.c
>> @@ -1754,6 +1754,7 @@ void
>> final_start_function (rtx_insn *first, FILE *file,
>> int optimize_p ATTRIBUTE_UNUSED)
>> {
>> + unsigned HOST_WIDE_INT pad_size = prolog_nop_pad_size;
>
> Declaration of pad_size can be moved under "if (prolog_pad_attr)" clause,
> since we don't need value of prolog_nop_pad_size here anymore (it's already
> encoded in the __attribute__).
>
>> block_depth = 0;
>>
>> this_is_asm_operands = 0;
>> @@ -1766,6 +1767,21 @@ final_start_function (rtx_insn *first, FILE *file,
>>
>> high_block_linenum = high_function_linenum = last_linenum;
>>
>> + tree prolog_pad_attr
>> + = lookup_attribute ("prolog_pad", TYPE_ATTRIBUTES (TREE_TYPE
>> (current_function_decl)));
>> + if (prolog_pad_attr)
>> + {
>> + tree prolog_pad_value = TREE_VALUE (TREE_VALUE (prolog_pad_attr));
>> +
>> + if (tree_fits_uhwi_p (prolog_pad_value))
>> + pad_size = tree_to_uhwi (prolog_pad_value);
>> + else
>> + gcc_unreachable ();
>> +
>> + }
>> + if (pad_size > 0)
>> + targetm.asm_out.print_prolog_pad (file, pad_size, true);
>
> Similarly, this clause can be moved inside "if (prolog_pad_attr)" clause.
>
>> +
>> if (flag_sanitize & SANITIZE_ADDRESS)
>> asan_function_start ();
>>
>> diff --git a/gcc/target.def b/gcc/target.def
>> index 20f2b32..e0a4cc4 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 a342277..41e9850 100644
>> --- a/gcc/targhooks.c
>> +++ b/gcc/targhooks.c
>> @@ -1503,6 +1503,22 @@ default_use_by_pieces_infrastructure_p (unsigned
>> HOST_WIDE_INT size,
>> return move_by_pieces_ninsns (size, alignment, max_size + 1) < ratio;
>> }
>>
>> +void
>> +default_print_prolog_pad (FILE *file, unsigned HOST_WIDE_INT pad_size,
>> + bool record_p)
>> +{
>> + if (record_p)
>> + fprintf (file, "1:");
>> + for (unsigned 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");
>> + }
>> +}
>> +
>> bool
>> default_profile_before_prologue (void)
>> {
>> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
>> index 7687c39..6bb41c4 100644
>> --- a/gcc/targhooks.h
>> +++ b/gcc/targhooks.h
>> @@ -200,6 +200,7 @@ extern bool default_use_by_pieces_infrastructure_p
>> (unsigned HOST_WIDE_INT,
>> enum by_pieces_operation,
>> bool);
>>
>> +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..7404dc5
>> --- /dev/null
>> +++ b/gcc/testsuite/c-c++-common/attribute-prolog_pad-1.c
>> @@ -0,0 +1,15 @@
>> +/* { dg-do compile } */
>> +
>> +void f1(void) __attribute__((prolog_pad(1)));
>> +void f2(void) __attribute__((prolog_pad(2)));
>> +
>> +void
>> +f1 (void)
>> +{
>> + f2 ();
>> +}
>> +
>> +void f2 (void)
>> +{
>> + f1 ();
>> +}
>> diff --git a/gcc/toplev.c b/gcc/toplev.c
>> index 8979d26..2339ce8 100644
>> --- a/gcc/toplev.c
>> +++ b/gcc/toplev.c
>> @@ -1580,8 +1580,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
>
> Thanks!
>
> --
> Maxim Kuvyrkov
> www.linaro.org
