Hi,
I was told about an issue with our romstage handling, as the linker happily links in global variables (with addresses in ROM).
That obviously doesn't work for writable values, but is a silent error (until things _really_ go wrong on runtime), so I looked for a way to break coreboot at build time in such a scenario.
Two possible solutions, both need to be added to src/arch/i386/init/ldscript_fallback_cbfs.lb:
1. _bogus = ASSERT((SIZEOF(.bss) + SIZEOF(.data)) == 0, "Do not use global variables in romstage");
This one looks for the size of .bss and .data (initialized and uninitialized globals) and breaks the build if it finds any. It doesn't tell, which global variables are involved.
2. Add .bss and .data to /DISCARD/.
That way, the linker complains about "missing" symbols (as they were discarded), but only if they were used by any surviving code. This means, it's no problem that the global is defined. If you use it later-on, it "mysteriously" breaks. The error message is rather cryptic, too: `variable' referenced in section `.rom.text' of coreboot-builds/kontron_986lcd-m/mainboard/kontron/986lcd-m/crt0.initobj.o: defined in discarded section `.data' of coreboot-builds/kontron_986lcd-m/mainboard/kontron/986lcd-m/crt0.initobj.o collect2: ld returned 1 exit status
I tend to prefer the first option, but it doesn't give _any_ clues which variable is responsible (except for "look for globals"), but at least it does tell the user what the real problem is.
Opinions?
There might also be a good opportunity for some naming cleanup: Rename and move ldscript_fallback_cbfs.lb to src/arch/i386/coreboot_rom.ld. Or better call it "romstage.ld" (and adapt coreboot_ram.ld in arch/i386 in the same way)?
Regards, Patrick
On 8/12/10 9:27 PM, Patrick Georgi wrote:
- _bogus = ASSERT((SIZEOF(.bss) + SIZEOF(.data)) == 0, "Do not use
global variables in romstage");
This one looks for the size of .bss and .data (initialized and uninitialized globals) and breaks the build if it finds any. It doesn't tell, which global variables are involved.
- Add .bss and .data to /DISCARD/.
That way, the linker complains about "missing" symbols (as they were discarded), but only if they were used by any surviving code. This means, it's no problem that the global is defined. If you use it later-on, it "mysteriously" breaks. The error message is rather cryptic, too: `variable' referenced in section `.rom.text' of coreboot-builds/kontron_986lcd-m/mainboard/kontron/986lcd-m/crt0.initobj.o: defined in discarded section `.data' of coreboot-builds/kontron_986lcd-m/mainboard/kontron/986lcd-m/crt0.initobj.o collect2: ld returned 1 exit status
I tend to prefer the first option, but it doesn't give _any_ clues which variable is responsible (except for "look for globals"), but at least it does tell the user what the real problem is.
I prefer option one, too, as - it catches all global variables, not just the used ones. We want to be sure that no such code sneaks in even if it's not used. - it provides a clear error message on what is wrong, inszead of the more cryptic one saying that a variable is defined in a discarded section.
There might also be a good opportunity for some naming cleanup: Rename and move ldscript_fallback_cbfs.lb to src/arch/i386/coreboot_rom.ld. Or better call it "romstage.ld" (and adapt coreboot_ram.ld in arch/i386 in the same way)?
I like the latter.
Fixes to both issues are:
Acked-by: Stefan Reinauer stepan@coresystems.de
Stefan
On 14.08.2010 11:40, Stefan Reinauer wrote:
On 8/12/10 9:27 PM, Patrick Georgi wrote:
- [...]
This one looks for the size of .bss and .data (initialized and uninitialized globals) and breaks the build if it finds any. It doesn't tell, which global variables are involved.
- Add .bss and .data to /DISCARD/.
[...]
I prefer option one, too, as
- it catches all global variables, not just the used ones. We want to be
sure that no such code sneaks in even if it's not used.
- it provides a clear error message on what is wrong, inszead of the
more cryptic one saying that a variable is defined in a discarded section.
Should we move some of the section checking of v3 to v4 as well? I will soon have a bit more free time and plan to get up to speed on coreboot development again.
Regards, Carl-Daniel
On 8/14/10 11:50 AM, Carl-Daniel Hailfinger wrote:
On 14.08.2010 11:40, Stefan Reinauer wrote:
On 8/12/10 9:27 PM, Patrick Georgi wrote:
- [...]
This one looks for the size of .bss and .data (initialized and uninitialized globals) and breaks the build if it finds any. It doesn't tell, which global variables are involved.
- Add .bss and .data to /DISCARD/.
[...]
I prefer option one, too, as
- it catches all global variables, not just the used ones. We want to be
sure that no such code sneaks in even if it's not used.
- it provides a clear error message on what is wrong, inszead of the
more cryptic one saying that a variable is defined in a discarded section.
Should we move some of the section checking of v3 to v4 as well? I will soon have a bit more free time and plan to get up to speed on coreboot development again.
util/sectionchecker/sectionchecker ?
Looks nice. I think this might be a way to go.
What are the advantages over the two suggestions above?
Stefan
Possibly stupid question. Why don't allow the global vars in CAR? The CAR is always fixed on some addresses it looks it could work?
Thanks, Rudolf
On 14.08.2010 12:38, Rudolf Marek wrote:
Possibly stupid question. Why don't allow the global vars in CAR? The CAR is always fixed on some addresses it looks it could work?
v3 had global variables in CAR, and they were even accessible in the RAM stage. In fact, I consider this to be one of the really nice features we want. Global variables in CAR are not simple to do, and AFAIK that part of the v4 codebase is still unchanged from v2. One day I will have time to port that feature from v3 to v4.
Regards, Carl-Daniel
On 14.08.2010 12:24, Stefan Reinauer wrote:
On 8/14/10 11:50 AM, Carl-Daniel Hailfinger wrote:
On 14.08.2010 11:40, Stefan Reinauer wrote:
On 8/12/10 9:27 PM, Patrick Georgi wrote:
- [...]
This one looks for the size of .bss and .data (initialized and uninitialized globals) and breaks the build if it finds any. It doesn't tell, which global variables are involved.
- Add .bss and .data to /DISCARD/.
[...]
I prefer option one, too, as
- it catches all global variables, not just the used ones. We want to be
sure that no such code sneaks in even if it's not used.
- it provides a clear error message on what is wrong, inszead of the
more cryptic one saying that a variable is defined in a discarded section.
Should we move some of the section checking of v3 to v4 as well? I will soon have a bit more free time and plan to get up to speed on coreboot development again.
util/sectionchecker/sectionchecker ?
Looks nice. I think this might be a way to go.
What are the advantages over the two suggestions above?
Quoting from that file: # This program checks whether a given list of files ($3+) has any writable # and allocatable non-empty sections. If so, the name of the file, the name of # the section and the symbols in that section are printed and an error is # returned.
Given that the code already exists and worked well back then, it might be a good starting point. The sectionchecker is not a ldscript and that might make it more attractive for some people.
Regards, Carl-Daniel
On 8/14/10 12:40 PM, Carl-Daniel Hailfinger wrote:
On 14.08.2010 12:24, Stefan Reinauer wrote:
On 8/14/10 11:50 AM, Carl-Daniel Hailfinger wrote:
On 14.08.2010 11:40, Stefan Reinauer wrote:
On 8/12/10 9:27 PM, Patrick Georgi wrote:
- [...]
This one looks for the size of .bss and .data (initialized and uninitialized globals) and breaks the build if it finds any. It doesn't tell, which global variables are involved.
- Add .bss and .data to /DISCARD/.
[...]
I prefer option one, too, as
- it catches all global variables, not just the used ones. We want to be
sure that no such code sneaks in even if it's not used.
- it provides a clear error message on what is wrong, inszead of the
more cryptic one saying that a variable is defined in a discarded section.
Should we move some of the section checking of v3 to v4 as well? I will soon have a bit more free time and plan to get up to speed on coreboot development again.
util/sectionchecker/sectionchecker ?
Looks nice. I think this might be a way to go.
What are the advantages over the two suggestions above?
Quoting from that file: # This program checks whether a given list of files ($3+) has any writable # and allocatable non-empty sections. If so, the name of the file, the name of # the section and the symbols in that section are printed and an error is # returned.
Given that the code already exists and worked well back then, it might be a good starting point. The sectionchecker is not a ldscript and that might make it more attractive for some people.
Yes, I like that part about it, too...
The only downside I see is that it does a lot of shell stuff which will slow things down on Windows significantly. (And the folks that ran into the problem to begin with were on Windows afaict) ... It might not matter however, since no problems we had solve in this project so far require an almost correct answer in shorter time. After all we're not running from lions.
I like the approach, but I would like to hear some opinions from the Windows using folks
Stefan
Am 14.08.2010 12:47, schrieb Stefan Reinauer:
The only downside I see is that it does a lot of shell stuff which will slow things down on Windows significantly. (And the folks that ran into the problem to begin with were on Windows afaict) ...
That is my main concern. And while we're there, "#!/bin/bash" is not only windows-unfriendly, but a horrible idea for anything that intends to work on non-linux. (and on linux, using any other shell than bash for scripts considerably speeds them up, too)
Patrick
On Sat, Aug 14, 2010 at 3:40 AM, Stefan Reinauer stefan.reinauer@coresystems.de wrote:
On 8/12/10 9:27 PM, Patrick Georgi wrote:
- _bogus = ASSERT((SIZEOF(.bss) + SIZEOF(.data)) == 0, "Do not use
global variables in romstage");
This one looks for the size of .bss and .data (initialized and uninitialized globals) and breaks the build if it finds any. It doesn't tell, which global variables are involved.
- Add .bss and .data to /DISCARD/.
That way, the linker complains about "missing" symbols (as they were discarded), but only if they were used by any surviving code. This means, it's no problem that the global is defined. If you use it later-on, it "mysteriously" breaks. The error message is rather cryptic, too: `variable' referenced in section `.rom.text' of coreboot-builds/kontron_986lcd-m/mainboard/kontron/986lcd-m/crt0.initobj.o: defined in discarded section `.data' of coreboot-builds/kontron_986lcd-m/mainboard/kontron/986lcd-m/crt0.initobj.o collect2: ld returned 1 exit status
I tend to prefer the first option, but it doesn't give _any_ clues which variable is responsible (except for "look for globals"), but at least it does tell the user what the real problem is.
I prefer option one, too, as
- it catches all global variables, not just the used ones. We want to be
sure that no such code sneaks in even if it's not used.
- it provides a clear error message on what is wrong, inszead of the
more cryptic one saying that a variable is defined in a discarded section.
I agree too. The problem was that a global variable was added unintentionally. Once the problem is indicated, it isn't hard to find the offending code.
Marc
On Thu, Aug 12, 2010 at 09:27:39PM +0200, Patrick Georgi wrote:
Two possible solutions, both need to be added to src/arch/i386/init/ldscript_fallback_cbfs.lb:
- _bogus = ASSERT((SIZEOF(.bss) + SIZEOF(.data)) == 0, "Do not use
global variables in romstage");
[...]
- Add .bss and .data to /DISCARD/.
Just as a point of reference, SeaBIOS has been using method 2. (SeaBIOS needs to ensure the 16bit code doesn't use any regular global variables.)
-Kevin
-
Carl-Daniel Hailfinger -
Kevin O'Connor -
Marc Jones -
Patrick Georgi -
Rudolf Marek -
Stefan Reinauer