[U-Boot] [RFC PATCH v2 01/15] Implement autoconf header file
Simon Glass
sjg at chromium.org
Thu Feb 28 15:11:36 CET 2013
Hi Joe,
On Wed, Feb 27, 2013 at 1:08 AM, Joe Hershberger
<joe.hershberger at gmail.com> wrote:
> Hi Simon,
>
> On Mon, Feb 25, 2013 at 12:10 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Joe,
>>
>> On Sun, Feb 24, 2013 at 11:50 AM, Joe Hershberger
>> <joe.hershberger at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Sun, Feb 24, 2013 at 11:25 AM, Simon Glass <sjg at chromium.org> wrote:
>>>> Add support for generating an autoconf.h header file that can be used in
>>>> the source instead of #ifdef.
>>>>
>>>> For example, instead of:
>>>>
>>>> #ifdef CONFIG_VERSION_VARIABLE
>>>> setenv("ver", version_string); /* set version variable */
>>>> #endif
>>>>
>>>> you can do:
>>>>
>>>> if (autoconf_version_variable())
>>>> setenv("ver", version_string); /* set version variable */
>>>
>>> You are changing the meaning between these two examples. The old code
>>> was #ifDEF, which means the new example needs to be autoconf_HAS_*.
>>> Is there a reason to muddy the waters by recommending people use this
>>> automatic value of 0 instead of using the "has" function? Any more
>>> than without this patch we should go change most all the #ifdef to
>>> #if?
>>
>> Thanks much for going through this so carefully.
>
> No problem.
>
>> Here is my thinking on this point - I will respond to the rest of your
>> comments when I get back to this.
>>
>> I believe we should get rid of the 'has' function eventually.
>
> Fair enough. I think it would be nice if your mechanism provided a
> way to segregate these two. Some sort of naming convention that
> differed between the two cases. The cases should be easy to identify.
> If it's an enable switch, then it will be #define'd, but with no
> value. If it is defined to a value, then it is configuration. This
> will also help to end the bad practice of board configs using "#define
> CONFIG_ENABLE_MY_FEATURE 1", which of course it only checked for
> defined or not... then the unsuspecting user juct changes the 1 to a 0
> and gets no change in behavior. By using a different naming
> convention, adding that "1" would prevent the feature from working up
> front.
That sounds like you want to change the CONFIG_ defines to separate
out the two meanings. I think it would be nice to have something like:
#define CONFIG_ENABLE_MY_FEATURE
#define PARAMS_MY_FEATURE_PORT 4
in the board config files, so that it is clear that one is a
Makefile/code selection CONFIG and the other is changing parameters.
But that is a big change, out of scope of this patch I think.
I could potentially add a check for the "#define
CONFIG_ENABLE_MY_FEATURE 1" error you mention, but how would I know
whether it is valid? There is currently no way to distinguish the two
types of config.
>
> The more I see "autoconf" the more I think that people will confuse
> this with GNU autoconf. maybe it would be good to avoid that.
>
Well we can't use config or cfg. I think autoconf is a reasonable
choice as we already have an autoconf.mk generated. This extends that
to provide access to C code using autoconf.h.
But if you don't like autoconf, please put your thinking hat on and
suggest a new name.
>> Most features are anyway just a 0 or a 1 - either the feature is
>> enabled or it is disabled. There are some where a value is provided,
>> and that means that the feature is automatically enabled. I'm not a
>> huge fan of that. To me we should have two completely different
>> concepts in U-Boot:
>>
>> - enabling an option, which generally just affects which files are
>> compiled - i.e. an option that should generally only appear in
>> Makefiles
>> - configuring something, by which I mean giving the code a value to
>> work with. This should come either from CONFIG for compile-time
>> config, or device tree for run-time config
>>
>> At present these two are mixed up, and I don't think it aids clarity.
>
> Maybe something like enable_[lower-case-name]() and config_[lower-case
> name](). But in matching your current feature-set, you would not even
> need to generate the config_ functions... only the enable_ functions.
Well config was ruled out unfortunately because it is already used in
some places. enable has the same problem. I went with:
autoconf_...()
and for the uncommon case that I would like to deprecate:
autoconf_has_...()
>
>> There are very very few CONFIGs that need this 'has' option I think. I
>> know about BOOTDELAY, and I'm sure there are others, but there was
>> recent discussion on the list about whether we should separate out the
>> bootdelay/autoboot value from the enablement, wasn't there?
>
> Indeed. It was a patch I submitted for this release.
> http://patchwork.ozlabs.org/patch/219303/
> It at least removed he dependence on the default value. Even better
> would be to separate the default value from the selection of building
> in support as a separate variable.
OK great. Yes that would be best. I suspect that would apply to other
similar corner cases.
>
>>>
>>>> The compiler will ensure that the dead code is eliminated, so the result
>>>> is the same.
>>>>
>>>> Where the value of the CONFIG define is 0, you can use the autoconf_has...()
>>>> form. For example CONFIG_BOOTDELAY can be -ve, 0 or +ve, but if it is
>>>> defined at all, it affects behaviour:
>>>>
>>>> #if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
>>>> s = getenv ("bootdelay");
>>>> #endif
>>>>
>>>> So we use:
>>>>
>>>> if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0)
>>>> s = getenv ("bootdelay");
>>>>
>>>> This later form should only be used for such 'difficult' defines where a
>>>> zero value still means that the CONFIG should be considered to be defined.
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>> Changes in v2:
>>>> - Split out changes to main.c into separate patches
>>>> - Fix up a few errors and comments in the original RFC
>>>> - Use autoconf_...() instead of config_...()
>>>> - Use autoconf_has_...() instead of config_..._enabled()
>>>> - Add a grep to the sed/sort pipe to speed up processing
>>>>
>>>> Makefile | 42 ++++++++++++++++++++-
>>>> README | 87 +++++++++++++++++++++++++++++++++++++++++--
>>>> include/common.h | 3 ++
>>>> include/config_drop.h | 17 +++++++++
>>>> tools/scripts/define2conf.sed | 37 ++++++++++++++++++
>>>> tools/scripts/define2list.sed | 31 +++++++++++++++
>>>> 6 files changed, 213 insertions(+), 4 deletions(-)
>>>> create mode 100644 include/config_drop.h
>>>> create mode 100644 tools/scripts/define2conf.sed
>>>> create mode 100644 tools/scripts/define2list.sed
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index fc18dd4..9f4f55d 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -614,6 +614,7 @@ updater:
>>>> # parallel sub-makes creating .depend files simultaneously.
>>>> depend dep: $(TIMESTAMP_FILE) $(VERSION_FILE) \
>>>> $(obj)include/autoconf.mk \
>>>> + $(obj)include/generated/autoconf.h \
>>>> $(obj)include/generated/generic-asm-offsets.h \
>>>> $(obj)include/generated/asm-offsets.h
>>>> for dir in $(SUBDIRS) $(CPUDIR) $(LDSCRIPT_MAKEFILE_DIR) ; do \
>>>> @@ -688,6 +689,44 @@ $(obj)include/autoconf.mk: $(obj)include/config.h
>>>> sed -n -f tools/scripts/define2mk.sed > $@.tmp && \
>>>> mv $@.tmp $@
>>>>
>>>> +# 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
>>>
>>> You forgot to update this comment when changing to autoconf_has. Grep perhaps?
>>
>> Yes this whole commit message needs updating, sorry. I didn't get any
>> response on the desirability of autoconf, so just sent out another
>> RFC.
>
> Again, after considering this more, I don't like autoconf. Sorry.
>
>>>
>>>> +# value to 0 if the option is disabled, 1 if enabled. This last feature will
>>>> +# hopefully be deprecated soon.
>>>> +# The file is regenerated when any U-Boot header file changes.
>>>> +$(obj)include/generated/autoconf.h: $(obj)include/config.h
>>>> + @$(XECHO) Generating $@ ; \
>>>> + set -e ; \
>>>> + : Extract the config macros to a C header file ; \
>>>> + $(CPP) $(CFLAGS) -DDO_DEPS_ONLY -dM include/common.h | \
>>>> + sed -n -f tools/scripts/define2conf.sed > $@.tmp; \
>>>> + : Regenerate our list of all config macros if neeed ; \
>>>> + if [ ! -f $@-all.tmp ] || \
>>>> + find $(src) -name '*.h' -type f -newer $@-all.tmp | \
>>>> + egrep -qv 'include/(autoconf.h|generated|config.h)'; \
>>>> + then \
>>>> + : Extract all config macros from all C header files ; \
>>>> + : We can grep for CONFIG since the value will be dropped ; \
>>>> + ( \
>>>> + find ${src} -name "*.h" -type f | xargs \
>>>> + cat | grep CONFIG | \
>>>> + sed -n -f tools/scripts/define2list.sed \
>>>> + ) | sort | uniq > $@-all.tmp; \
>>>> + fi; \
>>>> + : Extract the enabled config macros to a C header file ; \
>>>> + $(CPP) $(CFLAGS) -DDO_DEPS_ONLY -dM include/common.h | \
>>>> + sed -n -f tools/scripts/define2list.sed | sort > $@-enabled.tmp; \
>>>
>>> From looking at define2list.sed (its name is confusing so sorry if I'm
>>> interpreting wrong) it lists all things NOT enabled. But I guess you
>>> are wanting exact text with which to prune the -all.tmp. Could you
>>> craft it such that you can use .tmp directly to prune -all.tmp instead
>>> of having to generate this -enable.tmp file (which is misleading and
>>> potentially a waste of sed time)? Please rename define2list.sed and
>>> define2conf.sed to something which has meaning. This is quite hard to
>>> read as it.
>>
>> Actually the sed script turns any files containing CONFIG defines into
>> a file that defines autoconf versions of these to zero. It can be used
>> either on things that are enabled, or on things that are not. The
>> point of the 'comm' is to figure which (by a process of elimination)
>> which CONFIGs are present but not enabled.
>
> I'm pretty sure I understand how this was working. What I was asking
> is if there was a straightforward way to use the output of
> define2value.sed run on config.h to prune the -all output of
> define2zero.sed, instead of running define2zero.sed on the config.h.
OK I see. It's not that easy - comm requires that the lines be the
same, so it is easier just to generate them with the same script. The
time impact of this is tiny - the enabled config is a very small file
and does not require scanning all the headers files in U-Boot :-)
>
>>>
>>>> + set -e ; \
>>>> + : Find CONFIGs that are not enabled ; \
>>>> + comm -13 $@-enabled.tmp $@-all.tmp >>$@.tmp && \
>>>> + mv $@.tmp $@
>>>> +
>>>> $(obj)include/generated/generic-asm-offsets.h: $(obj)include/autoconf.mk.dep \
>>>> $(obj)lib/asm-offsets.s
>>>> @$(XECHO) Generating $@
>>>> @@ -770,7 +809,8 @@ include/license.h: tools/bin2header COPYING
>>>> unconfig:
>>>> @rm -f $(obj)include/config.h $(obj)include/config.mk \
>>>> $(obj)board/*/config.tmp $(obj)board/*/*/config.tmp \
>>>> - $(obj)include/autoconf.mk $(obj)include/autoconf.mk.dep
>>>> + $(obj)include/autoconf.mk $(obj)include/autoconf.mk.dep \
>>>> + $(obj)include/generated/autoconf.h
>>>>
>>>> %_config:: unconfig
>>>> @$(MKCONFIG) -A $(@:_config=)
>>>> diff --git a/README b/README
>>>> index d8cb394..3e89551 100644
>>>> --- a/README
>>>> +++ b/README
>>>> @@ -5434,11 +5434,92 @@ Notes:
>>>> * If you modify existing code, make sure that your new code does not
>>>> add to the memory footprint of the code ;-) Small is beautiful!
>>>> When adding new features, these should compile conditionally only
>>>> - (using #ifdef), and the resulting code with the new feature
>>>> - disabled must not need more memory than the old code without your
>>>> - modification.
>>>> + (avoiding #ifdef where at all possible), and the resulting code with
>>>> + the new feature disabled must not need more memory than the old code
>>>> + without your modification.
>>>>
>>>> * Remember that there is a size limit of 100 kB per message on the
>>>> u-boot mailing list. Bigger patches will be moderated. If they are
>>>> reasonable and not too big, they will be acknowledged. But patches
>>>> bigger than the size limit should be avoided.
>>>> +
>>>> +
>>>> +Use of #ifdef:
>>>> +--------------
>>>> +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
>>>> +an #ifdef and there is not as much checking of this by the compiler. For
>>>> +someone coming new into the code base, #ifdefs are a big turn-off. Multiple
>>>> +dependent #ifdefs are harder to do than with if..then..else. Variable
>>>> +declarations must be #idefed as well as the code that uses them, often much
>>>
>>> Isn't it true that dead code stripping doesn't strip variables only
>>> used by that dead code? So stack usage is wasted everywhere in this
>>> new model?
>>
>> Yes, that does seem to happen with my gcc - thanks for pointing it
>> out. It's a little odd, I wonder if there is a justification for it,
>> or if it is just overlooked. It seems particularly strange considering
>> the effort gcc goes to to eliminate unused variables.
>
> Indeed. It seems this would be a big concern for SPL, right?
In the case where people write large functions with lots of
conditional code, yes. But that would be bad practice anyway - does it
happen in other situations? I prefer that we try to keep our functions
a bit smaller if possible.
>
>>
>>>
>>>> +later in the file/function. #ifdef indents don't match code indents and
>>>> +have their own separate indent feature. Overall, excessive use of #idef
>>>> +hurts readability and makes the code harder to modify and refactor.
>>>> +
>>>> +In an effort to reduce the use of #ifdef in U-Boot, without requiring lots
>>>> +of special static inlines all over the header files, a single autoconf.h
>>>> +header file with lower-case function-type macros has been made available.
>>>> +
>>>> +This file has either:
>>>> +
>>>> +# #define config_xxx() value
>>>
>>> Use the new names here...
>>>
>>>> +
>>>> +for enabled options, or:
>>>> +
>>>> +# #define config_xxx() 0
>>>> +
>>>> +for disabled options. You can therefore generally change code like this:
>>>> +
>>>> + #ifdef CONFIG_XXX
>>>> + do_something
>>>> + #else
>>>> + do_something_else
>>>> + #endif
>>>> +
>>>> +to this:
>>>> +
>>>> + if (config_xxx())
>>>> + do_something;
>>>> + else
>>>> + do_something_else;
>>>> +
>>>> +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.
>>>> +
>>>> +Multiple #ifdefs can be converted also:
>>>> +
>>>> + #if defined(CONFIG_XXX) && !defined(CONFIG_YYY)
>>>> + do_something
>>>> + #endif
>>>> +
>>>> + if (config_xxx() && !config_yyy())
>>>> + do_something;
>>>> +
>>>> +Where the macro evaluates to a string, it will be non-NULL, so the above
>>>> +will work whether the macro is a string or a number.
>>>> +
>>>> +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)
>>>> +
>>>> +U-Boot already has a Makefile scheme to permit files to be easily included
>>>> +based on CONFIG. This can be used where the code to be compiled exists in
>>>> +its own source file. So the following rules apply:
>>>> +
>>>> + 1. Use #ifdef to conditionally compile an exported function or variable
>>>> + 2. Use ordinary C code with config_xxx() everywhere else
>>>> + 3. Mark your functions and data structures static where possible
>>>> + 4. Use the config_xxx_enabled() variants only if essential
>>>
>>> Fix names here.
>>>
>>>> + 5. When changing existing code, first create a new patch to replace
>>>> + #ifdefs in the surrounding area
>>>> diff --git a/include/common.h b/include/common.h
>>>> index 4ad17ea..491783b 100644
>>>> --- a/include/common.h
>>>> +++ b/include/common.h
>>>> @@ -35,6 +35,9 @@ typedef volatile unsigned short vu_short;
>>>> typedef volatile unsigned char vu_char;
>>>>
>>>> #include <config.h>
>>>> +#ifndef DO_DEPS_ONLY
>>>> +#include <generated/autoconf.h>
>>>> +#endif
>>>> #include <asm-offsets.h>
>>>> #include <linux/bitops.h>
>>>> #include <linux/types.h>
>>>> diff --git a/include/config_drop.h b/include/config_drop.h
>>>> new file mode 100644
>>>> index 0000000..bf2beaa
>>>> --- /dev/null
>>>> +++ b/include/config_drop.h
>>>> @@ -0,0 +1,17 @@
>>>> +/*
>>>> + * Copyright 2013 Google, Inc
>>>> + *
>>>> + * This file is licensed under the terms of the GNU General Public
>>>> + * License Version 2. This file is licensed "as is" without any
>>>> + * warranty of any kind, whether express or implied.
>>>> + */
>>>> +
>>>> +#ifndef _CONFIG_DROP_H
>>>> +#define _CONFIG_DROP_H
>>>> +
>>>> +/* Options which don't seem to be referred to anywhere in U-Boot */
>>>
>>> You mean not referred to by any config header?
>>
>> Or in fact any header.
>
> If they are not referred to in any header, they why are they needed
> here? What is the purpose? I guess I just don't yet understand what
> problem this solves.
They are needed only because there is code in U-Boot (C files) that
checks these configs. If we don't create an autoconf define for them,
then that code will not build. It's actually a good check for the dead
code problem.
>
>>
>>>
>>>> +#define CONFIG_MENUPROMPT "Auto-boot prompt"
>>>> +#define CONFIG_MENUKEY
>>>> +#define CONFIG_UPDATE_TFTP
>>>> +
>>>> +#endif
>>>> diff --git a/tools/scripts/define2conf.sed b/tools/scripts/define2conf.sed
>>>> new file mode 100644
>>>> index 0000000..2c4a2ef
>>>> --- /dev/null
>>>> +++ b/tools/scripts/define2conf.sed
>>>> @@ -0,0 +1,37 @@
>>>> +#
>>>> +# Sed script to parse CPP macros and generate a list of CONFIG macros
>>>> +#
>>>> +# This converts:
>>>> +# #define CONFIG_XXX value
>>>> +#into:
>>>> +# #define config_xxx() value
>>>> +# #define config_xxx_enabled() 1
>>>
>>> Fix the names here.
>>>
>>>> +#
>>>> +
>>>> +# Macros with parameters are ignored.
>>>> +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ {
>>>> + d
>>>
>>> Any reason not to use "[A-Za-z0-9_]+" instead of "[A-Za-z0-9_][A-Za-z0-9_]*"?
>>
>> Should be fine, probably I just overlooked it.
>
> It's bizarre that + doesn't work.
Yes it did throw me, but on the bizarre scale I rate this only a 3.
There is a note in my sed man page which might explain it, but is very
vague.
POSIX.2 BREs should be supported, but they aren't completely because of
performance problems. The \n sequence in a regular expression matches
the newline character, and similarly for \a, \t, and other sequences.
>
>>>
>>>> +}
>>>> +
>>>> +# Only process values prefixed with #define CONFIG_
>>>> +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ {
>>>> + # Strip the #define prefix
>>>> + s/#define[ \t]*CONFIG_/autoconf_/;
>>>> + # Change to form CONFIG_*=VALUE
>>>> + s/[\t ][\t ]*/=/;
>>>> + # Handle lines with no value
>>>> + s/^\([^=]*\)$/\1=/;
>>>> + # Drop trailing spaces
>>>> + s/ *$//;
>>>> + # Change empty values to '1'
>>>> + s/=$/=1/;
>>>> + # Add #define at the start
>>>> + s/^\([^=]*\)=/#define \L\1() /
>>>> + # print the line
>>>> + p
>>>> + # Create autoconf_has_...(), value 1
>>>> + s/().*/() 1/
>>>> + s/\(autoconf_\)/\1has_/
>>>> + # print the line
>>>> + p
>>>> +}
>>>> diff --git a/tools/scripts/define2list.sed b/tools/scripts/define2list.sed
>>>
>>> Please change the name to be more clear as to its purpose. "list"
>>> doesn't convey to me that it is all configs that should be set as
>>> undefined.
>>
>> Will do.
>
> The new names are much clearer.
OK good. Please have another think about autoconf - if you really
don't like it then please suggest something else that doesn't
currently exist in U-Boot.
>
> <snip>
>
> Cheers,
> -Joe
Regards,
Simon
More information about the U-Boot
mailing list