clang-cl error: unknown tokens in libtheora's x86_vc mmxidct.c

RESOLVED FIXED in Firefox 52

Status

()

P5
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla52
All
Windows
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
clang-cl complains thusly:

 0:24.71 c:/m-c/media/libtheora/lib/x86_vc/mmxidct.c(532,5):  error: unknown token in expression
 0:24.71     OC_ROW_IDCT_10(Y,X)
 0:24.71     ^
 0:24.71 c:/m-c/media/libtheora/lib/x86_vc/mmxidct.c(417,3):  note: expanded from macro 'OC_ROW_IDCT_10'
 0:24.71   OC_IDCT_BEGIN_10(_y,_x) \
 0:24.71   ^
 0:24.71 c:/m-c/media/libtheora/lib/x86_vc/mmxidct.c(412,13):  note: expanded from macro 'OC_IDCT_BEGIN_10'
 0:24.71   __asm nop \
 0:24.71             ^

It's not obvious to me what the error actually is...this is probably something that needs to be fixed on the clang-cl compatibility side in any event.

In the meantime, is it possible to not use these files for a clang-cl build and have libtheora still compile correctly?  I think that would be an acceptable workaround for the time being.
(In reply to Nathan Froyd [:froydnj] from comment #0)
> In the meantime, is it possible to not use these files for a clang-cl build
> and have libtheora still compile correctly?  I think that would be an
> acceptable workaround for the time being.

If you're using clang on Windows then I'm guessing the problem is here https://dxr.mozilla.org/mozilla-central/source/media/libtheora/moz.build#59
Priority: -- → P5
Ideas?  who should worry about it?
Component: Audio/Video → Audio/Video: Playback
Flags: needinfo?(giles)
(Reporter)

Comment 3

2 years ago
I have a patch that just turns the assembly bits off for clang-cl.
(In reply to Nathan Froyd [:froydnj] from comment #3)
> I have a patch that just turns the assembly bits off for clang-cl.

I think you want to compile x86/ instead of x86_vc/ (where I believe vc refers to Visual C) but I've got bigger fish to fry than winclang and theora. If you want it fixed then you should go ahead and fix it.
What Anthony said. Clang should support the same inline assembly syntax as gcc, so it should use the "linux" asm source, rather than the "windows". moz.build is using _MSC_VER to mark this, which I guess is ambiguous? You should be able to just add a 'not clang_cl' clause to that conditional.
Flags: needinfo?(giles)
(Reporter)

Comment 6

2 years ago
(In reply to Ralph Giles (:rillian) needinfo me from comment #5)
> What Anthony said. Clang should support the same inline assembly syntax as
> gcc, so it should use the "linux" asm source, rather than the "windows".
> moz.build is using _MSC_VER to mark this, which I guess is ambiguous? You
> should be able to just add a 'not clang_cl' clause to that conditional.

This approach doesn't seem to work either.  I'm not sure if this points to some sort of problem in clang-cl's inline assembly support, or something else.
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8793451 [details]
Bug 1298387 - disable x86 assembly sources for libtheora when compiling with clang-cl;

https://reviewboard.mozilla.org/r/80188/#review78880

:-(
Attachment #8793451 - Flags: review?(giles) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8793451 [details]
Bug 1298387 - disable x86 assembly sources for libtheora when compiling with clang-cl;

https://reviewboard.mozilla.org/r/80188/#review78882

:-/

Comment 10

2 years ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ab39d701f57
disable x86 assembly sources for libtheora when compiling with clang-cl; r=rillian

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8ab39d701f57
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.