Closed Bug 1491848 Opened Last year Closed Last year

libyuv is confused about the mingw-clang build and which assembly version to use

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- fixed
firefox64 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(2 files, 2 obsolete files)

Component: General: Unsupported Platforms → Graphics
Product: Firefox Build System → Core
libyuv is confused. It some pretty stupid #ifdefs, making a lot of assumptions it shouldn't do. It hurts our clang build, because it did assumptions that we're clang-cl or clang GCC randomly. Now that we use -fms-extensions, we can actually use any version of assemblies - both MSVC and GCC variants should work, but we  need libyuv to be consequent. I hacked it to use MSVC versions consequently. I'm tempted to try using -fms-compatibility and see if we can get it working this way.
Priority: -- → P3
libyuv has never seen a mingw-clang build so it is all confused about what
assembly implementations it should use.

mingw-clang does not define _MSC_VER without -fms-compatibility so everywhere
we see _MSC_VER we generally need to add an identical case for clang-on-windows

This makes some of the if statements very complicated, so we simplified some of
them by turning it into two if statements.

I detect AVX2 support with __clang_major__ > 3 which is not strictly accurate:
some versions of clang 3 have AVX2 support; but it is fairly close and we only
barely support clang 3.9
So I have a patch to libyuv that compiles on all platforms. There's a different way we could (try to) solve this though: we could ask mingw-clang to define _MSC_VER using a build flag.  This would probably remove the need for most of the #if changes in the patch; but we would probably still need to make a few.  We would also need to add it to the .gyp build files.

I'm also unsure what your local-patch-vs-upstream-changes policy for this library is.  Randell, could you weigh in on what you'd like to see here?
Attachment #9009653 - Attachment is obsolete: true
We could run that sort of patch, though the likelihood of conflicts on updates is significant.  if we did, I'd try to get it upstreamed (or a more general fix upstreamed, to factor out all those ifdef changes into one define).  We could also set _MSC_VER for now locally, and file an issue/your-patch upstream with the libyuv maintainer so it can get fixed in our next import (or some later one); this side-steps merge issues.  Both work; the big deal is not carrying this patch long-term and dealing with hand-fixes on updates
Flags: needinfo?(rjesup)
Turns out Martin found and fixed this for us.  We just need to take these patches:

https://chromium.googlesource.com/libyuv/libyuv/+/d8680893ecfcffd952c7a5d0dc1878d1c486a4e4%5E%21/
https://chromium.googlesource.com/libyuv/libyuv/+/6e498475c749063485a046dfb24e1fa1a81cbedf%5E%21/

I'm going to send in a patch for these, and then when we update we'll automatically get the fixes.
Comment on attachment 9010311 [details]
Bug 1491848 Update libyuv's preprocessor defines for mingw-clang r=jesup

Randell Jesup [:jesup] has approved the revision.
Attachment #9010311 - Flags: review+
Attachment #9010311 - Attachment is obsolete: true
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd1d38fb6248
Patch libyuv to fix the x86 mingw-clang build r=jesup
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bd1d38fb6248
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Needed to fix the mingw-clang build for esr60

User impact if declined: Tor will have to carry this patch themselves, and we won't be able to run mingw-clang in automation.

Fix Landed on Version: 64.0a1 / 20181004100222

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): Compiler guards that should only change things for MinGW. If they don't; however, the build will almost certainly break.

String or UUID changes made by this patch:
Attachment #9024548 - Flags: review?(rjesup)
Attachment #9024548 - Flags: approval-mozilla-esr60?
Comment on attachment 9024548 [details] [diff] [review]
Bug 1491848 Patch libyuv to fix the x86 mingw-clang build (rebased) r=jesup

Review of attachment 9024548 [details] [diff] [review]:
-----------------------------------------------------------------

r+ though not really needed
Attachment #9024548 - Flags: review?(rjesup) → review+
Comment on attachment 9024548 [details] [diff] [review]
Bug 1491848 Patch libyuv to fix the x86 mingw-clang build (rebased) r=jesup

OK for uplift, modifications to the build, useful for downstream maintainers.
Attachment #9024548 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.