[LinuxBIOS] v3: Miscompilation problems

[Carl-Daniel Hailfinger]

Hi,

it seems that
gcc (GCC) 4.2.1 (SUSE Linux)
can not compile our XIP code in v3 correctly unless -fno-unit-at-a-time
is set as compiler flag. While the compiler will not error out, the
generated code will NOT do what you expect.

If a shared function such as printk is called from multiple functions in
a file, only the printk calls from the most deeply nested function, or
in case there are two functions at the deepest nesting level, the printk
calls from the function executed last in sequence will be executed. The
others seem to be skipped. While I can see the calls in the generated
assembly code, their parameters/offsets are wrong except for the working
ones.

While the recent patches make our code compile again, they have
triggered another compiler bug, at least on my machine. If there is no
printk assignment in the C file (because _MAINOBJECT is not defined),
the generated assembly code is incorrect as well and printk calls will
end up somewhere in memory, hanging the machine.

How do we proceed? Blacklist compiler versions? Complain to the gcc
maintainers? Look for flags which fix the miscompilation (seems to work
for the first problem)? Hack around with ld scripts (seems to work for
the second problem)?

I have a patch for the first issue in my tree and will send it in as
soon as my net access works again well enough to update my tree to HEAD.

Regards,
Carl-Daniel

[ron minnich]

I think we're going to have to take the approach of buildroot, where
we pull down a working gcc as part of linuxbios build.

I don't like this. But I'm getting tired of the various distros breaking gnubin.

ron

[Carl-Daniel Hailfinger]

On 25.11.2007 03:57, ron minnich wrote:
> I think we're going to have to take the approach of buildroot, where
> we pull down a working gcc as part of linuxbios build.
>
> I don't like this. But I'm getting tired of the various distros breaking gnubin.

After spending hours looking at the generated assembly output, it seems
the problem is NOT with the compiler, but with the linker. Basically,
for XIP code, only the first function in the assembly output is
executed. With -funit-at-a-time, that was usually the function placed
last in the code. So I'm now chasing a bug either in our linker commands
or in binutils.

Regards,
Carl-Daniel

[ron minnich]

On Nov 26, 2007 2:12 AM, Carl-Daniel Hailfinger
 wrote:
> After spending hours looking at the generated assembly output, it seems
> the problem is NOT with the compiler, but with the linker. Basically,
> for XIP code, only the first function in the assembly output is
> executed. With -funit-at-a-time, that was usually the function placed
> last in the code. So I'm now chasing a bug either in our linker commands
> or in binutils.

I think we can take a better approach. The real problem is the binary
file created with objcopy. When we use a binary file, we remove a lot
of useful information. Since lar can now handle ELF, we should take
advantage of that, rather than forever relying on the order in which
gld places functions in files.

So how do we fix this? It's a few steps.

First, have the mainboard makefile link the initram starting at 0, not
the linux application default of 0x08whatever. Note the -Ttext 0
addition below to ld. What does this do? It gives us an entry point
that is in effect an offset from the start of the initram, so we can
just add it to the starting point of initram.

+       $(Q)$(LD) -Ttext 0 --entry main -N -R $(obj)/stage0-prefixed.o \
+               $(patsubst %.o,%_xip.o,$(INITRAM_OBJ)) -o
$(obj)/linuxbios.initram.o

This actually is only one step. The problem is that LAR is processing
the binary blob of initram, not the ELF file. So lots of info is gone.
Now that LAR can process ELF files, we should stop using binary files
as much as we can.

Have LAR use the initram.o, not the binary version of initram:
Index: arch/x86/Makefile
===================================================================
--- arch/x86/Makefile   (revision 518)
+++ arch/x86/Makefile   (working copy)
@@ -36,7 +36,7 @@

ROM_SIZE := $(shell expr $(CONFIG_LINUXBIOS_ROMSIZE_KB) * 1024)

-LARFILES := nocompress:normal/initram normal/stage2
nocompress:normal/option_table
+LARFILES := nocompress:normal/initram.o normal/stage2
nocompress:normal/option_table
 ifneq ($(CONFIG_PAYLOAD_NONE),y)
 LARFILES += normal/payload
 endif
@@ -57,11 +57,11 @@
 COMPRESSFLAG := -C nrv2b
 endif

-$(obj)/linuxbios.rom $(obj)/linuxbios.map: $(obj)/linuxbios.bootblock
$(obj)/util/lar/lar lzma nrv2b $(obj)/linuxbios.initram
$(obj)/linuxbios.stage2 $(obj)/option_table
+$(obj)/linuxbios.rom $(obj)/linuxbios.map: $(obj)/linuxbios.bootblock
$(obj)/util/lar/lar lzma nrv2b $(obj)/linuxbios.initram.o
$(obj)/linuxbios.stage2 $(obj)/option_table
        $(Q)rm -rf $(obj)/lar.tmp
        $(Q)mkdir $(obj)/lar.tmp
        $(Q)mkdir $(obj)/lar.tmp/normal
-       $(Q)cp $(obj)/linuxbios.initram $(obj)/lar.tmp/normal/initram
+       $(Q)cp $(obj)/linuxbios.initram.o $(obj)/lar.tmp/normal/initram.o
        $(Q)cp $(obj)/linuxbios.stage2 $(obj)/lar.tmp/normal/stage2
        $(Q)cp $(obj)/option_table $(obj)/lar.tmp/normal/option_table
 ifeq ($(CONFIG_PAYLOAD_NONE),y)

When it builds, we get this in the LAR:
  normal/initram.o/segment0 (4948 bytes @0x13bd0);loadaddress 0x0 entry 0x0x6c

note the entry: 0x6c, a nice clean offset from the start. At runtime,
this can be added to the start of the lar entry and we will have a
good entry point.

Now, the name has changed: normal/initram.o/segment0, so we change
stage1.c just a bit:
Index: arch/x86/stage1.c
===================================================================
--- arch/x86/stage1.c   (revision 518)
+++ arch/x86/stage1.c   (working copy)
@@ -138,10 +138,10 @@
        // find first initram
        if (check_normal_boot_flag()) {
                printk(BIOS_DEBUG, "Choosing normal boot.\n");
-               ret = execute_in_place(&archive, "normal/initram");
+               ret = execute_in_place(&archive, "normal/initram.o/segment0");
        } else {
                printk(BIOS_DEBUG, "Choosing fallback boot.\n");
-               ret = execute_in_place(&archive, "fallback/initram");
+               ret = execute_in_place(&archive, "fallback/initram.o/segment0");
                /* Try a normal boot if fallback doesn't exist in the lar.
                 * TODO: There are other ways to do this.
                 * It could be ifdef or the boot flag could be forced.

So the name is a but clumsier but overall this is better. We're using
the ability of LAR to parse elf, and we simplify our messing around
with ld.

Comments?

ron