Config.lb WTF

[Carl-Daniel Hailfinger]

We have 22 different ways to invoke gcc inside Config.lb. That's just
embarrassing.

action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CFLAGS) $(CPPFLAGS)
$(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -Os -nostdinc -nostdlib -fno-builtin
$(DEBUG_CFLAGS) -Wall -c -S -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS) $(CPU_OPT)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
$(DEBUG_CFLAGS) -Wall -c -S -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/apc_auto.c -Os -nostdinc -nostdlib -fno-builtin -Wall -c -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I.  $(CPPFLAGS)
$(MAINBOARD)/auto.c -Os -nostdinc -nostdlib -fno-builtin $(DEBUG_CFLAGS)
-Wall -c -S -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/auto.c -Os -nostdinc -nostdlib -fno-builtin -Wall -c -o auto.o"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
-Wall -c -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
-Wall -c -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -Os -nostdinc -nostdlib -fno-builtin
-Wall -c -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
-Wall -c -o auto.o"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
-Wall -c -o auto.o"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
-Wall -c -S -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
-Wall $(DEBUG_CFLAGS) -c -S -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
-Wall $(DEBUG_CFLAGS) -c -S -o $@"        
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -Os -nostdinc -nostdlib -fno-builtin
-Wall $(DEBUG_CFLAGS) -c -S -o $@"
action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
-Wall -g -dA -fverbose-asm -c -S -o $@"
action "$(CC) -I$(TOP)/src -I. $(CFLAGS) $(CPPFLAGS)
$(MAINBOARD)/apc_auto.c -Os -nostdinc -nostdlib -fno-builtin -Wall -c -o $@"
action "$(CC) -I$(TOP)/src -I. $(CFLAGS) $(CPPFLAGS)
$(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -Os -nostdinc -nostdlib -fno-builtin
$(DEBUG_CFLAGS) -Wall -c -S -o $@"
action "$(CC) -I$(TOP)/src -I. $(CFLAGS) $(CPPFLAGS)
$(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -Os -nostdinc -nostdlib -fno-builtin
-Wall -c -o $@"
action "$(CC) -I$(TOP)/src -I. $(CPPFLAGS) $(MAINBOARD)/apc_auto.c -Os
-nostdinc -nostdlib -fno-builtin -Wall -c -o $@"
action "$(CC) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
$(DEBUG_CFLAGS) -Wall -c -S -o $@"
action "$(CC) -I$(TOP)/src -I. $(CPPFLAGS)
$(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin
-Wall -c -o $@"
action "$(CC) -I$(TOP)/src -I. $(DISTRO_CFLAGS) $(CPPFLAGS)
$(MAINBOARD)/apc_auto.c -Os -nostdinc -nostdlib -fno-builtin -Wall -c -o $@"

Now if you cry when looking at the mess above, you have my sympathy.

Can we please decide on one order and one common set of parameters? Or
is there a reason why some parameters are used only sometimes? (Except
the -S parameter for asm output.)

Suggestion:
action "$(CC) $(DISTRO_CFLAGS) $(CFLAGS) $(CPPFLAGS) $(DEBUG_CFLAGS)
-I$(TOP)/src -I. -nostdinc -nostdlib -fno-builtin -Wall -Os -c -S
$(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -o $@"

The idea behind this parameter order is:
- *FLAGS at the beginning.
- Use a common set of *FLAGS.
- Include files and directories listed afterwards.
- nostdinc, nostdlib, no-builtin tell the compiler this is standalone code.
- Warnings. They do not influence source or compilation.
- Compilation strategy (small) and output mode (asm or binary).
- File to be compiled.
- Output name.

Seems logical to me, but I'm open to alternatives as long as we can
consolidate this horror.

Regards,
Carl-Daniel

[Peter Stuge]

Carl-Daniel Hailfinger wrote:
> We have 22 different ways to invoke gcc inside Config.lb. That's
> just embarrassing.

I'd agree.
> Suggestion:
> action "$(CC) $(DISTRO_CFLAGS) $(CFLAGS) $(CPPFLAGS) $(DEBUG_CFLAGS)
> -I$(TOP)/src -I. -nostdinc -nostdlib -fno-builtin -Wall -Os -c -S
> $(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -o $@"

Perfect.

//Peter

[ron minnich]

On Sat, Apr 11, 2009 at 2:24 PM, Peter Stuge  wrote:
> Carl-Daniel Hailfinger wrote:
> > We have 22 different ways to invoke gcc inside Config.lb. That's
> > just embarrassing.
>
> I'd agree.
> > Suggestion:
> > action "$(CC) $(DISTRO_CFLAGS) $(CFLAGS) $(CPPFLAGS) $(DEBUG_CFLAGS)
> > -I$(TOP)/src -I. -nostdinc -nostdlib -fno-builtin -Wall -Os -c -S
> > $(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -o $@"

it's not really a fix because it depends on people not to drift again
over time. In the beginning, there was one way to set up this rule,
and then people changed it, and new files were created, and ... that's
why we are where we are now.

Since the config tool supports include files, why not exploit that
capability to fix this problem.

Longer term, we need a real set of Makefiles a la v3, and we need to
remove makerule from the Config.lb files. I suggest putting effort
there -- this is a flowery fix, and flowers are pretty, but this is
not fixing the real problem.

thanks

ron

[Carl-Daniel Hailfinger]

On 12.04.2009 00:02, ron minnich wrote:
> On Sat, Apr 11, 2009 at 2:24 PM, Peter Stuge  wrote:
> > Carl-Daniel Hailfinger wrote:
> > > We have 22 different ways to invoke gcc inside Config.lb. That's
> > > just embarrassing.
> >
> > I'd agree.
> > > Suggestion:
> > > action "$(CC) $(DISTRO_CFLAGS) $(CFLAGS) $(CPPFLAGS) $(DEBUG_CFLAGS)
> > > -I$(TOP)/src -I. -nostdinc -nostdlib -fno-builtin -Wall -Os -c -S
> > > $(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -o $@"
>
> it's not really a fix because it depends on people not to drift again
> over time. In the beginning, there was one way to set up this rule,
> and then people changed it, and new files were created, and ... that's
> why we are where we are now.

And once all files in the tree are the same, people will have exactly
_one_ version to choose from regardless of which target they clone and
modify.
> Since the config tool supports include files, why not exploit that
> capability to fix this problem.

And how do you determine which version you have to include? Guess? Hope?
I'd prefer a systematic solution.
1. Unify the code as much as possible.
2. Explain the remaining differences (and spot the bugs).
3. Automatic/scripted conversion to include files.
> Longer term, we need a real set of Makefiles a la v3, and we need to
> remove makerule from the Config.lb files. I suggest putting effort
> there -- this is a flowery fix, and flowers are pretty, but this is
> not fixing the real problem.

Unless someone comes up with the real fix and actually bothers to check
for correctness (ha!), which will probably not happen in the next 6
months, my 3-step plan is the only viable step forward.

However, if you are willing to convert all targets to real makefiles (or
the Config.lb include solution) _and_ check for correctness (not just
rip the current code out and hope for the best), I'll gladly drop my
patch and work on other areas of coreboot. I know you can do magic with
coreboot, but are you really willing to perform this tedious task?

Regards,
Carl-Daniel

[ron minnich]

On Sat, Apr 11, 2009 at 3:23 PM, Carl-Daniel Hailfinger
 wrote:
> And once all files in the tree are the same, people will have exactly
> _one_ version to choose from regardless of which target they clone and
> modify.

well, that makes sense, and I appreciate your good contribution. At
the same time, drift happened: over time,
all the files in the tree *were* the same at one point. This is a good
fix for now, however, as long as that variable comes out :-)

ron

[Peter Stuge]

ron minnich wrote:
> all the files in the tree *were* the same at one point

Excellent point. We should all think about how we can ensure
consistency as far as possible, not only in the build system, but
really throughout the codebase. It's not super easy.

Thanks!

//Peter

[Peter Stuge]

Carl-Daniel Hailfinger wrote:
> > > > action "$(CC) $(DISTRO_CFLAGS) $(CFLAGS) $(CPPFLAGS) $(DEBUG_CFLAGS)
> > > > -I$(TOP)/src -I. -nostdinc -nostdlib -fno-builtin -Wall -Os -c -S
> > > > $(MAINBOARD)/$(CACHE_AS_RAM_AUTO_C) -o $@"
> > Since the config tool supports include files, why not exploit that
> > capability to fix this problem.
>
> And how do you determine which version you have to include? Guess? Hope?
> I'd prefer a systematic solution.
> Unify the code as much as possible.

Didn't you just do this? Or almost? :)
> Explain the remaining differences (and spot the bugs).

Are there any?
> Automatic/scripted conversion to include files.

Sure thing.
> > Longer term, we need a real set of Makefiles a la v3,

I completely agree.
> my 3-step plan is the only viable step forward.

In the short term, agreed. Please go ahead!

//Peter