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

[Steven Munroe <munroesj at linux dot vnet dot ibm dot comcom>]

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.

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.

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. 

> Martin
> 
> >
> > 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
> >
>