[FFmpeg-devel] [PATCH] ac3_parser type punning fix

[David DeHaven]

On Oct 19, 2008, at 8:05 AM, Michael Niedermayer wrote:

> On Sun, Oct 19, 2008 at 11:26:29AM +0100, M?ns Rullg?rd wrote:
>> Michael Niedermayer <michaelni at gmx.at> writes:
>>
>>> On Sat, Oct 18, 2008 at 10:44:16PM -0700, David DeHaven wrote:
>>>>
>>>>> Nothing major, I just applied the same fix that was applied to
>>>>> aac_parser.c recently...
>>>>>
>>>>> -DrD-
>>>>> <ac3_parser.patch>
>>>>
>>>> I think there was confusion about the whole point of this patch...
>>>>
>>>> On the four x86 based machine/OS combos I built and tested on  
>>>> (i686-pc-
>>>> mingw32, 2x i686-pc-linux, i686-apple-darwin), gcc with  
>>>> optimizations
>>>> at more than -O0 would produce machine code that would *only*  
>>>> byteswap
>>>> 32 of the 64 bits in the "state" variable passed to both aac_sync  
>>>> and
>>>> ac3_sync leading to complete and utter misinterpretation of the  
>>>> header
>>>> information. The result: at the *least* would be that it reported  
>>>> the
>>>> wrong information to stdout, at worst it would crash due to  
>>>> having the
>>>> wrong codec parameters. I can't honestly believe that noone else  
>>>> has
>>>> seen this problem, especially considering aac_sync has been already
>>>> fixed unless that was simply a "compiler warning" fix.
>>>>
>>>> I can provide disassembly snippets if you need further  
>>>> convincing...
>>>
>>> I have no doubt that gcc generates wrong code i also have no doubt  
>>> that
>>> the code breaks strict aliassing rules.
>>> The doubt i have is the relation, so to convince me first tell us  
>>> if the
>>> code works with all identical but -fno-strict-aliasing added. If not
>>> the bug is elsewhere, and this patch is hiding it.
>>
>> The C code is the same as in aac_parser, and I confirmed it was  
>> broken
>> myself.  IIRC, it is only gcc 4.3 that optimises it "incorrectly".
>> Scare quotes because the C code is what is incorrect, breaking strict
>> aliasing rules.  You accepted the same patch for the aac parser
>> without questions, so why the hesitation now?
>
> several reasons,
> 1st i tried with and without -fno-strict-aliasing
> and i get:
> -00000480 <ac3_sync>:
> +000004a0 <ac3_sync>:
> :      53                      push   %ebx
> :      83 ec 58                sub    $0x58,%esp
> :      8b 44 24 60             mov    0x60(%esp),%eax
> @@ -397,12 +404,11 @@
> :      c7 44 24 48 00 00 00    movl   $0x0,0x48(%esp)
> :      00
> :      89 04 24                mov    %eax,(%esp)
> -:      e8 fc ff ff ff          call   4cc <ac3_sync+0x4c>
> +:      e8 fc ff ff ff          call   4ec <ac3_sync+0x4c>
> :      31 d2                   xor    %edx,%edx
> :      85 c0                   test   %eax,%eax
> -:      78 43                   js     519 <ac3_sync+0x99>
> +:      78 43                   js     539 <ac3_sync+0x99>
> :      0f b7 44 24 36          movzwl 0x36(%esp),%eax
> -:      8b 54 24 6c             mov    0x6c(%esp),%edx
> :      89 43 38                mov    %eax,0x38(%ebx)
> :      8b 44 24 38             mov    0x38(%esp),%eax
> :      89 43 3c                mov    %eax,0x3c(%ebx)
> @@ -410,6 +416,7 @@
> :      c7 43 40 00 06 00 00    movl   $0x600,0x40(%ebx)
> :      89 43 34                mov    %eax,0x34(%ebx)
> :      31 c0                   xor    %eax,%eax
> +:      8b 54 24 6c             mov    0x6c(%esp),%edx
> :      80 7c 24 1c 02          cmpb   $0x2,0x1c(%esp)
> :      0f 95 c0                setne  %al
> :      89 02                   mov    %eax,(%edx)
> @@ -424,7 +431,7 @@
> :      5b                      pop    %ebx
> :      c3                      ret
>
> ------------
> now i didnt confirm that the asm was wrong, so i asked if david sees
> -fno-strict-aliasing fixing the issue he has ...
> (and above is with gcc 4.3.1)
>
> The second reason is
> #define AV_RN64(a) (*((const uint64_t*)(a)))
> #define AV_WN64(a, b) *((uint64_t*)(a)) = (b)
> ...
> #  define AV_RB64(x)    bswap_64(AV_RN64(x))
> #  define AV_WB64(p, d) AV_WN64(p, bswap_64(d))
>
> The third reason is that i have doubts this really is a strict  
> aliasing issue
> aliasing does not apply to intXY <-> int8 it just applies to things  
> not
> involving int8. Now the truth is we do mix 64 write and 32 bit read  
> with a
> int8 pointer thus our code is wrong with strict aliasing but for gcc  
> to mess
> up it has to proof that the int8 pointer written is not read as int8  
> or int64.
> Iam surprissed it does this but at the same time does then only  
> ommit half
> of the int64 code. It really could ommit all if it thought its never  
> read.
> Also gcc would have to analyze some non static and not inlined  
> functions to
> pull this trick off.
> Thats all assuming i do not miss something ...

I don't think it's necessarily a problem with strict aliasing, it's  
likely a problem (possibly a bug?) in the gcc optimizer. The problem  
also disappears if I do something with "state" argument prior to  
creating the GetBitContext, like printing the contents to stdout,  
which it why I had to start digging into assembly... I didn't dig too  
deeply as I saw the fix in aac_parser and the problem went away there,  
so I figured we could just apply it here too.

Actually, I was wrong on one count, it works fine on gcc 4.0.1 for  
i686-apple-darwin (XCode 3.0)... it was only broken on Linux and MinGW  
builds (using gcc 4.2.4, 4.3.0 and 4.3.1)

-DrD-