[U-Boot] [RFC PATCH] Provide a mechanism to avoid using #ifdef everywhere
Simon Glass
sjg at chromium.org
Tue Feb 19 06:13:44 CET 2013
Hi Wolfgang,
On Mon, Feb 18, 2013 at 11:23 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon,
>
> In message <1361207920-24983-1-git-send-email-sjg at chromium.org> you wrote:
>> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
>> different boards compile different versions of the source code, meaning
>> that we must build all boards to check for failures. It is easy to misspell
> ...
>> +# Create a C header file where every '#define CONFIG_XXX value' becomes
>> +# '#define config_xxx() value', or '#define config_xxx() 0' where the CONFIG
>> +# is not used by this board configuration. This allows C code to do things
>> +# like 'if (config_xxx())' and have the compiler remove the dead code,
>> +# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases
>> +# if the config_...() returns 0 then the option is not enabled. In some rare
>> +# cases such as CONFIG_BOOTDELAY, the config can be enabled but still have a
>> +# a value of 0. So in addition we a #define config_xxx_enabled(), setting the
>> +# value to 0 if the option is disabled, 1 if enabled. This last feature will
>> +# hopefully be deprecated soon.
Thanks for looking at this so closely - it's just an RFC at this
stage, but I think it has promise.
>
> I think config_* is not a good name to use here - it has never been a
> reserved prefix so far, so it is IMO a bad idea to turn it into one
> now . We already have quite a number such variable names in the code
> all over the place (not sure I caught them all):
>
> config_2 config_io_ctrl
> config_8260_ioports config_kbc_gpio
> config_8560_ioports config_list
> config_address config_list_t
> config_backside_crossbar_mux config_mpc8xx_ioports
> config_base config_nfc_clk
> config_bit config_node
> config_buf config_on_ebc_cs4_is_small_flash
> config_byte config_pci_bridge
> config_change config_periph_clk
> config_clock config_pin
> config_cmd_ctrl config_pll_clk
> config_core_clk config_qe_ioports
> config_data config_reg
> config_data_eye_leveling_samples config_reg_helper
> config_ddr config_s
> config_ddr_clk config_sdram
> config_ddr_data config_select_P
> config_ddr_phy config_selection
> config_desc config_selection_t
> config_device config_status
> config_fifo config_table
> config_file_size config_val
> config_frontside_crossbar_vsc3316 config_val_P
> config_genmii_advert config_validity
> config_hub_port config_validity_t
> config_id config_vtp
> config_instance
>
These are variables, so won't conflict with the function macros used
by this patch. But maybe we should try for something like cfg_...()?
>
>> +The compiler will see that config_xxx() evalutes to a constant and will
>> +eliminate the dead code. The resulting code (and code size) is the same.
>
> Did you measure the impact on compile time?
I did for the 'sed' step - it was a 2-3 seconds to regenerate the header files.
Full reconfig/recompile goes from about about 30s to 34s.
Incremental build doesn't change noticeably.
I also tried recompiling both the mainline U-Boot's main.c 100 times,
and then this one. I could not see any time difference, which is a
little surprising. Maybe the C compiler's DCE is pretty early in the
the process?
>
>> +This takes care of almost all CONFIG macros. Unfortunately there are a few
>> +cases where a value of 0 does not mean the option is disabled. For example
>> +CONFIG_BOOTDELAY can be defined to 0, which means that the bootdelay
>> +code should be used, but with a value of 0. To get around this and other
>> +sticky cases, an addition macro with an '_enabled' suffix is provided, where
>> +the value is always either 0 or 1:
>> +
>> + // Will work even if boaard config has '#define CONFIG_BOOTDELAY 0'
>> + if (config_bootdelay_enabled())
>> + do_something;
>> +
>> +(Probably such config options should be deprecated and then we can remove
>> +this feature)
>
> These are perfectly valid cases, I think - there are quite a number of
> these, including defines for console index, PHY address, etc. etc.
Well the issue is not so much the value, but whether the value enables
something. So for example:
config_console_index()
is perfectly fine if it just specifies the console index. But if it
also enables are particular feature (e.g. turning the console on),
then causes problems. I actually think such cases are quite rare, and
probably cause other confusion also.
>
>> --- a/common/cmd_fitupd.c
>> +++ b/common/cmd_fitupd.c
>> @@ -8,6 +8,7 @@
>>
>> #include <common.h>
>> #include <command.h>
>> +#include <net.h>
>>
>> #if !defined(CONFIG_UPDATE_TFTP)
>> #error "CONFIG_UPDATE_TFTP required"
>
> This seems to be an unrelated change?
Yes - just to make it fail here rather than die with an obscure message later.
>
>
>> diff --git a/common/main.c b/common/main.c
>> index e2d2e09..cd42b67 100644
>> --- a/common/main.c
>> +++ b/common/main.c
> ...
>> -#include <malloc.h> /* for free() prototype */
> ...
>> +#include <malloc.h>
>
> Why dropping the comment?
Actually main.c includes this file twice - I dropped the one with the
comment. There are far to many changes in main.c for the diff to be
very useful unfortunately. It's just an RFC, but my intent with main.c
was to take the concept as far as I could.
>
>> -#undef DEBUG_PARSER
>> +#define DEBUG_PARSER 0 /* set to 1 to debug */
>> +
>> +
>> +#define debug_parser(fmt, args...) \
>> + debug_cond(DEBUG_PARSER, fmt, ##args)
>> +
>> +#ifndef DEBUG_BOOTKEYS
>> +#define DEBUG_BOOTKEYS 0
>> +#endif
>> +#define debug_bootkeys(fmt, args...) \
>> + debug_cond(DEBUG_BOOTKEYS, fmt, ##args)
>
> This is also a different type of changes. Please keep such separate.
Yes
>
>> -#ifndef CONFIG_ZERO_BOOTDELAY_CHECK
>> - if (bootdelay == 0)
>> + if (config_zero_bootdelay_check() && bootdelay == 0)
>
> This appears to be plain wrong. That was a "#ifndef" before...
>
>> + if (config_autoboot_delay_str() && delaykey[0].str == NULL)
>> + delaykey[0].str = config_autoboot_delay_str();
>
> Hm.... constructs like these make me think about side effects. As is,
> your implementation does not pretect against any. This may be
> dangerous - you are evaluating the macro multiple times now, while
> before it was only a defined() test, folowed by a single evaluation.
>
> And to be honest - I find the old code easier to read.
>
>> - for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) {
>> + for (i = 0; i < ARRAY_SIZE(delaykey); i++) {
>
> Once more, a totally unrelated code change. Please split into
> independent commits.
>
I will sort out these comments when I put together a proper series for this.
>
>> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK
>> /*
>> * Check if key already pressed
>> * Don't check if bootdelay < 0
>> */
>> - if (bootdelay >= 0) {
>> + if (config_zero_bootdelay_check() && bootdelay >= 0) {
>> if (tstc()) { /* we got a key press */
>> (void) getc(); /* consume input */
>> puts ("\b\b\b 0");
>> abort = 1; /* don't auto boot */
>> }
>> }
>> -#endif
>
> I think code like this needs more careful editing.
>
> With the #ifdef, it was clear that the comment only applies if
> CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly becomes
> valid _always_, which is pretty much misleading to someone trying to
> understand this code. I think it would be necessary to rephrase the
> commend, and move it partially into the if() block.
Yes I see.
>
>
>> -void main_loop (void)
>> +static void process_boot_delay(void)
>> {
>> -#ifndef CONFIG_SYS_HUSH_PARSER
>> - static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, };
>> - int len;
>> - int rc = 1;
>> - int flag;
>> -#endif
>> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \
>> - defined(CONFIG_OF_CONTROL)
>> - char *env;
>> -#endif
>> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
>> - char *s;
>> - int bootdelay;
>> -#endif
>> -#ifdef CONFIG_PREBOOT
>> - char *p;
>> -#endif
>> -#ifdef CONFIG_BOOTCOUNT_LIMIT
>> unsigned long bootcount = 0;
>> unsigned long bootlimit = 0;
>> - char *bcs;
>> - char bcs_set[16];
>> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
>> -
>> - bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");
>> -
>> -#ifdef CONFIG_BOOTCOUNT_LIMIT
>> - bootcount = bootcount_load();
>> - bootcount++;
>> - bootcount_store (bootcount);
>> - sprintf (bcs_set, "%lu", bootcount);
>> - setenv ("bootcount", bcs_set);
>> - bcs = getenv ("bootlimit");
>> - bootlimit = bcs ? simple_strtoul (bcs, NULL, 10) : 0;
>> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
>> -
>> -#ifdef CONFIG_MODEM_SUPPORT
>> - debug ("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init);
>> - if (do_mdm_init) {
>> - char *str = strdup(getenv("mdm_cmd"));
>> - setenv ("preboot", str); /* set or delete definition */
>> - if (str != NULL)
>> - free (str);
>> - mdm_init(); /* wait for modem connection */
>> - }
>> -#endif /* CONFIG_MODEM_SUPPORT */
>> -
>> -#ifdef CONFIG_VERSION_VARIABLE
>> - {
>> - setenv ("ver", version_string); /* set version variable */
>> - }
>> -#endif /* CONFIG_VERSION_VARIABLE */
>> -
>> -#ifdef CONFIG_SYS_HUSH_PARSER
>> - u_boot_hush_start ();
>> -#endif
>> -
>> -#if defined(CONFIG_HUSH_INIT_VAR)
>> - hush_init_var ();
>> -#endif
>> -
>> -#ifdef CONFIG_PREBOOT
>> - if ((p = getenv ("preboot")) != NULL) {
>> -# ifdef CONFIG_AUTOBOOT_KEYED
>> - int prev = disable_ctrlc(1); /* disable Control C checking */
>> -# endif
>> -
>> - run_command_list(p, -1, 0);
>> + const char *s;
>> + int bootdelay;
>>
>> -# ifdef CONFIG_AUTOBOOT_KEYED
>> - disable_ctrlc(prev); /* restore Control C checking */
>> -# endif
>
> I'm afraid here the patch becomes unreadable, at least to me - I give
> up.
>
Agreed, it's just too hard to follow. in this form. What is happening
here is that I split the boot delay code into two functions, one for
each of the available options. It really needs a number of smaller
patches to be useful.
But if you have time, please take a look at the sed scripts and the Makefile.
>
> I find the idea interesting and definitely worth to carry on, but it
> appears we're still a pretty long way off from usable code.
OK good.
The background here is that I have been wondering about this for a
while, but have never really come up with a good way (in the absence
of a unified Kconfig) of getting a complete list of CONFIG variables.
There was a mailing list post about this a few weeks ago. But then
David Hendrix complained about main.c which spurred me into action,
and I thought maybe we could just live with a brute force
'find/grep/sed' approach for now, rather than trying to be too clever.
That's what I have done here.
If you look at the image series I did, for example
http://patchwork.ozlabs.org/patch/209601/, you will see that I was
trying to avoid adding back #ifdefs into the cleaned-up code. That was
a point solution, not particularly elegant, and I would much prefer
that we get a built-in solution.
I will come back to this when I have time - will be a week or two.
Regards,
Simon
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Too bad that all the people who know how to run the country are busy
> driving taxicabs and cutting hair. - George Burns
More information about the U-Boot
mailing list