Message ID | 550715C8.7020508@redhat.com |
---|---|
State | New |
Headers | show |
On 03/16/2015 01:41 PM, Martin Sebor wrote: > The attached patch fixes a glibc build failure with gcc 5 on > powerpc64le caused by a recent change in gcc where the compiler > defines the _ARCH_PWR6 macro when processing assembly files > but doesn't invoke the assembler in the corresponding machine > mode (unless it has been explicitly configured to target POWER > 6 or later). A bug had been filed with gcc for this (65341) but > was closed as won't fix. Glibc relies on the _ARCH_PWR6 macro > in a few .S files to make use of Power ISA 2.5 instructions > (specifically, the four-argument form of the mtfsf insn). > A similar problem had occurred in the past (bug 10118) but > the fix that was committed for it didn't anticipate this new > problem. The fix in the proposed patch introduces the .machine > "power6" directive unconditionaly, regardless of whether > _ARCH_PWR6 is defined. For avoidance of doubt the `#ifdef _ARCH_PWR6` need cleaning up, but that's another issue orthogonal to this patch. If gcc no longer defines _ARCH_PWR6 when invoking the assembler, then glibc has use it's own configury to determine exactly which of the two code segments to include in the final assembly. Given that both branches of the #ifdef use mtfsf they both require power6 or higher, and therefore it is correct to move the .machine directives outside of the #ifdef. If you tested this worked on pp64 and ppc64le, then I'd say this is an obvious fix that you can commit immediately without waiting for Steven or Tulio to comment (machine maintainers). Cheers, Carlos. > 2015-03-11 Martin Sebor <msebor@redhat.com> > > * sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S > (__setcontext): Set machine to power6 regardless of whether > or not _ARCH_PWR6 is defined. > * sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S > (__novec_swapcontext): Likewise. > > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S > index e47a57a..a1ed419 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S > @@ -79,12 +79,13 @@ ENTRY(__novec_setcontext) > lfd fp31,(SIGCONTEXT_FP_REGS+(PT_R31*8))(r31) > lfd fp30,(SIGCONTEXT_FP_REGS+(PT_R30*8))(r31) > > + .machine push > + .machine "power6" > + > # ifdef _ARCH_PWR6 > /* Use the extended four-operand version of the mtfsf insn. */ > mtfsf 0xff,fp0,1,0 > # else > - .machine push > - .machine "power6" > /* Availability of DFP indicates a 64-bit FPSCR. */ > andi. r6,r5,PPC_FEATURE_HAS_DFP > beq 5f > @@ -95,8 +96,10 @@ ENTRY(__novec_setcontext) > 5: > mtfsf 0xff,fp0 > 6: > - .machine pop > # endif /* _ARCH_PWR6 */ > + > + .machine pop > + > lfd fp29,(SIGCONTEXT_FP_REGS+(PT_R29*8))(r31) > lfd fp28,(SIGCONTEXT_FP_REGS+(PT_R28*8))(r31) > lfd fp27,(SIGCONTEXT_FP_REGS+(PT_R27*8))(r31) > @@ -362,12 +365,13 @@ L(has_no_vec): > lfd fp31,(SIGCONTEXT_FP_REGS+(PT_R31*8))(r31) > lfd fp30,(SIGCONTEXT_FP_REGS+(PT_R30*8))(r31) > > + .machine push > + .machine "power6" > + > # ifdef _ARCH_PWR6 > /* Use the extended four-operand version of the mtfsf insn. */ > mtfsf 0xff,fp0,1,0 > # else > - .machine push > - .machine "power6" > /* Availability of DFP indicates a 64-bit FPSCR. */ > andi. r6,r5,PPC_FEATURE_HAS_DFP > beq 7f > @@ -378,8 +382,10 @@ L(has_no_vec): > 7: > mtfsf 0xff,fp0 > 8: > - .machine pop > # endif /* _ARCH_PWR6 */ > + > + .machine pop > + > lfd fp29,(SIGCONTEXT_FP_REGS+(PT_R29*8))(r31) > lfd fp28,(SIGCONTEXT_FP_REGS+(PT_R28*8))(r31) > lfd fp27,(SIGCONTEXT_FP_REGS+(PT_R27*8))(r31) > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S > index bc02a21..b25904d 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S > @@ -173,6 +173,10 @@ ENTRY(__novec_swapcontext) > lfd fp0,(SIGCONTEXT_FP_REGS+(32*8))(r31) > lfd fp31,(SIGCONTEXT_FP_REGS+(PT_R31*8))(r31) > lfd fp30,(SIGCONTEXT_FP_REGS+(PT_R30*8))(r31) > + > + .machine push > + .machine "power6" > + > # ifdef _ARCH_PWR6 > /* Use the extended four-operand version of the mtfsf insn. */ > mtfsf 0xff,fp0,1,0 > @@ -189,8 +193,10 @@ ENTRY(__novec_swapcontext) > 5: > mtfsf 0xff,fp0 > 6: > - .machine pop > #endif /* _ARCH_PWR6 */ > + > + .machine pop > + > lfd fp29,(SIGCONTEXT_FP_REGS+(PT_R29*8))(r31) > lfd fp28,(SIGCONTEXT_FP_REGS+(PT_R28*8))(r31) > lfd fp27,(SIGCONTEXT_FP_REGS+(PT_R27*8))(r31) > @@ -652,12 +658,14 @@ L(has_no_vec2): > lfd fp0,(SIGCONTEXT_FP_REGS+(32*8))(r31) > lfd fp31,(SIGCONTEXT_FP_REGS+(PT_R31*8))(r31) > lfd fp30,(SIGCONTEXT_FP_REGS+(PT_R30*8))(r31) > + > + .machine push > + .machine "power6" > + > # ifdef _ARCH_PWR6 > /* Use the extended four-operand version of the mtfsf insn. */ > mtfsf 0xff,fp0,1,0 > # else > - .machine push > - .machine "power6" > /* Availability of DFP indicates a 64-bit FPSCR. */ > andi. r6,r8,PPC_FEATURE_HAS_DFP > beq 7f > @@ -668,8 +676,10 @@ L(has_no_vec2): > 7: > mtfsf 0xff,fp0 > 8: > - .machine pop > #endif /* _ARCH_PWR6 */ > + > + .machine pop > + > lfd fp29,(SIGCONTEXT_FP_REGS+(PT_R29*8))(r31) > lfd fp28,(SIGCONTEXT_FP_REGS+(PT_R28*8))(r31) > lfd fp27,(SIGCONTEXT_FP_REGS+(PT_R27*8))(r31)
On Mon, 2015-03-16 at 14:08 -0400, Carlos O'Donell wrote: > On 03/16/2015 01:41 PM, Martin Sebor wrote: > > The fix in the proposed patch introduces the .machine > > "power6" directive unconditionaly, regardless of whether > > _ARCH_PWR6 is defined. I'll let Steve and/or Tulio have the last say, but I think this is the correct fix. > Given that both branches of the #ifdef use mtfsf they both require > power6 or higher, and therefore it is correct to move the .machine > directives outside of the #ifdef. To be pedantic, the mtspr instruction has existed long before power6. What was added with power6 was we doubled the size of the FPSCR for use with DFP and we added an extra operand to mtfsf to specify which word of the FPSCR we were writing to. The mtfsf here is using the extra operand, so it does need ".machine power[678]" to assemble without error. Peter
On Mon, 2015-03-16 at 14:08 -0400, Carlos O'Donell wrote: > On 03/16/2015 01:41 PM, Martin Sebor wrote: > > The attached patch fixes a glibc build failure with gcc 5 on > > powerpc64le caused by a recent change in gcc where the compiler > > defines the _ARCH_PWR6 macro when processing assembly files > > but doesn't invoke the assembler in the corresponding machine > > mode (unless it has been explicitly configured to target POWER > > 6 or later). A bug had been filed with gcc for this (65341) but > > was closed as won't fix. Glibc relies on the _ARCH_PWR6 macro > > in a few .S files to make use of Power ISA 2.5 instructions > > (specifically, the four-argument form of the mtfsf insn). > > A similar problem had occurred in the past (bug 10118) but > > the fix that was committed for it didn't anticipate this new > > problem. The fix in the proposed patch introduces the .machine > > "power6" directive unconditionaly, regardless of whether > > _ARCH_PWR6 is defined. > > For avoidance of doubt the `#ifdef _ARCH_PWR6` need cleaning up, > but that's another issue orthogonal to this patch. If gcc no longer > defines _ARCH_PWR6 when invoking the assembler, then glibc has use > it's own configury to determine exactly which of the two code > segments to include in the final assembly. > I believe that problem is different then stated. With GCC-5.0 the gcc driver for the powerpc64le target defaults to power8. This sets the _ARCH_PWR8, _ARCH_PWR7, _ARCH_PWR6, etc. macros. But without an explicit -mcpu=power6|7|8, gcc will not pass a -mpower6|7|8 option to the assembler. The 4 operand form of mtfsf and require for the handled the extended FPSCR with Decimal Floating Point support. The DFP feature is supported on POWER6|7|8 but I don't think it is supported for Freescale e5500. So the 4 operand form should be used only for powerpc processors that also support DFP. stops passing the -many or -mpowerX option for the assembler (as). As far as I know the compiler continues to set the _ARCH_PWRx defines based on the defaults or -mcpu= option. GCC will generate the appropriate .machine for code it generates but this creates a problem for existing asm source codes. And So the code should use #ifdef _ARCH_PWR6 to both assert .machine power6 and use the 4 operand form of mtfsf. The #else leg should use the 2 operand mtfsf only. But we may need to assert .macine power6 again to allow us of the vector ops. PS still waiting for my account access to cleared so I cant do much with this yet.
On 03/16/2015 03:50 PM, Steven Munroe wrote: > On Mon, 2015-03-16 at 14:08 -0400, Carlos O'Donell wrote: >> On 03/16/2015 01:41 PM, Martin Sebor wrote: >>> The attached patch fixes a glibc build failure with gcc 5 on >>> powerpc64le caused by a recent change in gcc where the compiler >>> defines the _ARCH_PWR6 macro when processing assembly files >>> but doesn't invoke the assembler in the corresponding machine >>> mode (unless it has been explicitly configured to target POWER >>> 6 or later). A bug had been filed with gcc for this (65341) but >>> was closed as won't fix. Glibc relies on the _ARCH_PWR6 macro >>> in a few .S files to make use of Power ISA 2.5 instructions >>> (specifically, the four-argument form of the mtfsf insn). >>> A similar problem had occurred in the past (bug 10118) but >>> the fix that was committed for it didn't anticipate this new >>> problem. The fix in the proposed patch introduces the .machine >>> "power6" directive unconditionaly, regardless of whether >>> _ARCH_PWR6 is defined. >> >> For avoidance of doubt the `#ifdef _ARCH_PWR6` need cleaning up, >> but that's another issue orthogonal to this patch. If gcc no longer >> defines _ARCH_PWR6 when invoking the assembler, then glibc has use >> it's own configury to determine exactly which of the two code >> segments to include in the final assembly. >> > I believe that problem is different then stated. With GCC-5.0 the gcc > driver for the powerpc64le target defaults to power8. This sets the > _ARCH_PWR8, _ARCH_PWR7, _ARCH_PWR6, etc. macros. But without an explicit > -mcpu=power6|7|8, gcc will not pass a -mpower6|7|8 option to the > assembler. So the macro is set, but the assembler support is now orthogonal to the macro. > The 4 operand form of mtfsf and require for the handled the extended > FPSCR with Decimal Floating Point support. The DFP feature is supported > on POWER6|7|8 but I don't think it is supported for Freescale e5500. That would be an orthogonal issue that would still need fixing outside of this correction. > So the 4 operand form should be used only for powerpc processors that > also support DFP. I suggest breaking that out into a distinct bug. > stops passing the -many or -mpowerX option for the assembler (as). As > far as I know the compiler continues to set the _ARCH_PWRx defines based > on the defaults or -mcpu= option. OK. > GCC will generate the appropriate .machine for code it generates but > this creates a problem for existing asm source codes. And OK. > So the code should use #ifdef _ARCH_PWR6 to both assert .machine power6 > and use the 4 operand form of mtfsf. The #else leg should use the 2 > operand mtfsf only. But we may need to assert .macine power6 again to > allow us of the vector ops. > > PS still waiting for my account access to cleared so I cant do much with > this yet. You can do everything except checkin the code? If you review Martin's patch and it looks good as a fix for the immediate build failures, please ACK it? Cheers, Carlos.
On Mon, 2015-03-16 at 11:41 -0600, Martin Sebor wrote: > The attached patch fixes a glibc build failure with gcc 5 on > powerpc64le caused by a recent change in gcc where the compiler > defines the _ARCH_PWR6 macro when processing assembly files > but doesn't invoke the assembler in the corresponding machine > mode (unless it has been explicitly configured to target POWER > 6 or later). A bug had been filed with gcc for this (65341) but > was closed as won't fix. Glibc relies on the _ARCH_PWR6 macro > in a few .S files to make use of Power ISA 2.5 instructions > (specifically, the four-argument form of the mtfsf insn). > A similar problem had occurred in the past (bug 10118) but > the fix that was committed for it didn't anticipate this new > problem. The fix in the proposed patch introduces the .machine > "power6" directive unconditionaly, regardless of whether > _ARCH_PWR6 is defined. > I think this patch is incorrect for the architecture. The 4 operand form of mtfsf should only be used with processors that support the DFP category. So the .machine power6 should not be moved above the #ifdef. The .machine should be in the #ifdef _ARCH_PWR6 #else "leg" specifically around the mtfsf 4 operand form following the PPC_FEATURE_HAS_DFP test. > Martin
On 03/16/2015 02:37 PM, Steven Munroe wrote: > On Mon, 2015-03-16 at 11:41 -0600, Martin Sebor wrote: >> The attached patch fixes a glibc build failure with gcc 5 on >> powerpc64le caused by a recent change in gcc where the compiler >> defines the _ARCH_PWR6 macro when processing assembly files >> but doesn't invoke the assembler in the corresponding machine >> mode (unless it has been explicitly configured to target POWER >> 6 or later). A bug had been filed with gcc for this (65341) but >> was closed as won't fix. Glibc relies on the _ARCH_PWR6 macro >> in a few .S files to make use of Power ISA 2.5 instructions >> (specifically, the four-argument form of the mtfsf insn). >> A similar problem had occurred in the past (bug 10118) but >> the fix that was committed for it didn't anticipate this new >> problem. The fix in the proposed patch introduces the .machine >> "power6" directive unconditionaly, regardless of whether >> _ARCH_PWR6 is defined. >> > > I think this patch is incorrect for the architecture. The 4 operand form > of mtfsf should only be used with processors that support the DFP > category. > > So the .machine power6 should not be moved above the #ifdef. > The .machine should be in the #ifdef _ARCH_PWR6 #else "leg" specifically > around the mtfsf 4 operand form following the PPC_FEATURE_HAS_DFP test. That's where the directive currently is, i.e., only in the #else block. The problem is that the four operand mtfsf is used in both blocks. That causes the error when the assembler sees the #if block without the .machine "power6" directive. Since the Power 2.5 instruction is used in both conditionally included blocks the proposed patch introduces the directive unconditionally, outside either. Another way to fix the problem would be to do both of: a) move the .machine directive to the #if block b) change the #else block to avoid using the four operand form of the mtfsf instruction. or both of: a) keep the .machine directive in the #else block b) change the #if block to avoid using the four operand form of the mtfsf instruction. Is one of these what you're recommending? Martin
2015-03-11 Martin Sebor <msebor@redhat.com> * sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S (__setcontext): Set machine to power6 regardless of whether or not _ARCH_PWR6 is defined. * sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S (__novec_swapcontext): Likewise. diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S index e47a57a..a1ed419 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S @@ -79,12 +79,13 @@ ENTRY(__novec_setcontext) lfd fp31,(SIGCONTEXT_FP_REGS+(PT_R31*8))(r31) lfd fp30,(SIGCONTEXT_FP_REGS+(PT_R30*8))(r31) + .machine push + .machine "power6" + # ifdef _ARCH_PWR6 /* Use the extended four-operand version of the mtfsf insn. */ mtfsf 0xff,fp0,1,0 # else - .machine push - .machine "power6" /* Availability of DFP indicates a 64-bit FPSCR. */ andi. r6,r5,PPC_FEATURE_HAS_DFP beq 5f @@ -95,8 +96,10 @@ ENTRY(__novec_setcontext) 5: mtfsf 0xff,fp0 6: - .machine pop # endif /* _ARCH_PWR6 */ + + .machine pop + lfd fp29,(SIGCONTEXT_FP_REGS+(PT_R29*8))(r31) lfd fp28,(SIGCONTEXT_FP_REGS+(PT_R28*8))(r31) lfd fp27,(SIGCONTEXT_FP_REGS+(PT_R27*8))(r31) @@ -362,12 +365,13 @@ L(has_no_vec): lfd fp31,(SIGCONTEXT_FP_REGS+(PT_R31*8))(r31) lfd fp30,(SIGCONTEXT_FP_REGS+(PT_R30*8))(r31) + .machine push + .machine "power6" + # ifdef _ARCH_PWR6 /* Use the extended four-operand version of the mtfsf insn. */ mtfsf 0xff,fp0,1,0 # else - .machine push - .machine "power6" /* Availability of DFP indicates a 64-bit FPSCR. */ andi. r6,r5,PPC_FEATURE_HAS_DFP beq 7f @@ -378,8 +382,10 @@ L(has_no_vec): 7: mtfsf 0xff,fp0 8: - .machine pop # endif /* _ARCH_PWR6 */ + + .machine pop + lfd fp29,(SIGCONTEXT_FP_REGS+(PT_R29*8))(r31) lfd fp28,(SIGCONTEXT_FP_REGS+(PT_R28*8))(r31) lfd fp27,(SIGCONTEXT_FP_REGS+(PT_R27*8))(r31) diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S index bc02a21..b25904d 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S @@ -173,6 +173,10 @@ ENTRY(__novec_swapcontext) lfd fp0,(SIGCONTEXT_FP_REGS+(32*8))(r31) lfd fp31,(SIGCONTEXT_FP_REGS+(PT_R31*8))(r31) lfd fp30,(SIGCONTEXT_FP_REGS+(PT_R30*8))(r31) + + .machine push + .machine "power6" + # ifdef _ARCH_PWR6 /* Use the extended four-operand version of the mtfsf insn. */ mtfsf 0xff,fp0,1,0 @@ -189,8 +193,10 @@ ENTRY(__novec_swapcontext) 5: mtfsf 0xff,fp0 6: - .machine pop #endif /* _ARCH_PWR6 */ + + .machine pop + lfd fp29,(SIGCONTEXT_FP_REGS+(PT_R29*8))(r31) lfd fp28,(SIGCONTEXT_FP_REGS+(PT_R28*8))(r31) lfd fp27,(SIGCONTEXT_FP_REGS+(PT_R27*8))(r31) @@ -652,12 +658,14 @@ L(has_no_vec2): lfd fp0,(SIGCONTEXT_FP_REGS+(32*8))(r31) lfd fp31,(SIGCONTEXT_FP_REGS+(PT_R31*8))(r31) lfd fp30,(SIGCONTEXT_FP_REGS+(PT_R30*8))(r31) + + .machine push + .machine "power6" + # ifdef _ARCH_PWR6 /* Use the extended four-operand version of the mtfsf insn. */ mtfsf 0xff,fp0,1,0 # else - .machine push - .machine "power6" /* Availability of DFP indicates a 64-bit FPSCR. */ andi. r6,r8,PPC_FEATURE_HAS_DFP beq 7f @@ -668,8 +676,10 @@ L(has_no_vec2): 7: mtfsf 0xff,fp0 8: - .machine pop #endif /* _ARCH_PWR6 */ + + .machine pop + lfd fp29,(SIGCONTEXT_FP_REGS+(PT_R29*8))(r31) lfd fp28,(SIGCONTEXT_FP_REGS+(PT_R28*8))(r31) lfd fp27,(SIGCONTEXT_FP_REGS+(PT_R27*8))(r31)