[PATCH] checkpatch: Warn about data_race() without comment

[PATCH] checkpatch: Warn about data_race() without comment

[Marco Elver]

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

Re: [PATCH] checkpatch: Warn about data_race() without comment

[Will Deacon]

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

Re: [PATCH] checkpatch: Warn about data_race() without comment

[Joe Perches]

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

Re: [PATCH] checkpatch: Warn about data_race() without comment

[Paul E. McKenney]

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

[PATCH] checkpatch: Look for c99 comments in ctx_locate_comment

[Joe Perches]

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

Re: [PATCH] checkpatch: Look for c99 comments in ctx_locate_comment

[Paul E. McKenney]

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
> 
> 

Re: [PATCH] checkpatch: Warn about data_race() without comment

[Marco Elver]

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