Re: [PATCH] checkpatch: Add test for possible misuse of IS_ENABLED() without CONFIG_
Might s well post one that works
Interdiff similar to:
+ if ($sym !~ /^CONFIG_/) {
+ WARN("IS_ENABLED_CONFIG",
+ "IS_ENABLED($sym) is normally used as
IS_ENABLED(CONFIG_$1)\n" . $herecurr);
++ } else {
++ $sym =~ s/^CONFIG_//;
+ }
+ if (!exists($Kconfig_syms{$sym})) {
+ WARN("IS_ENABLED_CONFIG",
-+ "'$sym' is not a known Kconfig config
entry in the current kernel sources\n" . $herecurr);
-+
++ "'config $sym' is not a known Kconfig
config entry in the current kernel sources\n" . $herecurr);
+ }
+ }
+
---
scripts/checkpatch.pl | 102 ++
1 file changed, 102 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5f00df2c3f59..02814c689676 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -47,6 +47,7 @@ my $gitroot = $ENV{'GIT_DIR'};
$gitroot = ".git" if !defined($gitroot);
my %debug;
my %camelcase = ();
+my %Kconfig_syms = ();
my %use_type = ();
my @use = ();
my %ignore_type = ();
@@ -911,6 +912,90 @@ sub is_SPDX_License_valid {
return 1;
}
+sub seed_Kconfig_file {
+ my ($file) = @_;
+
+ return if (!(-f $file));
+
+ local $/;
+
+ open(my $Kconfig_file, '<', "$file")
+ or warn "$P: Can't read '$file' $!\n";
+ my $text = <$Kconfig_file>;
+ close($Kconfig_file);
+
+ my @lines = split('\n', $text);
+
+ foreach my $line (@lines) {
+ next if ($line !~ /^\s*config\s+(\w+)/);
+ $Kconfig_syms{$1} = 1;
+ }
+}
+
+my $Kconfig_symbols_seeded = 0;
+sub seed_Kconfig_symbols {
+ return if ($Kconfig_symbols_seeded);
+
+ my $files;
+ my @Kconfig_files = ();
+ my $Kconfig_syms_cache = "";
+
+ $Kconfig_symbols_seeded = 1;
+
+ if (-e "$gitroot") {
+ my $git_last_include_commit = `${git_command} log --no-merges
--pretty=format:"%h%n" -1 -- include`;
+ chomp $git_last_include_commit;
+ $Kconfig_syms_cache =
".checkpatch-Kconfig_syms.git.$git_last_include_commit";
+ } else {
+ my $last_mod_date = 0;
+ $files = `find $root/ -name "Kconfig*"`;
+ @Kconfig_files = split('\n', $files);
+ foreach my $file (@Kconfig_files) {
+ my $date = POSIX::strftime("%Y%m%d%H%M",
+ localtime((stat $file)[9]));
+ $last_mod_date = $date if ($last_mod_date < $date);
+ }
+ $Kconfig_syms_cache =
".checkpatch-Kconfig_syms.date.$last_mod_date";
+ }
+
+ if ($Kconfig_syms_cache ne "" && -f $Kconfig_syms_cache) {
+ open(my $Kconfig_syms_file, '<', "$Kconfig_syms_cache")
+ or warn "$P: Can't read '$Kconfig_syms_cache' $!\n";
+ while (<$Kconfig_syms_file>) {
+ chomp;
+ $Kconfig_syms{$_} = 1;
+ }
+ close($Kconfig_syms_file);
+
+ return;
+ }
+
+ if (-e "$gitroot") {
+ my @syms = `${git_command} grep -P -oh '^\\s*config\\s+\\w+' --
'*/Kconfig*'`;
+ s/^\s+// for @syms;
+ s/config\s+// for @syms;
+ s/\n$// for @syms;
+ @syms = sort(uniq(@syms));
+ foreach my $sym (@syms) {
+ $Kconfig_syms{$sym} = 1;
+ }
+ } else {
+ foreach my $file (@Kconfig_files) {
+ seed_Kconfig_file($file);
+ }
+ }
+
+ if ($Kconfig_syms_cache ne "") {
+ unlink glob ".checkpatch-Kconfig_syms.*";
+ open(my $Kconfig_syms_file, '>', "$Kconfig_syms_cache")
+ or warn "$P: Can't write '$Kconfig_syms_cache' $!\n";
+ foreach (sort { lc($a) cmp lc($b) } keys(%Kconfig_syms)) {
+ print $Kconfig_syms_file ("$_\n");
+ }
+ close($Kconfig_syms_file);
+ }
+}
+
my $camelcase_seeded = 0;
sub seed_camelcase_includes {
return if ($camelcase_seeded);
@@ -6480,6 +6565,23 @@ sub process {
}
}
+# check for IS_ENABLED() used without CONFIG_ ($rawline for comment use)
+# or if the CONFIG_ symbol is not a known Kconfig entry
+ if ($rawline =~ /\bIS_ENABLED\s*\(\s*(\w+)\s*\)/) {
+ my $sym = $1;
+ seed_Kconfig_symbols();
+ if ($sym !~ /^CONFIG_/) {
+ WARN("IS_ENABLED_CONFIG",
+"IS_ENABLED($sym) is normally used
Re: [PATCH] checkpatch: Add test for possible misuse of IS_ENABLED() without CONFIG_
On Fri, 2020-06-05 at 17:32 -0700, Andrew Morton wrote:
> On Fri, 05 Jun 2020 11:24:43 -0700 Joe Perches wrote:
>
> > IS_ENABLED is almost always used with CONFIG_ defines.
> >
> > Add a test to verify that the #define being tested starts with CONFIG_.
>
> Yay.
>
> I wonder if there's a simple way of testing whether the CONFIG_ thing
> can *ever* be enabled. So detect if someone does
>
> if (IS_ENABLED(CONFIG_BLOCKK))
No, not really. There's no simple way to do that.
It's doable, but it's not at all simple.
I think it would require something similar to the
checkpatch seed_camelcase_includes function to look
for all current config symbols and verify whatever
CONFIG_ against that list.
$ git grep -P -oh "^\s*config\s+\w+" -- '*/Kconfig*' | \
sed -r -e 's/^\s+//' -e 's/\s+/ /g' | \
sort | uniq -cym
Right now that takes ~1.5 seconds with my laptop
against an uncached git tree, and ~0.25 seconds cached.
Without a git tree it takes 20+ seconds.
Anyway, maybe this.
It only does the time consuming lookup when
it finds a IS_ENABLED line.
---
scripts/checkpatch.pl | 101 ++
1 file changed, 101 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5f00df2c3f59..aabb01cf1e6c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -47,6 +47,7 @@ my $gitroot = $ENV{'GIT_DIR'};
$gitroot = ".git" if !defined($gitroot);
my %debug;
my %camelcase = ();
+my %Kconfig_syms = ();
my %use_type = ();
my @use = ();
my %ignore_type = ();
@@ -911,6 +912,90 @@ sub is_SPDX_License_valid {
return 1;
}
+sub seed_Kconfig_file {
+ my ($file) = @_;
+
+ return if (!(-f $file));
+
+ local $/;
+
+ open(my $Kconfig_file, '<', "$file")
+ or warn "$P: Can't read '$file' $!\n";
+ my $text = <$Kconfig_file>;
+ close($Kconfig_file);
+
+ my @lines = split('\n', $text);
+
+ foreach my $line (@lines) {
+ next if ($line !~ /^\s*config\s+(\w+)/);
+ $Kconfig_syms{$1} = 1;
+ }
+}
+
+my $Kconfig_symbols_seeded = 0;
+sub seed_Kconfig_symbols {
+ return if ($Kconfig_symbols_seeded);
+
+ my $files;
+ my @Kconfig_files = ();
+ my $Kconfig_syms_cache = "";
+
+ $Kconfig_symbols_seeded = 1;
+
+ if (-e "$gitroot") {
+ my $git_last_include_commit = `${git_command} log --no-merges
--pretty=format:"%h%n" -1 -- include`;
+ chomp $git_last_include_commit;
+ $Kconfig_syms_cache =
".checkpatch-Kconfig_syms.git.$git_last_include_commit";
+ } else {
+ my $last_mod_date = 0;
+ $files = `find $root/ -name "Kconfig*"`;
+ @Kconfig_files = split('\n', $files);
+ foreach my $file (@Kconfig_files) {
+ my $date = POSIX::strftime("%Y%m%d%H%M",
+ localtime((stat $file)[9]));
+ $last_mod_date = $date if ($last_mod_date < $date);
+ }
+ $Kconfig_syms_cache =
".checkpatch-Kconfig_syms.date.$last_mod_date";
+ }
+
+ if ($Kconfig_syms_cache ne "" && -f $Kconfig_syms_cache) {
+ open(my $Kconfig_syms_file, '<', "$Kconfig_syms_cache")
+ or warn "$P: Can't read '$Kconfig_syms_cache' $!\n";
+ while (<$Kconfig_syms_file>) {
+ chomp;
+ $Kconfig_syms{$_} = 1;
+ }
+ close($Kconfig_syms_file);
+
+ return;
+ }
+
+ if (-e "$gitroot") {
+ my @syms = `${git_command} grep -P -oh '^\\s*config\\s+\\w+' --
'*/Kconfig*'`;
+ s/^\s+// for @syms;
+ s/config\s+// for @syms;
+ s/\n$// for @syms;
+ @syms = sort(uniq(@syms));
+ foreach my $sym (@syms) {
+ $Kconfig_syms{$sym} = 1;
+ }
+ } else {
+ foreach my $file (@Kconfig_files) {
+ seed_Kconfig_file($file);
+ }
+ }
+
+ if ($Kconfig_syms_cache ne "") {
+ unlink glob ".checkpatch-Kconfig_syms.*";
+ open(my $Kconfig_syms_file, '>', "$Kconfig_syms_cache")
+ or warn "$P: Can't write '$Kconfig_syms_cache' $!\n";
+ foreach (sort { lc($a) cmp lc($b) } keys(%Kconfig_syms)) {
+ print $Kconfig_syms_file ("$_\n");
+ }
+ close($Kconfig_syms_file);
+ }
+}
+
my $camelcase_seeded = 0;
sub seed_camelcase_includes {
return if ($camelcase_seeded);
@@ -6480,6 +6565,22 @@ sub process {
}
}
+# check for IS_ENABLED() used without CONFIG_ ($rawline for comment use)
+# or if the CONFIG_ symbol is not a known Kconfig entry
+ if ($rawline =~ /\bIS_ENABLED\s*\(\s*(\w+)\s*\)/) {
+
Re: [PATCH] checkpatch: Add test for possible misuse of IS_ENABLED() without CONFIG_
On Fri, 05 Jun 2020 11:24:43 -0700 Joe Perches wrote: > IS_ENABLED is almost always used with CONFIG_ defines. > > Add a test to verify that the #define being tested starts with CONFIG_. Yay. I wonder if there's a simple way of testing whether the CONFIG_ thing can *ever* be enabled. So detect if someone does if (IS_ENABLED(CONFIG_BLOCKK))
Re: [PATCH] checkpatch: Add test for possible misuse of IS_ENABLED() without CONFIG_
On Fri, Jun 05, 2020 at 11:24:43AM -0700, Joe Perches wrote: > IS_ENABLED is almost always used with CONFIG_ defines. > > Add a test to verify that the #define being tested starts with CONFIG_. > > Signed-off-by: Joe Perches Reviewed-by: Kees Cook -- Kees Cook
