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

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Comment 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1491848 libyuv assembly r=Alex_Gaynor

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.

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1491848 Update libyuv's preprocessor defines for mingw-clang r=jesup

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

Comment 3

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1491848 Patch libyuv to fix the x86 mingw-clang build r=jesup

This patch combines the following two upstream patches
(which we are missing, because we're out of date):

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

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9010311 [details]
Bug 1491848 Update libyuv's preprocessor defines for mingw-clang r=jesup

Randell Jesup [:jesup] has approved the revision.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd1d38fb6248
Patch libyuv to fix the x86 mingw-clang build r=jesup

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/bd1d38fb6248

Comment 10

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1491848 Patch libyuv to fix the x86 mingw-clang build (rebased) r=jesup

[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:

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/1cab3104b7a6