linux-kernel - Re: [PATCH] checkpatch: types found in one source file do not affect processing of others

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150518170312.GL29885@bark>
Date:	Mon, 18 May 2015 18:03:12 +0100
From:	Andy Whitcroft <apw@...onical.com>
To:	Alex Dowad <alexinbeijing@...il.com>
Cc:	Joe Perches <joe@...ches.com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] checkpatch: types found in one source file do not affect
 processing of others

On Mon, May 18, 2015 at 03:33:29PM +0200, Alex Dowad wrote:
> checkpatch uses various cues in its input files to discover the names of
> user-defined types. It then uses that information when processing expressions,
> to discover more style issues.
> 
> Unfortunately, in rare cases, this means that checkpatch may give different
> results if you run it on several files at the same time, or one by one! The
> reason is that it may identify a type (or something that looks like a type)
> in one file, and then carry this information over when processing a different
> file.
> 
> As an example, drivers/staging/media/bcm2048/radio-bcm2048.c contains this
> line (in a macro):
> 
>   size value;
> 
> Then drivers/staging/media/davinci_vpfe/vpfe_video.c has this line:
> 
>   while (size * *nbuffers > vpfe_dev->video_limit)
> 
> If checkpatch processes these 2 files together, the (spurious) "size" type
> detected in the first file will cause it to flag the second file for
> improper use of the pointer dereference operator!
> 
> Therefore, keep user-defined types in a separate array from built-in ones,
> and reset the array of user-defined types at the beginning of each new
> source file.

Yes that sounds like confusing behaviour.  Resetting the list per file
is probabally more in line with user expectation.

> Signed-off-by: Alex Dowad <alexinbeijing@...il.com>
> ---
> 
> Dear patch checkers,
> 
> I am not a Perl programmer -- please let me know if there is a better way to
> accomplish what I am trying to do here.
> 
> Your feedback will be appreciated,
> Alex Dowad
> 
>  scripts/checkpatch.pl | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 89b1df4..5a5668f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -424,6 +424,10 @@ our @typeListWithAttr = (
>  	qr{union\s+$InitAttribute\s+$Ident},
>  );
>  
> +# includes user-defined types discovered in the code
> +# reset at the beginning of each source file
> +our @allTypeList = (@typeList);
> +

Perhaps @typeListFile or something to say it is per file, then when you
reset it it might be more obvious what you are doing.  Or rename the
original list to typeListDefault and then you are making the current one
the default list.  Otherwise the change seems to make sense.  I did not
check you got all the places which use it.

>  our @modifierList = (
>  	qr{fastcall},
>  );
> @@ -511,7 +515,7 @@ $misspellings = join("|", sort keys %spelling_fix) if keys %spelling_fix;
>  
>  sub build_types {
>  	my $mods = "(?x:  \n" . join("|\n  ", @modifierList) . "\n)";
> -	my $all = "(?x:  \n" . join("|\n  ", @typeList) . "\n)";
> +	my $all = "(?x:  \n" . join("|\n  ", @allTypeList) . "\n)";
>  	my $Misordered = "(?x:  \n" . join("|\n  ", @typeListMisordered) . "\n)";
>  	my $allWithAttr = "(?x:  \n" . join("|\n  ", @typeListWithAttr) . "\n)";
>  	$Modifier	= qr{(?:$Attribute|$Sparse|$mods)};
> @@ -745,6 +749,7 @@ for my $filename (@ARGV) {
>  	@fixed = ();
>  	@fixed_inserted = ();
>  	@fixed_deleted = ();
> +	@allTypeList = (@typeList);
>  	$fixlinenr = -1;
>  }
>  
> @@ -1616,7 +1621,7 @@ sub possible {
>  
>  		} else {
>  			warn "POSSIBLE: $possible ($line)\n" if ($dbg_possible);
> -			push(@typeList, $possible);
> +			push(@allTypeList, $possible);
>  		}
>  		build_types();
>  	} else {
 -- 
> 2.0.0.GIT

-apw
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/