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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081019/a0a279d2/attachment.pgp>