Closed
Bug 982693
Opened 9 years ago
Closed 9 years ago
Cannot build with clang trunk (libvpx vp8_asm_enc_offsets.c)
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: rafael.espindola, Assigned: gaston)
References
(Blocks 1 open bug)
Details
(Whiteboard: [bugday-20140317])
Attachments
(2 files)
2.24 KB,
patch
|
derf
:
review+
|
Details | Diff | Splinter Review |
1012 bytes,
patch
|
gps
:
review+
gaston
:
feedback+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.146 Safari/537.36 Steps to reproduce: Clang will parse inline assembly, so the vp8_asm_enc_offsets.c hack doesn't work. The upstream bug is https://code.google.com/p/webm/issues/detail?id=729&thanks=729&ts=1394640687. With an additional check for -fno-integrated-as being support, something like diff --git a/media/libvpx/Makefile.in b/media/libvpx/Makefile.in index b224881..d9e2999 100644 --- a/media/libvpx/Makefile.in +++ b/media/libvpx/Makefile.in @@ -75,7 +75,7 @@ vp8_asm_enc_offsets.s: CFLAGS := -DINLINE_ASM vp8_asm_enc_offsets.s: $(srcdir)/vp8/encoder/vp8_asm_enc_offsets.c $(REPORT_BUILD) - $(CC) -S $(COMPILE_CFLAGS) $(TARGET_LOCAL_INCLUDES) $(_VPATH_SRCS) + $(CC) -S $(COMPILE_CFLAGS) $(TARGET_LOCAL_INCLUDES) $(_VPATH_SRCS) -fno-integrated-as vp8_asm_enc_offsets.asm: vp8_asm_enc_offsets.s grep $(OFFSET_PATTERN) $< | sed -e 's/[$$\#]//g' \ should work.
![]() |
||
Comment 1•9 years ago
|
||
bug 772112?
Component: Untriaged → Video/Audio
Product: Firefox → Core
Summary: Cannot build with clang trunk → Cannot build with clang trunk (libvpx vp8_asm_enc_offsets.c)
Whiteboard: [bugday-20140317]
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to [:Aleksej] from comment #1) > bug 772112? Close, but no. That one is about building with LTO. This one fails without lto.
Comment 3•9 years ago
|
||
My personal LLVM/Clang SVN HEAD + m-c builder has been failing since at least Feb 15. http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/ http://jenkins.gregoryszorc.com:9000/job/mozilla-central-linux-x64-optimized-llvm-tip/874/console /home/jenkins-slave/workspace/mozilla-central-linux-x64-optimized-llvm-tip/media/libvpx/vp8/encoder/vp8_asm_enc_offsets.c:23:1: error: unexpected token in argument list DEFINE(vp8_block_coeff, offsetof(BLOCK, coeff)); ^ /home/jenkins-slave/workspace/mozilla-central-linux-x64-optimized-llvm-tip/media/libvpx/vpx_ports/asm_offsets.h:22:35: note: expanded from macro 'DEFINE' #define DEFINE(sym, val) asm("\n" #sym " EQU %0" : : "i" (val)) ^ <scratch space>:103:2: note: expanded from here "vp8_block_coeff" ^ <inline asm>:2:21: note: instantiated into assembly here vp8_block_coeff EQU $8 ^ /home/jenkins-slave/workspace/mozilla-central-linux-x64-optimized-llvm-tip/media/libvpx/vp8/encoder/vp8_asm_enc_offsets.c:24:1: error: unexpected token in argument list DEFINE(vp8_block_zbin, offsetof(BLOCK, zbin)); ^ /home/jenkins-slave/workspace/mozilla-central-linux-x64-optimized-llvm-tip/media/libvpx/vpx_ports/asm_offsets.h:22:35: note: expanded from macro 'DEFINE' #define DEFINE(sym, val) asm("\n" #sym " EQU %0" : : "i" (val)) ^ <scratch space>:104:2: note: expanded from here "vp8_block_zbin" ^ <inline asm>:2:20: note: instantiated into assembly here vp8_block_zbin EQU $40 ^
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•9 years ago
|
||
The correct place to add the test for -fno-integrated-as is probably in the inline-asm whitelist around line 5431 of configure.in: if test -z "$GNU_CC" -a -z "$INTEL_CC" -a -z "$CLANG_CC" ; then dnl We prefer to get asm offsets using inline assembler, which the above The other option is just to remove CLANG_CC from that whitelist, but that means you can't cross-compile using clang unless you have the appropriate headers for the target system's object file format available to programs compiled for the host system. But given that this used to work fine, despite clang parsing inline asm for years, I think the _actual_ upstream bug should be a clang bug, not a libvpx bug.
Comment 5•9 years ago
|
||
(FWIW, the Ubuntu 14.04 final beta was released this last week. I ran into this bug when trying to build with the "clang-3.5" package that ships in that release.)
Comment 6•9 years ago
|
||
I was able to work around this bug (using clang 3.5) by doing the following: a) apt-get install libvpx-dev b) Add "ac_add_options --with-system-libvpx" to your .mozconfig c) Apply attachment 8391668 [details] [diff] [review] (workaround for issue w/ system-libvpx, bug 983953)
Assignee | ||
Comment 7•9 years ago
|
||
Hitting this bug too on OpenBSD, w/ clang/llvm-3.5.20140228.
Assignee | ||
Comment 8•9 years ago
|
||
just to be more precise: i see the same failure as gps in comment 3 - and using --with-system-libvpx is not an option since we only have 1.2, and an update to 1.3 is not in the pipe yet... so either a fix in bundled libvpx build machinery, or in clang/llvm would be welcome.
Assignee | ||
Comment 9•9 years ago
|
||
adding -fno-integrated-as as said in comment 0 workarounds the issue, but i'm not sure this is a proper fix.
Assignee | ||
Comment 10•9 years ago
|
||
This is annoying since i need to patch all my builders that use clang - anyone having an idea on how to properly fix that ?
Reporter | ||
Comment 11•9 years ago
|
||
(In reply to Landry Breuil (:gaston) from comment #10) > This is annoying since i need to patch all my builders that use clang - > anyone having an idea on how to properly fix that ? Two options: * Use -fno-integrated-as in that file * Make it produce valid assembly. For example, by producing value_we_want_to_find = expr
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Rafael Ávila de Espíndola from comment #11) > (In reply to Landry Breuil (:gaston) from comment #10) > > This is annoying since i need to patch all my builders that use clang - > > anyone having an idea on how to properly fix that ? > > Two options: > * Use -fno-integrated-as in that file Yes, that's the workaround i'm using now, but i doubt it's a candidate for the tree. > * Make it produce valid assembly. For example, by producing > value_we_want_to_find = expr What do you mean ? And would it be suitable for inclusion in our version ?
Reporter | ||
Comment 13•9 years ago
|
||
> > * Make it produce valid assembly. For example, by producing
> > value_we_want_to_find = expr
>
> What do you mean ? And would it be suitable for inclusion in our version ?
The build failure comes from the fact that clang is insisting that inline assembly is just that: assembly.
The code is expanding to something like
#bar EQU integer
the fix might be as simple as making it expand to something that is valid assembly, like
bar = integer
and changing the custom asm parser to search for '=' instead of EQU
Assignee | ||
Comment 14•9 years ago
|
||
I see this in lots of places actually: ./build/make/ads2gas.pl -> is that the custom asm parser ? ./vpx_ports/asm_offsets.h:#define DEFINE(sym, val) asm("\n" #sym " EQU %0" : : "i" (val)) -> that's the one crashing apparently ? ./build/make/obj_int_extract.c: printf("%-40s EQU %5d\n", name, val); ./build/make/obj_int_extract.c: printf("%-40s EQU %5d\n", ./build/make/obj_int_extract.c: printf("%-40s EQU %5d\n", ./build/make/obj_int_extract.c: printf("%-40s EQU ", name + 1); ./build/make/obj_int_extract.c: printf("%-40s EQU ", name); ./build/make/obj_int_extract.c: printf("%-40s EQU ", ./build/make/obj_int_extract.c: printf("%-40s EQU ", buf + strtab_ptr + get_le32(ptr + 4)); what's that part ? the arm/neon asm also has a bunch of ./vp9/common/arm/neon/vp9_short_idct32x32_add_neon.asm:cospi_1_64 EQU 16364 ./vp9/common/arm/neon/vp9_short_idct32x32_add_neon.asm:cospi_2_64 EQU 16305
Comment 15•9 years ago
|
||
I would really appreciate a fix for this, since older versions of clang crash when I try to produce ASan Firefox on Mac OS X 10.9.
![]() |
||
Comment 16•9 years ago
|
||
roc, can we take a patch in our tree while we wait for upstream to fix this?
Flags: needinfo?(roc)
I think so, but this is more Tim's call?
Flags: needinfo?(roc) → needinfo?(tterribe)
Assignee | ||
Comment 19•9 years ago
|
||
Problem with adding -fno-integrated-as blindly is that it is a clang-only option.. is there something in the build system we can use to add flags depending on the compiler ?
Comment 20•9 years ago
|
||
See comment 4. https://hg.mozilla.org/mozilla-central/file/349a2f003529/configure.in#l5463
Assignee | ||
Comment 21•9 years ago
|
||
Oh right, missed that.. so should we remove clang from the whitelist instead ? Trying it locally..
Comment 22•9 years ago
|
||
Well, either approach should fix the short-term pain. But removing clang from the whitelist breaks cross-compiling to a platform with a different object file format using clang (which is why we prefer the inline-asm approach when we can make it work). I just meant the logic should probably be centralized so that the tests are consistent.
Assignee | ||
Comment 23•9 years ago
|
||
anyway, the build still fails with clang if i remove -a -z $CLANG_CC from the configure line..
Comment 24•9 years ago
|
||
I think that's because GNU_CC is still defined for clang.
Comment 25•9 years ago
|
||
Old versions of Clang don't recognize "-fno-integrated-as", so configure might have to check whether the specific version of the compiler has the option.
Comment 26•9 years ago
|
||
I guess this is a blocker for bug 1026162 because that bug might need a Clang upgrade to recent trunk. Also blocking on the ASan maintenance bug in general since some people depend on Clang trunk for that.
Blocks: asan-macbuilds, asan-maintenance
Assignee | ||
Comment 27•9 years ago
|
||
So, how about this ? Seems to do the right thing with clang (supporting the option) and g++ 4.8 (not supporting it)
Assignee: nobody → landry
Attachment #8454312 -
Flags: review?(tterribe)
Attachment #8454312 -
Flags: feedback?(rafael.espindola)
Updated•9 years ago
|
Attachment #8454312 -
Flags: review?(tterribe) → review+
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8454312 [details] [diff] [review] libvpx-as https://tbpl.mozilla.org/?tree=Try&rev=9b9e409f5155
Assignee | ||
Comment 29•9 years ago
|
||
Interesting, arm/b2g/the whole world is burning... probably because i sent it the WRONG patch. sry.
Assignee | ||
Comment 30•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e6f6a0bdee0b
Assignee | ||
Comment 31•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1eae42835bb
Comment 32•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f1eae42835bb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 33•9 years ago
|
||
With the fix in MC and clang checked out on 20140708 on MacOSX the try compile results in this: configure:25731: checking whether C compiler supports -fno-integrated-as configure:25740: /Users/nohlmeier/checkouts/llvm-build-20140708/bin/clang -c -fsanitize=address -Dxmalloc=myxmalloc -std=gnu99 -fno-strict-aliasing -fno-math-errno -pthread -DNO_X11 -pipe -fno-integrated-as -Qunused-arguments conftest.c 1>&5 /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:2:Unknown pseudo-op: .macosx_version_min /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:2:Rest of line ignored. 1st junk character valued 49 (1). /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:6:Unknown pseudo-op: .cfi_startproc /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:10:Unknown pseudo-op: .cfi_def_cfa_offset /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:10:Rest of line ignored. 1st junk character valued 49 (1). /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:12:Unknown pseudo-op: .cfi_offset /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:12:Rest of line ignored. 1st junk character valued 37 (%). /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:15:Unknown pseudo-op: .cfi_def_cfa_register /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:15:Rest of line ignored. 1st junk character valued 37 (%). /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:103:Unknown pseudo-op: .cfi_endproc /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:107:Unknown pseudo-op: .cfi_startproc /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:111:Unknown pseudo-op: .cfi_def_cfa_offset /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:111:Rest of line ignored. 1st junk character valued 49 (1). /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:113:Unknown pseudo-op: .cfi_offset /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:113:Rest of line ignored. 1st junk character valued 37 (%). /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:116:Unknown pseudo-op: .cfi_def_cfa_register /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:116:Rest of line ignored. 1st junk character valued 37 (%). /var/folders/3g/bm146dzn2zs42vgkw80svyx80000gn/T/conftest-7a8708.s:120:Unknown pseudo-op: .cfi_endproc clang-3.5: error: assembler command failed with exit code 1 (use -v to see invocation) configure: failed program was: #line 25733 "configure" #include "confdefs.h" int main() { return 0; ; return 0; } Obviously then the build fails. Has anyone an idea what could be wrong here?
Comment 34•9 years ago
|
||
With clang from the same day it works for me on Linux. So it seems to be a Mac specific issue. I'm encountering the trouble on Mavericks 10.9.4.
Comment 35•9 years ago
|
||
I hit the same problem as Nils on Mac 10.9. The try compile fails (for the wrong reason), so it doesn't pass "-fno-integrated-as" while compiling Firefox, so it fails in vp8 just like before. In fact, even just > /Users/jruderman/llvm//build/Release/bin/clang -fno-integrated-as ~/Desktop/confin.c fails with > confin-d546c5.s:2:Unknown pseudo-op: .macosx_version_min
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 36•9 years ago
|
||
This seems to work. Is it the right thing to do? https://lists.linuxfoundation.org/pipermail/llvmlinux/2014-February/000892.html
Attachment #8458191 -
Flags: review?(tterribe)
Attachment #8458191 -
Flags: feedback?(rafael.espindola)
Updated•9 years ago
|
Attachment #8458191 -
Flags: feedback?(gps)
Comment 37•9 years ago
|
||
Comment on attachment 8458191 [details] [diff] [review] In the configure check, also pass in -S Landry, does this seem like a reasonable addition to your patch? I'm not sure why it worked without -S for you but not for me.
Attachment #8458191 -
Flags: feedback?(landry)
Comment 38•9 years ago
|
||
Comment on attachment 8458191 [details] [diff] [review] In the configure check, also pass in -S Review of attachment 8458191 [details] [diff] [review]: ----------------------------------------------------------------- Adding -S to the configure test should be sane. This just changes the tested compilation mode from "produce an object file" to "produce an assembly file." I don't see how this could impact the check. I'd add a comment that says Clang insists of using -S with -fno-integrated-as. I don't know enough to know if this is a compiler bug or what. But this seems like a sufficient workaround regardless.
Attachment #8458191 -
Flags: review?(tterribe)
Attachment #8458191 -
Flags: review+
Attachment #8458191 -
Flags: feedback?(gps)
Comment 40•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/451581502090
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•9 years ago
|
Attachment #8454312 -
Flags: feedback?(rafael.espindola)
Reporter | ||
Updated•9 years ago
|
Attachment #8458191 -
Flags: feedback?(rafael.espindola)
Assignee | ||
Comment 41•9 years ago
|
||
Comment on attachment 8458191 [details] [diff] [review] In the configure check, also pass in -S Reading gps' coment on clang, that looks sane to me (even if this was already pushed :). And after testing with the clang i have here, it also works with -S.
Attachment #8458191 -
Flags: feedback?(landry) → feedback+
Assignee | ||
Comment 42•9 years ago
|
||
Note that i'd also have added a comment explaining why -S is needed, as it wasnt the case for linux/bsd, and the fact that it seems to be specific to macosx.... the commit message seem to imply that this was broken in all cases :)
You need to log in
before you can comment on or make changes to this bug.
Description
•