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

[Simon Glass <sjg@chromium.org>]

Hi,

On Thu, 13 Jan 2022 at 05:52, Tom Rini <trini@konsulko.com> wrote:
>
> 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.

Yes it should work fine and it checks there is no effective access,
which is nice.

For now I'm going ahead with the bloblist series without this, but
I'll send an updated patch on its own.

Regards,
Simon