| Message ID | 20211101011734.1614781-3-sjg@chromium.org |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | passage: Define a standard for firmware data flow | expand |
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. > static void init_machine() > { > ... > #ifdef CONFIG_WIBBLE > handle_wibble(); > #endif > } > > Add a new IF_ENABLED_INT() to help with this. So now it is possible to > write, without #ifdefs: > > static void handle_wibble(void) > { > int val = IF_ENABLED_INT(CONFIG_WIBBLE, CONFIG_WIBBLE_ADDR); > > ... > } > > static void init_machine() > { > ... > if (IS_ENABLED(CONFIG_WIBBLE)) > handle_wibble(); > } > > The value will be 0 if CONFIG_WIBBLE is not defined, and > CONFIG_WIBBLE_ADDR if it is. This allows us to reduce the use of #ifdef in > the code, ensuring that the compiler still checks the code even if it is > not ultimately used for a particular build. > > Add a CONFIG_IF_ENABLED_INT() version as well. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > include/linux/kconfig.h | 18 ++++++++++++++++++ > scripts/config_whitelist.txt | 1 + > 2 files changed, 19 insertions(+) > > diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h > index a1d1a298426..119c698a158 100644 > --- a/include/linux/kconfig.h > +++ b/include/linux/kconfig.h > @@ -59,6 +59,18 @@ > */ > #define CONFIG_VAL(option) config_val(option) > > +/* This use a similar mechanism to config_enabled() above */ > +#define config_opt_enabled(cfg, opt_cfg) _config_opt_enabled(cfg, opt_cfg) > +#define _config_opt_enabled(cfg_val, opt_value) \ > + __config_opt_enabled(__ARG_PLACEHOLDER_##cfg_val, opt_value) > +#define __config_opt_enabled(arg1_or_junk, arg2) \ > + ___config_opt_enabled(arg1_or_junk arg2, 0) > +#define ___config_opt_enabled(__ignored, val, ...) val > + > +/* Evaluates to 0 if option is not defined, int_option if it is defined */ > +#define IF_ENABLED_INT(option, int_option) \ > + config_opt_enabled(option, int_option) > + > /* > * Count number of arguments to a variadic macro. Currently only need > * it for 1, 2 or 3 arguments. > @@ -113,5 +125,11 @@ > #define CONFIG_IS_ENABLED(option, ...) \ > __concat(__CONFIG_IS_ENABLED_, __count_args(option, ##__VA_ARGS__)) (option, ##__VA_ARGS__) > > +/* > + * Evaluates to 0 if SPL_/TPL_/option is not defined, SPL_/TPL_int_option if it > + * is defined > + */ > +#define CONFIG_IF_ENABLED_INT(option, int_option) \ > + CONFIG_IS_ENABLED(option, (CONFIG_VAL(int_option)), (0)) > > #endif /* __LINUX_KCONFIG_H */ > diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt > index 022a27288c9..f9d9f4a9cfe 100644 > --- a/scripts/config_whitelist.txt > +++ b/scripts/config_whitelist.txt > @@ -609,6 +609,7 @@ CONFIG_ICS307_REFCLK_HZ > CONFIG_IDE_PREINIT > CONFIG_IDE_RESET > CONFIG_IDE_SWAP_IO > +CONFIG_IF_ENABLED_INT > CONFIG_IMA > CONFIG_IMX > CONFIG_IMX6_PWM_PER_CLK > -- > 2.33.1.1089.g2158813163f-goog > Regards /Ilias
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. Regards, Simon > > > static void init_machine() > > { > > ... > > #ifdef CONFIG_WIBBLE > > handle_wibble(); > > #endif > > } > > > > Add a new IF_ENABLED_INT() to help with this. So now it is possible to > > write, without #ifdefs: > > > > static void handle_wibble(void) > > { > > int val = IF_ENABLED_INT(CONFIG_WIBBLE, CONFIG_WIBBLE_ADDR); > > > > ... > > } > > > > static void init_machine() > > { > > ... > > if (IS_ENABLED(CONFIG_WIBBLE)) > > handle_wibble(); > > } > > > > The value will be 0 if CONFIG_WIBBLE is not defined, and > > CONFIG_WIBBLE_ADDR if it is. This allows us to reduce the use of #ifdef in > > the code, ensuring that the compiler still checks the code even if it is > > not ultimately used for a particular build. > > > > Add a CONFIG_IF_ENABLED_INT() version as well. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > include/linux/kconfig.h | 18 ++++++++++++++++++ > > scripts/config_whitelist.txt | 1 + > > 2 files changed, 19 insertions(+) > > > > diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h > > index a1d1a298426..119c698a158 100644 > > --- a/include/linux/kconfig.h > > +++ b/include/linux/kconfig.h > > @@ -59,6 +59,18 @@ > > */ > > #define CONFIG_VAL(option) config_val(option) > > > > +/* This use a similar mechanism to config_enabled() above */ > > +#define config_opt_enabled(cfg, opt_cfg) _config_opt_enabled(cfg, opt_cfg) > > +#define _config_opt_enabled(cfg_val, opt_value) \ > > + __config_opt_enabled(__ARG_PLACEHOLDER_##cfg_val, opt_value) > > +#define __config_opt_enabled(arg1_or_junk, arg2) \ > > + ___config_opt_enabled(arg1_or_junk arg2, 0) > > +#define ___config_opt_enabled(__ignored, val, ...) val > > + > > +/* Evaluates to 0 if option is not defined, int_option if it is defined */ > > +#define IF_ENABLED_INT(option, int_option) \ > > + config_opt_enabled(option, int_option) > > + > > /* > > * Count number of arguments to a variadic macro. Currently only need > > * it for 1, 2 or 3 arguments. > > @@ -113,5 +125,11 @@ > > #define CONFIG_IS_ENABLED(option, ...) \ > > __concat(__CONFIG_IS_ENABLED_, __count_args(option, ##__VA_ARGS__)) (option, ##__VA_ARGS__) > > > > +/* > > + * Evaluates to 0 if SPL_/TPL_/option is not defined, SPL_/TPL_int_option if it > > + * is defined > > + */ > > +#define CONFIG_IF_ENABLED_INT(option, int_option) \ > > + CONFIG_IS_ENABLED(option, (CONFIG_VAL(int_option)), (0)) > > > > #endif /* __LINUX_KCONFIG_H */ > > diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt > > index 022a27288c9..f9d9f4a9cfe 100644 > > --- a/scripts/config_whitelist.txt > > +++ b/scripts/config_whitelist.txt > > @@ -609,6 +609,7 @@ CONFIG_ICS307_REFCLK_HZ > > CONFIG_IDE_PREINIT > > CONFIG_IDE_RESET > > CONFIG_IDE_SWAP_IO > > +CONFIG_IF_ENABLED_INT > > CONFIG_IMA > > CONFIG_IMX > > CONFIG_IMX6_PWM_PER_CLK > > -- > > 2.33.1.1089.g2158813163f-goog > > > > Regards > /Ilias
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. 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.
Hi Tom, On Wed, 12 Jan 2022 at 14:56, Tom Rini <trini@konsulko.com> 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. I'd like to see that. > > 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. The specific case where this is (to be) used is here: https://patchwork.ozlabs.org/project/uboot/patch/20211101011734.1614781-14-sjg@chromium.org/ addr = IF_ENABLED_INT(CONFIG_BLOBLIST_FIXED, CONFIG_BLOBLIST_ADDR); This is because BLOBLIST_ADDR depends on BLOBLIST_FIXED (it is meaningless to have an address if the bloblits is allocated). One fix is to always have an address, and set it to 0 by default (and not use it) when BLOBLIST_FIXED is not enabled. But it does lead to a strange Kconfig since options are present which are not really used. Another option is to add an accessor to the header file, as is down with global_data (e.g. as is done with gd_of_root()). Thoughts? Regards, Simon
On Wed, Jan 12, 2022 at 03:22:51PM -0700, Simon Glass wrote: > Hi Tom, > > On Wed, 12 Jan 2022 at 14:56, Tom Rini <trini@konsulko.com> 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. > > I'd like to see that. It's one of those fairly maddening cases when it happens. I _think_ it's more correctly shown as: if (CONFIG_FOO && bar->baz) which yes, was excluded but resulted in different code optimization? I went to far as to compare the disassembly and just left scratching my head why it changed. > > 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. > > The specific case where this is (to be) used is here: > > https://patchwork.ozlabs.org/project/uboot/patch/20211101011734.1614781-14-sjg@chromium.org/ > > addr = IF_ENABLED_INT(CONFIG_BLOBLIST_FIXED, CONFIG_BLOBLIST_ADDR); > > This is because BLOBLIST_ADDR depends on BLOBLIST_FIXED (it is > meaningless to have an address if the bloblits is allocated). > > One fix is to always have an address, and set it to 0 by default (and > not use it) when BLOBLIST_FIXED is not enabled. > > But it does lead to a strange Kconfig since options are present which > are not really used. > > Another option is to add an accessor to the header file, as is down > with global_data (e.g. as is done with gd_of_root()). Yeah, lets solve this any other way. If we can do it local to the file, that's best.
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
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.
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
On 13/01/2022 13.52, Tom Rini wrote: > On Thu, Jan 13, 2022 at 08:56:02AM +0100, Rasmus Villemoes wrote: >> On 12/01/2022 22.56, Tom Rini wrote: >>> 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 Can you remember which ones? I'd like to see if I can reproduce. That said, that commit made the Kconfig symbol 'default y if PPC'. Are you really sure all ppc-boards that set CONFIG_PCI also used to set SYS_PCI_64BIT? And another thing I notice is that a lot of the #define removals remove #define CONFIG_SYS_PCI_64BIT and not #define CONFIG_SYS_PCI_64BIT 1 Now that doesn't matter for the places that test the definedness of CONFIG_SYS_PCI_64BIT, because kconfig either doesn't define it or define it with value 1. But it does matter for (the single) IS_ENABLED() use, because IS_ENABLED(bla) evaluates to 1 if and only if bla expands to 1. Or rather, if and only if __ARG_PLACEHOLDER_ concatenated with the expansion of bla in turn expands to "0, " - which only happens if we hit the __ARG_PLACEHOLDER_1 macro. So when bla is defined with an empty expansion, for the purpose of IS_ENABLED, it might as well not be defined or expand to 0 or to gobbledygook. And when one knows what to look for, it's easy to demonstrate: $ export ARCH=ppc $ export CROSS_COMPILE=powerpc-linux-gnu- $ git checkout 7856cd5a6dd6~1 $ make T1042D4RDB_defconfig $ make drivers/pci/pci-uclass.i $ grep -C3 'beyond the 32-bit boundary' drivers/pci/pci-uclass.i if (!(0) && type == 0x00000000 && ((u32)(((pci_addr) >> 16) >> 16))) { ({ if (0) printf(" - beyond the 32-bit boundary, ignoring\n"); }); continue; } $ git checkout 7856cd5a6dd6 $ make T1042D4RDB_defconfig $ make drivers/pci/pci-uclass.i $ grep -C3 'beyond the 32-bit boundary' drivers/pci/pci-uclass.i if (!(1) && type == 0x00000000 && ((u32)(((pci_addr) >> 16) >> 16))) { ({ if (0) printf(" - beyond the 32-bit boundary, ignoring\n"); }); continue; } Whether that change makes the generated code smaller or larger I can't say, but it's certainly not a nop semantically. [Of course, the change is for the better, as the generated code now matches the intention; previously 64 bit pci addresses would be ignored for the boards that had an empty definition of CONFIG_SYS_PCI_64BIT.] But it has nothing whatsoever to do with whether gcc is capable of throwing away a whole "if (0)" block. But I will believe that other Kconfig conversions have been bit by the same issue, making it _seem_ like IS_ENABLED() is somehow at fault and #ifdefs are "better". Rasmus
On Thu, Jan 13, 2022 at 04:01:45PM +0100, Rasmus Villemoes wrote: > On 13/01/2022 13.52, Tom Rini wrote: > > On Thu, Jan 13, 2022 at 08:56:02AM +0100, Rasmus Villemoes wrote: > >> On 12/01/2022 22.56, Tom Rini wrote: > >>> 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 > > Can you remember which ones? I'd like to see if I can reproduce. > > That said, that commit made the Kconfig symbol 'default y if PPC'. Are > you really sure all ppc-boards that set CONFIG_PCI also used to set > SYS_PCI_64BIT? > > And another thing I notice is that a lot of the #define removals remove > > #define CONFIG_SYS_PCI_64BIT > > and not > > #define CONFIG_SYS_PCI_64BIT 1 > > Now that doesn't matter for the places that test the definedness of > CONFIG_SYS_PCI_64BIT, because kconfig either doesn't define it or define > it with value 1. But it does matter for (the single) IS_ENABLED() use, > because IS_ENABLED(bla) evaluates to 1 if and only if bla expands to 1. > Or rather, if and only if __ARG_PLACEHOLDER_ concatenated with the > expansion of bla in turn expands to "0, " - which only happens if we hit > the __ARG_PLACEHOLDER_1 macro. > > So when bla is defined with an empty expansion, for the purpose of > IS_ENABLED, it might as well not be defined or expand to 0 or to > gobbledygook. > > And when one knows what to look for, it's easy to demonstrate: > > $ export ARCH=ppc > $ export CROSS_COMPILE=powerpc-linux-gnu- > $ git checkout 7856cd5a6dd6~1 > $ make T1042D4RDB_defconfig > $ make drivers/pci/pci-uclass.i > $ grep -C3 'beyond the 32-bit boundary' drivers/pci/pci-uclass.i > > if (!(0) && > type == 0x00000000 && ((u32)(((pci_addr) >> 16) >> 16))) { > ({ if (0) printf(" - beyond the 32-bit boundary, ignoring\n"); }); > continue; > } > > $ git checkout 7856cd5a6dd6 > $ make T1042D4RDB_defconfig > $ make drivers/pci/pci-uclass.i > $ grep -C3 'beyond the 32-bit boundary' drivers/pci/pci-uclass.i > > if (!(1) && > type == 0x00000000 && ((u32)(((pci_addr) >> 16) >> 16))) { > ({ if (0) printf(" - beyond the 32-bit boundary, ignoring\n"); }); > continue; > } > > Whether that change makes the generated code smaller or larger I can't > say, but it's certainly not a nop semantically. [Of course, the change > is for the better, as the generated code now matches the intention; > previously 64 bit pci addresses would be ignored for the boards that had > an empty definition of CONFIG_SYS_PCI_64BIT.] > > But it has nothing whatsoever to do with whether gcc is capable of > throwing away a whole "if (0)" block. But I will believe that other > Kconfig conversions have been bit by the same issue, making it _seem_ > like IS_ENABLED() is somehow at fault and #ifdefs are "better". Yes, that's what happened there, thanks for digging in and explaining. What I really meant by "better" was "same size when converting" which is important when migrating a ton of machines I can only very partially test.
diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h index a1d1a298426..119c698a158 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -59,6 +59,18 @@ */ #define CONFIG_VAL(option) config_val(option) +/* This use a similar mechanism to config_enabled() above */ +#define config_opt_enabled(cfg, opt_cfg) _config_opt_enabled(cfg, opt_cfg) +#define _config_opt_enabled(cfg_val, opt_value) \ + __config_opt_enabled(__ARG_PLACEHOLDER_##cfg_val, opt_value) +#define __config_opt_enabled(arg1_or_junk, arg2) \ + ___config_opt_enabled(arg1_or_junk arg2, 0) +#define ___config_opt_enabled(__ignored, val, ...) val + +/* Evaluates to 0 if option is not defined, int_option if it is defined */ +#define IF_ENABLED_INT(option, int_option) \ + config_opt_enabled(option, int_option) + /* * Count number of arguments to a variadic macro. Currently only need * it for 1, 2 or 3 arguments. @@ -113,5 +125,11 @@ #define CONFIG_IS_ENABLED(option, ...) \ __concat(__CONFIG_IS_ENABLED_, __count_args(option, ##__VA_ARGS__)) (option, ##__VA_ARGS__) +/* + * Evaluates to 0 if SPL_/TPL_/option is not defined, SPL_/TPL_int_option if it + * is defined + */ +#define CONFIG_IF_ENABLED_INT(option, int_option) \ + CONFIG_IS_ENABLED(option, (CONFIG_VAL(int_option)), (0)) #endif /* __LINUX_KCONFIG_H */ diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt index 022a27288c9..f9d9f4a9cfe 100644 --- a/scripts/config_whitelist.txt +++ b/scripts/config_whitelist.txt @@ -609,6 +609,7 @@ CONFIG_ICS307_REFCLK_HZ CONFIG_IDE_PREINIT CONFIG_IDE_RESET CONFIG_IDE_SWAP_IO +CONFIG_IF_ENABLED_INT CONFIG_IMA CONFIG_IMX CONFIG_IMX6_PWM_PER_CLK
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 static void init_machine() { ... #ifdef CONFIG_WIBBLE handle_wibble(); #endif } Add a new IF_ENABLED_INT() to help with this. So now it is possible to write, without #ifdefs: static void handle_wibble(void) { int val = IF_ENABLED_INT(CONFIG_WIBBLE, CONFIG_WIBBLE_ADDR); ... } static void init_machine() { ... if (IS_ENABLED(CONFIG_WIBBLE)) handle_wibble(); } The value will be 0 if CONFIG_WIBBLE is not defined, and CONFIG_WIBBLE_ADDR if it is. This allows us to reduce the use of #ifdef in the code, ensuring that the compiler still checks the code even if it is not ultimately used for a particular build. Add a CONFIG_IF_ENABLED_INT() version as well. Signed-off-by: Simon Glass <sjg@chromium.org> --- include/linux/kconfig.h | 18 ++++++++++++++++++ scripts/config_whitelist.txt | 1 + 2 files changed, 19 insertions(+)