Re: [PATCH v2] add -fprolog-pad=N option to c-family

[Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>]

> On Oct 5, 2016, at 12:45 AM, Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org> wrote:
> 
>> On Sep 29, 2016, at 11:14 AM, Torsten Duwe <duwe@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 <duwe@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