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

[Martin Sebor]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: glibc-18116-power6-mtsf.patch
Type: text/x-patch
Size: 4076 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150317/bbc8326c/attachment.bin>