diff mbox

[FFmpeg-devel] swscale: also save ebx register when using PIE

Message ID 10c7e655-9da9-aa4c-fa59-6ed79b9750f6@googlemail.com
State New
Headers show

Commit Message

Andreas Cadhalpun Dec. 19, 2016, 10:28 p.m. UTC 
On 16.12.2016 04:08, Michael Niedermayer wrote:
> On Fri, Dec 16, 2016 at 02:36:53AM +0100, Andreas Cadhalpun wrote:
>> Otherwise the build fails when configuring with --toolchain=hardened
>> --disable-pic on i386 using gcc 4.8:
>> error: PIC register clobbered by '%ebx' in 'asm'
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libswscale/x86/hscale_fast_bilinear_simd.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/libswscale/x86/hscale_fast_bilinear_simd.c b/libswscale/x86/hscale_fast_bilinear_simd.c
>> index 2cba5f0..3f0f5f5 100644
>> --- a/libswscale/x86/hscale_fast_bilinear_simd.c
>> +++ b/libswscale/x86/hscale_fast_bilinear_simd.c
>> @@ -199,7 +199,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
>>  #if ARCH_X86_64
>>      uint64_t retsave;
>>  #else
>> -#if defined(PIC)
>> +#if defined(PIC) || defined(__PIE__)
> 
> please correct me if iam wrong
> our configure adds -DPIC to define PIC when its enabled,
> it does not add that in this case but gcc is still generating PIC code
> that doesnt seem good

gcc does not generate PIC, only PIE, which is subtly different.

What's wrong here is that this code in swscale tries to determine, whether
or not the ebx register can be used for asm, but doesn't check that correctly.
However, configure has a working check for that, the result of which can
be used here. Patch doing that is attached.

Best regards,
Andreas

Comments

Michael Niedermayer Dec. 20, 2016, 2:04 a.m. UTC | #1 
On Mon, Dec 19, 2016 at 11:28:44PM +0100, Andreas Cadhalpun wrote:
> On 16.12.2016 04:08, Michael Niedermayer wrote:
> > On Fri, Dec 16, 2016 at 02:36:53AM +0100, Andreas Cadhalpun wrote:
> >> Otherwise the build fails when configuring with --toolchain=hardened
> >> --disable-pic on i386 using gcc 4.8:
> >> error: PIC register clobbered by '%ebx' in 'asm'
> >>
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> >> ---
> >>  libswscale/x86/hscale_fast_bilinear_simd.c | 20 ++++++++++----------
> >>  1 file changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/libswscale/x86/hscale_fast_bilinear_simd.c b/libswscale/x86/hscale_fast_bilinear_simd.c
> >> index 2cba5f0..3f0f5f5 100644
> >> --- a/libswscale/x86/hscale_fast_bilinear_simd.c
> >> +++ b/libswscale/x86/hscale_fast_bilinear_simd.c
> >> @@ -199,7 +199,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
> >>  #if ARCH_X86_64
> >>      uint64_t retsave;
> >>  #else
> >> -#if defined(PIC)
> >> +#if defined(PIC) || defined(__PIE__)
> > 
> > please correct me if iam wrong
> > our configure adds -DPIC to define PIC when its enabled,
> > it does not add that in this case but gcc is still generating PIC code
> > that doesnt seem good
> 
> gcc does not generate PIC, only PIE, which is subtly different.

does all the code under PIC work with
PIE that does not have PIC set ?
the identifier seems used a bit in .asm files


> 
> What's wrong here is that this code in swscale tries to determine, whether
> or not the ebx register can be used for asm, but doesn't check that correctly.
> However, configure has a working check for that, the result of which can
> be used here. Patch doing that is attached.

i see your argument for this and it seems sound.
I hope this doesnt break anything as this logic was that way for a
really long time and worked fine and gcc inline asm can be annoying
that said, no objections to the patch 


[...]
Andreas Cadhalpun Dec. 21, 2016, 12:32 a.m. UTC | #2 
On 20.12.2016 03:04, Michael Niedermayer wrote:
> On Mon, Dec 19, 2016 at 11:28:44PM +0100, Andreas Cadhalpun wrote:
>> On 16.12.2016 04:08, Michael Niedermayer wrote:
>>> On Fri, Dec 16, 2016 at 02:36:53AM +0100, Andreas Cadhalpun wrote:
>>>> Otherwise the build fails when configuring with --toolchain=hardened
>>>> --disable-pic on i386 using gcc 4.8:
>>>> error: PIC register clobbered by '%ebx' in 'asm'
>>>>
>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>>> ---
>>>>  libswscale/x86/hscale_fast_bilinear_simd.c | 20 ++++++++++----------
>>>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/libswscale/x86/hscale_fast_bilinear_simd.c b/libswscale/x86/hscale_fast_bilinear_simd.c
>>>> index 2cba5f0..3f0f5f5 100644
>>>> --- a/libswscale/x86/hscale_fast_bilinear_simd.c
>>>> +++ b/libswscale/x86/hscale_fast_bilinear_simd.c
>>>> @@ -199,7 +199,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
>>>>  #if ARCH_X86_64
>>>>      uint64_t retsave;
>>>>  #else
>>>> -#if defined(PIC)
>>>> +#if defined(PIC) || defined(__PIE__)
>>>
>>> please correct me if iam wrong
>>> our configure adds -DPIC to define PIC when its enabled,
>>> it does not add that in this case but gcc is still generating PIC code
>>> that doesnt seem good
>>
>> gcc does not generate PIC, only PIE, which is subtly different.
> 
> does all the code under PIC work with
> PIE that does not have PIC set ?

It seems so, as the full FATE test passes.

> the identifier seems used a bit in .asm files
> 
>> What's wrong here is that this code in swscale tries to determine, whether
>> or not the ebx register can be used for asm, but doesn't check that correctly.
>> However, configure has a working check for that, the result of which can
>> be used here. Patch doing that is attached.
> 
> i see your argument for this and it seems sound.
> I hope this doesnt break anything as this logic was that way for a
> really long time and worked fine and gcc inline asm can be annoying

I think the regression potential is rather low, as current gcc does not require
saving the ebx register anymore.

> that said, no objections to the patch 

Pushed.

Best regards,
Andreas
diff mbox

Patch

From 7b5d0714482a0ec42d317fa1e9fa095180e39ccd Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
Date: Fri, 16 Dec 2016 02:29:56 +0100
Subject: [PATCH] swscale: save ebx register when it is not available

Configure checks if the ebx register can be used for asm and it has to
be saved if and only if this is not the case.
Without this the build fails when configuring with --toolchain=hardened
--disable-pic on i386 using gcc 4.8:
error: PIC register clobbered by '%ebx' in 'asm'

In that case gcc 4.8 reserves the ebx register for the GOT needed for
PIE, so it can't be used in asm directly.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libswscale/x86/hscale_fast_bilinear_simd.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/libswscale/x86/hscale_fast_bilinear_simd.c b/libswscale/x86/hscale_fast_bilinear_simd.c
index 2cba5f0a1c..60a2cbfc50 100644
--- a/libswscale/x86/hscale_fast_bilinear_simd.c
+++ b/libswscale/x86/hscale_fast_bilinear_simd.c
@@ -199,7 +199,7 @@  void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 #if ARCH_X86_64
     uint64_t retsave;
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
     uint64_t ebxsave;
 #endif
 #endif
@@ -209,7 +209,7 @@  void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
         "mov               -8(%%rsp), %%"FF_REG_a"    \n\t"
         "mov            %%"FF_REG_a", %5              \n\t"  // retsave
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
         "mov            %%"FF_REG_b", %5              \n\t"  // ebxsave
 #endif
 #endif
@@ -255,7 +255,7 @@  void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
         "mov                      %5, %%"FF_REG_a" \n\t"
         "mov            %%"FF_REG_a", -8(%%rsp)    \n\t"
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
         "mov                      %5, %%"FF_REG_b" \n\t"
 #endif
 #endif
@@ -264,12 +264,12 @@  void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 #if ARCH_X86_64
           ,"m"(retsave)
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
           ,"m" (ebxsave)
 #endif
 #endif
         : "%"FF_REG_a, "%"FF_REG_c, "%"FF_REG_d, "%"FF_REG_S, "%"FF_REG_D
-#if ARCH_X86_64 || !defined(PIC)
+#if ARCH_X86_64 || HAVE_EBX_AVAILABLE
          ,"%"FF_REG_b
 #endif
     );
@@ -289,7 +289,7 @@  void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, int16_t *dst2,
 #if ARCH_X86_64
     DECLARE_ALIGNED(8, uint64_t, retsave);
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
     DECLARE_ALIGNED(8, uint64_t, ebxsave);
 #endif
 #endif
@@ -298,7 +298,7 @@  void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, int16_t *dst2,
         "mov          -8(%%rsp), %%"FF_REG_a"    \n\t"
         "mov       %%"FF_REG_a", %7              \n\t"  // retsave
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
         "mov       %%"FF_REG_b", %7              \n\t"  // ebxsave
 #endif
 #endif
@@ -332,7 +332,7 @@  void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, int16_t *dst2,
         "mov                    %7, %%"FF_REG_a" \n\t"
         "mov          %%"FF_REG_a", -8(%%rsp)    \n\t"
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
         "mov %7, %%"FF_REG_b"    \n\t"
 #endif
 #endif
@@ -341,12 +341,12 @@  void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, int16_t *dst2,
 #if ARCH_X86_64
           ,"m"(retsave)
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
           ,"m" (ebxsave)
 #endif
 #endif
         : "%"FF_REG_a, "%"FF_REG_c, "%"FF_REG_d, "%"FF_REG_S, "%"FF_REG_D
-#if ARCH_X86_64 || !defined(PIC)
+#if ARCH_X86_64 || HAVE_EBX_AVAILABLE
          ,"%"FF_REG_b
 #endif
     );
-- 
2.11.0