This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] fix BZ 18116 - build failure on ppc64le: setcontext.S uses power6 mtfsf when not supported

On 03/16/2015 06:58 PM, Steven Munroe wrote:

On Mon, 2015-03-16 at 17:25 -0600, Martin Sebor wrote:

On 03/16/2015 03:18 PM, Martin Sebor wrote:

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.


Attached is a patch that does this.  It moves the .machine
directive from the "false" branch of the block controlled
by the _ARCH_PWR6 macro to the "true" branch, and replaces
the four operand mtfsf instruction in the "false" branch
with its opcode.

This way when _ARCH_PWR6 is defined by GCC we can safely
assume the assembler supports the four-operand form of
the instruction and so we just put it in the right machine
mode in case GCC didn't specify the appropriate -power6
or better option.  When the macro isn't defined we assume
that the assembler doesn't support the four operand form
and so we hardcode its opcode.  The preceding runtime test
will jump over it if it isn't supported by the cpu.



It simpler and more correct to place

    .machine push
    .machine "power6"
+
+  mtfsf  0xff,fp0,1,0
+
+  .machine pop

around all instances of the 4 operand form of mtfsf.


You mean around each of the instances, as in the attached patch,
as opposed to around the whole #if/#else block?  Would you mind
explaining in what way you think it's more correct than the first
patch (a single pair of .machine directives around the whole
conditional block)?



This also avoid the used of .long hex constants for the
PPC_FEATURE_HAS_DFP true case in the #else.

Also I don't see the need to resort to #if __BIG_ENDIAN__ tests as this
is not architectural issue, but an artifact of GCC configuration rules.


The endian tests are necessary in this version of the patch
because the big-endian encoding of the mtfsf instruction is
different from its powerpc64le encoding.  If we assume the
assembler supports the four operand form of the instruction
even if the _ARCH_PWR6 macro isn't defined then we don't need
to hardocode its opcode and thus don't need the __BIG_ENDIAN__
preprocessor tests.



What I suggest would be correct for the current situation and more
likely to remain correctly function for the future. This is important
give the flexibility given to implementations in the PowerISA for
embedded implementations (where DFP is optional) as exemplified by PA
Semi and FreeScale 64-bit implementations.


I'm not attached to any particular implementation of the fix
although I do think the first one is simpler and thus easier
to maintain, and the second is more robust because it doesn't
assume support for the four-operand form of the instruction,
either in the assembler or in the cpu.  But it's obviously your
call as the powerpc maintainer.

Martin

diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S
index e47a57a..8a08dc4 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S
@@ -81,22 +81,31 @@ ENTRY(__novec_setcontext)
 
 # ifdef _ARCH_PWR6
   /* Use the extended four-operand version of the mtfsf insn.  */
-  mtfsf  0xff,fp0,1,0
-# else
   .machine push
   .machine "power6"
+
+  mtfsf  0xff,fp0,1,0
+
+  .machine pop
+# else
   /* Availability of DFP indicates a 64-bit FPSCR.  */
   andi.  r6,r5,PPC_FEATURE_HAS_DFP
   beq    5f
   /* Use the extended four-operand version of the mtfsf insn.  */
+  .machine push
+  .machine "power6"
+
   mtfsf  0xff,fp0,1,0
+
+  .machine pop
+
   b      6f
   /* Continue to operate on the FPSCR as if it were 32-bits.  */
 5:
   mtfsf  0xff,fp0
 6:
-  .machine pop
 # endif /* _ARCH_PWR6 */
+
   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)
@@ -364,22 +373,31 @@ L(has_no_vec):
 
 # ifdef _ARCH_PWR6
   /* Use the extended four-operand version of the mtfsf insn.  */
-  mtfsf  0xff,fp0,1,0
-# else
   .machine push
   .machine "power6"
+
+  mtfsf  0xff,fp0,1,0
+
+  .machine pop
+# else
   /* Availability of DFP indicates a 64-bit FPSCR.  */
   andi.  r6,r5,PPC_FEATURE_HAS_DFP
   beq    7f
   /* Use the extended four-operand version of the mtfsf insn.  */
+  .machine push
+  .machine "power6"
+
   mtfsf  0xff,fp0,1,0
+
+  .machine pop
+
   b      8f
   /* Continue to operate on the FPSCR as if it were 32-bits.  */
 7:
   mtfsf  0xff,fp0
 8:
-  .machine pop
 # endif /* _ARCH_PWR6 */
+
   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..2421ca4 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S
@@ -173,24 +173,34 @@ 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)
+
 # ifdef _ARCH_PWR6
   /* Use the extended four-operand version of the mtfsf insn.  */
-  mtfsf  0xff,fp0,1,0
-# else
   .machine push
   .machine "power6"
+
+  mtfsf  0xff,fp0,1,0
+
+  .machine pop
+# else
   /* Availability of DFP indicates a 64-bit FPSCR.  */
   andi.  r6,r8,PPC_FEATURE_HAS_DFP
   beq    5f
-  /* Use the extended four-operand version of the mtfsf insn.  */
+
+  .machine push
+  .machine "power6"
+
   mtfsf  0xff,fp0,1,0
+
+  .machine pop
+
   b      6f
   /* Continue to operate on the FPSCR as if it were 32-bits.  */
 5:
   mtfsf  0xff,fp0
 6:
-  .machine pop
 #endif /* _ARCH_PWR6 */
+
   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,24 +662,34 @@ 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)
+
 # ifdef _ARCH_PWR6
   /* Use the extended four-operand version of the mtfsf insn.  */
-  mtfsf  0xff,fp0,1,0
-# else
   .machine push
   .machine "power6"
+
+  mtfsf  0xff,fp0,1,0
+
+  .machine pop
+# else
   /* Availability of DFP indicates a 64-bit FPSCR.  */
   andi.  r6,r8,PPC_FEATURE_HAS_DFP
   beq    7f
-  /* Use the extended four-operand version of the mtfsf insn.  */
+
+  .machine push
+  .machine "power6"
+
   mtfsf  0xff,fp0,1,0
+
+  .machine pop
+
   b      8f
   /* Continue to operate on the FPSCR as if it were 32-bits.  */
 7:
   mtfsf  0xff,fp0
 8:
-  .machine pop
 #endif /* _ARCH_PWR6 */
+
   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)
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]