1356991 - (stylo-nightly-build) Build Stylo in Nightly (always #define MOZ_STYLO but keep Stylo pref'd off by default)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Chris Peterson [:cpeterson]]

We want to #define MOZ_STYLO for desktop platforms by default so Stylo code is built and shipped with Nightly, but pref'd off by default.

Comment 1

[Bobby Holley (:bholley)]

Note that we'll need to sort out our jemalloc story first. I promised glandium we wouldn't flip this #define until that was sorted out. :-)

Comment 3

[Chris Peterson [:cpeterson]]

Bobby, do you want to prepare this patch to actually #define MOZ_STYLO by default? It would be good to have this patch in hand, so we can land it ASAP after the blocking bugs are resolved.

Comment 4

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a7834e4fbd1f96a9fdc20ad431084a4caf62e91

Comment 5

[Bobby Holley (:bholley)]

Hm, need to pass llvm-config. Trying with https://pastebin.mozilla.org/9021495

https://treeherder.mozilla.org/#/jobs?repo=try&revision=069bf3d4590d3d57dd28575add44e25d06b62dd4

Comment 6

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Hm, need to pass llvm-config. Trying with
> https://pastebin.mozilla.org/9021495
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=069bf3d4590d3d57dd28575add44e25d06b62dd4

Hm, still doesn't work.

Comment 7

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd1c1f49ee26c4be4d253ae7cc8a2cc2b4fff4e2

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eba5f69cabe244bcc9d8029a95f260dc2e68fbe7

Comment 9

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31cd852622457a38d6f1844a84f3ca027ee5d812

Comment 10

[Bobby Holley (:bholley)]

Attachment: Build stylo by default on nightly. v1

Comment 11

[Bobby Holley (:bholley)]

There's still more to do here, but I'm out of time and so Nathan said he'd take it over.

Outstanding issues:
* The "Don't use LLVM_CONFIG in non-stylo builds" doesn't quite seem to work, since we still bust on android.
* win32 bindgen busts because of an unknown calling convention. We don't actually need any bindgen-assisted C++ calls for stylo.
* linux32 bindgen triggers some static assertions in the JS engine. Presumably we're not passing the right defines.
* the "Pass bindir instead of libdir to llvm-config on windows" patch doesn't seem to work, win64 is still panicking with Unable to find libclang". See also bug 1363655.
* Some reftests seem to fail, probably because we have some code somewhere that depends on MOZ_STYLO instead of the pref. I can look at this, need more help on the build stuff.
* Possible some other as-yet-undiscovered stuff.

My most recent try push with these patches is https://treeherder.mozilla.org/#/jobs?repo=try&revision=79a6e92ec22176c4a2421aa34e84dfc27c992dd2

Thanks Nathan!

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> * The "Don't use LLVM_CONFIG in non-stylo builds" doesn't quite seem to
> work, since we still bust on android.

The easiest way to deal with this is to test for the existence of llvm-config in the mozconfig and then set LLVM_CONFIG only if it exists. :)

> * linux32 bindgen triggers some static assertions in the JS engine.
> Presumably we're not passing the right defines.

This is servo/servo#16843.

> * win32 bindgen busts because of an unknown calling convention. We don't
> actually need any bindgen-assisted C++ calls for stylo.
> * the "Pass bindir instead of libdir to llvm-config on windows" patch
> doesn't seem to work, win64 is still panicking with Unable to find
> libclang". See also bug 1363655.

I'm not entirely sure what's up with this.  Something about my patch to conditionally assign LLVM_CONFIG is causing the necessary variables to *not* get passed through to cargo, so I can't reproduce the original problem.  The original patches should have worked, though...

Comment 13

[Nathan Froyd [:froydnj]]

Also, linux32 is not completely functional, even with the servo patch, because something causes elfhack to not run correctly.  I think this is because g++ is picking up the libstdc++ that comes with clang (?!), even though I have no idea how that can happen.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #13)
> Also, linux32 is not completely functional, even with the servo patch,
> because something causes elfhack to not run correctly.  I think this is
> because g++ is picking up the libstdc++ that comes with clang (?!), even
> though I have no idea how that can happen.

Stranger and stranger:

* The g++ invocations for linking elfhack are identical for patched and unpatched.
* The actual linker invocations done by g++ for linking elfhack are identical for patched and unpatched.
* The patched version, however, links against the *system* libstdc++ (and can't be executed) whereas the unpatched version links against our g++'s libstdc++ and can be executed.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> * Some reftests seem to fail, probably because we have some code somewhere
> that depends on MOZ_STYLO instead of the pref. I can look at this, need more
> help on the build stuff.

This is bug 1364863. Got a patch.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #14)
> (In reply to Nathan Froyd [:froydnj] from comment #13)
> > Also, linux32 is not completely functional, even with the servo patch,
> > because something causes elfhack to not run correctly.  I think this is
> > because g++ is picking up the libstdc++ that comes with clang (?!), even
> > though I have no idea how that can happen.
> 
> Stranger and stranger:
> 
> * The g++ invocations for linking elfhack are identical for patched and
> unpatched.
> * The actual linker invocations done by g++ for linking elfhack are
> identical for patched and unpatched.
> * The patched version, however, links against the *system* libstdc++ (and
> can't be executed) whereas the unpatched version links against our g++'s
> libstdc++ and can be executed.

Seems like this could be responsible for it:
https://dxr.mozilla.org/mozilla-central/rev/0f4df67c5f162e00d6f52825badf468aefbfba19/build/unix/mozconfig.stdcxx#6-7

And it should only be a matter of making the gcc check first (presumably, if we pull gcc, it's to build with it, so we should prefer that).

Filed bug 1365182.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #12)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> > * linux32 bindgen triggers some static assertions in the JS engine.
> > Presumably we're not passing the right defines.
> 
> This is servo/servo#16843.

...or it would be, if I actually understood cross-compilation.  What we really need to do is something like:

https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668

but that still doesn't fix things, because clang is picking up 64-bit headers for reasons I don't yet understand.  The 32-bit headers *are* there and we are, correctly, requesting 32-bit compilation.

> > * win32 bindgen busts because of an unknown calling convention. We don't
> > actually need any bindgen-assisted C++ calls for stylo.

We don't, but it would be good to at least have an accurate representation of calling conventions.  This is servo/rust-bindgen#541; see my most recent comment there, which means we have some work lower down in the stack to do before bindgen can understand this.

> > * the "Pass bindir instead of libdir to llvm-config on windows" patch
> > doesn't seem to work, win64 is still panicking with Unable to find
> > libclang". See also bug 1363655.
> 
> I'm not entirely sure what's up with this.  Something about my patch to
> conditionally assign LLVM_CONFIG is causing the necessary variables to *not*
> get passed through to cargo, so I can't reproduce the original problem.  The
> original patches should have worked, though...

This appears to be because the path returned from `llvm-config --bindir` has backslashes; replacing those with forward slashes does the right thing (!).  I don't really understand this; perhaps it has something to do with the environment we're running in.  Fixing the slashes problem just runs into the calling convention issues, though.

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> > > * win32 bindgen busts because of an unknown calling convention. We don't
> > > actually need any bindgen-assisted C++ calls for stylo.
> 
> We don't, but it would be good to at least have an accurate representation
> of calling conventions.  This is servo/rust-bindgen#541; see my most recent
> comment there, which means we have some work lower down in the stack to do
> before bindgen can understand this.

Filed rust-lang/rust#42044 for adding support for the thiscall calling convention to rustc, which AFAICT is a necessary prerequisite to using it in bindgen.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> (In reply to Nathan Froyd [:froydnj] from comment #12)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> > > * linux32 bindgen triggers some static assertions in the JS engine.
> > > Presumably we're not passing the right defines.
> > 
> > This is servo/servo#16843.
> 
> ...or it would be, if I actually understood cross-compilation.  What we
> really need to do is something like:
> 
> https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668

The simpler thing to do, which should be done unconditionally for every platform, is to pass --target=$RUST_TARGET to clang. I think all rust targets are understood by clang.

Comment 20

[Masatoshi Kimura [:emk]]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> (In reply to Nathan Froyd [:froydnj] from comment #12)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> > > * linux32 bindgen triggers some static assertions in the JS engine.
> > > Presumably we're not passing the right defines.
> > 
> > This is servo/servo#16843.
> 
> ...or it would be, if I actually understood cross-compilation.  What we
> really need to do is something like:
> 
> https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668

Maybe we need a similar trick on win32? (see bug 1365254)

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #19)
> (In reply to Nathan Froyd [:froydnj] from comment #17)
> > (In reply to Nathan Froyd [:froydnj] from comment #12)
> > ...or it would be, if I actually understood cross-compilation.  What we
> > really need to do is something like:
> > 
> > https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668
> 
> The simpler thing to do, which should be done unconditionally for every
> platform, is to pass --target=$RUST_TARGET to clang. I think all rust
> targets are understood by clang.

This doesn't work for the linux32 case, though we do pass --target for the 32/64-bit MSVC case already.  clang picks up the 64-bit headers, rather than the 32-bit ones, and things do not end well from there:

https://public-artifacts.taskcluster.net/VF27iz1MR5C0EAItXU47MA/0/public/logs/live_backing.log

It looks like it's not even considering the 32-bit libstdc++ headers for some reason.  I *think* this is because it can't find an i686-pc-linux-gnu directory in the libstdc++ include directory(ies).  I don't know whether copying the x86_64-unknown-linux-gnu/32 directory to the appropriate i686-pc-linux-gnu/ in our clang packages would work or not, might try that tomorrow.

Using just -m32 instead of --target at least finds the libstdc++ headers, but fails to consider that it might want to look at the 32-bit multilib headers there:

https://public-artifacts.taskcluster.net/d4L3bVJCQsWSJP42eH05dw/0/public/logs/live_backing.log

Comment 22

[Nathan Froyd [:froydnj]]

(In reply to Masatoshi Kimura [:emk] from comment #20)
> (In reply to Nathan Froyd [:froydnj] from comment #17)
> > (In reply to Nathan Froyd [:froydnj] from comment #12)
> > > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> > > > * linux32 bindgen triggers some static assertions in the JS engine.
> > > > Presumably we're not passing the right defines.
> > > 
> > > This is servo/servo#16843.
> > 
> > ...or it would be, if I actually understood cross-compilation.  What we
> > really need to do is something like:
> > 
> > https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668
> 
> Maybe we need a similar trick on win32? (see bug 1365254)

Windows probably does require a similar trick, but there's also some fixups to be made for passing environment variables into bindgen.  I will post patches tomorrow.  And even once those fixes are applied, bindgen won't work for win32 because of comment 18.

Comment 23

[Nathan Froyd [:froydnj]]

Attachment: Build stylo by default on nightly

This is based on top of the patches in bug 1364428, and is somewhat more
straightforward than the previous version.

That reminds me, I need to file bugs about enabling Stylo on the platforms
mentioned in the patch.

Comment 24

[Ralph Giles (:rillian)]

Comment on attachment 8869074 [details] [diff] [review]
Build stylo by default on nightly

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

\o/

Should probably remind dev-platform that they can install llvm with `./mach boostrap` once this in the landing queue.

Comment 25

[Masatoshi Kimura [:emk]]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #24)
> Should probably remind dev-platform that they can install llvm with `./mach
> boostrap` once this in the landing queue.

Please fix bug 1364406 and 1364458 before landing. Otherwise the |./mach boostrap| instruction will just confuse and frustrate people.

Comment 26

[Chris Peterson [:cpeterson]]

We aren't going to block the Stylo built-but-disabled milestone on win32 and linux32 support.

Comment 27

[Chris Peterson [:cpeterson]]

Now that linux32 bug 1366050 has been fixed, we are building Stylo support (pref'd off) for all desktop platforms: linux32, linux64, macOS, win32, and win64! Stylo support for Android will ship in a later release, probably Firefox 58 or 59.