1422368 - Work around a clang-cl bug in yuv_row_win.cpp: unresolved symbol kCoefficientsRgbY

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[(Away)]

https://bugs.llvm.org/show_bug.cgi?id=35482

Comment 1

[(Away)]

Attachment: Work around a clang-cl bug in yuv_row_win.cpp

Comment 2

[(Away)]

(In reply to David Major [:dmajor] from comment #0)
Sorry, I meant https://bugs.llvm.org/show_bug.cgi?id=35290

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1355b9f0cfc1
Work around a clang-cl bug in yuv_row_win.cpp. r=jrmuizel

Comment 4

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1355b9f0cfc1

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This patch didn't actually fix it, but David has an updated patch that does.

Comment 6

[(Away)]

Yeah... the problem isn't one of discarding symbols, it's that the names referenced by inline asm don't get mangled. 

I suspect what happened was:
1. My try push got unresolved externals without the patch
2. My try push failed somewhere else with the patch
3. I assumed #2 was a later failure and thus the patch was successful.

Sorry about that.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Attachment: updated patch

As I mentioned, this is David's patch, I just happen to be the one who verified it on Try :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f90362bbc80
Work around a clang-cl complilation bug in yuv_row_win.cpp harder. r=jrmuizel

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Windows static bustage (unresolved externals):

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b7abcecda9598ee7d383ac71b27087b6a7afc3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5f90362bbc80cf7f9b09e41477c244f292fc9f1c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=150239136&repo=mozilla-inbound

18:12:54     INFO -      StaticXULComponentsEnd\StaticXULComponentsEnd.obj
18:12:54     INFO -     Creating library xul.lib and object xul.exp
18:12:54     INFO -  yuv_row_win.obj : error LNK2019: unresolved external symbol __kCoefficientsRgbY referenced in function _FastConvertYUVToRGB32Row_SSE
18:12:54     INFO -  xul.dll : fatal error LNK1120: 1 unresolved externals
18:12:54     INFO -  z:/build/build/src/config/rules.mk:699: recipe for target 'xul.dll' failed

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Yeah, I'll re-land it with bug 1423307 once it gets r+.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43410f74d526
Work around a clang-cl complilation bug in yuv_row_win.cpp harder. r=jrmuizel

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/43410f74d526

Comment 13

[(Away)]

...aaaaaand these fixes still weren't enough. ThinLTO builds still have a nullptr value for this symbol.

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/737b602f4c6f
Backed out fixes because they weren't sufficient.

Comment 15

[(Away)]

Attachment: Use the intrinsics from the Win64 file

I give up on trying to hack around this inline asm bug.

I propose that we switch Win32 clang-cl to the intrinsics-based versions of these functions from the Win64 code. It means we'll miss out on some of the handwritten specializations like DoubleYUVToRGB32Row_SSE, but if Win64 got away with that, then it can't be so bad.

(If this lands, we can back out the earlier fix)

Comment 16

[(Away)]

(In reply to Pulsebot from comment #14)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/737b602f4c6f
> Backed out fixes because they weren't sufficient.

Pulsebot didn't mention that I immediately backed out the above backout, because I don't want to break people's existing clang-cl builds while I revisit the fix. I wasn't thinking, sorry!

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8964408 [details] [diff] [review]
Use the intrinsics from the Win64 file

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

Sure. Maybe we could use the gcc style inline assembly?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a463b11e241
Use intrinsics-based YUV functions from Win64 under Win32 clang-cl. r=jrmuizel

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8a463b11e241