Re: [PATCH 02/31] kconfig: Add support for conditional values

From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
To: Tom Rini <trini@konsulko.com>, Simon Glass <sjg@chromium.org>
Cc: "Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
	"U-Boot Mailing List" <u-boot@lists.denx.de>,
	"Bin Meng" <bmeng.cn@gmail.com>,
	"Bill Mills" <bill.mills@linaro.org>,
	"Heinrich Schuchardt" <xypron.glpk@gmx.de>,
	"François Ozog" <francois.ozog@linaro.org>,
	"Masahiro Yamada" <yamada.masahiro@socionext.com>
Subject: Re: [PATCH 02/31] kconfig: Add support for conditional values
Date: Thu, 13 Jan 2022 08:56:02 +0100	[thread overview]
Message-ID: <3a1a318e-7103-c5d4-4068-a498b348e49a@prevas.dk> (raw)
In-Reply-To: <20220112215635.GD9207@bill-the-cat>

On 12/01/2022 22.56, Tom Rini wrote:
> On Wed, Jan 12, 2022 at 02:28:21PM -0700, Simon Glass wrote:
>> Hi Ilias,
>>
>> On Mon, 1 Nov 2021 at 01:05, Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>>>
>>> On Mon, 1 Nov 2021 at 03:19, Simon Glass <sjg@chromium.org> wrote:
>>>>
>>>> At present if an optional Kconfig value needs to be used it must be
>>>> bracketed by #ifdef. For example, with this Kconfig setup:
>>>>
>>>> config WIBBLE
>>>>         bool "Support wibbles, the world needs more wibbles"
>>>>
>>>> config WIBBLE_ADDR
>>>>         hex "Address of the wibble"
>>>>         depends on WIBBLE
>>>>
>>>> then the following code must be used:
>>>>
>>>>  #ifdef CONFIG_WIBBLE
>>>>  static void handle_wibble(void)
>>>>  {
>>>>         int val = CONFIG_WIBBLE_ADDR;
>>>>
>>>>         ...
>>>>  }
>>>>  #endif
>>>>
>>>
>>> The example here might be a bit off and we might need this for int
>>> related values. Was this function handle_wibble() supposed to return
>>> an int or not?  We could shield the linker easier here without adding
>>> macros. Something along the lines of
>>> static void handle_wibble(void)
>>> {
>>> #ifdef CONFIG_WIBBLE
>>> int val = CONFIG_WIBBLE_ADDR;
>>> #endif
>>> }
>>>
>>> In that case you don't an extra ifdef to call handle_wibble().
>>> Personally I find this easier to read.
>>
>> But how does that help with the problem here? I am trying to avoid
>> using preprocessor macros in this case.
> 
> I'm not sure I see a problem here.  A number of the finish-converting-X
> that I did recently had a guard symbol first because usage wasn't fully
> converted but really everyone using that area of code needed to set the
> value, or use the default.
> 
> There might be some cases where we do still need a guard symbol because
> usage is in common code and maybe shouldn't be, but instead moved to
> other usage-specific files.
> 
> I also think I've seen cases where doing:
> if (CONFIG_EVALUATES_TO_ZERO) {
>   ...
> }
> 
> takes more space in the binary than an #ifdef does.

Please provide a specific example. If CONFIG_EVALUATES_TO_ZERO is any
integer-constant-expression evaluating at compile-time to 0, gcc throws
away the whole block very early during parsing. If it doesn't, that's a
compiler bug, so let's please not make decisions based on
not-even-anecdotal data.

> And finally for the moment, we also have many cases where zero is a
> valid value.  That's what leads to potentially harder to read code or
> needing a guard, I think.
> 

I like Simon's idea, but the replacement/fallback should _not_ be a
literal 0. We want a guarantee that the code has actually been discarded
by the compiler or linker (i.e., that the access is done in code that is
otherwise guarded by the "parent" Kconfig symbol), so instead the
fallback should be a call to (the nowhere defined of course)

extern long invalid_use_of_IF_ENABLED_INT(void);

Of course, if people don't build with -O2 and
-ffunction-sections,-fdata-sections and link with --gc-sections, that
may break, but why should we care?

Rasmus