Closed Bug 1364560 Opened 7 years ago Closed 7 years ago

Skia 59 breaks MinGW Builds

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(2 files, 1 obsolete file)

Bug 1364560 Add support for disabling Skia Jumper assembly code
59 bytes, text/x-review-board-request
lsalzman
: review+
Details
Bug 1364560 Fix detection of Windows in Skia for MinGW build
59 bytes, text/x-review-board-request
lsalzman
: review+
Details 
Skia 59 breaks the MinGW build with errors such as

>cc1plus: warning: -Wformat-security ignored without -Wformat [-Wformat-security]
> 0:08.34 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S: >Assembler messages:
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:1: >Error: no such instruction: `copyright 2017 Google Inc.'
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:3: >Error: no such instruction: `use of this source code is governed by a BSD-style license that can be'
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:4: >Error: no such instruction: `found in the LICENSE file.'
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:6: >Error: no such instruction: `this file is generated semi-automatically with this command:'
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:7: >Error: invalid character '$' in mnemonic
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:9: >Error: no such instruction: `ifdef RAX'
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:10: >Error: no such instruction: `_text SEGMENT'
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:12: >Error: no such instruction: `public _sk_start_pipeline_hsw'
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:13: >Error: no such instruction: `_sk_start_pipeline_hsw LABEL PROC'
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:14: >Error: no such instruction: `db 65,87'
> 0:08.35 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated_win.S:14: >Error: bad register name `%r15'


If I change it to use skia/src/jumper/SkJumper_generated.S instead of _win.S it looks better but still gives an error:

> 0:08.30 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated.S:1:0: warning: -fPIC ignored for target (all code is position independent)
>  0:08.30  # Copyright 2017 Google Inc.
> 0:08.30  ^
>  0:08.31 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated.S: Assembler messages:
>  0:08.31 /home/tom/Documents/moz/mingw-work/just-build-4/gfx/skia/skia/src/jumper/SkJumper_generated.S:13: Error: junk at end of line, first unrecognized character is `-'
>  0:08.31 /home/tom/Documents/moz/mingw-work/just-build-4/config/rules.mk:1055: recipe for target 'SkJumper_generated.o' failed


Is there any option to forgo the assembly?
It _seems_ like things might build successfully if you just comment out the _win.S file.

There were a few other things I needed to clean up, comprising of upstream bugs https://bugs.chromium.org/p/skia/issues/detail?id=6635 and https://sourceforge.net/p/mingw-w64/bugs/612/ - but on this particular issue maybe the assembly just isn't strictly necessary?
Hey Lee, could you comment on this patch? 

The moz.build patch needs cleaning up to only exclude that file in the MinGW build. But assuming that's done, what do you think? Is this something we can safely omit?

The SkiaExecutor patch was resolved upstream, and I will match their patch: https://bugs.chromium.org/p/skia/issues/detail?id=6635#c1

The DWRITE constants are weird, I'll need to double check on them, but I had filed https://sourceforge.net/p/mingw-w64/bugs/612/ - I think MinGW is on the hook for these (in which case I will remove it from my patch).
Flags: needinfo?(lsalzman)
Merely deleting that file from the build won't work, because as far as I can see, the symbols in it are still referenced by other code. You would need to find out why the SkJumper_generated.S file isn't being correctly eaten by GAS.
Flags: needinfo?(lsalzman)
As a fallback, this patch in a later version of Skia is probably relevant: https://skia.googlesource.com/skia/+/6492afa7971cf295a3c3cb92a85218917c02bb4a
So for the non _win.S file, the error I'm hitting is:

SkJumper_generated.S:13: Error: junk at end of line, first unrecognized character is `-'

http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/jumper/SkJumper_generated.S#13

ted mentioned:

14:41:39 T<@ted> tjr: ah, ok
14:42:23 T<@ted> tjr: so alternate idea, it looks like maybe that file is only written to work on mac/linux
14:42:41 T<@ted> that #else block you're hitting is clearly for ELF targets: .section .note.GNU-stack,"",%progbits
14:42:54 T<@ted> tjr: jrmuizel would be a good person to ask about this


Jeff, do you have a thought here? If not, I'm guessing my best bet is to take the upstream patch to disable skjumper under MinGW.
Flags: needinfo?(jmuizelaar)
It seems reasonable to just disable the jumper for now.
Flags: needinfo?(jmuizelaar)
Attachment #8867867 - Flags: review?(lsalzman)
Attachment #8911058 - Flags: review?(lsalzman)
Comment on attachment 8867867 [details]
Bug 1364560 Add support for disabling Skia Jumper assembly code

https://reviewboard.mozilla.org/r/139410/#review188206

You need to add SkJumper_generated_win.S to the separated sources blacklist as well, otherwise it will get added to the 'win' source list again.
Attachment #8867867 - Flags: review?(lsalzman)
Comment on attachment 8911058 [details]
Bug 1364560 Fix detection of Windows in Skia for MinGW build

https://reviewboard.mozilla.org/r/182524/#review188208
Attachment #8911058 - Flags: review?(lsalzman) → review+
(In reply to Lee Salzman [:lsalzman] from comment #11)
> Comment on attachment 8867867 [details]
> Bug 1364560 Add support for disabling Skia Jumper assembly code
> 
> https://reviewboard.mozilla.org/r/139410/#review188206
> 
> You need to add SkJumper_generated_win.S to the separated sources blacklist
> as well, otherwise it will get added to the 'win' source list again.

Hm... I generated the moz.build to make sure it produced correctly, and it seems like it did. I believe everything Jumper-related is covered by the unified_blacklist here: http://searchfox.org/mozilla-central/source/gfx/skia/generate_mozbuild.py#367 which should accomplish what you're talking - if I understand correctly?
Flags: needinfo?(lsalzman)
The unified_blacklist merely prevents it from going into UNIFIED_SOURCES, but it will still get added onto SOURCES. You need to prevent it from getting automatically added onto both, which required you to add the file specifically to: http://searchfox.org/mozilla-central/source/gfx/skia/generate_mozbuild.py#189
Flags: needinfo?(lsalzman)
Attachment #8911059 - Attachment is obsolete: true
Comment on attachment 8867867 [details]
Bug 1364560 Add support for disabling Skia Jumper assembly code

https://reviewboard.mozilla.org/r/139410/#review188456

The moz.build does not appear correct, as despite the changes to generate_mozbuild.py, there is no code in moz.build itself to define SK_JUMPER_USE_ASSEMBLY. It looks like you hand-edited the moz.build, but did not actually generate it from generate_mozbuild.py?
Attachment #8867867 - Flags: review?(lsalzman)
(In reply to Lee Salzman [:lsalzman] from comment #17)
> Comment on attachment 8867867 [details]
> Bug 1364560 Add support for disabling Skia Jumper assembly code
> 
> https://reviewboard.mozilla.org/r/139410/#review188456
> 
> The moz.build does not appear correct, as despite the changes to
> generate_mozbuild.py, there is no code in moz.build itself to define
> SK_JUMPER_USE_ASSEMBLY. It looks like you hand-edited the moz.build, but did
> not actually generate it from generate_mozbuild.py?

Yes, sorry. I had run generate_mozbuild and looked at the SKJumper related changes to ensure they were correct, but hand-edited the file because it also updated the files - I didn't know what Skia git commit was used so I was using master. I dug in and took a stab at it and found a git commit that produced a new moz.build file that only added the missing SK_JUMPER_USE_ASSEMBLY lines, so I believe this moz.build is good now.

Try run on a vanilla Windows build:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8cf54ada63e08bc2c82557586a447377435f82a
Comment on attachment 8867867 [details]
Bug 1364560 Add support for disabling Skia Jumper assembly code

https://reviewboard.mozilla.org/r/139410/#review188624
Attachment #8867867 - Flags: review?(lsalzman) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8ffa060c7446
Add support for disabling Skia Jumper assembly code r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/c0402190aaca
Fix detection of Windows in Skia for MinGW build r=lsalzman
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ffa060c7446
https://hg.mozilla.org/mozilla-central/rev/c0402190aaca
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: