Re: checkpatch.pl should suggest __section

[Nick Desaulniers]

On Fri, Aug 9, 2019 at 3:58 PM Joe Perches <joe@xxxxxxxxxxx> wrote:
>
> On Fri, 2019-08-09 at 15:21 -0700, Nick Desaulniers wrote:
> > Hi Joe,
> > While debugging:
> > https://github.com/ClangBuiltLinux/linux/issues/619
> > we found a bunch of places where __section is not used but could be,
> > and uses a string literal when it probably should not be.
> >
> > Just a thought that maybe checkpatch.pl could warn if
> > `__attribute__((section` appeared in the added diff, and suggest
> > __section? Then further warn to not use `""` for the section name?
>
> Hmm, that makes me wonder about the existing __section uses
> _with_ a quote are actually in the proper sections.
>
> $ git grep -n -P '\b__section\s*\(\s*"'
> arch/arm64/kernel/smp_spin_table.c:22:volatile unsigned long __section(".mmuoff.data.read")
> arch/s390/boot/startup.c:49:static struct diag210 _diag210_tmp_dma __section(".dma.data");
> include/linux/compiler.h:27:                            __section("_ftrace_annotated_branch")   \
> include/linux/compiler.h:63:            __section("_ftrace_branch")             \
> include/linux/compiler.h:121:#define __annotate_jump_table __section(".rodata..c_jump_table")
> include/linux/compiler.h:158:   __section("___kentry" "+" #sym )                        \
> include/linux/compiler.h:301:   static void * __section(".discard.addressable") __used \
> include/linux/export.h:107:     static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
> include/linux/srcutree.h:127:           __section("___srcu_struct_ptrs") = &name

I'm going through and fixing all of these now.  Thinking about sending
one treewide fix to akpm.

>
> Maybe there should also be a __section("<foo>") test too.

I think so.  Some of the trickier ones are ones that use the
stringification C preprocessor operator.  I need to think more about
these.

>
> Anyway, how about:
> ---
>  scripts/checkpatch.pl | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 1cdacb4fd207..8e6693ca772c 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5901,6 +5901,15 @@ sub process {
>                              "__aligned(size) is preferred over __attribute__((aligned(size)))\n" . $herecurr);
>                 }
>
> +# Check for __attribute__ section, prefer __section (without quotes)
> +               if ($realfile !~ m@\binclude/uapi/@ &&
> +                   $line =~ /\b__attribute__\s*\(\s*\(.*_*section_*\s*\(\s*("[^"]*")/) {
> +                       my $old = substr($rawline, $-[1], $+[1] - $-[1]);
> +                       my $new = substr($old, 1, -1);
> +                       WARN("PREFER_SECTION",
> +                            "__section($new) is preferred over __attribute__((section($old)))\n" . $herecurr);
> +               }
> +

I can't read Perl, but this looks pretty good.
Acked-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
-- 
Thanks,
~Nick Desaulniers