IS_ENABLED() and non-available symbols

IS_ENABLED() and non-available symbols

[Rabin Vincent]

I'm trying to use the new IS_ENABLED() stuff, but it appears that it
only works if the symbol being tested has its dependencies met, and
results in compiler errors if that is not the case.

For example, I've added code using IS_ENABLED(CONFIG_THUMB2_KERNEL) as
in the below patch.  My config is arch/arm/configs/omap2plus_defconfig
which doesn't satisfy the dependencies for CONFIG_THUMB2_KERNEL (defined
in arch/arm/Kconfig).

Compiling results in the following errors (and indeed the macro hasn't
been generated in include/generated/autoconfg.h).  If I modify my config
to satisfy the dependencies for CONFIG_THUMB2_KERNEL so that it is
selectable, the __enabled_CONFIG_THUMB2_KERNEL macro gets generated.

arch/arm/kernel/setup.c: In function 'c_show':
arch/arm/kernel/setup.c:1056: error: '__enabled_CONFIG_THUMB2_KERNEL'
undeclared (first use in this function)
arch/arm/kernel/setup.c:1056: error: (Each undeclared identifier is
reported only once
arch/arm/kernel/setup.c:1056: error: for each function it appears in.)
arch/arm/kernel/setup.c:1056: error:
'__enabled_CONFIG_THUMB2_KERNEL_MODULE' undeclared (first use in this
function)

Kernel is latest linus (v3.1-rc1-272-g73e0881).

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 70bca64..59aa480 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -1053,6 +1053,9 @@ static int c_show(struct seq_file *m, void *v)
        seq_printf(m, "Serial\t\t: %08x%08x\n",
                   system_serial_high, system_serial_low);

+       if (IS_ENABLED(CONFIG_THUMB2_KERNEL))
+               printk("Thumb-2\n");
+
        return 0;
 }

Re: IS_ENABLED() and non-available symbols

[Arnaud Lacombe]

Hi,

On Sun, Aug 14, 2011 at 6:56 AM, Rabin Vincent <rabin@rab.in> wrote:
> I'm trying to use the new IS_ENABLED() stuff, but it appears that it
> only works if the symbol being tested has its dependencies met, and
> results in compiler errors if that is not the case.
>
> For example, I've added code using IS_ENABLED(CONFIG_THUMB2_KERNEL) as
> in the below patch.  My config is arch/arm/configs/omap2plus_defconfig
> which doesn't satisfy the dependencies for CONFIG_THUMB2_KERNEL (defined
> in arch/arm/Kconfig).
>
> Compiling results in the following errors (and indeed the macro hasn't
> been generated in include/generated/autoconfg.h).  If I modify my config
> to satisfy the dependencies for CONFIG_THUMB2_KERNEL so that it is
> selectable, the __enabled_CONFIG_THUMB2_KERNEL macro gets generated.
>
> arch/arm/kernel/setup.c: In function 'c_show':
> arch/arm/kernel/setup.c:1056: error: '__enabled_CONFIG_THUMB2_KERNEL'
> undeclared (first use in this function)
> arch/arm/kernel/setup.c:1056: error: (Each undeclared identifier is
> reported only once
> arch/arm/kernel/setup.c:1056: error: for each function it appears in.)
> arch/arm/kernel/setup.c:1056: error:
> '__enabled_CONFIG_THUMB2_KERNEL_MODULE' undeclared (first use in this
> function)
>
> Kernel is latest linus (v3.1-rc1-272-g73e0881).
>
Ahhh! That is why we have been stuck with #ifdef all these years!

What you point out is quite interesting. I guess that we did not
carefully understood the context in which this was printed. That is,
we will only ends up printing something if the symbol is visible, or
has been selected, or has a default. That said
__enabled_CONFIG_THUMB2_KERNEL will get generated whenever it appears
in the menuconfig (ie. it's dependency are met).

The solution is trivial, but I am not sure we want to go that way: we
need to generated a __enabled_ entry for symbols for _all_ symbols in
the configuration, even internal one, as I am sure someone will at
some point do:

if (IS_ENABLED(CONFIG_WIRELESS_EXT)) {
    ....
}

but this will continue to be broken until we have a global symbol
namespace for all architecture, ie:

if (IS_ENABLED(CONFIG_ARM)) {
    ....
}

will not be expected to work for an ARCH=mips build.

 - Arnaud

> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 70bca64..59aa480 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -1053,6 +1053,9 @@ static int c_show(struct seq_file *m, void *v)
>        seq_printf(m, "Serial\t\t: %08x%08x\n",
>                   system_serial_high, system_serial_low);
>
> +       if (IS_ENABLED(CONFIG_THUMB2_KERNEL))
> +               printk("Thumb-2\n");
> +
>        return 0;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: IS_ENABLED() and non-available symbols

[Arnaud Lacombe]

Hi,

On Sun, Aug 14, 2011 at 12:55 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> The solution is trivial, but I am not sure we want to go that way: we
> need to generated a __enabled_ entry for symbols for _all_ symbols in
> the configuration, even internal one
>
That should do the job...

 -= NOT FOR MERGE =-

Not-Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>

---
 scripts/kconfig/confdata.c |   42 +++++++++++++++++++++++++++++++-----------
 1 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 59b667c..e976ccb 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -503,17 +503,6 @@ header_print_symbol(FILE *fp, struct symbol *sym, const char *value, void *arg)
 			fprintf(fp, "#define %s%s%s 1\n",
 			    CONFIG_, sym->name, suffix);
 		}
-		/*
-		 * Generate the __enabled_CONFIG_* and
-		 * __enabled_CONFIG_*_MODULE macros for use by the
-		 * IS_{ENABLED,BUILTIN,MODULE} macros. The _MODULE variant is
-		 * generated even for booleans so that the IS_ENABLED() macro
-		 * works.
-		 */
-		fprintf(fp, "#define __enabled_" CONFIG_ "%s %d\n",
-				sym->name, (*value == 'y'));
-		fprintf(fp, "#define __enabled_" CONFIG_ "%s_MODULE %d\n",
-				sym->name, (*value == 'm'));
 		break;
 	}
 	case S_HEX: {
@@ -565,6 +554,35 @@ static struct conf_printer header_printer_cb =
 };
 
 /*
+ * Generate the __enabled_CONFIG_* and __enabled_CONFIG_*_MODULE macros for
+ * use by the IS_{ENABLED,BUILTIN,MODULE} macros. The _MODULE variant is
+ * generated even for booleans so that the IS_ENABLED() macro works.
+ */
+static void
+header_print__enabled_symbol(FILE *fp, struct symbol *sym, const char *value, void *arg)
+{
+
+	switch (sym->type) {
+	case S_BOOLEAN:
+	case S_TRISTATE: {
+		fprintf(fp, "#define __enabled_" CONFIG_ "%s %d\n",
+		    sym->name, (*value == 'y'));
+		fprintf(fp, "#define __enabled_" CONFIG_ "%s_MODULE %d\n",
+		    sym->name, (*value == 'm'));
+		break;
+	}
+	default:
+		break;
+	}
+}
+
+static struct conf_printer header__enabled_printer_cb =
+{
+	.print_symbol = header_print__enabled_symbol,
+	.print_comment = header_print_comment,
+};
+
+/*
  * Tristate printer
  *
  * This printer is used when generating the `include/config/tristate.conf' file.
@@ -945,6 +963,8 @@ int conf_write_autoconf(void)
 	conf_write_heading(out_h, &header_printer_cb, NULL);
 
 	for_all_symbols(i, sym) {
+		conf_write_symbol(out_h, sym, &header__enabled_printer_cb, NULL);
+
 		sym_calc_value(sym);
 		if (!(sym->flags & SYMBOL_WRITE) || !sym->name)
 			continue;
-- 
1.7.6.153.g78432

Re: IS_ENABLED() and non-available symbols

[Michal Marek]

On 14.8.2011 19:02, Arnaud Lacombe wrote:
> Hi,
> 
> On Sun, Aug 14, 2011 at 12:55 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>> The solution is trivial, but I am not sure we want to go that way: we
>> need to generated a __enabled_ entry for symbols for _all_ symbols in
>> the configuration, even internal one
>>
> That should do the job...
> 
>  -= NOT FOR MERGE =-
> 
> Not-Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>
> 
> ---
>  scripts/kconfig/confdata.c |   42 +++++++++++++++++++++++++++++++-----------
>  1 files changed, 31 insertions(+), 11 deletions(-)
> 
>  	for_all_symbols(i, sym) {
> +		conf_write_symbol(out_h, sym, &header__enabled_printer_cb, NULL);
> +
>  		sym_calc_value(sym);
>  		if (!(sym->flags & SYMBOL_WRITE) || !sym->name)
>  			continue;

I like the approach, it will work even with not visible symbols and
still catch typos (which #ifdef does not protect against). But with the
patch as-is, the generated autoconf.h starts with lots of

#define __enabled_CONFIG_(null) 1
#define __enabled_CONFIG_(null)_MODULE 0
#define __enabled_CONFIG_(null) 1
#define __enabled_CONFIG_(null)_MODULE 0
#define __enabled_CONFIG_(null) 0

Michal

Re: IS_ENABLED() and non-available symbols

[Arnaud Lacombe]

Hi,

2011/8/15 Michal Marek <mmarek@suse.cz>:
> On 14.8.2011 19:02, Arnaud Lacombe wrote:
>> Hi,
>>
>> On Sun, Aug 14, 2011 at 12:55 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>>> The solution is trivial, but I am not sure we want to go that way: we
>>> need to generated a __enabled_ entry for symbols for _all_ symbols in
>>> the configuration, even internal one
>>>
>> That should do the job...
>>
>>  -= NOT FOR MERGE =-
>>
>> Not-Signed-off-by: Arnaud Lacombe <lacombar@gmail.com>
>>
>> ---
>>  scripts/kconfig/confdata.c |   42 +++++++++++++++++++++++++++++++-----------
>>  1 files changed, 31 insertions(+), 11 deletions(-)
>>
>>       for_all_symbols(i, sym) {
>> +             conf_write_symbol(out_h, sym, &header__enabled_printer_cb, NULL);
>> +
>>               sym_calc_value(sym);
>>               if (!(sym->flags & SYMBOL_WRITE) || !sym->name)
>>                       continue;
>
> I like the approach, it will work even with not visible symbols and
> still catch typos (which #ifdef does not protect against).
>
actually, the print should be after `sym_calc_value(sym)' to get the
value right..

> But with the
> patch as-is, the generated autoconf.h starts with lots of
>
> #define __enabled_CONFIG_(null) 1
> #define __enabled_CONFIG_(null)_MODULE 0
> #define __enabled_CONFIG_(null) 1
> #define __enabled_CONFIG_(null)_MODULE 0
> #define __enabled_CONFIG_(null) 0
>
that should be trivial to fix. Sunday morning patches...

I'd be interested to also know the cost of all those new symbols on
build time...

 - Arnaud

Re: IS_ENABLED() and non-available symbols

[Arnaud Lacombe]

Hi,

On Mon, Aug 15, 2011 at 10:52 AM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> [...]
> I'd be interested to also know the cost of all those new symbols on
> build time...
>
here it is; on the parsing itself:

After 1000 iteration of:
% /usr/libexec/gcc/x86_64-redhat-linux/4.5.1/cc1 -E -quiet -v -include
include/generated/autoconf.h /dev/null -o /dev/null -mtune=generic
-march=x86-64 2>/dev/null

x include.before
+ include.after
+--------------------------------------------------------------------------+
| x  x x  x  x x                                  +  + +  +  + +  +  +    +|
||MA_|                                             |_A_|                   |
+--------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x 1000         0.016         0.021         0.016       0.01637 0.00077310911
+ 1000         0.034         0.043         0.035      0.035227 0.00076161083
Difference at 95.0% confidence
        0.018857 +/- 6.7264e-05
        115.192% +/- 0.410898%
        (Student's t, pooled s = 0.000767382)

After 1000 successive build of `init/main.o' (directly calling gcc):

x include.before
+ include.after
+--------------------------------------------------------------------------+
|     xxxxx**++++++                +              +                        |
|     xxxxx***+++++        x      ++       x      +            +           |
|     xxxxx***++*+++ +x+   x  x  x++ x   + x+   ++*  x     x   *           |
|     xxxxx****+*++**+**+*+*x x*xx*++*x++*+**x+ *+* +x *++x*  x*++ +      +|
||____|_A___M_A|_____|                                                     |
+--------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x 1000         0.628         0.835         0.631      0.636217   0.024724466
+ 1000         0.648         0.873         0.651      0.657341   0.026185882
Difference at 95.0% confidence
        0.021124 +/- 0.00223216
        3.32025% +/- 0.350849%
        (Student's t, pooled s = 0.0254657)

Now, an `allyesconfig' take about 5h30 hours to complete on my  Core2
T5670 (capped at 1.2GHz), so this patch would cost an extra 10min.
Still significant, but not too much :-)

 - Arnaud