Hello Aaron Durbin, Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/31777
to review the following change.
Change subject: lint/kconfig: More checks, more errors ......................................................................
lint/kconfig: More checks, more errors
This patch changes a few more Kconfig linter warnings to errors that currently do not show up in the tree and that seem unlikely to become false positive in the future. One instance of duplicated code that essentially checks for the same thing was consolidated.
It also adds a new test for references to boolean Kconfig options that do not use the CONFIG() wrapper macro. It's a little flaky (e.g. hard to handle multi-line comments), but it should be helpful the majority of the time as a warning in a Jenkins comment.
Change-Id: I975ee77d392ed426f76f7671d9b6ef9441656e6a Signed-off-by: Julius Werner jwerner@chromium.org --- M util/lint/kconfig_lint 1 file changed, 27 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/31777/1
diff --git a/util/lint/kconfig_lint b/util/lint/kconfig_lint index 6cf05a4..4d0a755 100755 --- a/util/lint/kconfig_lint +++ b/util/lint/kconfig_lint @@ -199,35 +199,16 @@ my $symbol = $3;
if ( ( exists $symbols{$symbol} ) && ( $symbols{$symbol}{type} ne "string" ) ) { - show_warning( "#ifdef 'CONFIG_$symbol' used at $file:$lineno." + show_error( "#ifdef 'CONFIG_$symbol' used at $file:$lineno." . " Symbols of type '$symbols{$symbol}{type}' are always defined." ); } - } elsif ( $line =~ /^([^:]+):(\d+):\s*#\s*if\s+!?\s*defined\s*(?\s*CONFIG(?:_|()(\w+)/ ) { + } elsif ( $line =~ /^([^:]+):(\d+):.+defined\s*(?\s*CONFIG(?:_|()(\w+)/ ) { my $file = $1; my $lineno = $2; my $symbol = $3;
if ( ( exists $symbols{$symbol} ) && ( $symbols{$symbol}{type} ne "string" ) ) { - show_warning( "#ifdef 'CONFIG_$symbol' used at $file:$lineno." - . " Symbols of type '$symbols{$symbol}{type}' are always defined." ); - } - } - } - - # look for (#if) defined SYMBOL - @ifdef_symbols = @collected_symbols; - while ( my $line = shift @ifdef_symbols ) { - if ( $line =~ /^([^:]+):(\d+):.+defined\s*(\s*CONFIG(?:_|()(\w+)/ ) { - my $file = $1; - my $lineno = $2; - my $symbol = $3; - - #ignore '#if defined(symbol) && symbol' type statements - next - if ( $line =~ /^([^:]+):(\d+):.+defined\s*(\s*CONFIG_$symbol.*(&&|||)\s*!?\s*(?\s*CONFIG_$symbol/ ); - - if ( ( exists $symbols{$symbol} ) && ( $symbols{$symbol}{type} ne "string" ) ) { - show_warning( "defined 'CONFIG_$symbol' used at $file:$lineno." + show_error( "defined(CONFIG_$symbol) used at $file:$lineno." . " Symbols of type '$symbols{$symbol}{type}' are always defined." ); } } @@ -272,10 +253,10 @@ my $symbol = $3;
if ( ( exists $symbols{$symbol} ) ) { - show_warning("#define of symbol 'CONFIG_$symbol' used at $file:$lineno."); + show_error("#define of symbol 'CONFIG_$symbol' used at $file:$lineno."); } else { - show_warning( "#define 'CONFIG_$symbol' used at $file:$lineno." + show_error( "#define 'CONFIG_$symbol' used at $file:$lineno." . " Other #defines should not look like Kconfig symbols." ); } } @@ -383,6 +364,24 @@ . " CONFIG($symbol) should be used for type 'bool'" ); } } + } elsif ( $line =~ /^([^:]+):(\d+):(.+\bCONFIG_.+)/ ) { + my $file = $1; + my $lineno = $2; + $line = $3; + if ( $file =~ /.*.(c|h|asl|ld)/ ) { + while ( $line =~ /(.*)\bCONFIG_(\w+)(.*)/ && $1 !~ ///|/*/ ) { + my $symbol = $2; + $line = $1 . $3; + if ( exists $symbols{$symbol} ) { + if ( $symbols{$symbol}{type} eq "bool" ) { + show_warning( "Naked reference to CONFIG_$symbol used at $file:$lineno." + . " A 'bool' Kconfig should always be accessed through CONFIG($symbol)." ); + } + } else { + show_warning( "Unknown config option CONFIG_$symbol used at $file:$lineno." ); + } + } + } } } } @@ -437,7 +436,7 @@ } } elsif ($symbols{$sym}{type} eq "bool") { if ($symbols{$sym}{$sym_num}{default}{$def_num}{default} =~ /[01YN]/) { - show_warning("default value ($symbols{$sym}{$sym_num}{default}{$def_num}{default}) for bool symbol $sym uses value other than y/n at $filename:$line_no."); + show_error("default value ($symbols{$sym}{$sym_num}{default}{$def_num}{default}) for bool symbol $sym uses value other than y/n at $filename:$line_no."); } else { show_error("non bool default value ($symbols{$sym}{$sym_num}{default}{$def_num}{default}) used for bool symbol $sym at $filename:$line_no."); } @@ -447,7 +446,7 @@
#if a default is already set, display an error if ($default_set) { - show_warning( "Default for '$sym' referenced at $filename:$line_no will never be set" + show_error( "Default for '$sym' referenced at $filename:$line_no will never be set" . " - overridden by default set at $default_filename:$default_line_no" ); } else { @@ -1140,7 +1139,7 @@ ( $type, $expression ) = handle_if_line( $type, $inside_config, $filename, $line_no );
if ( $type =~ /tristate/ ) { - show_warning("$filename:$line_no - tristate types are not used."); + show_error("$filename:$line_no - tristate types are not used."); }
if ($inside_config) { @@ -1265,7 +1264,7 @@
#the directory should exist when using a glob else { - show_warning("Could not find dir '$dir_prefix'"); + show_error("Could not find dir '$dir_prefix'"); } }
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31777 )
Change subject: lint/kconfig: More checks, more errors ......................................................................
Patch Set 2: Code-Review+2
Update the kconfig readme as well?
https://review.coreboot.org/cgit/coreboot.git/tree/util/lint/kconfig_lint_RE...
Hello Aaron Durbin, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31777
to look at the new patch set (#3).
Change subject: lint/kconfig: More checks, more errors ......................................................................
lint/kconfig: More checks, more errors
This patch changes a few more Kconfig linter warnings to errors that currently do not show up in the tree and that seem unlikely to become false positive in the future. One instance of duplicated code that essentially checks for the same thing was consolidated.
It also adds a new test for references to boolean Kconfig options that do not use the CONFIG() wrapper macro. It's a little flaky (e.g. hard to handle multi-line comments), but it should be helpful the majority of the time as a warning in a Jenkins comment.
Change-Id: I975ee77d392ed426f76f7671d9b6ef9441656e6a Signed-off-by: Julius Werner jwerner@chromium.org --- M util/lint/kconfig_lint M util/lint/kconfig_lint_README 2 files changed, 39 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/31777/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31777 )
Change subject: lint/kconfig: More checks, more errors ......................................................................
Patch Set 3:
Update the kconfig readme as well?
Thanks, didn't notice that was there. Updated.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31777 )
Change subject: lint/kconfig: More checks, more errors ......................................................................
Patch Set 3:
*ping*
Can I get another +2?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31777 )
Change subject: lint/kconfig: More checks, more errors ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31777 )
Change subject: lint/kconfig: More checks, more errors ......................................................................
lint/kconfig: More checks, more errors
This patch changes a few more Kconfig linter warnings to errors that currently do not show up in the tree and that seem unlikely to become false positive in the future. One instance of duplicated code that essentially checks for the same thing was consolidated.
It also adds a new test for references to boolean Kconfig options that do not use the CONFIG() wrapper macro. It's a little flaky (e.g. hard to handle multi-line comments), but it should be helpful the majority of the time as a warning in a Jenkins comment.
Change-Id: I975ee77d392ed426f76f7671d9b6ef9441656e6a Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/31777 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M util/lint/kconfig_lint M util/lint/kconfig_lint_README 2 files changed, 39 insertions(+), 38 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/util/lint/kconfig_lint b/util/lint/kconfig_lint index 6cf05a4..4d0a755 100755 --- a/util/lint/kconfig_lint +++ b/util/lint/kconfig_lint @@ -199,35 +199,16 @@ my $symbol = $3;
if ( ( exists $symbols{$symbol} ) && ( $symbols{$symbol}{type} ne "string" ) ) { - show_warning( "#ifdef 'CONFIG_$symbol' used at $file:$lineno." + show_error( "#ifdef 'CONFIG_$symbol' used at $file:$lineno." . " Symbols of type '$symbols{$symbol}{type}' are always defined." ); } - } elsif ( $line =~ /^([^:]+):(\d+):\s*#\s*if\s+!?\s*defined\s*(?\s*CONFIG(?:_|()(\w+)/ ) { + } elsif ( $line =~ /^([^:]+):(\d+):.+defined\s*(?\s*CONFIG(?:_|()(\w+)/ ) { my $file = $1; my $lineno = $2; my $symbol = $3;
if ( ( exists $symbols{$symbol} ) && ( $symbols{$symbol}{type} ne "string" ) ) { - show_warning( "#ifdef 'CONFIG_$symbol' used at $file:$lineno." - . " Symbols of type '$symbols{$symbol}{type}' are always defined." ); - } - } - } - - # look for (#if) defined SYMBOL - @ifdef_symbols = @collected_symbols; - while ( my $line = shift @ifdef_symbols ) { - if ( $line =~ /^([^:]+):(\d+):.+defined\s*(\s*CONFIG(?:_|()(\w+)/ ) { - my $file = $1; - my $lineno = $2; - my $symbol = $3; - - #ignore '#if defined(symbol) && symbol' type statements - next - if ( $line =~ /^([^:]+):(\d+):.+defined\s*(\s*CONFIG_$symbol.*(&&|||)\s*!?\s*(?\s*CONFIG_$symbol/ ); - - if ( ( exists $symbols{$symbol} ) && ( $symbols{$symbol}{type} ne "string" ) ) { - show_warning( "defined 'CONFIG_$symbol' used at $file:$lineno." + show_error( "defined(CONFIG_$symbol) used at $file:$lineno." . " Symbols of type '$symbols{$symbol}{type}' are always defined." ); } } @@ -272,10 +253,10 @@ my $symbol = $3;
if ( ( exists $symbols{$symbol} ) ) { - show_warning("#define of symbol 'CONFIG_$symbol' used at $file:$lineno."); + show_error("#define of symbol 'CONFIG_$symbol' used at $file:$lineno."); } else { - show_warning( "#define 'CONFIG_$symbol' used at $file:$lineno." + show_error( "#define 'CONFIG_$symbol' used at $file:$lineno." . " Other #defines should not look like Kconfig symbols." ); } } @@ -383,6 +364,24 @@ . " CONFIG($symbol) should be used for type 'bool'" ); } } + } elsif ( $line =~ /^([^:]+):(\d+):(.+\bCONFIG_.+)/ ) { + my $file = $1; + my $lineno = $2; + $line = $3; + if ( $file =~ /.*.(c|h|asl|ld)/ ) { + while ( $line =~ /(.*)\bCONFIG_(\w+)(.*)/ && $1 !~ ///|/*/ ) { + my $symbol = $2; + $line = $1 . $3; + if ( exists $symbols{$symbol} ) { + if ( $symbols{$symbol}{type} eq "bool" ) { + show_warning( "Naked reference to CONFIG_$symbol used at $file:$lineno." + . " A 'bool' Kconfig should always be accessed through CONFIG($symbol)." ); + } + } else { + show_warning( "Unknown config option CONFIG_$symbol used at $file:$lineno." ); + } + } + } } } } @@ -437,7 +436,7 @@ } } elsif ($symbols{$sym}{type} eq "bool") { if ($symbols{$sym}{$sym_num}{default}{$def_num}{default} =~ /[01YN]/) { - show_warning("default value ($symbols{$sym}{$sym_num}{default}{$def_num}{default}) for bool symbol $sym uses value other than y/n at $filename:$line_no."); + show_error("default value ($symbols{$sym}{$sym_num}{default}{$def_num}{default}) for bool symbol $sym uses value other than y/n at $filename:$line_no."); } else { show_error("non bool default value ($symbols{$sym}{$sym_num}{default}{$def_num}{default}) used for bool symbol $sym at $filename:$line_no."); } @@ -447,7 +446,7 @@
#if a default is already set, display an error if ($default_set) { - show_warning( "Default for '$sym' referenced at $filename:$line_no will never be set" + show_error( "Default for '$sym' referenced at $filename:$line_no will never be set" . " - overridden by default set at $default_filename:$default_line_no" ); } else { @@ -1140,7 +1139,7 @@ ( $type, $expression ) = handle_if_line( $type, $inside_config, $filename, $line_no );
if ( $type =~ /tristate/ ) { - show_warning("$filename:$line_no - tristate types are not used."); + show_error("$filename:$line_no - tristate types are not used."); }
if ($inside_config) { @@ -1265,7 +1264,7 @@
#the directory should exist when using a glob else { - show_warning("Could not find dir '$dir_prefix'"); + show_error("Could not find dir '$dir_prefix'"); } }
diff --git a/util/lint/kconfig_lint_README b/util/lint/kconfig_lint_README index d20a933..832f862 100644 --- a/util/lint/kconfig_lint_README +++ b/util/lint/kconfig_lint_README @@ -43,23 +43,20 @@ - Show when the range set for a hex or int does not match a previous range
Warnings in Kconfig files: -- Any 'default' expressions that can never be reached. - Symbols that are defined but never used. -- Directories specified in a 'source' keyword do not exist. - A 'source' keyword loading a Kconfig file that has already been loaded. - A 'source' keyword loading a Kconfig file that doesn't exist. Note that globs are excluded from this check.
Warnings in coreboot source files: -- #define of Kconfig symbol - Symbols should only be defined in Kconfig. -- #define starting with 'CONFIG_' - these should be reserved for Kconfig - symbols. - 'IS_ENABLED()' block that could not be interpreted. - Kconfig files that are not loaded by a 'source' keyword. -- '#ifdef' or '#if defined' used on bool, int, or hex - these are always - defined in coreboot's version of Kconfig. +- IS_ENABLED() used on a Kconfig (deprecated in favor of CONFIG()) +- Naked use of boolean CONFIG_XXX Kconfig in C that's not wrapped in CONFIG()
Errors in Kconfig files: +- Any 'default' expressions that can never be reached. +- Directories specified in a 'source' keyword do not exist. - Selects do not work on symbols created in a choice block. - All symbols used in selects or expressions must be defined in a config statement. @@ -94,8 +91,13 @@ - Symbols with no defined type.
Errors in coreboot source files: -- The IS_ENABLED macro is only valid for bool symbols. -- The IS_ENABLED used on unknown CONFIG_ value, like an obsolete symbol. -- The IS_ENABLED macro is used on a symbol without the CONFIG_ prefix. +- #define of Kconfig symbol - Symbols should only be defined in Kconfig. +- #define starting with 'CONFIG_' - these should be reserved for Kconfig + symbols. +- '#ifdef' or '#if defined' used on bool, int, or hex - these are always + defined in coreboot's version of Kconfig. +- The CONFIG() and IS_ENABLED() macros is only valid for bool symbols. +- CONFIG() or IS_ENABLED() used on unknown Kconfig, like an obsolete symbol. +- The IS_ENABLED() macro is used on a symbol without the CONFIG_ prefix.
TODO: check for choice entries at the top level
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31777 )
Change subject: lint/kconfig: More checks, more errors ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31777/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31777/4//COMMIT_MSG@7 PS4, Line 7: lint/kconfig: More checks, more errors
Change more checks to error level
-
Julius Werner (Code Review) -
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)