Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32229 ) Change subject: util/lint: Make usage of IS_ENABLED() an error ...................................................................... util/lint: Make usage of IS_ENABLED() an error As long as we keep the IS_ENABLED() definition in libpayload for compatibility, we should check that IS_ENABLED() usage doesn't sneak back in. Also remove all other IS_ENABLED() checks. Change-Id: Id30ffa0089cec6c24fc3dbbb10a1be35f63b3d89 Signed-off-by: Nico Huber <nico.h@gmx.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/32229 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Julius Werner <jwerner@chromium.org> Reviewed-by: Patrick Georgi <pgeorgi@google.com> --- M util/lint/checkpatch.pl M util/lint/kconfig_lint M util/lint/kconfig_lint_README 3 files changed, 16 insertions(+), 57 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved Julius Werner: Looks good to me, but someone else must approve diff --git a/util/lint/checkpatch.pl b/util/lint/checkpatch.pl index 265b04c..1affdb7 100755 --- a/util/lint/checkpatch.pl +++ b/util/lint/checkpatch.pl @@ -6184,16 +6184,6 @@ } } -# check for #if defined CONFIG_<FOO> || defined CONFIG_<FOO>_MODULE - if ($line =~ /^\+\s*#\s*if\s+defined(?:\s*\(?\s*|\s+)(CONFIG_[A-Z_]+)\s*\)?\s*\|\|\s*defined(?:\s*\(?\s*|\s+)\1_MODULE\s*\)?\s*$/) { - my $config = $1; - if (WARN("PREFER_IS_ENABLED", - "Prefer IS_ENABLED(<FOO>) to CONFIG_<FOO> || CONFIG_<FOO>_MODULE\n" . $herecurr) && - $fix) { - $fixed[$fixlinenr] = "\+#if IS_ENABLED($config)"; - } - } - # check for case / default statements not preceded by break/fallthrough/switch if ($line =~ /^.\s*(?:case\s+(?:$Ident|$Constant)\s*|default):/) { my $has_break = 0; diff --git a/util/lint/kconfig_lint b/util/lint/kconfig_lint index 502a618..47f0494 100755 --- a/util/lint/kconfig_lint +++ b/util/lint/kconfig_lint @@ -93,7 +93,7 @@ check_used_symbols(); check_for_ifdef(); check_for_def(); - check_is_enabled(); + check_config_macro(); check_selected_symbols(); # Run checks based on the data that was found @@ -213,27 +213,6 @@ } } } - - my @collected_is_enabled; - if ($dont_use_git_grep) { - @collected_is_enabled = - `grep -Irn -- "\\<IS_ENABLED[[:space:]]*(.*)" | grep -v '$exclude_dirs_and_files' | grep -v "kconfig.h"`; - } - else { - @collected_is_enabled = - `git grep -In -- "\\<IS_ENABLED[[:space:]]*(.*)" | grep -v '$exclude_dirs_and_files' | grep -v "kconfig.h"`; - } - - while ( my $line = shift @collected_is_enabled ) { - if ($line !~ /CONFIG_/ && $line =~ /^([^:]+):(\d+):.+IS_ENABLED\s*\(\s*(\w+)/ ) { - my $file = $1; - my $lineno = $2; - my $symbol = $3; - if ( ( exists $symbols{$symbol} ) ) { - show_error("IS_ENABLED missing CONFIG_ prefix on symbol '$symbol' at $file:$lineno."); - } - } - } } #------------------------------------------------------------------------------- @@ -289,11 +268,15 @@ } #------------------------------------------------------------------------------- -# check_is_enabled - The IS_ENABLED() and CONFIG() macros are only valid for -# symbols of type bool. It would probably work on type hex or int if the value -# was 0 or 1, but this seems like a bad plan. Using it on strings is dead out. +# check_config_macro - The CONFIG() macro is only valid for symbols of type +# bool. It would probably work on type hex or int if the value was 0 or 1, +# but this seems like a bad plan. Using it on strings is dead out. +# +# The IS_ENABLED() macro is forbidden in coreboot now. Though, as long as +# we keep its definition in libpayload for compatibility, we have to check +# that it doesn't sneak back in. #------------------------------------------------------------------------------- -sub check_is_enabled { +sub check_config_macro { my @is_enabled_symbols = @collected_symbols; #sort through symbols found by grep and store them in a hash for easy access @@ -322,31 +305,19 @@ my $lineno = $2; $line = $3; if ( ( $line !~ /(.*)IS_ENABLED\s*\(\s*CONFIG_(\w+)(.*)/ ) && ( $line !~ /(\/[\*\/])(.*)IS_ENABLED/ ) ) { - show_warning("# uninterpreted IS_ENABLED at $file:$lineno: $line"); + show_error("# uninterpreted IS_ENABLED at $file:$lineno: $line"); next; } while ( $line =~ /(.*)IS_ENABLED\s*\(\s*CONFIG_(\w+)(.*)/ ) { my $symbol = $2; $line = $1 . $3; - - #make sure that the type is bool - if ( exists $symbols{$symbol} ) { - if ( $symbols{$symbol}{type} ne "bool" ) { - show_error( "IS_ENABLED(CONFIG_$symbol) used at $file:$lineno." - . " IS_ENABLED is only valid for type 'bool', not '$symbols{$symbol}{type}'." ); - } else { - show_warning("IS_ENABLED(CONFIG_$symbol) at $file:$lineno is deprecated. Use CONFIG($symbol) instead." ); - } - } - else { - show_error("IS_ENABLED() used on unknown value CONFIG_$symbol at $file:$lineno."); - } + show_error("IS_ENABLED(CONFIG_$symbol) at $file:$lineno is deprecated. Use CONFIG($symbol) instead."); } } elsif ( $line =~ /^([^:]+):(\d+):\s*#\s*(?:el)?if\s+!?\s*\(?\s*CONFIG_(\w+)\)?(\s*==\s*1)?.*?$/ ) { my $file = $1; my $lineno = $2; my $symbol = $3; - # If the type is bool, give a warning that IS_ENABLED should be used + # If the type is bool, give a warning that CONFIG() should be used if ( exists $symbols{$symbol} ) { if ( $symbols{$symbol}{type} eq "bool" ) { show_error( "#if CONFIG_$symbol used at $file:$lineno." @@ -357,7 +328,7 @@ my $file = $1; my $lineno = $2; my $symbol = $3; - # If the type is bool, give a warning that IS_ENABLED should be used + # If the type is bool, give a warning that CONFIG() should be used if ( exists $symbols{$symbol} ) { if ( $symbols{$symbol}{type} eq "bool" ) { show_error( "#if CONFIG_$symbol used at $file:$lineno." diff --git a/util/lint/kconfig_lint_README b/util/lint/kconfig_lint_README index 832f862..fdf11e0 100644 --- a/util/lint/kconfig_lint_README +++ b/util/lint/kconfig_lint_README @@ -49,9 +49,7 @@ globs are excluded from this check. Warnings in coreboot source files: -- 'IS_ENABLED()' block that could not be interpreted. - Kconfig files that are not loaded by a 'source' keyword. -- 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: @@ -96,8 +94,8 @@ 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. +- The CONFIG() macro is only valid for bool symbols. +- CONFIG() used on unknown Kconfig, like an obsolete symbol. +- The deprecated IS_ENABLED() macro is used. TODO: check for choice entries at the top level -- To view, visit https://review.coreboot.org/c/coreboot/+/32229 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id30ffa0089cec6c24fc3dbbb10a1be35f63b3d89 Gerrit-Change-Number: 32229 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: merged