> On Dec 19, 2016, at 6:04 PM, Bernd Schmidt <bschm...@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 <d...@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