[FFmpeg-devel] [PATCH] ac3_parser type punning fix
Sun Oct 19 17:05:02 CEST 2008
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?
1st i tried with and without -fno-strict-aliasing
and i get:
: 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)
: 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
Size: 189 bytes
Desc: Digital signature
More information about the ffmpeg-devel