* [PATCH] checkpatch: Warn about data_race() without comment
@ 2020-04-01 10:17 Marco Elver
2020-04-01 10:32 ` Will Deacon
2020-04-01 15:17 ` Joe Perches
0 siblings, 2 replies; 7+ messages in thread
From: Marco Elver @ 2020-04-01 10:17 UTC (permalink / raw)
To: elver
Cc: paulmck, dvyukov, glider, andreyknvl, kasan-dev, linux-kernel,
apw, joe, Will Deacon
Warn about applications of data_race() without a comment, to encourage
documenting the reasoning behind why it was deemed safe.
Suggested-by: Will Deacon <will@kernel.org>
Signed-off-by: Marco Elver <elver@google.com>
---
scripts/checkpatch.pl | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a63380c6b0d2..48bb9508e300 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5833,6 +5833,14 @@ sub process {
}
}
+# check for data_race without a comment.
+ if ($line =~ /\bdata_race\s*\(/) {
+ if (!ctx_has_comment($first_line, $linenr)) {
+ WARN("DATA_RACE",
+ "data_race without comment\n" . $herecurr);
+ }
+ }
+
# check for smp_read_barrier_depends and read_barrier_depends
if (!$file && $line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
WARN("READ_BARRIER_DEPENDS",
--
2.26.0.rc2.310.g2932bb562d-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: Warn about data_race() without comment
2020-04-01 10:17 [PATCH] checkpatch: Warn about data_race() without comment Marco Elver
@ 2020-04-01 10:32 ` Will Deacon
2020-04-01 15:17 ` Joe Perches
1 sibling, 0 replies; 7+ messages in thread
From: Will Deacon @ 2020-04-01 10:32 UTC (permalink / raw)
To: Marco Elver
Cc: paulmck, dvyukov, glider, andreyknvl, kasan-dev, linux-kernel, apw, joe
On Wed, Apr 01, 2020 at 12:17:14PM +0200, Marco Elver wrote:
> Warn about applications of data_race() without a comment, to encourage
> documenting the reasoning behind why it was deemed safe.
>
> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> scripts/checkpatch.pl | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index a63380c6b0d2..48bb9508e300 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5833,6 +5833,14 @@ sub process {
> }
> }
>
> +# check for data_race without a comment.
> + if ($line =~ /\bdata_race\s*\(/) {
> + if (!ctx_has_comment($first_line, $linenr)) {
> + WARN("DATA_RACE",
> + "data_race without comment\n" . $herecurr);
> + }
> + }
> +
Thanks, looks sane to me:
Acked-by: Will Deacon <will@kernel.org>
Although I suppose I now need to add some comments to my list stuff. I
didn't think that through, did I? ;)
Will
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: Warn about data_race() without comment
2020-04-01 10:17 [PATCH] checkpatch: Warn about data_race() without comment Marco Elver
2020-04-01 10:32 ` Will Deacon
@ 2020-04-01 15:17 ` Joe Perches
2020-04-01 15:38 ` Paul E. McKenney
2020-04-16 9:27 ` [PATCH] checkpatch: Warn about data_race() without comment Marco Elver
1 sibling, 2 replies; 7+ messages in thread
From: Joe Perches @ 2020-04-01 15:17 UTC (permalink / raw)
To: Marco Elver, Andrew Morton
Cc: paulmck, dvyukov, glider, andreyknvl, kasan-dev, linux-kernel,
apw, Will Deacon
On Wed, 2020-04-01 at 12:17 +0200, Marco Elver wrote:
> Warn about applications of data_race() without a comment, to encourage
> documenting the reasoning behind why it was deemed safe.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -5833,6 +5833,14 @@ sub process {
> }
> }
>
> +# check for data_race without a comment.
> + if ($line =~ /\bdata_race\s*\(/) {
> + if (!ctx_has_comment($first_line, $linenr)) {
> + WARN("DATA_RACE",
> + "data_race without comment\n" . $herecurr);
> + }
> + }
> +
> # check for smp_read_barrier_depends and read_barrier_depends
> if (!$file && $line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
> WARN("READ_BARRIER_DEPENDS",
Sensible enough but it looks like ctx_has_comment should
be updated to allow c99 comments too, but that should be
a separate change from this patch.
Otherwise, this style emits a message:
WARNING: data_race without comment
#135: FILE: kernel/rcu/tasks.h:135:
+ int i = data_race(rtp->gp_state); // Let KCSAN detect update races
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: Warn about data_race() without comment
2020-04-01 15:17 ` Joe Perches
@ 2020-04-01 15:38 ` Paul E. McKenney
2020-04-02 2:20 ` [PATCH] checkpatch: Look for c99 comments in ctx_locate_comment Joe Perches
2020-04-16 9:27 ` [PATCH] checkpatch: Warn about data_race() without comment Marco Elver
1 sibling, 1 reply; 7+ messages in thread
From: Paul E. McKenney @ 2020-04-01 15:38 UTC (permalink / raw)
To: Joe Perches
Cc: Marco Elver, Andrew Morton, dvyukov, glider, andreyknvl,
kasan-dev, linux-kernel, apw, Will Deacon
On Wed, Apr 01, 2020 at 08:17:52AM -0700, Joe Perches wrote:
> On Wed, 2020-04-01 at 12:17 +0200, Marco Elver wrote:
> > Warn about applications of data_race() without a comment, to encourage
> > documenting the reasoning behind why it was deemed safe.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -5833,6 +5833,14 @@ sub process {
> > }
> > }
> >
> > +# check for data_race without a comment.
> > + if ($line =~ /\bdata_race\s*\(/) {
> > + if (!ctx_has_comment($first_line, $linenr)) {
> > + WARN("DATA_RACE",
> > + "data_race without comment\n" . $herecurr);
> > + }
> > + }
> > +
> > # check for smp_read_barrier_depends and read_barrier_depends
> > if (!$file && $line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
> > WARN("READ_BARRIER_DEPENDS",
>
> Sensible enough but it looks like ctx_has_comment should
> be updated to allow c99 comments too, but that should be
> a separate change from this patch.
>
> Otherwise, this style emits a message:
>
> WARNING: data_race without comment
> #135: FILE: kernel/rcu/tasks.h:135:
> + int i = data_race(rtp->gp_state); // Let KCSAN detect update races
>
Yes, please!
Thanx, Paul
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] checkpatch: Look for c99 comments in ctx_locate_comment
2020-04-01 15:38 ` Paul E. McKenney
@ 2020-04-02 2:20 ` Joe Perches
2020-04-02 3:12 ` Paul E. McKenney
0 siblings, 1 reply; 7+ messages in thread
From: Joe Perches @ 2020-04-02 2:20 UTC (permalink / raw)
To: Andrew Morton
Cc: paulmck, Marco Elver, dvyukov, glider, andreyknvl, kasan-dev,
linux-kernel, apw, Will Deacon
Some checks look for comments around a specific function like
read_barrier_depends.
Extend the check to support both c89 and c90 comment styles.
c89 /* comment */
or
c99 // comment
For c99 comments, only look a 3 single lines, the line being scanned,
the line above and the line below the line being scanned rather than
the patch diff context.
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d64c67..0f4db4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1674,8 +1674,16 @@ sub ctx_statement_level {
sub ctx_locate_comment {
my ($first_line, $end_line) = @_;
+ # If c99 comment on the current line, or the line before or after
+ my ($current_comment) = ($rawlines[$end_line - 1] =~ m@^\+.*(//.*$)@);
+ return $current_comment if (defined $current_comment);
+ ($current_comment) = ($rawlines[$end_line - 2] =~ m@^[\+ ].*(//.*$)@);
+ return $current_comment if (defined $current_comment);
+ ($current_comment) = ($rawlines[$end_line] =~ m@^[\+ ].*(//.*$)@);
+ return $current_comment if (defined $current_comment);
+
# Catch a comment on the end of the line itself.
- my ($current_comment) = ($rawlines[$end_line - 1] =~ m@.*(/\*.*\*/)\s*(?:\\\s*)?$@);
+ ($current_comment) = ($rawlines[$end_line - 1] =~ m@.*(/\*.*\*/)\s*(?:\\\s*)?$@);
return $current_comment if (defined $current_comment);
# Look through the context and try and figure out if there is a
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: Look for c99 comments in ctx_locate_comment
2020-04-02 2:20 ` [PATCH] checkpatch: Look for c99 comments in ctx_locate_comment Joe Perches
@ 2020-04-02 3:12 ` Paul E. McKenney
0 siblings, 0 replies; 7+ messages in thread
From: Paul E. McKenney @ 2020-04-02 3:12 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Marco Elver, dvyukov, glider, andreyknvl,
kasan-dev, linux-kernel, apw, Will Deacon
On Wed, Apr 01, 2020 at 07:20:30PM -0700, Joe Perches wrote:
> Some checks look for comments around a specific function like
> read_barrier_depends.
>
> Extend the check to support both c89 and c90 comment styles.
>
> c89 /* comment */
> or
> c99 // comment
>
> For c99 comments, only look a 3 single lines, the line being scanned,
> the line above and the line below the line being scanned rather than
> the patch diff context.
>
> Signed-off-by: Joe Perches <joe@perches.com>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
> ---
> scripts/checkpatch.pl | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d64c67..0f4db4 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1674,8 +1674,16 @@ sub ctx_statement_level {
> sub ctx_locate_comment {
> my ($first_line, $end_line) = @_;
>
> + # If c99 comment on the current line, or the line before or after
> + my ($current_comment) = ($rawlines[$end_line - 1] =~ m@^\+.*(//.*$)@);
> + return $current_comment if (defined $current_comment);
> + ($current_comment) = ($rawlines[$end_line - 2] =~ m@^[\+ ].*(//.*$)@);
> + return $current_comment if (defined $current_comment);
> + ($current_comment) = ($rawlines[$end_line] =~ m@^[\+ ].*(//.*$)@);
> + return $current_comment if (defined $current_comment);
> +
> # Catch a comment on the end of the line itself.
> - my ($current_comment) = ($rawlines[$end_line - 1] =~ m@.*(/\*.*\*/)\s*(?:\\\s*)?$@);
> + ($current_comment) = ($rawlines[$end_line - 1] =~ m@.*(/\*.*\*/)\s*(?:\\\s*)?$@);
> return $current_comment if (defined $current_comment);
>
> # Look through the context and try and figure out if there is a
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: Warn about data_race() without comment
2020-04-01 15:17 ` Joe Perches
2020-04-01 15:38 ` Paul E. McKenney
@ 2020-04-16 9:27 ` Marco Elver
1 sibling, 0 replies; 7+ messages in thread
From: Marco Elver @ 2020-04-16 9:27 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Paul E. McKenney, Dmitry Vyukov,
Alexander Potapenko, Andrey Konovalov, kasan-dev, LKML, apw,
Will Deacon
On Wed, 1 Apr 2020 at 17:19, Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2020-04-01 at 12:17 +0200, Marco Elver wrote:
> > Warn about applications of data_race() without a comment, to encourage
> > documenting the reasoning behind why it was deemed safe.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -5833,6 +5833,14 @@ sub process {
> > }
> > }
> >
> > +# check for data_race without a comment.
> > + if ($line =~ /\bdata_race\s*\(/) {
> > + if (!ctx_has_comment($first_line, $linenr)) {
> > + WARN("DATA_RACE",
> > + "data_race without comment\n" . $herecurr);
> > + }
> > + }
> > +
> > # check for smp_read_barrier_depends and read_barrier_depends
> > if (!$file && $line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
> > WARN("READ_BARRIER_DEPENDS",
Do we still want to do this? Which tree can pick this up? Or was there
anything left that we missed?
> Sensible enough but it looks like ctx_has_comment should
> be updated to allow c99 comments too, but that should be
> a separate change from this patch.
AFAIK the C99 comment patch is in -mm now.
> Otherwise, this style emits a message:
>
> WARNING: data_race without comment
> #135: FILE: kernel/rcu/tasks.h:135:
> + int i = data_race(rtp->gp_state); // Let KCSAN detect update races
>
Thanks,
-- Marco
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-16 9:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 10:17 [PATCH] checkpatch: Warn about data_race() without comment Marco Elver
2020-04-01 10:32 ` Will Deacon
2020-04-01 15:17 ` Joe Perches
2020-04-01 15:38 ` Paul E. McKenney
2020-04-02 2:20 ` [PATCH] checkpatch: Look for c99 comments in ctx_locate_comment Joe Perches
2020-04-02 3:12 ` Paul E. McKenney
2020-04-16 9:27 ` [PATCH] checkpatch: Warn about data_race() without comment Marco Elver
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).