checkpatch.pl should suggest __section

checkpatch.pl should suggest __section

[Nick Desaulniers]

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?
-- 
Thanks,
~Nick Desaulniers

Re: checkpatch.pl should suggest __section

[Joe Perches]

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

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

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);
+		}
+
 # Check for __attribute__ format(printf, prefer __printf
 		if ($realfile !~ m@\binclude/uapi/@ &&
 		    $line =~ /\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf/) {

Re: checkpatch.pl should suggest __section

[Nick Desaulniers]

On Fri, Aug 9, 2019 at 3:58 PM Joe Perches <joe@perches.com> 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@google.com>
-- 
Thanks,
~Nick Desaulniers

Re: checkpatch.pl should suggest __section

[Joe Perches]

On Fri, 2019-08-09 at 16:04 -0700, Nick Desaulniers wrote:
> > 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@google.com>

I'll add a Suggested-by: for you.

But a Tested-by would be more valuable than an Acked-by if you
don't actually know how it works.

Re: checkpatch.pl should suggest __section

[Nick Desaulniers]

On Fri, Aug 9, 2019 at 4:17 PM Joe Perches <joe@perches.com> wrote:
>
> On Fri, 2019-08-09 at 16:04 -0700, Nick Desaulniers wrote:
> > > 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@google.com>
>
> I'll add a Suggested-by: for you.
>
> But a Tested-by would be more valuable than an Acked-by if you
> don't actually know how it works.

$ git am joes.patch
$ echo "int foo __attribute__((section(.hello)));" >> arch/x86/boot/a20.c
$ git commit arch/x86/boot/a20.c -sm "foo: bar\n baz"
$ git format-patch HEAD~
$ ./scripts/checkpatch.pl 0001-foo-bar.patch
total: 0 errors, 0 warnings, 4 lines checked

0001-foo-bar.patch has no obvious style problems and is ready for submission.

hmm...
-- 
Thanks,
~Nick Desaulniers

Re: checkpatch.pl should suggest __section

[Joe Perches]

On Mon, 2019-08-12 at 11:20 -0700, Nick Desaulniers wrote:
> On Fri, Aug 9, 2019 at 4:17 PM Joe Perches <joe@perches.com> wrote:
> > On Fri, 2019-08-09 at 16:04 -0700, Nick Desaulniers wrote:
> > > > 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@google.com>
> > 
> > I'll add a Suggested-by: for you.
> > 
> > But a Tested-by would be more valuable than an Acked-by if you
> > don't actually know how it works.
> 
> $ git am joes.patch
> $ echo "int foo __attribute__((section(.hello)));" >> arch/x86/boot/a20.c

Does this compile?

checkpatch is not a compiler.

I think you need

__attribute__((section(".hello")))

> $ git commit arch/x86/boot/a20.c -sm "foo: bar\n baz"
> $ git format-patch HEAD~
> $ ./scripts/checkpatch.pl 0001-foo-bar.patch
> total: 0 errors, 0 warnings, 4 lines checked
> 
> 0001-foo-bar.patch has no obvious style problems and is ready for submission.
> 
> hmm...

Re: checkpatch.pl should suggest __section

[Nick Desaulniers]

On Mon, Aug 12, 2019 at 11:27 AM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2019-08-12 at 11:20 -0700, Nick Desaulniers wrote:
> > On Fri, Aug 9, 2019 at 4:17 PM Joe Perches <joe@perches.com> wrote:
> > > On Fri, 2019-08-09 at 16:04 -0700, Nick Desaulniers wrote:
> > > > > 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@google.com>
> > >
> > > I'll add a Suggested-by: for you.
> > >
> > > But a Tested-by would be more valuable than an Acked-by if you
> > > don't actually know how it works.
> >
> > $ git am joes.patch
> > $ echo "int foo __attribute__((section(.hello)));" >> arch/x86/boot/a20.c
>
> Does this compile?
>
> checkpatch is not a compiler.
>
> I think you need
>
> __attribute__((section(".hello")))

PEBKAC
➜  kernel-all git:(section_escaping) ✗ ./scripts/checkpatch.pl
0001-x86-boot-hello.patch
WARNING: __section(.hello) is preferred over __attribute__((section(".hello")))
#20: FILE: arch/x86/boot/a20.c:164:
+int foo __attribute__((section(".hello")));

total: 0 errors, 1 warnings, 4 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0001-x86-boot-hello.patch has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Tested-by: Nick Desaulniers <ndesaulniers@google.com>
-- 
Thanks,
~Nick Desaulniers

Re: checkpatch.pl should suggest __section

[Miguel Ojeda]

On Sat, Aug 10, 2019 at 12:21 AM Nick Desaulniers
<ndesaulniers@google.com> 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?

+1 There are a few other attributes that should be renamed, too. It
has been on my TODO list for a while, but I decided to go first to add
support for the missing ones that we do not have (e.g. __nonnull), so
that at some point we could achieve a __attribute__-clean kernel.

But in the mean time, adding this to checkpatch.pl (and other
attributes if not there yet) is a great idea!

Cheers,
Miguel