697821 - Build failure on Mac OS Lion in libtheora

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: build log

Some assembly fails.

Comment 1

[Ralph Giles (:rillian)]

I can't reproduce, although my bsd system is confused. Can you try:

gcc-4.2 -g -Wall -isysroot /Developer/SDKs/MacOSX10.7.sdk -c -o sse2idct.o -I media/libtheora/include/ -I media/libogg/include/ -DOC_X86_ASM -DOC_X86_64_ASM media/libtheora/lib/x86/sse2idct.c 

from the top level as a simpler command line to reproduce?

The text looks a little odd. Maybe the expansion didn't work properly?

Comment 2

[Ralph Giles (:rillian)]

Fabrice reports the above command fails for him.

I reinstalled XCode and can reproduce. (This is with XCode 4.2, I never had a problem with 4.1)

Note that our configure script uses gcc-4.2 unconditionally on darwin, and XCode 4.2 doesn't provide this alias, so I had to hack around that to build at all.

The single command line fails as well. Perhaps because gcc is really gcc-llvm?

$ gcc --version
i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.1.00)

Comment 3

[Ralph Giles (:rillian)]

It does compile with clang.

I've filed Bug 697881 for the missing gcc-4.2 issue.

Comment 4

[Timothy B. Terriberry (:derf)]

(In reply to Ralph Giles (:rillian) from comment #2)
> Note that our configure script uses gcc-4.2 unconditionally on darwin, and
> XCode 4.2 doesn't provide this alias, so I had to hack around that to build
> at all.

This sounds like bug 627981. If you have to hack things to get around configure, you may be failing to trigger the clang version check in the patch for that bug, or the check may no longer be working correctly for Xcode 4.2. Either way, the only solution is to disable inline asm for the platform until Apple pulls in a recent enough clang (they have their own internal fork, which does not correspond to any actual released clang version).

Comment 5

[Ralph Giles (:rillian)]

The clang version check is still running, and doesn't find a problem. On XCode 4.2, 'cc' and 'gcc' are still llvm-gcc:

glaucomys:firefox giles$ gcc --version
i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.1.00)
Copyright (C) 2007 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

glaucomys:firefox giles$ clang --version
Apple clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM 3.0svn)
Target: x86_64-apple-darwin11.2.0
Thread model: posix


Clang 3.0 can actually compile sse2idct.c, &c. So the fix from bug 627981 remains appropriate. Looks like this is a regression in llvm-gcc.

Your recommendation is just to disable theora asm for this gcc version?

Comment 6

[Timothy B. Terriberry (:derf)]

(In reply to Ralph Giles (:rillian) from comment #5)
> Your recommendation is just to disable theora asm for this gcc version?

I don't think we have another choice. This compiler is busted. The upstream version works correctly. Trying to hack around this would break the asm on other compilers. When Apple fixes their compiler they can have inline asm.

Comment 7

[Owen Coutts [:tallOwen]]

I seem to get a similar but slightly different error in the libtheora code using XCode 4.2. I get the same output as rillian in comment 5. For reference I had to use the following .mozconfig to get by the aliasing of gcc: 

CC="gcc"
CXX="g++"

HOST_CC="gcc"
HOST_CXX="g++"

Here is the error I'm getting:

gcc -o mmxidct.o -c -fvisibility=hidden -DTHEORA_DISABLE_ENCODE -DOC_X86_ASM -DOC_X86_64_ASM -DOSTYPE=\"Darwin11.2.0\" -DOSARCH=Darwin -I/Users/ocoutts/workspace/mozilla-central/media/libtheora/lib -I/Users/ocoutts/workspace/mozilla-central/media/libtheora/lib -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/Users/ocoutts/workspace/mozilla-central/obj-x86_64-apple-darwin11.2.0/dist/include/nspr -I/Users/ocoutts/workspace/mozilla-central/obj-x86_64-apple-darwin11.2.0/dist/include/nss      -fPIC  -Wall -W -Wno-unused -Wpointer-arith -Wdeclaration-after-statement -Wcast-align -W -isysroot /Developer/SDKs/MacOSX10.6.sdk -fno-strict-aliasing -fno-common -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fomit-frame-pointer   -include ../../../mozilla-config.h -DMOZILLA_CLIENT -MD -MF .deps/mmxidct.pp /Users/ocoutts/workspace/mozilla-central/media/libtheora/lib/x86/mmxidct.c

Let me know if I'm doing something obviously wrong!

Comment 8

[Ralph Giles (:rillian)]

Owen, that's the compiler command line. What's the slightly different error message? Note that several files from libtheora/lib/x86 fail for me under xcode 4.2's 'gcc'; the one Fabrice reported is just one of them.

Comment 9

[Owen Coutts [:tallOwen]]

(In reply to Ralph Giles (:rillian) from comment #8)
> What's the slightly different error message? 
Sorry I mispasted. The full error is here: http://pastebin.com/ZK2pd9yb

On IRC fabrice advized me to apply a patch that disables asm. Since then I've been able to successfully work around this issue.

Should MDN be updated to reflect these steps?

Comment 10

[Ralph Giles (:rillian)]

Yep, that's the same problem. Thanks for confirming.

If you do update MDN, please say so here, so we can revert it once we fix the build system to do this automatically, as derf suggested in comment #6.

Comment 11

[Ralph Giles (:rillian)]

Attachment: proposed fix

Proposed fix. Define CC_BROKEN_ASM if this particular gcc version is detected and only build libtheora with inline assembly if it's not defined. Also updates the similar pre-2.9 clang version check from bug 627981 to use the same flag.

Comment 12

[Timothy B. Terriberry (:derf)]

Comment on attachment 572365 [details] [diff] [review]
proposed fix

Review of attachment 572365 [details] [diff] [review]:
-----------------------------------------------------------------

r+ from me, but this really needs review by someone from the build team (I recommend ted or khuey).

::: configure.in
@@ +1217,5 @@
>  AC_LANG_RESTORE
> +dnl XCode 4.2 also shipped an llvm-gcc which couldn't compile
> +dnl the libtheora inline asm.
> +AC_MSG_CHECKING([bad gcc versions])
> +if test `$CC --version | grep -c "Apple Inc. build 5658"` > 0; then

Instead of using test you can just use
  if $CC --version | grep -q "..."
directly.

Comment 13

[Ralph Giles (:rillian)]

Attachment: proposed fix v2

Thanks, that's rather cleaner. Especially since it needs to be '-gt' rather than '>' to get algebraic comparison.

Revised patch attached.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 572369 [details] [diff] [review]
proposed fix v2

Review of attachment 572369 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/autoconf.mk.in
@@ +374,5 @@
>  GNU_AS		= @GNU_AS@
>  GNU_LD		= @GNU_LD@
>  GNU_CC		= @GNU_CC@
>  GNU_CXX		= @GNU_CXX@
> +CC_BROKEN_ASM   = @CC_BROKEN_ASM@

Can we rename this CC_BROKEN_LIBTHEORA_ASM or something?  Assembly isn't broken on this compilers for everything is it?

Assuming we do that, please move this substitution down to where the rest of the media stuff is.

::: configure.in
@@ +1193,5 @@
>  
>  dnl clang prior to 2.9 (including Xcode 4) does not support all the
>  dnl constructs required by the libtheora inline asm. This is used to
>  dnl detect and disable it
> +AC_MSG_CHECKING([whether the C compiler is old clang])

Specify that we're looking for clang 2.9 here.

@@ +1212,1 @@
>      AC_DEFINE(HAVE_OLD_CLANG)

This AC_DEFINE is unused, and can be removed.

@@ +1219,5 @@
> +dnl the libtheora inline asm.
> +AC_MSG_CHECKING([bad gcc versions])
> +if `$CC --version | grep -q "Apple Inc. build 5658"`; then
> +  AC_MSG_RESULT([Apple build 5658])
> +  AC_DEFINE(CC_BROKEN_ASM)

This AC_DEFINE is also unnecessary.

@@ +1224,5 @@
> +  CC_BROKEN_ASM=1
> +else
> +  AC_MSG_RESULT([we're ok])
> +fi
> +AC_SUBST(CC_BROKEN_ASM)

newlines above and below this AC_SUBST please.

@@ +1226,5 @@
> +  AC_MSG_RESULT([we're ok])
> +fi
> +AC_SUBST(CC_BROKEN_ASM)
> +if test "x$CC_BROKEN_ASM" = x1; then
> +  AC_MSG_WARN([Disabling inline assembly])

for libtheora.

Comment 15

[Ralph Giles (:rillian)]

Attachment: proposed fix v3

Thanks. All addressed, hopefully.

Comment 16

[Ralph Giles (:rillian)]

Thanks!

Comment 17

[Timothy B. Terriberry (:derf)]

https://tbpl.mozilla.org/?tree=Try&rev=af822bbbb24a

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d7a275acaa

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c6d7a275acaa