[FFmpeg-devel] [PATCH] remove gcc 3.3 workaround in swscale_template.c

[Diego Biurrun]

On Tue, Sep 22, 2009 at 04:23:44PM +0200, Benoit Fouet wrote:
> On 2009-09-22 16:10, Diego Biurrun wrote:
> > On Tue, Sep 22, 2009 at 03:53:19PM +0200, Benoit Fouet wrote:
> >> On 2009-09-21 00:16, Diego Biurrun wrote:
> >>> On Sun, Sep 20, 2009 at 09:02:46PM +0200, Reimar D?ffinger wrote:
> >>>> On Sun, Sep 20, 2009 at 08:00:40PM +0200, Diego Biurrun wrote:
> >>>>> On Sun, Sep 20, 2009 at 07:12:44PM +0200, Reimar D?ffinger wrote:
> >>>>>> On Sun, Sep 20, 2009 at 06:53:36PM +0200, Diego Biurrun wrote:
> >>>>>>> There's an ugly preprocessor gcc 3.3 workaround in swscale_template.c:
> >>>>>>>
> >>>>>>>   /* GCC 3.3 makes MPlayer crash on IA-32 machines when using "g" operand here,
> >>>>>>>      which is needed to support GCC 4.0. */
> >>>>>>>   #if ARCH_X86_64 && ((__GNUC__ > 3) || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4))
> >>>>>>>                   :: "m" (src1), "m" (dst), "g" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask),
> >>>>>>>   #else
> >>>>>>>                   :: "m" (src1), "m" (dst), "m" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask),
> >>>>>>>   #endif
> >>>>>>>
> >>>>>>> At the very least it should be updated to use the AV_GCC_VERSION_AT_LEAST
> >>>>>>> macro from libavutil.  However, I would prefer to get rid of it
> >>>>>>> completely.  If I understand the comment correctly, deleting the whole
> >>>>>>> #else clause would be the way to achieve this.  Since I know little
> >>>>>>> enough assembler to have no real idea what "g" and "m" operands are all
> >>>>>>> about I'd like to hear an informed opinion.
> >>>>>> Huh? No, the upper line should give faster code but causes crashes due
> >>>>>> to miscompilation with gcc 3.3
> >>>>>> The lower line does not compile with gcc 4.0 on x86_64 but produces correct code
> >>>>>> with 3.3.
> >>>>>> At least that's what the comment says.
> >>>>> OK, then I propose getting rid of the workaround completely.  I don't
> >>>>> think x86_64 users with gcc 3.3 exist, so it's not worth the ugliness on
> >>>>> our side.
> >>>> Hm? Which do you consider a workaround? I think you might still need two cases,
> >>>> one for x86_32 and one for x86_64 for gcc 4.x, you could only get rid of the
> >>>> gcc version check.
> >>> OK, I propose the attached patch.
> >> which results in code as ugly, but with no support for gcc < 3.3 on x86_64
> > 
> > Excuse me, but how can 
> > 
> >   #if ARCH_X86_64
> > 
> > be as ugly as
> > 
> >   #if ARCH_X86_64 && ((__GNUC__ > 3) || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4))
> > 
> > ?
> > 
> > You must be deeply confused.
> 
> why, no, but thank you for feeling concerned...
> it's an ifdeffery anyway; whether it is 15 or 77 characters long, I
> don't care...

Not caring about the complexity of a statement makes you a very weird
programmer indeed.

> >> I'd say, if you really want to change that code, change it to use
> >> AV_GCC_VERSION_AT_LEAST, and that should do, no ?
> > 
> > No, that only works for gcc, which sucks.
> 
> I don't understand you; isn't the following code working ?
> #ifdef ARCH_X86_64 && AV_GCC_VERSION_AT_LEAST(3, 4)

I repeat: It only works for gcc.  Possibly for other compilers
pretending to be gcc.

> >  Also, we don't know if the
> > problem exists for gcc 3.3 only or for gcc <3.3 as well..
> > 
> > Note that this is the last occurrence of __GNUC__ outside of appropriate
> > places.
> 
> which can be fixed by my previous remark, unless I'm missing something.

You are missing that your suggestion is just as unportable as what we
have now.

I just committed the AV_GCC_VERSION_AT_LEAST change.  That solves the
code duplication problem.  What I still want to fix is the portability
issue.

Here is an updated patch.

Diego
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gnuc_x86_64.diff
Type: text/x-diff
Size: 738 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090922/ada23e99/attachment.diff>