Closed Bug 982693 Opened 6 years ago Closed 5 years ago

Cannot build with clang trunk (libvpx vp8_asm_enc_offsets.c)

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
x86
macOS
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: rafael.espindola, Assigned: gaston)

References

(Blocks 1 open bug)

Details

(Whiteboard: [bugday-20140317])

Attachments

(2 files)

libvpx-as
2.24 KB, patch
derf
: review+
Details | Diff | Splinter Review
In the configure check, also pass in -S
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.
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]
(In reply to [:Aleksej] from comment #1)
> bug 772112?

Close, but no. That one is about building with LTO. This one fails without lto.
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
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.
(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.)
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)
Hitting this bug too on OpenBSD, w/ clang/llvm-3.5.20140228.
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.
adding -fno-integrated-as as said in comment 0 workarounds the issue, but i'm not sure this is a proper fix.
This is annoying since i need to patch all my builders that use clang - anyone having an idea on how to properly fix that ?
(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
(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 ?
> > * 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
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
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.
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)
Yes, I think that would be fine.
Flags: needinfo?(tterribe)
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 ?
Oh right, missed that.. so should we remove clang from the whitelist instead ? Trying it locally..
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.
anyway, the build still fails with clang if i remove -a -z $CLANG_CC from the configure line..
I think that's because GNU_CC is still defined for clang.
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.
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
Attached patch libvpx-as — — Splinter Review 
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)
Attachment #8454312 - Flags: review?(tterribe) → review+
Interesting, arm/b2g/the whole world is burning... probably because i sent it the WRONG patch. sry.
https://hg.mozilla.org/mozilla-central/rev/f1eae42835bb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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?
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Attachment #8458191 - Flags: feedback?(gps)
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 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)
https://hg.mozilla.org/mozilla-central/rev/451581502090
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Attachment #8454312 - Flags: feedback?(rafael.espindola)
Attachment #8458191 - Flags: feedback?(rafael.espindola)
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+
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 :)
Blocks: 1159706
You need to log in before you can comment on or make changes to this bug.