[U-Boot] [PATCH] envcrc: extract default environment from target ELF files

[Mike Frysinger]

On Tuesday 16 June 2009 14:02:16 Wolfgang Denk wrote:
> Mike Frysinger wrote:
> > Rather than rely on dirty hacks to compile the environment on the host
> > and extract the CRC from that, have envcrc extract the environment
> > straight from the ELF object that will be linked into u-boot itself. 
> > This makes the envcrc code a bit more complicated, but it simplifies the
> > build process and host requirements because we don't have to try and
> > recreate the environment that the target will be seeing on the host. 
> > This avoids some issues that crop up from time to time where the
> > preprocessor defines on the host don't expand in the same way as for the
> > target -- in case the target uses those defines to customize the
> > environment, or the host defines conflicts with some of the target
> > values.
>
> Can you please be a bit more specific about which sort of problems you
> are talking here?

sure ... i'll be verbose so hopefully you can see what i see

> We've been using this code for many years now, but I
> cannot remember any problems with it.

that's because the code paths have slowly acquired platform-specific hacks so 
that it kept working :)

at any rate, here's a longer problem description and surrounding issues ...

if the default environment utilizes defines that come from the toolchain in 
any level of indirection, the environment that is built into the target u-boot 
will not match the environment that is compiled on the host for crc 
generation.

a real world example is the unified ADI config header.  it generates a default 
environment based on functionality actually available.  the BF537 family has 
networking hardware in some parts, but not all (BF534 does not).  so we key 
off of the variant to determine whether to enable networking by default.  or 
we determine that some on-chip rom functions are available for certain 
variants and enable only those functions.

so the CPP dependency chain looks something like:
- Blackfin toolchain sets up variant defines
- ADI board headers set up capabilities based on variant defines
- common ADI board header enables some commands based on variant defines
- common ADI board header sets up default environment based on capabilities 
and commands available

since the host toolchain does not set up the same CPP dependencies as the 
Blackfin toolchain, this tree falls apart and the resulting environment string 
that envcrc operates on differs, so the CRCs differ.

i can understand your position of "setting up board configs this way is 
wrong", but i've structured it this way because it greatly reduces my 
maintenance burden while increasing regression capabilities.  for example, i 
have a Blackfin-specific patch to the MAKEALL script that allows me to 
regression test not only the board-specific configuration, but also uncommon 
or otherwise untested cpu and boot mode configurations.  for example, the 
bf537-stamp defaults to the BF537 cpu and BYPASS boot mode.  but now my 
regression builds can automatically verify BF534/BF536 cpu code paths as well 
as PARALLEL/SPI/NAND/UART code paths.  previously, some changes would 
accidentally break these uncommon edge cases and wouldnt be noticed until 
someone else happened to build.  to say that these code paths were horribly 
broken most of the time is an understatement :).

i have also seen a case or two where the host toolchain inadvertently expanded 
things that ended up in the environment because they were not declared as 
strings.  for example, many defines are not:
#define CONFIG_FOO "foo"
but rather they are:
#define CONFIG_FOO foo
just grep common/env_*.c for MKSTR() to see just how many things are affected 
in this way.  having to worry about values here which may be arbitrarily 
expanded across host toolchains is a bad design practice imo.

if the environment is only ever compiled by one toolchain, the target one, 
then it significantly reduces the chance for unexpected and unwelcomed 
surprises.  after all, the only way to notice something has gone wrong is to 
build the source with a specific host toolchain, burn the image, reset, and 
see the dreaded "CRC mismatch, using default environment" error from u-boot.  
unless you're familiar with this problem and/or have seen/root caused this 
before, attempting to track down the source of this problem can be very time 
consuming (as it was the first time i hit this problem).

> >  common/Makefile       |    4 +-
> >  common/env_embedded.c |   30 +---------
> >  tools/Makefile        |    3 +-
> >  tools/envcrc.c        |  156
> > ++++++++++++++++++++++++++++++++++++++++++-------
>
> The diffstat indicates that the new solution is more complicated than
> the old one, so I'd like to understand why this is needed (or if).

if LoC are the metric, then yes, but conceptually this change is simpler.  the 
current behavior requires mixing target u-boot environment headers with the 
host toolchain, and a whole bunch of hacks to make sure things actually 
compile, produce the same end environment with two vastly different build 
environments, and end up with the same crc value.  my proposed change means we 
only compile 1 environment object -- the one that is going into the target u-
boot binary.  we then extract the environment directly from the object which 
is actually going into u-boot (so we are absolutely assured that the 
environment we are working with on the host matches the target).

hope this clears things up
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090619/1e3a4b95/attachment-0001.pgp