On 11/6/10 4:01 PM, Patrick Georgi wrote:
Hi,
I'm currently thinking about various tests we could do on our tree to avoid regressions, not just in functionality (which is somewhat hard to do with all the board specific code we have), but also in style and practices.
Great idea!
My first attempt is the attached script (lint-001-no-global-config-in-romstage), which checks for preprocessor symbols defined in mainboard romstages that are used elsewhere in the tree. As you can see from my other mails these days, I'm currently trying to cut down on these, by moving them to Kconfig. This has several reasons: First, it's necessary to have these symbols available globally when we start building the romstage in more separate compilation units (more on that below). Second, it's basically a second configuration mechanism besides the one we have, which is Kconfig.
The script, when run from the top level directory of a coreboot checkout, emits the symbols that are defined in some romstage and are used elsewhere. No further information is given as the idea is to give a relatively quick overview on the tree, and fetching a list of files where those symbols are in use would be expensive.
Once you feel like this should go in the tree, I will gladly ack it.
I propose to store this script (and similar ones) somewhere under util/, and hook them up in the Makefile ("make lint"?) and in the autobuilder (qa.coreboot.org), and have that report failure if they return any output.
Excellent!
I don't propose to do this now - that would mean that the autobuilder reports errors for the next couple of weeks, but once we got the tree in shape that it passes the test of the script, it could go in and make sure that this issue doesn't come up again.
Please, if anyone can help with the task of cleaning this up so we can add the check earlier, go ahead! It's a great way of improving coreboot without having to know too much about hardware details!
Another test could ensure that no mainboard code (and maybe even more parts of the tree later-on) #includes *.c files. That effort is useful because smaller compilation units make it easier to track down bugs, to validate code, and so on. If I ask you, could you tell (without looking) if init_cpus.c or fidvid.c should be included first on an AMD K8/Fam10 board? It shouldn't matter, but it does.
It's init_cpus.c, by the way ;-)
This kind of stuff has been nasty ever since we were able to write RAM init in C. And the small differences in the variants make the code incredibly hard to maintain. Why does romstage.c have to cope with init_cpus.c at all... CONFIG_CPU_AMD_K8 should let Makefile.inc somewhere under the cpu/ directory know that we want this. Not each individual mainboard.
That said, can we use the Makefile.inc file lists to fake #include statements for romcc? This way we could get rid of the annoyance for non-CAR boards, too. I wonder how complicated it would be to keep the order intact so romcc does not choke. Maybe we can also fix/enhance romcc to be a bit nicer to us...?
External tools, such as lint, splint could also be added into that framework, should they be actually useful for our code (many aren't because of their assumptions on C coding within OS environments).
Should we go ahead and enable scan-build? It suffers from the same problems, and it makes the build incredibly slow, but some of the stuff it finds sure is interesting. (Maybe we should just manually run it and post the results somewhere on qa.coreboot.org if people are interested in fixing this stuff?
This mail is marked RFC for a reason - what do you think?
This is the right path. Can't wait to see this go in the tree.
Stefan