Received-SPF: pass (google.com: domain of linux-kernel+bounces-199734-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <96132fe8-a0ba-4bd7-8019-816f02afc426@infradead.org>
Date: Mon, 3 Jun 2024 14:30:12 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/3] kconfig: remove wrong expr_trans_bool()
To: Masahiro Yamada <masahiroy@kernel.org>, linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
References: <20240603161904.1663388-1-masahiroy@kernel.org>
 <20240603161904.1663388-3-masahiroy@kernel.org>
Content-Language: en-US
From: Randy Dunlap <rdunlap@infradead.org>
In-Reply-To: <20240603161904.1663388-3-masahiroy@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 6/3/24 9:19 AM, Masahiro Yamada wrote:
> expr_trans_bool() performs an incorrect transformation.
> 
> [Test Code]
> 
>     config MODULES
>             def_bool y
>             modules
> 
>     config A
>             def_bool y
>             select C if B != n
> 
>     config B
>             def_tristate m
> 
>     config C
>             tristate
> 
> [Result]
> 
>     CONFIG_MODULES=y
>     CONFIG_A=y
>     CONFIG_B=m
>     CONFIG_C=m
> 
> This result is incorrect because CONFIG_C=y is expected.
> 
> Documentation/kbuild/kconfig-language.rst clearly explains the function
> of the '!=' operator:
> 
>   (3) If the values of both symbols are equal, it returns 'n',
>       otherwise 'y'.
> 
> Therefore, the statement:
> 
>     select C if A != n
> 
> should be equivalent to:
> 
>     select C if y
> 
> Hence, the symbol C should be selected by 'y' instead of 'm'.
> 
> The comment block of expr_trans_bool() correctly explains its intention:
> 
>   * bool FOO!=n => FOO
>     ^^^^
> 
> If FOO is bool, FOO!=n can be simplified into FOO. This is correct.
> 
> However, the actual code performs this transformation when FOO is
> tristate.
> 
>     if (e->left.sym->type == S_TRISTATE) {
>                              ^^^^^^^^^^
> 
> While, it can be fixed to S_BOOLEAN, there is no point in doing so

  While it can

> because expr_tranform() already transforms FOO!=n to FOO when FOO is
> bool. (see the "case E_UNEQUAL" part)
> 
> expr_trans_bool() is wrong and unnecessary.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Acked-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.

> ---
> 
>  scripts/kconfig/expr.c | 29 -----------------------------
>  scripts/kconfig/expr.h |  1 -
>  scripts/kconfig/menu.c |  2 --
>  3 files changed, 32 deletions(-)
> 
> diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
> index 4d95fce5f9a7..fcc190b67b6f 100644
> --- a/scripts/kconfig/expr.c
> +++ b/scripts/kconfig/expr.c
> @@ -396,35 +396,6 @@ static struct expr *expr_eliminate_yn(struct expr *e)
>  	return e;
>  }
>  
> -/*
> - * bool FOO!=n => FOO
> - */
> -struct expr *expr_trans_bool(struct expr *e)
> -{
> -	if (!e)
> -		return NULL;
> -	switch (e->type) {
> -	case E_AND:
> -	case E_OR:
> -	case E_NOT:
> -		e->left.expr = expr_trans_bool(e->left.expr);
> -		e->right.expr = expr_trans_bool(e->right.expr);
> -		break;
> -	case E_UNEQUAL:
> -		// FOO!=n -> FOO
> -		if (e->left.sym->type == S_TRISTATE) {
> -			if (e->right.sym == &symbol_no) {
> -				e->type = E_SYMBOL;
> -				e->right.sym = NULL;
> -			}
> -		}
> -		break;
> -	default:
> -		;
> -	}
> -	return e;
> -}
> -
>  /*
>   * e1 || e2 -> ?
>   */
> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> index fa50fc45622e..7c0c242318bc 100644
> --- a/scripts/kconfig/expr.h
> +++ b/scripts/kconfig/expr.h
> @@ -284,7 +284,6 @@ void expr_free(struct expr *e);
>  void expr_eliminate_eq(struct expr **ep1, struct expr **ep2);
>  int expr_eq(struct expr *e1, struct expr *e2);
>  tristate expr_calc_value(struct expr *e);
> -struct expr *expr_trans_bool(struct expr *e);
>  struct expr *expr_eliminate_dups(struct expr *e);
>  struct expr *expr_transform(struct expr *e);
>  int expr_contains_symbol(struct expr *dep, struct symbol *sym);
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index 53151c5a6028..eef9b63cdf11 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -398,8 +398,6 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
>  				dep = expr_transform(dep);
>  				dep = expr_alloc_and(expr_copy(basedep), dep);
>  				dep = expr_eliminate_dups(dep);
> -				if (menu->sym && menu->sym->type != S_TRISTATE)
> -					dep = expr_trans_bool(dep);
>  				prop->visible.expr = dep;
>  
>  				/*

-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html