* checkpatch.pl should suggest __section
@ 2019-08-09 22:21 Nick Desaulniers
2019-08-09 22:58 ` Joe Perches
2019-08-10 13:37 ` Miguel Ojeda
0 siblings, 2 replies; 8+ messages in thread
From: Nick Desaulniers @ 2019-08-09 22:21 UTC (permalink / raw)
To: Joe Perches; +Cc: LKML, Josh Poimboeuf, Sedat Dilek, Miguel Ojeda
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: checkpatch.pl should suggest __section
2019-08-09 22:21 checkpatch.pl should suggest __section Nick Desaulniers
@ 2019-08-09 22:58 ` Joe Perches
2019-08-09 23:04 ` Nick Desaulniers
2019-08-10 13:37 ` Miguel Ojeda
1 sibling, 1 reply; 8+ messages in thread
From: Joe Perches @ 2019-08-09 22:58 UTC (permalink / raw)
To: Nick Desaulniers; +Cc: LKML, Josh Poimboeuf, Sedat Dilek, Miguel Ojeda
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/) {
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: checkpatch.pl should suggest __section
2019-08-09 22:58 ` Joe Perches
@ 2019-08-09 23:04 ` Nick Desaulniers
2019-08-09 23:17 ` Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2019-08-09 23:04 UTC (permalink / raw)
To: Joe Perches
Cc: LKML, Josh Poimboeuf, Sedat Dilek, Miguel Ojeda, Andrew Morton
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: checkpatch.pl should suggest __section
2019-08-09 23:04 ` Nick Desaulniers
@ 2019-08-09 23:17 ` Joe Perches
2019-08-12 18:20 ` Nick Desaulniers
0 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2019-08-09 23:17 UTC (permalink / raw)
To: Nick Desaulniers
Cc: LKML, Josh Poimboeuf, Sedat Dilek, Miguel Ojeda, Andrew Morton
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: checkpatch.pl should suggest __section
2019-08-09 22:21 checkpatch.pl should suggest __section Nick Desaulniers
2019-08-09 22:58 ` Joe Perches
@ 2019-08-10 13:37 ` Miguel Ojeda
1 sibling, 0 replies; 8+ messages in thread
From: Miguel Ojeda @ 2019-08-10 13:37 UTC (permalink / raw)
To: Nick Desaulniers; +Cc: Joe Perches, LKML, Josh Poimboeuf, Sedat Dilek
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: checkpatch.pl should suggest __section
2019-08-09 23:17 ` Joe Perches
@ 2019-08-12 18:20 ` Nick Desaulniers
2019-08-12 18:27 ` Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2019-08-12 18:20 UTC (permalink / raw)
To: Joe Perches
Cc: LKML, Josh Poimboeuf, Sedat Dilek, Miguel Ojeda, Andrew Morton
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: checkpatch.pl should suggest __section
2019-08-12 18:20 ` Nick Desaulniers
@ 2019-08-12 18:27 ` Joe Perches
2019-08-12 19:41 ` Nick Desaulniers
0 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2019-08-12 18:27 UTC (permalink / raw)
To: Nick Desaulniers
Cc: LKML, Josh Poimboeuf, Sedat Dilek, Miguel Ojeda, Andrew Morton
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...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: checkpatch.pl should suggest __section
2019-08-12 18:27 ` Joe Perches
@ 2019-08-12 19:41 ` Nick Desaulniers
0 siblings, 0 replies; 8+ messages in thread
From: Nick Desaulniers @ 2019-08-12 19:41 UTC (permalink / raw)
To: Joe Perches
Cc: LKML, Josh Poimboeuf, Sedat Dilek, Miguel Ojeda, Andrew Morton
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-08-12 19:41 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 22:21 checkpatch.pl should suggest __section Nick Desaulniers
2019-08-09 22:58 ` Joe Perches
2019-08-09 23:04 ` Nick Desaulniers
2019-08-09 23:17 ` Joe Perches
2019-08-12 18:20 ` Nick Desaulniers
2019-08-12 18:27 ` Joe Perches
2019-08-12 19:41 ` Nick Desaulniers
2019-08-10 13:37 ` Miguel Ojeda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).