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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]

On Thu, Jan 13, 2022 at 08:56:02AM +0100, Rasmus Villemoes wrote:
> 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.

OK.  I believe it was commit 7856cd5a6dd6 ("Convert CONFIG_SYS_PCI_64BIT
to Kconfig") a few platforms changed size and as best I can tell, the
used / evaluated value for CONFIG_SYS_PCI_64BIT didn't change.

> > 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?

LTO also gets this correct I assume and yes, I like that better.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]