Change in ...coreboot[master]: kconfig_lint: Make usage of IS_ENABLED() an error

[Nico Huber (Code Review)]

Hello HAOUAS Elyes, Julius Werner, Angel Pons, Patrick Georgi,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/32229

to review the following change.

Change subject: kconfig_lint: Make usage of IS_ENABLED() an error
......................................................................

kconfig_lint: Make usage of IS_ENABLED() an error

Warnings are not caught by our pre-commit hook.

Change-Id: Id30ffa0089cec6c24fc3dbbb10a1be35f63b3d89
Signed-off-by: Nico Huber 
---
M util/lint/kconfig_lint
1 file changed, 1 insertion(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/32229/1

diff --git a/util/lint/kconfig_lint b/util/lint/kconfig_lint
index 4d0a755..25b77bc 100755
--- a/util/lint/kconfig_lint
+++ b/util/lint/kconfig_lint
@@ -328,19 +328,7 @@
             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;

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32229 )

Change subject: kconfig_lint: Make usage of IS_ENABLED() an error
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32229 )

Change subject: kconfig_lint: Make usage of IS_ENABLED() an error
......................................................................

Patch Set 1: Code-Review+1

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32229 )

Change subject: kconfig_lint: Make usage of IS_ENABLED() an error
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/32229/1/util/lint/kconfig_lint 
File util/lint/kconfig_lint:

https://review.coreboot.org/#/c/32229/1/util/lint/kconfig_lint@296 
PS1, Line 296: sub check_is_enabled {
Rename the function too?

https://review.coreboot.org/#/c/32229/1/util/lint/kconfig_lint@320 
PS1, Line 320:         } elsif ( $line =~ /^([^:]+):(\d+):(.+IS_ENABLED.*)/ ) {
I think you might also just kill all the IS_ENABLED() code completely? If you take out the macro itself the compiler will already notice when it's used somewhere.

Change in ...coreboot[master]: util/lint: Make usage of IS_ENABLED() an error

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32229 )

Change subject: util/lint: Make usage of IS_ENABLED() an error
......................................................................

Patch Set 2:

(2 comments)

This change is ready for review.

https://review.coreboot.org/#/c/32229/1/util/lint/kconfig_lint 
File util/lint/kconfig_lint:

https://review.coreboot.org/#/c/32229/1/util/lint/kconfig_lint@296 
PS1, Line 296: sub check_is_enabled {
> Rename the function too?

Done

https://review.coreboot.org/#/c/32229/1/util/lint/kconfig_lint@320 
PS1, Line 320:         } elsif ( $line =~ /^([^:]+):(\d+):(.+IS_ENABLED.*)/ ) {
> I think you might also just kill all the IS_ENABLED() code completely? If you take out the macro its […]

I think it's better to keep it for now, as long as we have the definition
in libpayload.

Change in ...coreboot[master]: util/lint: Make usage of IS_ENABLED() an error

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32229 )

Change subject: util/lint: Make usage of IS_ENABLED() an error
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

LGTM but maybe wait for Patrick since I'm not sure how we deal with checkpatch.pl.

https://review.coreboot.org/#/c/32229/2/util/lint/checkpatch.pl 
File util/lint/checkpatch.pl:

https://review.coreboot.org/#/c/32229/2/util/lint/checkpatch.pl@a6187 
PS2, Line 6187: 
Should we change this to say "prefer CONFIG()" instead? I'm not sure what our stance is on modifying this file. Is the rest of it a clean copy from the kernel repo or are there already other modifications?

Change in ...coreboot[master]: util/lint: Make usage of IS_ENABLED() an error

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32229 )

Change subject: util/lint: Make usage of IS_ENABLED() an error
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: util/lint: Make usage of IS_ENABLED() an error

[Nico Huber (Code Review)]

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 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32229
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
Reviewed-by: Patrick Georgi 
---
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