Warn about applications of data_race() without a comment, to encourage
documenting the reasoning behind why it was deemed safe.
Suggested-by: Will Deacon <[email protected]>
Signed-off-by: Marco Elver <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Marco Elver <[email protected]>
> ---
> 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 <[email protected]>
Although I suppose I now need to add some comments to my list stuff. I
didn't think that through, did I? ;)
Will
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
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
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 <[email protected]>
---
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
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 <[email protected]>
Tested-by: Paul E. McKenney <[email protected]>
> ---
> 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
>
>
On Wed, 1 Apr 2020 at 17:19, Joe Perches <[email protected]> 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