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.

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. :-)

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.

Hm, need to pass llvm-config. Trying with https://pastebin.mozilla.org/9021495 https://treeherder.mozilla.org/#/jobs?repo=try&revision=069bf3d4590d3d57dd28575add44e25d06b62dd4

(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.

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!

(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...

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.

(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.

(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.

(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.

(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.

(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.

(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.

(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)

(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

(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.

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 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.

(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.

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

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.