Re: [PATCH v3] add -fprolog-pad=N,M option

[Maxim Kuvyrkov <maxim dot kuvyrkov at linaro dot org>]

> On Dec 19, 2016, at 6:04 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> 
> I'll consider myself agnostic as to whether this is a feature we want or need,

Hi Bernd, thanks for reviewing this!

Regarding the usefulness of this feature, it has been discussed here (2 years ago): http://gcc.gcc.gnu.narkive.com/JfWUDn8Y/rfc-kernel-livepatching-support-in-gcc .

Kernel live-patching is of interest to several major distros, and it already supported by GCC via architecture-specific implementations (x86, s390, sparc).  The -fprolog-pad= option is an architecture-neutral equivalent that can be used for porting kernel live-patching to new architectures.

Existing support for kernel live-patching in x86, s390 and sparc backends can be redirected to use -fprolog-pad= functionality and, thus, simplified.

--
Maxim Kuvyrkov
www.linaro.org


> so I'll just comment on some style questions. There's a fair amount of coding style violations, I'll point some of them out but please read the documents we have linked on this page:
>  https://gcc.gnu.org/contribute.html
> 
> On 12/16/2016 03:14 PM, Torsten Duwe wrote:
>> Signed-off-by: Torsten Duwe <duwe@suse.de>
> 
> This is meaningless for the GCC project. We require a copyright assignment; I assume SuSE has a blanket one that covers you. You should also write a ChangeLog entry.
> 
>> diff --git a/gcc/attribs.c b/gcc/attribs.c
>> index e66349a..6ff81a8 100644
>> --- a/gcc/attribs.c
>> +++ b/gcc/attribs.c
>> @@ -365,6 +365,28 @@ 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. */
> 
> Two spaces at the end of a sentence, including at the end of a comment.
> 
>> +  if (TREE_CODE (*node) == FUNCTION_DECL &&
>> +      prolog_nop_pad_size > 0)
> 
> Operators go on the next line when wrapping.
> 
>> +					     ),
> 
> Don't put closing parens on their own line.
> 
>> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
>> index db293fe..7f3e558 100644
>> --- a/gcc/c/c-decl.c
>> +++ b/gcc/c/c-decl.c
>> @@ -2322,6 +2322,34 @@ merge_decls (tree newdecl, tree olddecl, tree newtype, tree oldtype)
>>       TREE_ASM_WRITTEN (olddecl) = 0;
>>     }
>> 
>> +  /* Prolog pad size may be set wrongly by a forward declaration;
>> +     fix it up by pulling the final value in front.
>> +  */
> 
> The "*/" should go on the previous line.
> 
>> +      for (it = &DECL_ATTRIBUTES (newdecl); *it; it = &TREE_CHAIN(*it))
> 
> Space before opening parentheses (many occurrences).
>> 
>> +void
>> +default_print_prolog_pad (FILE *file, unsigned HOST_WIDE_INT pad_size,
>> +			  bool record_p)
> 
> Every function needs a comment describing its purpose and that of its arguments. Look around for examples. There are cases in this patch of hook implementations which ignore all their args, there I believe we can relax this rule a little (but a comment saying which hook is implemented would still be good).
> 
>> 
>> +extern void default_print_prolog_pad (FILE *, unsigned HOST_WIDE_INT , bool);
> 
> Careful with stray whitespace.
>> +      if (tree_fits_uhwi_p (prolog_pad_value1) )
> 
> Here too.
>> +	{
>> +	  pad_size = tree_to_uhwi (prolog_pad_value1);
>> +	}
> 
> Lose { } braces around single statements. Several cases.
> 
> Am I missing it or is there no actual implementation of the target hook anywhere?
> 
> 
> Bernd