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.
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.
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. That way, no change can readd this "second config system" without being noticed. 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.
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 ;-)
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).
This mail is marked RFC for a reason - what do you think?
Regards, Patrick
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
Am 07.11.2010 05:49, schrieb Stefan Reinauer:
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!
I improved the script to be location independent and to avoid a couple of false positives (GPIO_DEV used to match FOO_GPIO_DEV).
The current list is: CK804_MB_SETUP is defined in mainboard(s) and used elsewhere DEFAULT_RCBA is defined in mainboard(s) and used elsewhere DEVPRES1_CONFIG is defined in mainboard(s) and used elsewhere DEVPRES_CONFIG is defined in mainboard(s) and used elsewhere DIMM0 is defined in mainboard(s) and used elsewhere DIMM1 is defined in mainboard(s) and used elsewhere DIMM_MAP_LOGICAL is defined in mainboard(s) and used elsewhere DQS_TRAIN_DEBUG is defined in mainboard(s) and used elsewhere FIRST_CPU is defined in mainboard(s) and used elsewhere IA32_PERF_STS is defined in mainboard(s) and used elsewhere K8_ALLOCATE_IO_RANGE is defined in mainboard(s) and used elsewhere K8_REV_F_SUPPORT_F0_F1_WORKAROUND is defined in mainboard(s) and used elsewhere MCP55_MB_SETUP is defined in mainboard(s) and used elsewhere MCP55_PCI_E_X_0 is defined in mainboard(s) and used elsewhere MCP55_PCI_E_X_1 is defined in mainboard(s) and used elsewhere PAYLOAD_IS_SEABIOS is defined in mainboard(s) and used elsewhere PLLMSRhi is defined in mainboard(s) and used elsewhere PLLMSRlo is defined in mainboard(s) and used elsewhere RCBA is defined in mainboard(s) and used elsewhere RES_DEBUG is defined in mainboard(s) and used elsewhere SATA_MAP is defined in mainboard(s) and used elsewhere SB_VFSMAF is defined in mainboard(s) and used elsewhere SECOND_CPU is defined in mainboard(s) and used elsewhere SET_NB_CFG_54 is defined in mainboard(s) and used elsewhere SYSTEM_TYPE is defined in mainboard(s) and used elsewhere TOTAL_CPUS is defined in mainboard(s) and used elsewhere UART_DLL is defined in mainboard(s) and used elsewhere UART_DLM is defined in mainboard(s) and used elsewhere UART_FCR is defined in mainboard(s) and used elsewhere UART_IER is defined in mainboard(s) and used elsewhere UART_IIR is defined in mainboard(s) and used elsewhere UART_LCR is defined in mainboard(s) and used elsewhere UART_LSR is defined in mainboard(s) and used elsewhere UART_MCR is defined in mainboard(s) and used elsewhere UART_MSR is defined in mainboard(s) and used elsewhere UART_RBR is defined in mainboard(s) and used elsewhere UART_SCR is defined in mainboard(s) and used elsewhere UART_TBR is defined in mainboard(s) and used elsewhere USE_COM1 is defined in mainboard(s) and used elsewhere USE_COM2 is defined in mainboard(s) and used elsewhere USE_VCP is defined in mainboard(s) and used elsewhere gCom1Base is defined in mainboard(s) and used elsewhere gCom2Base is defined in mainboard(s) and used elsewhere
(in a tree with the SET_FIDVID patch, so that group is already taken care of)
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's part romcc, part newconfig legacy. With romcc the intuitive way was to include .c-files at a central place, and the build system was painful enough that no-one bothered to try to autogenerate that central place from higher level data.
Once we have the config stuff nailed down, the only issue with romcc is the order of the files - if we'd get function prototype support in romcc, that would simplify that step a lot.
From a quick glance it seems that romcc immediately flattens functions
as they come in and thus requires all functions to be present that are used at that point, so that won't be a simple change.
I think we can live without that change.
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...?
I intend to work on that once CAR boards are done, so the order is: - remove the #define config mechanism - remove #include *.c from CAR romstages - remove #include *.c from romcc romstages
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?
Maybe a periodic run (every 10 revisions), similar to how doxygen is handled? But that only makes sense if someone is actually interested in taking them on.
Patrick
Patrick Georgi wrote:
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.
How long does it take to run?
As soon as there are no more warnings I think it should be in the commit hook.
//Peter
Am 08.11.2010 15:44, schrieb Peter Stuge:
Patrick Georgi wrote:
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.
How long does it take to run?
I committed it for now, but sed on coreboot.org doesn't seem to like the sed script. It's about 30 seconds on my mingw install, I think. I'd expect it to be slightly faster on Linux due to it being I/O bound (and Windows doesn't cache and preload as aggressively as Linux).
As soon as there are no more warnings I think it should be in the commit hook.
Not sure if it really belongs in the commit hook - the problem is that it blocks the commit, and already takes a while now. No need to make it even slower.
Patrick
Patrick Georgi wrote:
("make lint"?)
$ make lint make: *** No rule to make target `lint'. Stop.
I like your idea!
How long does it take to run?
I committed it for now, but sed on coreboot.org doesn't seem to like the sed script. It's about 30 seconds on my mingw install, I think. I'd expect it to be slightly faster on Linux
Yes, but maybe still uncomfortably long..
As soon as there are no more warnings I think it should be in the commit hook.
Not sure if it really belongs in the commit hook - the problem is that it blocks the commit, and already takes a while now. No need to make it even slower.
It should block the commit, to stop ugly commits. Sure it takes a while, but I think that's OK. QA is important. It depends on exactly how long it will take..
//Peter
On 12.11.2010, at 09:48, Peter Stuge peter@stuge.se wrote:
As soon as there are no more warnings I think it should be in the commit hook.
Not sure if it really belongs in the commit hook - the problem is that it blocks the commit, and already takes a while now. No need to make it even slower.
It should block the commit, to stop ugly commits. Sure it takes a while, but I think that's OK. QA is important. It depends on exactly how long it will take..
I think it should run asynchronously on commit just like abuild does. And add it's findings to the abuild mail report.
Or maybe abuild should call it?
Stefan
//Peter
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Stefan Reinauer wrote:
It should block the commit, to stop ugly commits. Sure it takes a while, but I think that's OK. QA is important. It depends on exactly how long it will take..
I think it should run asynchronously on commit just like abuild does. And add it's findings to the abuild mail report.
Or maybe abuild should call it?
If it turns out to take too long for actually in the hook then yes, I think in abuild is a great idea!
//Peter
On 12.11.2010, at 10:51, Peter Stuge peter@stuge.se wrote:
Stefan Reinauer wrote:
It should block the commit, to stop ugly commits. Sure it takes a while, but I think that's OK. QA is important. It depends on exactly how long it will take..
I think it should run asynchronously on commit just like abuild does. And add it's findings to the abuild mail report.
Or maybe abuild should call it?
If it turns out to take too long for actually in the hook then yes, I think in abuild is a great idea!
Any time of 5 seconds or longer will be too long.
Stefan
Am 12.11.2010 20:04, schrieb Stefan Reinauer:
Any time of 5 seconds or longer will be too long.
Given that it scans the entire source text of the entire tree several times, it will probably take more than 5 seconds in any case. Also, there might be more tests in the future, adding even more runtime.
Patrick
-
Patrick Georgi -
Peter Stuge -
Stefan Reinauer