* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.