While building with clang, there are a lot of errors for libtheora. Because the list is so long, I've attached it.

This appears to be because clang support for inline asm is very incompatible with gcc's. a) It does not support comments in the asm (which are very useful for finding where the compiler has emitted a copy of an inline asm block in the intermediate assembly output). b) It does the most _astounding_ things with memory operands, including putting operands which were explicitly declared "=m" in an xmm register instead. This is completely broken. c) I don't know what it thinks it is doing with the %H operand modifier (which is supposed to access the high 8 bytes of a 16-byte memory operand), but whatever it is it cannot assemble its own output. libtheora uses %H mostly to guarantee that memory operands have a constant offset in front of them (something which, for esp-relative addresses, is not otherwise guaranteed), so that additional arithmetic can be placed in front of that. This is needed to work around incompatibilities in Apple's very outdated, forked gas, which will not assemble expressions like 0x10+(%esp) (modern gas will warn about the trailing +, but ignore it). Until this is fixed, I would recommend disabling the inline asm for clang. This can be done at the build-system level, if someone tells me how to detect that we're compiling with clang there.

I think we should fix clang instead of adding more work arounds and avoiding giving clang the best performing code. I have fixed the %H issue in r122667. I cannot reproduce this failure with newer clangs. Can we close this bug or do we need to support old ones?

Bug 574346 would suggest we're targeting Xcode 4, which appears to be shipping clang 2.0. Since the latest release, 2.8, is still too old to include your +H fix, we have to pick either dropping support for Xcode 4, or adding work-arounds for old clang. I don't know how many other clang issues we have that require fixes in clang itself, or how reasonable dropping Xcode 4 support would be, but it would certainly be my preferred solution.

Before you make a decision, someone ought to check to ensure that the output with the 'fixed' clang is actually bit exact. It's quite possible for the compiler to fail in a way which still runs but gives wrong results. The seriousness of the problems Tim pointed out makes me a bit suspicious. I can test this with the latest standalone clang, but not with xcode.

On comment 3: Apple uses meaningless numbers on the xcode release :-( It is likely to actually be a 2.9 snapshot. On comment 4: I tried watching some of the theora movies on mozilla.org and it works just fine. Is there some automated test I should be running?

Okay, I was afraid Xcode 4 had already been released. If it is still pending, and will include these latest fixes, that is good news. Mochitests will cover basic playback, though they do not, I believe, check the decoded output for correctness (only crashes, and the fact that _some_ output is produced). Otherwise they might be able to catch things like bug 618458 or bug 625773, which AFAIK they did not. However, I don't think the issues here, if gotten wrong, would be at all subtle, so a visual inspection is probably fine.

You can run the reftests in layout/reftests/ogg-video for some basic video rendering tests. `cd $objdir; make reftest TEST_PATH=layout/reftests/ogg-video/reftest.list` ought to do it.

This builds with Clang trunk. But sadly it still fails to build with the released Xcode 4 final (Build version 4A304a). Apple clang version 2.0 (tags/Apple/clang-137) (based on LLVM 2.9svn) Target: x86_64-apple-darwin10 Thread model: posix

Sounds like we're back to, "disabling the inline asm for clang," at least for the version included with Xcode 4, if we can detect it. I have no idea how to do that and have neither a Mac nor Xcode 4 to test on, so patches welcome.

Which is the missing feature, the %H? It could be avoided with some ugly #ifdefs for pic/non-pic I think. Another option is using a newer clang.

This fixes the problem for me using the clang that ships with Xcode 4, and doesn't appear to affect a gcc build (at least, it gets past this part of the build; it fails later on in nss, but that is probably unrelated). This patch simply disables the inline asm for clang before 2.9. It should automatically work with a newer clang (though I have not tested this). I don't know if it will ever work with the clang Xcode ships, as that still claims to be 2.0.

2.0 is Apple's marketing name for their 2.9 branch :-(

You have a typo: __clang__major__ should be __clang_major__

(In reply to comment #12) > 2.0 is Apple's marketing name for their 2.9 branch :-( Right, but it's not even the real 2.9 (which hasn't been released yet), is it? I'd look at it more as their 2.8 branch, but I still have no assurances that a future Xcode that is based of the real 2.9 will bump the minor version number. However, I am pretty tired of coddling Apple's broken forks of ancient versions of everything in their toolchain, so I'm willing to let them deal with the consequences of lying about their version number if that's what they want to do. (In reply to comment #13) > You have a typo: > > __clang__major__ should be __clang_major__ Good catch. This is why I mention what things I have and have not tested! Updated patch attached.

Yes, I agree with you. We know that right now a "< 2.9" test will do what we want for every release of clang so far: *) accept the open source 2.9 one (when it is out) *) drop the one in xcode 4 *) drop older open source releases We might have to revisit this oncer newer versions of xcode 4 are out, but there is to way to guess what version they will claim to be. I will test your patch and add it to my fork if it is OK (asm still used with newer clang versions).

I added your patch to http://hg.mozilla.org/users/respindola_mozilla.com/clang Thanks.

(In reply to comment #15) > We might have to revisit this oncer newer versions of xcode 4 are out, but > there is to way to guess what version they will claim to be. What I can tell is, that it also doesn't work with the clang version Apple ships with Xcode 4.1 Lion DP. :-(

So far I have found no differences between 4.0 and 4.1 other than 4.1 supporting darwin11 and 4.0 darwin10. For using an official xcode we will probably have to wait for the next rebasing and hope they update the version number.

Comment on attachment 519925 [details] [diff] [review] Proposed fix (verson 2) Is there a reason you're explicitly version-checking clang here rather than just trying to compile some of the ASM constructs in question? Seems like that'd be more foolproof.

(In reply to comment #19) > Is there a reason you're explicitly version-checking clang here rather than > just trying to compile some of the ASM constructs in question? Seems like > that'd be more foolproof. There were at least three things that went wrong in the initial list of errors (I'm not sure that list is exhaustive, and I've not tried to construct small, individual test cases, though that should be easy for at least a) and c), so that'd be at least three separate checks. Of those three, b) could potentially generate code which compiles but which would instead fail at runtime. It probably wouldn't even crash, just silently generate the wrong output (or get the right output by random chance, depending on what was in whatever register it picked), so I'm not even sure how to construct a "foolproof" test for it. So 1) it'd be a lot more work, and 2) I doubt it'd actually be more foolproof.

Comment on attachment 519925 [details] [diff] [review] Proposed fix (verson 2) Okay, thanks for the explanation.

Thanks for the review! (It was a good question.)

I think only the H modifier issue is missing in xcode 4.0. If you want to go the other way, you can use the test I added to llvm when fixing it: test/CodeGen/X86/inline-asm-h.ll I am fine with both as currently the tests are equivalent.

Is this patch ready to land?

As far as I'm concerned, yes. You may wish to run it through try, as lacking a hg account, I have not.

Can someone confirm if this is fixed?

(In reply to comment #27) > Can someone confirm if this is fixed? I can confirm that this is fixed now, if you build FF from current mozilla-central with Apples Xcode 4 Clang.

and HAVE_OLD_CLANG is not set when building with 2.9.