Build failure on Mac OS Lion in libtheora

RESOLVED FIXED in mozilla10

Status

()

Core
Audio/Video
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: fabrice, Assigned: rillian)

Tracking

unspecified
mozilla10
x86_64
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 570085 [details]
build log

Some assembly fails.
(Assignee)

Comment 1

6 years ago
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?
(Assignee)

Comment 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
It does compile with clang.

I've filed Bug 697881 for the missing gcc-4.2 issue.
Depends on: 697881
(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).
(Assignee)

Comment 5

6 years ago
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?
(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.
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!
(Assignee)

Comment 8

6 years ago
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.
Assignee: nobody → giles
(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?
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.
Created attachment 572365 [details] [diff] [review]
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.
Attachment #572365 - Flags: review?(tterribe)
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.
Attachment #572365 - Flags: review?(tterribe) → review+
Created attachment 572369 [details] [diff] [review]
proposed fix v2

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

Revised patch attached.
Attachment #572365 - Attachment is obsolete: true
Attachment #572369 - Flags: review?(khuey)
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.
Attachment #572369 - Flags: review?(khuey) → review+
Created attachment 572560 [details] [diff] [review]
proposed fix v3

Thanks. All addressed, hopefully.
Attachment #572369 - Attachment is obsolete: true
Attachment #572560 - Flags: review?(khuey)
Attachment #572560 - Flags: review?(khuey) → review+
Thanks!
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=af822bbbb24a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d7a275acaa
Keywords: checkin-needed
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/c6d7a275acaa
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.