Subject: Re: linux-next: Tree for March 8 (BROKEN: arch/x86/kernel/entry_32.S? Debian's binutils/as?)

From: Ingo Molnar <[email-masked]>
Date: Wed, 9 Mar 2011 09:51:50 +0100

* H.J. Lu <[email-masked]> wrote:

> On Tue, Mar 8, 2011 at 12:33 PM, Sedat Dilek <[email-masked]> wrote:
> > On Tue, Mar 8, 2011 at 9:25 PM, Alexander van Heukelum
> > <[email-masked]> wrote:
> >> On Tue, 08 Mar 2011 18:53 +0100, "Sedat Dilek" <[email-masked]> wrote:
> >>> On Tue, Mar 8, 2011 at 6:27 PM, Alexander van Heukelum
> >>> <[email-masked]> wrote:
> >>> > On Tue, 08 Mar 2011 16:42 +0100, "Sedat Dilek" <[email-masked]> wrote:
> >>> >> On 3/8/11, Sedat Dilek <[email-masked]> wrote:
> >>> >> > On 3/8/11, H.J. Lu <[email-masked]> wrote:
> >>> >> >> On Tue, Mar 8, 2011 at 2:44 AM, Sedat Dilek <[email-masked]>
> >>> >> >> wrote:
> >>> >> >>> Hi,
> >>> >> >>>
> >>> >> >>> my build of linux-next (next-20110308, the same with the one from
> >>> >> >>> yesterday) is broken.
> >>> >> >>> (I translated the German output.)
> >>> >> >>>
> >>> >> >>> [ build.log ]
> >>> >> >>>  AS      arch/x86/kernel/entry_32.o
> >>> >> >>> /home/sd/src/linux-2.6/linux-2.6.38-rc7/debian/build/source_i386_none/arch/x86/kernel/entry_32.S:
> >>> >> >>> Assembler messages:
> >>> >> >>> /home/sd/src/linux-2.6/linux-2.6.38-rc7/debian/build/source_i386_none/arch/x86/kernel/entry_32.S:1421:
> >>> >> >>> Error: .size expression does not evaluate to a constant
> >>> >> >>> make[6]: *** [arch/x86/kernel/entry_32.o] Fehler 1 (Error 1)
> >>> >> >>> make[5]: *** [arch/x86/kernel] Fehler 2 (Error 2)
> >>> >> >>> make[4]: *** [arch/x86] Fehler 2 (Error 2)
> >>> >> >>> make[4]: *** Warte auf noch nicht beendete Prozesse... (Waiting for
> >>> >> >>> unfinished jobs...)
> >>> >> >>>
> >>> >> >>
> >>> >> >> This is a kernel bug.  Please use the latest binutils from CVS.
> >>> >> >> It will tell you which symbol causes this.
> >>> >> >>
> >>> >> >>
> >>> >> >> --
> >>> >> >> H.J.
> >>> >> >>
> >>> >> >
> >>> >> > Yeah, I have cherry-picked these two upstream commits before you have
> >>> >> > mentionned it...
> >>> >> >
> >>> >> > 0001-Mention-symbol-name-in-non-constant-.size-expression.patch
> >>> >> >        (Cherry-picked from commit b9521fc0be7945fc842ce1197e241a023378125d)
> >>> >> > 0002-Revert-the-last-change-on-gas-elf-bad-size.err.patch
> >>> >> >        (Cherry-picked from commit cbd141bb69f791de7ea1581abe7afb34f0c61288)
> >>> >> >
> >>> >> > ... and have built with them a new binutils Debian package.
> >>> >> >
> >>> >> > The error looks now like this (sorry for the German output):
> >>> >> > ...
> >>> >> >   AS      arch/x86/kernel/entry_32.o
> >>> >> > /home/sd/src/linux-2.6/linux-2.6.38-rc7/debian/build/source_i386_none/arch/x86/kernel/entry_32.S:
> >>> >> > Assembler messages:
> >>> >> > /home/sd/src/linux-2.6/linux-2.6.38-rc7/debian/build/source_i386_none/arch/x86/kernel/entry_32.S:1421:
> >>> >> > Error: .size expression with symbol `apf_page_fault' does not evaluate
> >>> >> > to a constant
> >>> >> > make[6]: *** [arch/x86/kernel/entry_32.o] Fehler 1
> >>> >> > make[5]: *** [arch/x86/kernel] Fehler 2
> >>> >> > make[5]: *** Warte auf noch nicht beendete Prozesse...
> >>> >> >
> >>> >> > Anyway, before more riddling around it would be very helpful to have a
> >>> >> > clear pointer if there is a fix around... That building, testing and
> >>> >> > installing took me now several hours.
> >>> >> > And... yeah, backports to 2.21-branch appreciated.
> >>> >> >
> >>> >> > - Sedat -
> >>> >> >
> >>> >>
> >>> >> After a quick look into the source, it seems attached patch fixes the
> >>> >> issue.
> >>> >> Is that OK?
> >>> >
> >>> > Hi Sedat,
> >>> >
> >>> > The patch ( https://lkml.org/lkml/2011/3/8/203 ) is ok, feel free to add
> >>> > Acked-by: Alexander van Heukelum <[email-masked]>
> >>> >
> >>> > Better description might be something like:
> >>> >
> >>> > i386: Fix mismatched ENTRY/END pair.
> >>> >
> >>> > Under CONFIG_KVM_GUEST=y, the following part of entry_32.S causes a compile failure.
> >>> >
> >>> > 1409 #ifdef CONFIG_KVM_GUEST
> >>> > 1410 ENTRY(async_page_fault)
> >>> > 1411         RING0_EC_FRAME
> >>> > 1412         pushl $do_async_page_fault
> >>> > 1413         CFI_ADJUST_CFA_OFFSET 4
> >>> > 1414         jmp error_code
> >>> > 1415         CFI_ENDPROC
> >>> > 1416 END(apf_page_fault)
> >>> > 1417 #endif
> >>> >
> >>> > Replace apf_page_fault with async_page_fault, as intended.
> >>> >
> >>> > Greetings,
> >>> >    Alexander
> >>> >
> >>> >> - Sedat -
> >>> >>
> >>> >> Email had 1 attachment:
> >>> >> + 0001-x86-Fix-build-failure-with-binutils-as-from-upstream.patch
> >>> >>   1k (text/x-patch)
> >>> >
> >>>
> >>> As I said, quick view on the code, quick fix :-).
> >>>
> >>> Your description is definitive more meaningful.
> >>> I can refresh my patch and add your ACK.
> >>>
> >>> Anyway, I continued after dinner and with the above patch I ran into
> >>> the next problem:
> >>> [ build.log ]
> >>> ...
> >>>   AS      arch/x86/kernel/acpi/wakeup_rm.o
> >>> /home/sd/src/linux-2.6/linux-2.6.38-rc7/debian/build/source_i386_none/arch/x86/kernel/acpi/wakeup_rm.S:
> >>> Assembler messages:
> >>> /home/sd/src/linux-2.6/linux-2.6.38-rc7/debian/build/source_i386_none/arch/x86/kernel/acpi/wakeup_rm.S:12:
> >>> Error: .size expression with symbol `wakeup_code_start' does not
> >>> evaluate to a constant
> >>
> >> No idea what's wrong there. But my version of wakeup_rm.S has only 10 lines...
> >>
> >>     1  /*
> >>     2   * Wrapper script for the realmode binary as a transport object
> >>     3   * before copying to low memory.
> >>     4   */
> >>     5          .section ".rodata","a"
> >>     6          .globl  wakeup_code_start, wakeup_code_end
> >>     7  wakeup_code_start:
> >>     8          .incbin "arch/x86/kernel/acpi/realmode/wakeup.bin"
> >>     9  wakeup_code_end:
> >>    10          .size   wakeup_code_start, .-wakeup_code_start
> >>
> >> And it compiles just fine.
> >> The fix for entry_32.S is valid, though, and necessary for mainline.
> >>
> >> Greetings,
> >>    Alexander
> >>
> >>> I am unsure how to fix that and open for feedback.
> >>>
> >>> - Sedat -
> >>>
> >>
> >
> > The file in linux-next (next-20110308) looks different (the above code
> > looks more logical to me)
> >
> > [ arch/x86/kernel/acpi/wakeup_rm.S ]
> >
> > /*
> >  * Wrapper script for the realmode binary as a transport object
> >  * before copying to low memory.
> >  */
> > #include <asm/page_types.h>
> >
> >        .section ".x86_trampoline","a"
> >        .balign PAGE_SIZE
> >        .globl  acpi_wakeup_code
> > acpi_wakeup_code:
> >        .incbin "arch/x86/kernel/acpi/realmode/wakeup.bin"
> >        .size   wakeup_code_start, .-wakeup_code_start
> >
> 
> Those are simply wrong.  2.6.38-rc8 is OK.

2.6.37-rc8 is not OK: for example commit 631bc4878220932fe67fc46fc7cf7cccdb1ec597 is 
already upstream and if you enable KVM you see a broken kernel build with new 
binutils. This is from 2.6.38-rc8:

 #ifdef CONFIG_KVM_GUEST
 ENTRY(async_page_fault)
        RING0_EC_FRAME
        pushl $do_async_page_fault
        CFI_ADJUST_CFA_OFFSET 4
        jmp error_code
        CFI_ENDPROC
 END(apf_page_fault)
 #endif

Yes, the .size directive not matching up is technically a bug, but it was not 
checked by binutils before, for *years* - and you cannot just flip a switch and 
break who knows how much code.

You need to at least emit a warning for some time to give people a *chance* to fix 
bugs - not just stuff an incompatible binutils down their throat and break the 
kernel build for thousands of commits and break bisection.

This binutils change is breaking numerous upstream kernel builds (and is making 
bisection with new binutils impossible) for no particular good reason: binutils was 
capable to figure out the symbol name before this change.

At minimum you need to *understand* that what you are doing is an incompatible 
change and is disruptive to others ...

Thanks,

	Ingo