1373878 - Run stylo struct layout tests on automation

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Right now, they only run on Linux on the checked-in bindings, it'd be nice to run them also on Gecko automation, otherwise we don't catch bugs like bug 1373828 until someone goes and updates the bindings.

This involves compiling the bindings with test configuration, in debug and release mode and running the auto-generated tests.

Comment 1

[Bobby Holley (:bholley)]

Specifically, we need to run these tests as part of |mach check|. Otherwise, somebody can totally break stylo by rearranging unrelated Gecko data structures in ways that bindgen gets wrong.

Chris, this falls into the same bucket as the static analysis: it's something we can live without for now, but we need it to ship. Can you track it appropriately?

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Bug 1323140 is probably related? Maybe a dupe of that?

Comment 3

[Gregory Szorc [:gps]]

What is the mechanism for running these tests? i.e. what command do we need to run?

Comment 4

[Manish Goregaokar [:manishearth]]

./mach test-stylo 

It basically runs a test crate, we may need to hook that up to gtest or something.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Manish Goregaokar [:manishearth] from comment #4)
> ./mach test-stylo 
> 
> It basically runs a test crate, we may need to hook that up to gtest or
> something.

Does it build a standalone binary?  A standalone binary would be straightforward to add to the existing tests.

Comment 6

[Manish Goregaokar [:manishearth]]

Yep. It builds the test crate servo/tests/unit/stylo/Cargo.toml.

Comment 7

[Chris Peterson [:cpeterson]]

Running the Stylo struct layout tests should block enabling Stylo on Nightly to avoid crash bugs.

Comment 8

[Manish Goregaokar [:manishearth]]

The actual command that needs to be run is `cargo test -p stylo_tests`, perhaps with a `--features testing`

Comment 9

[Chris Peterson [:cpeterson]]

Nathan said he could look at this bug. We need to run these tests before we can safely enable Stylo by default.

Comment 10

[(not currently active) Ted Mielczarek]

bug 1331022 covers the more general "run `cargo test` tests".

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson [:cpeterson] from comment #9)
> Nathan said he could look at this bug. We need to run these tests before we
> can safely enable Stylo by default.

I looked at this today.  There are a couple of things to figure out:

* `cargo test --no-run` generates a stylo_tests-${HASH} binary.  This is great for running everything from Cargo, but not so great for things that want to grab the binaries out of the compilation directory after Cargo finishes compiling them.  I don't see any obvious way of getting this information out of Cargo.

* Running `cargo test --manifest-path $GECKO/servo/tests/unit/stylo/Cargo.toml` assumes that the crate is part of the servo workspace...which it is, but for our purposes, we don't care.  We especially don't care that Cargo wants to pull in Servo's entire dependency graph (due to the workspace) for the purposes of compiling this crate.  The only way I can think of getting around this is to duplicate said Cargo.toml somewhere in the Gecko tree and point at Servo's copy of everything else.  This idea makes any updates to the tests unpleasant, since we can't atomically update the files on both Servo and Gecko: somebody has to remember that these tests are special.

* Even if you did that, some of the paths don't work out quite right, so we might wind up copying and pasting some or all of the test infrastructure anyway.

* stylo_tests aren't run on Servo's Mac and Win builders.  I think Stylo's build script has some workarounds for finding Python on Windows, which we'd need to port over to stylo_tests.  Or we could rewrite the Python script in Rust.

Unless somebody has a better idea, I am going to try moving the Python script into the stylo_tests build script, using the duplicated Cargo.toml file, and...not sure what to do about the stylo_tests-${HASH} binary.  Ted, Manish, do you have any thoughts here?

Comment 12

[Manish Goregaokar [:manishearth]]

> see any obvious way of getting this information out of Cargo.

I think if you use -v you can grep it out of the output. Not great though.

> Running `cargo test --manifest-path $GECKO/servo/tests/unit/stylo/Cargo.toml` ...

I'm unclear why we need to do this. Can't we just run cargo test from the same folder (with the same args) as we run cargo build?

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Manish Goregaokar [:manishearth] from comment #12)
> > see any obvious way of getting this information out of Cargo.
> 
> I think if you use -v you can grep it out of the output. Not great though.

`-v` on which command?  Oh, `cargo test --no-run -v`?  We really want it at configure time, not build time, so we can write out makefile rules for installing the generated binary.  I guess we could hack in this as a special case, but if we want to do more Rust test running, I'd really like to come up with something that's not so hacky.

> > Running `cargo test --manifest-path $GECKO/servo/tests/unit/stylo/Cargo.toml` ...
> 
> I'm unclear why we need to do this. Can't we just run cargo test from the
> same folder (with the same args) as we run cargo build?

I'm unclear on how that would help--we don't have any tests defined in the folder we're running `cargo build` from (toolkit/library/rust/)?

Comment 14

[(not currently active) Ted Mielczarek]

Cargo supports `--message-format json`, which can give you the output filenames. For example:
luser@eye7:/build/rust-test-assembler$ cargo test --no-run --message-format json 2>/dev/null
{"features":["default","std"],"filenames":["/build/rust-test-assembler/target/debug/deps/libbyteorder-fadd50430087d61b.rlib"],"fresh":true,"package_id":"byteorder 0.5.3 (registry+https://github.com/rust-lang/crates.io-index)","profile":{"debug_assertions":true,"debuginfo":2,"opt_level":"0","overflow_checks":true,"test":false},"reason":"compiler-artifact","target":{"crate_types":["lib"],"kind":["lib"],"name":"byteorder","src_path":"/home/luser/.cargo/registry/src/github.com-1ecc6299db9ec823/byteorder-0.5.3/src/lib.rs"}}
{"features":[],"filenames":["/build/rust-test-assembler/target/debug/test_assembler-c674d791962c0945"],"fresh":true,"package_id":"test-assembler 0.1.5 (path+file:///build/rust-test-assembler)","profile":{"debug_assertions":true,"debuginfo":2,"opt_level":"0","overflow_checks":true,"test":true},"reason":"compiler-artifact","target":{"crate_types":["lib"],"kind":["lib"],"name":"test-assembler","src_path":"/build/rust-test-assembler/src/lib.rs"}}
{"features":[],"filenames":["/build/rust-test-assembler/target/debug/libtest_assembler.rlib"],"fresh":true,"package_id":"test-assembler 0.1.5 (path+file:///build/rust-test-assembler)","profile":{"debug_assertions":true,"debuginfo":2,"opt_level":"0","overflow_checks":true,"test":false},"reason":"compiler-artifact","target":{"crate_types":["lib"],"kind":["lib"],"name":"test-assembler","src_path":"/build/rust-test-assembler/src/lib.rs"}}


(the standard cargo output goes to stderr).

Comment 15

[Manish Goregaokar [:manishearth]]

> I'm unclear on how that would help--we don't have any tests defined in the folder we're running `cargo build` from (toolkit/library/rust/)?

stylo_tests is a dev-dependency of geckolib.

We should be able to run tests via `cargo test -p stylo_tests`. If not, I can reconfigure it such that we can.

What servo does is cd to ports/geckolib (toplevel cargo file for geckolib-related things), configure the target directory to be target/geckolib (as it does for all geckolib builds), and run cargo test -p stylo_tests.

We should be able to do something similar from toolkit/library/rust if we apply this diff:


diff --git a/toolkit/library/rust/Cargo.toml b/toolkit/library/rust/Cargo.toml
index f676695..ee1a2a0 100644
--- a/toolkit/library/rust/Cargo.toml
+++ b/toolkit/library/rust/Cargo.toml
@@ -43,3 +43,6 @@ rpath = false
 lto = true
 debug-assertions = false
 panic = "abort"
+
+[dev-dependencies]
+stylo_tests = {path = "../../../servo/tests/unit/stylo"}

(This could also go in the gktest Cargo.toml, any toplevel cargo.toml will do.

Comment 16

[Manish Goregaokar [:manishearth]]

FWIW with the above diff and associated lockfile updates, `$ MOZ_TOPOBJDIR=blah cargo test --features servo -p stylo_tests` gets to the point of compiling the tests for me.

You need this diff for the tests to pass:

diff --git a/servo/components/style/properties/properties.mako.rs b/servo/components/style/properties/properties.mako.rs
index 877ec1a..c0ce7aa 100644
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -3417,7 +3417,6 @@ macro_rules! longhand_properties_idents {
 }
 
 /// Testing function to check the size of all SpecifiedValues.
-#[cfg(feature = "testing")]
 pub fn test_size_of_specified_values() {
     use std::mem::size_of;
     let threshold = 24;

We could alternatively pass down the "testing" feature to the crate but that's more work and unnecessary really.

Comment 17

[Manish Goregaokar [:manishearth]]

>  `$ MOZ_TOPOBJDIR=blah cargo test ...`

running it from toolkit/library/rust that is. This should continue to work if you run it from elsewhere via manifest-path.

Comment 19

[Nathan Froyd [:froydnj]]

One issue with this is that since we compile our Rust code with panic=abort and `cargo test` requires panic=unwind, compiling the necessary binary requires recompiling *everything* in the gkrust dependency graph.  There's just no way around this.

gps suggested earlier in the week having a separate job for compiling stylo_tests (maybe all Rust tests, given the above?) so it wouldn't block getting our main automation build done and running mochitests, etc.  That seemed like kind of an unnecessary thing, but given this, we may want to go ahead and create separate jobs anyway...

Comment 20

[Bobby Holley (:bholley)]

(In reply to Nathan Froyd [:froydnj] from comment #19)
> One issue with this is that since we compile our Rust code with panic=abort
> and `cargo test` requires panic=unwind, compiling the necessary binary
> requires recompiling *everything* in the gkrust dependency graph.  There's
> just no way around this.

Is it possible to just run cargo test with panic=abort, even if it means only getting the first failure?

Comment 21

[Bobby Holley (:bholley)]

(though I guess maybe there are also advantages to running separate test builds on automation anyway?)

Comment 22

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #20)
> (In reply to Nathan Froyd [:froydnj] from comment #19)
> > One issue with this is that since we compile our Rust code with panic=abort
> > and `cargo test` requires panic=unwind, compiling the necessary binary
> > requires recompiling *everything* in the gkrust dependency graph.  There's
> > just no way around this.
> 
> Is it possible to just run cargo test with panic=abort, even if it means
> only getting the first failure?

Cargo forcibly ignores your panic=foo settings in Cargo.toml.  I guess we could try setting it through RUSTFLAGS, and see if anything blows up?

Alternative approach: run stylo_tests as a normal binary so we can continue to use our panic=abort code.  This would require rewriting things to not depend on rustc-style testing, and probably some servo changes as well.

Comment 23

[Nathan Froyd [:froydnj]]

Running `RUSTFLAGS='-C panic=abort' cargo test --no-run ...` results in:

error: the linked panic runtime `panic_unwind` is not compiled with this crate's panic strategy `abort`

So that's not going to work. =/

Comment 24

[Nathan Froyd [:froydnj]]

Oh, also: we'd have a hard time running the test binaries during the build, because cross-compiling is a thing: we could run them during the build on Linux and (for now) Windows, but for Mac and (later) for Android, we'd have to run them some other time than the build.

The options break down roughly like this, with some +/- bullets for different options.  stylo_tests is really a standin for "any Rust unit tests we want to run":

* When do we compile stylo_tests?
  * During a normal build job, perhaps controlled by --enable-rust-tests
    + Similar to what we do for C++
    - Slows down builds and therefore packaging and starting 95% of tests (mochitest, reftest, etc.)
  * In an entirely separate --enable-rust-tests build job
    + Nobody cares how fast these run, 'cause they don't block much
    - One more thing for try runs/local testing to remember
    - More build machines required (maybe we don't care about this because builds are all AWS now?)
* When do we run stylo_tests?
  * During the build job
    + Straightforward to do
    - Only doable for "native" compilation scenarios
    - Slows down whatever build job they are running in
  * In an entirely separate "rust test" job, like C++ unit tests or gtests
    + Runs tests for cross-compilation scenarios (Mac, Android for post-57, maybe Windows later)
    - More plumbing
    - Another test for people to remember to run on try

I'm leaning towards having a separate --enable-rust-tests build job, and then running the tests directly from that job: this would get things up and running, not slow down our automation significantly (or local builds), and leave things open for handling cross-compilation.  We wouldn't get any test coverage on Mac, which is unfortunate...

gps, WDYT?

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

How long would comoiling stylo test take? Does it really take significant time?

If we want to have a separate task for stylo test because building it is slow, should we also move C++ tests from normal build tasks so that we speed up building?

Comment 26

[Manish Goregaokar [:manishearth]]

It should be reasonably fast to compile.

Comment 27

[Nathan Froyd [:froydnj]]

Compiling stylo_tests takes as long as compiling gkrust and more besides.  We can't reuse the style/etc. crates we already have because the libxul ones are compiled with panic=abort and the stylo_tests ones have to be compiled with panic=unwind.

Comment 28

[Manish Goregaokar [:manishearth]]

Oh, right. Hm. Can we continue using panic=abort and link in a backtrace handler the way we already do for libxul?

Comment 29

[Nathan Froyd [:froydnj]]

(In reply to Manish Goregaokar [:manishearth] from comment #28)
> Oh, right. Hm. Can we continue using panic=abort and link in a backtrace
> handler the way we already do for libxul?

What is this backtrace handler to which you refer?  I'm not familiar with this.

Comment 23 suggests that the compiler/cargo is going to get in our way no matter what.

Comment 30

[Manish Goregaokar [:manishearth]]

Oh, never mind. I was talking about the way we get backtraces on abort in Firefox but that won't help here. Hm.

Comment 31

[Gregory Szorc [:gps]]

If we're using sccache, I think the overhead of building will be negligible. And it only takes a few seconds to run `mach test-stylo` if a build is available. So I'm not too worried about overhead, no matter how it is done. Do whatever makes sense.

I'm leaning towards a separate job to do the build + test. We can hook up Try syntax, etc to make sure it gets treated as a regular build task for all intents and purposes so people don't forget to run it.

Comment 32

[Manish Goregaokar [:manishearth]]

My concern is that we'll end up running bindgen twice, which can't really be eliminated by sccache.

Comment 33

[Nathan Froyd [:froydnj]]

I have made all the necessary build and taskcluster changes to run separate jobs whose sole purpose is to build Rust tests and run them during `make check`:

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

The Linux64 tests work, which is nice and expected.

The Windows debug (64- and 32-bit) tests fail to link, for reasons I do not understand, as the Linux64 debug tests link just fine.  It looks like the linker is complaining about all the things that would live in ServoBindings.cpp.  Maybe we should just skip the tests on debug Windows builds, since we really only care about them on opt?

The Win64 opt tests work.

The Win32 opt tests fail:

https://treeherder.mozilla.org/logviewer.html#?job_id=125932289&repo=try&lineNumber=44039

16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 20. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 20. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 24. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 24. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 24. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 24. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 20. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 24. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 24. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.
16:47:34     INFO - Your changes have decreased the size of $name SpecifiedValue to 16. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.', Z:\build\build\src\servo\tests\unit\stylo\specified_values.rs:47

In addition to fixing the message...I guess we would like to choose boxing of SpecifiedValue variants depending whether we are on 32-bit or 64-bit platforms?  That seems a little tedious.

Comment 34

[Bobby Holley (:bholley)]

The size_of tests are only meant to be run on 64-bit platforms. We can just turn them off on 32-bit.

Comment 35

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We should also aim to run the layout tests (minus size_of) on Linux 32-bit now that we build and test Stylo there.  This will help guard against issues like bug 1391103 which only affected Linux 32-bit.

Comment 36

[Xidorn Quan [:xidorn] UTC+11]

Stylo layout tests runs on Windows opt? That surprises me. How can it work at all?

The link failure in debug build seems to match what I've always seen locally when I run "mach test-stylo" in servo repo. I think the reason is that, the symbols are from xul.dll, and we are not linking stylo test with that library, and thus the issue.

That's why I'm surprised that it works on opt build. It seems to me that we don't link xul.dll in that case either. Maybe only debug linking really needs those symbols, and opt linking is totally fine to ignore that?

I hope we would be able to run stylo tests locally without switching to opt build, so it would be great if we can have everything work in debug build as well.

Comment 37

[Xidorn Quan [:xidorn] UTC+11]

Another possibility for that is, in debug linking, we keep all the function code around, while in opt build, all actual functions using those symbols are stripped for stylo-tests, and thus there is no need to know the symbols anymore.

Then it would be a question that whether we can do the same thing for debug...

Comment 38

[Chris Peterson [:cpeterson]]

It seems like we should test both opt and debug, unless there is a good reason not to. Is it possible that debug and opt struct sizes may be different if someone adds debug-only struct member variables?

Comment 39

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Chris Peterson [:cpeterson] from comment #38)
> It seems like we should test both opt and debug, unless there is a good
> reason not to. Is it possible that debug and opt struct sizes may be
> different if someone adds debug-only struct member variables?

That's the reason we generate separate structs binding files for debug and release (structs_debug.rs and structs_release.rs).

Comment 40

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #37)
> Another possibility for that is, in debug linking, we keep all the function
> code around, while in opt build, all actual functions using those symbols
> are stripped for stylo-tests, and thus there is no need to know the symbols
> anymore.
> 
> Then it would be a question that whether we can do the same thing for
> debug...

Indeed, we only pass -OPT:REF [1] for non-debug builds, so that's probably the problem here.  Optimized builds delete the functions that would call the Gecko FFI functions, so the FFI functions are not needed at actual symbol resolution time.  At least that would be the theory!

[1] https://msdn.microsoft.com/en-us/library/bxwfs976.aspx

Comment 41

[Xidorn Quan [:xidorn] UTC+11]

Can we probably pass -OPT:REF for debug build as well in this special task so that we don't hit this link error?

Comment 42

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #41)
> Can we probably pass -OPT:REF for debug build as well in this special task
> so that we don't hit this link error?

The thing is that rustc always passes this unless you specifically tell it to link dead code:

https://github.com/rust-lang/rust/blob/master/src/librustc_trans/back/link.rs#L888
https://github.com/rust-lang/rust/blob/master/src/librustc_trans/back/linker.rs#L405

so I don't know why it feels it has to keep the functions calling out to FFI in this case.  Unless we just didn't inline enough (I mean, this *is* a debug build...), or something else in the middle-end didn't delete enough code, or something else?

I guess we could try compiling tests at least at -O1 always...

Comment 43

[Xidorn Quan [:xidorn] UTC+11]

Yeah, I guess it doesn't matter what optimization level do we use for this test, as far as we have the correct compile flags set.

Comment 44

[Nathan Froyd [:froydnj]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #35)
> We should also aim to run the layout tests (minus size_of) on Linux 32-bit
> now that we build and test Stylo there.  This will help guard against issues
> like bug 1391103 which only affected Linux 32-bit.

Indeed.  We have failures for linux32 opt:

https://treeherder.mozilla.org/logviewer.html#?job_id=128626978&repo=try&lineNumber=36373

thread 'bindings::root::mozilla::bindgen_test_layout_ErrorResult' panicked at 'assertion failed: `(left == right)` (left: `4`, right: `8`): Alignment of ErrorResult', /builds/worker/workspace/build/src/obj-firefox/toolkit/library/i686-unknown-linux-gnu/release/build/style-dc7c941fc56784c9/out/gecko/structs_debug.rs:5585

thread 'bindings::root::__bindgen_test_layout_TErrorResult_open0_AssertAndSuppressCleanupPolicy_close0_instantiation' panicked at 'assertion failed: `(left == right)` (left: `24`, right: `32`): Size of template specialization: root :: mozilla :: binding_danger :: TErrorResult', /builds/worker/workspace/build/src/obj-firefox/toolkit/library/i686-unknown-linux-gnu/release/build/style-dc7c941fc56784c9/out/gecko/structs_debug.rs:34372

thread 'bindings::root::JS::bindgen_test_layout_Value' panicked at 'assertion failed: `(left == right)` (left: `4`, right: `8`): Alignment of Value', /builds/worker/workspace/build/src/obj-firefox/toolkit/library/i686-unknown-linux-gnu/release/build/style-dc7c941fc56784c9/out/gecko/structs_debug.rs:11299

Is it possible that these all have to do with the same underlying float alignment issue?

The same failures are present on debug, too.  Bobby, do you know who should look at these?

Comment 45

[Bobby Holley (:bholley)]

Ryan did most of the linux32 stuff, so he's probably the right person to field this one.

Comment 46

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nathan Froyd [:froydnj] from comment #44)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #35)
> > We should also aim to run the layout tests (minus size_of) on Linux 32-bit
> > now that we build and test Stylo there.  This will help guard against issues
> > like bug 1391103 which only affected Linux 32-bit.
> 
> Indeed.  We have failures for linux32 opt:
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=128626978&repo=try&lineNumber=36373
> 
> thread 'bindings::root::mozilla::bindgen_test_layout_ErrorResult' panicked
> at 'assertion failed: `(left == right)` (left: `4`, right: `8`): Alignment
> of ErrorResult',
> /builds/worker/workspace/build/src/obj-firefox/toolkit/library/i686-unknown-
> linux-gnu/release/build/style-dc7c941fc56784c9/out/gecko/structs_debug.rs:
> 5585
> 
> thread
> 'bindings::root::
> __bindgen_test_layout_TErrorResult_open0_AssertAndSuppressCleanupPolicy_close
> 0_instantiation' panicked at 'assertion failed: `(left == right)` (left:
> `24`, right: `32`): Size of template specialization: root :: mozilla ::
> binding_danger :: TErrorResult',
> /builds/worker/workspace/build/src/obj-firefox/toolkit/library/i686-unknown-
> linux-gnu/release/build/style-dc7c941fc56784c9/out/gecko/structs_debug.rs:
> 34372
> 
> thread 'bindings::root::JS::bindgen_test_layout_Value' panicked at
> 'assertion failed: `(left == right)` (left: `4`, right: `8`): Alignment of
> Value',
> /builds/worker/workspace/build/src/obj-firefox/toolkit/library/i686-unknown-
> linux-gnu/release/build/style-dc7c941fc56784c9/out/gecko/structs_debug.rs:
> 11299

The issue here is JS::Value uses alignas(8) and bindgen tries to simulate this alignment with u64, but this fails on Linux 32-bit which only uses 4 byte alignment for u64 types.  So, Rust needs something like the `#[repr(align="8")]` feature not yet implemented to describe this properly.  I believe this is the same as an existing issue:

https://github.com/rust-lang-nursery/rust-bindgen/issues/917

Since I don't believe we're actually manipulating these types in Stylo today, I'll see if its possible to hide them and avoid the issue for now.

Comment 47

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Filed bug 1397937 to prune the unused types causing issues for Linux 32-bit.

Comment 48

[Gregory Szorc [:gps]]

froydnj: this bug is showing up in the Firefox 57 tracking dashboard. Being the bug assignee and someone who understands the impact to the release, could you please set the 57 tracking flag to an appropriate value so it is triaged?

Comment 49

[Chris Peterson [:cpeterson]]

We want to stand up these tests in Nightly 57.

Comment 50

[Nathan Froyd [:froydnj]]

Looks like we have a new failure after bug 1397937:

Linux64:

INFO - 	thread 'bindings::root::mozilla::bindgen_test_layout_SchedulerGroup' panicked at 'assertion failed: `(left == right)` (left: `176`, right: `168`): Size of: SchedulerGroup', /builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-bccbf2b1c0916c0e/out/gecko/structs_debug.rs:6501

Linux32:

INFO - 	thread 'bindings::root::mozilla::bindgen_test_layout_SchedulerGroup' panicked at 'assertion failed: `(left == right)` (left: `88`, right: `84`): Size of: SchedulerGroup', /builds/worker/workspace/build/src/obj-firefox/toolkit/library/i686-unknown-linux-gnu/debug/build/style-bccbf2b1c0916c0e/out/gecko/structs_debug.rs:6498

Curiously, it works correctly on Windows.

Not sure why this is happening, but since Stylo doesn't really need to interact with this struct, can we just prune that type from the generated bindings?

Comment 51

[Xidorn Quan [:xidorn] UTC+11]

I think we can. Just add this type to opaque-types list in ServoBindings.toml should work.

Comment 52

[Nathan Froyd [:froydnj]]

My money for why this happens is because we have:

class SchedulerGroup : public LinkedListElement<SchedulerGroup>
{
  bool mIsRunning;
  ...other members...
};

and LinkedListElement<T> has members:

  LinkedListElement<T>* mNext;
  LinkedListElement<T>* mPrev;
  bool mIsSentinel;
  // sizeof(void*) - sizeof(bool) space until the end of the struct

and one side thinks SchedulerGroup::mIsRunning can be packed into that empty space at the end of LinkedListElement<T>, and the other side thinks that mIsRunning should not.  That change would account for the one word difference in the error messages.

Comment 53

[Nathan Froyd [:froydnj]]

Filed https://github.com/rust-lang-nursery/rust-bindgen/issues/983 for comment 52.

Comment 54

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yep, seems safe to prune.  Try using opaque-types first to treat as a blob of bits.  If that's not enough, you can add it to hide-types to not emit anything at all for this type, and then additionally mark any other structs that referenced it as opaque-types (so they don't trigger errors for the now missing type).

Be sure to edit the `[structs]` section of ServoBindings.toml (since the errors are in structs_* files), not the `[bindings]` section.

(I have been wondering for a while how much of the generated bindings are "wasted" / completely unused things like this...  Would the bindgen step complete more quickly if we trimmed away everything we don't need, through an explicit allow list or some other approach?)

Comment 55

[Nathan Froyd [:froydnj]]

Attachment: part 0 - treat mozilla::SchedulerGroup as an opaque type during bindgen

This struct uses tail packing on platforms using the Itanium C++ ABI.
bindgen doesn't support tail packing at the moment, which causes it to
generate an incorrect Rust-side struct, which then fails tests.  We
don't need to access this struct from Rust, so treating it as an opaque
blob of bytes is reasonable, and fixes the bindgen layout tests.

Comment 56

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add --enable-rust-tests configure option

Because Rust tests require panic=unwind crates and therefore recompiling
all crates we normally use (since most of our crates use panic=abort),
we've elected to enable Rust tests only if the user asks for it.  Hence,
this configure option.

The configure option also enables build-time execution of the crates,
since it's not straightforward to determine at configure time the name
of the test binary Cargo will produce (Cargo produces test binaries of
the form ${NAME}-${HEX_FINGERPRINT}, but there's no way to determine
what ${HEX_FINGERPRINT} is without actually compiling the test binary).

Comment 57

[Nathan Froyd [:froydnj]]

Attachment: part 2 - build system support for Rust tests

Comment 58

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add stylo_tests as a RustTest

The easy part of this patch is the addition of the RustTest itself.

The more difficult to understand part of the patch is the changes to all
of our Rust build configuration.  We do this due to a bug in cargo:

https://github.com/rust-lang/cargo/issues/3923

where features on dependent crates are not correctly taken into account
when determining whether cached artifacts on disk are valid and whether
they should be evicted from the disk cache.  The practical upshot of
this behavior is that, say, running gtests during normal development
when files in libxul are modified will:

* rebuild some Rust dependencies for libxul;
* link libxul;
* rebuild those same Rust dependencies *again* for libxul-gtest, since
  we have different features active and therefore the old artifacts look
  to be out of date;
* link libxul-gtest.

Needless to say, this is highly annoying and counterproductive behavior.

The "fix" is to ensure that the gkrust-shared crate explicitly depends
on crates and assigns features to them such that the feature sets do not
change between normal builds and testing builds.  This is admittedly
fragile, but it is not the first time this has come up, and is probably
not the last.

(The servo/ part of this patch has been submitted as servo/servo#18489, and is
only included so that my local tree will continue to build properly while I'm
getting this reviewed.)

Comment 59

[Nathan Froyd [:froydnj]]

Attachment: part 4 - mozharness and mozconfigs for rusttest builds

We need mozharness configurations and mozconfigs for rusttests.  We are
explicitly not doing Windows debug configurations currently because of
peculiar link errors in such configurations.

mshal is out, so asking Chris for review.

Comment 60

[Nathan Froyd [:froydnj]]

Attachment: part 5 - taskcluster configuration for rusttest builds

Comment 61

[Ralph Giles (:rillian)]

Comment on attachment 8907747 [details] [diff] [review]
part 3 - add stylo_tests as a RustTest

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

Yuck. But if it works, I supposed it's the best we can do until upstream fixes things.

All the incidental updates to Cargo.lock might mean we can remove some of the alternate versions of vendored crates, but it's not important.

Comment 62

[Ralph Giles (:rillian)]

Comment on attachment 8907746 [details] [diff] [review]
part 2 - build system support for Rust tests

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

Looks reasonable. Thanks for taking this on.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1269,5 @@
>              backend_file.write('NO_EXPAND_LIBS := 1\n')
>  
>      def _process_rust_library(self, libdef, backend_file):
>          backend_file.write_once('%s := %s\n' % (libdef.LIB_FILE_VAR, libdef.import_name))
> +        backend_file.write_once('CARGO_FILE := $(srcdir)/Cargo.toml\n')

I wonder how many duplicates we end up with from forgetting `write_once`. Fortunately, it's one Cargo.toml per directory, so duplicates shouldn't cause bugs?

Comment 63

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8907744 [details] [diff] [review]
part 0 - treat mozilla::SchedulerGroup as an opaque type during bindgen

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

Makes sense, thanks! :)

Comment 64

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8907752 [details] [diff] [review]
part 5 - taskcluster configuration for rusttest builds

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

I kind of dislike that these tasks are tests but are in the build kind.  It means, among other things, that they will be treated as `-p`latforms and not `-u`nittests.

But I get that it makes more sense that way for Rust, and that these are using build-oriented mozharness scripts and have symbol tc(BR).  So, I think this is probably the best approach.

::: taskcluster/ci/build/linux.yml
@@ +321,5 @@
> +        secrets: true
> +        custom-build-variant-cfg: rusttests
> +        tooltool-downloads: public
> +        keep-artifacts: false
> +        need-xvfb: true

do these really need xvfb?

Comment 65

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8907746 [details] [diff] [review]
part 2 - build system support for Rust tests

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

::: python/mozbuild/mozbuild/frontend/context.py
@@ +1009,5 @@
> +
> +        This variable should not be used directly; you should be using the
> +        RustTest template instead.
> +        """),
> +

Add these to TEMPLATE_VARIABLES?

Comment 66

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8907751 [details] [diff] [review]
part 4 - mozharness and mozconfigs for rusttest builds

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

A couple of small things here... 

In general it's unfortunate we're spending all this time compiling C++ in this job. Since cargo test is re-compiling everything with panic=unwind anyway, shouldn't this job be running something like `./mach build check` in toolkit/library/rust without doing a full build first?

::: browser/config/mozconfigs/linux32/rusttests
@@ +1,1 @@
> +ac_add_options --enable-rust-tests

I don't see where in this patch series running of rust tests actually gets keyed off the option (by my reading they'll be unconditionally enabled).

::: browser/config/mozconfigs/linux32/rusttests-debug
@@ +1,1 @@
> +ac_add_options --enable-rust-tests

Might as well also turn off symbol dumping and packaging tests, and uploading things.

::: testing/mozharness/configs/builds/taskcluster_firefox_win32_rusttests_opt.py
@@ +72,5 @@
> +        'MINIDUMP_STACKWALK': '%(abs_tools_dir)s\\breakpad\\win32\\minidump_stackwalk.exe',
> +        'MINIDUMP_SAVE_PATH': os.path.join(os.getcwd(), 'public', 'build'),
> +    },
> +    'src_mozconfig': 'browser\\config\\mozconfigs\\win32\\rusttests',
> +    'artifact_flag_build_variant_in_try': None,

The linux configs probably also need "'artifact_flag_build_variant_in_try': None,"

Comment 67

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #66)
> In general it's unfortunate we're spending all this time compiling C++ in
> this job. Since cargo test is re-compiling everything with panic=unwind
> anyway, shouldn't this job be running something like `./mach build check` in
> toolkit/library/rust without doing a full build first?

Is it not the greatest thing ever yes.  Previous discussion seemed to think that sccache was sufficient to help us out here; compiling the Rust (twice!) takes up quite a bit of time anyway, so I'm not sure we'd benefit all that much.

> ::: browser/config/mozconfigs/linux32/rusttests
> @@ +1,1 @@
> > +ac_add_options --enable-rust-tests
> 
> I don't see where in this patch series running of rust tests actually gets
> keyed off the option (by my reading they'll be unconditionally enabled).

This is a good point!  I thought I enabled the these in moz.build based on MOZ_RUST_TESTS, but you're right, nothing checks that.  That should definitely be fixed.

> ::: browser/config/mozconfigs/linux32/rusttests-debug
> @@ +1,1 @@
> > +ac_add_options --enable-rust-tests
> 
> Might as well also turn off symbol dumping and packaging tests, and
> uploading things.

Good point, will do.

> testing/mozharness/configs/builds/taskcluster_firefox_win32_rusttests_opt.py
> @@ +72,5 @@
> > +        'MINIDUMP_STACKWALK': '%(abs_tools_dir)s\\breakpad\\win32\\minidump_stackwalk.exe',
> > +        'MINIDUMP_SAVE_PATH': os.path.join(os.getcwd(), 'public', 'build'),
> > +    },
> > +    'src_mozconfig': 'browser\\config\\mozconfigs\\win32\\rusttests',
> > +    'artifact_flag_build_variant_in_try': None,
> 
> The linux configs probably also need "'artifact_flag_build_variant_in_try':
> None,"

Consider it done.

Comment 68

[Nathan Froyd [:froydnj]]

Attachment: part 2 - build system support for Rust tests

Chris noted that we unconditionally enable Rust tests; this revision addresses
that by checking whether MOZ_RUST_TESTS is enabled in emitter.py.

I tried putting RUST_TEST* in TEMPLATE_VARIABLES, also per Chris's suggestion,
but that variable seems to be reserved for things that once appeared in
Makefile.in files (see the deprecation hint check at the bottom of
context.py).  There are a number of other template-only variables that have
been added as the result of Rust support being added to the build system; I
think I'll tackle those in a separate bug.

Comment 69

[Nathan Froyd [:froydnj]]

Attachment: part 4 - mozharness and mozconfigs for rusttest builds

Revised mozconfigs and mozharness bits:

* Added artifact_flag_build_variant_in_try option to linux mozharness bits;

* Set appropriate variables in rusttests* mozconfigs; the variables that are
  set are similar to the clang or noopt-debug mozconfigs that we have in tree;
  if there are other variables that we should be setting, it would be good to
  make those configs conform as well.

It is wasteful to build all of the C/C++ code, I agree.  sccache should take
care of a lot of that, and simply changing things to run `check` in the
appropriate directory(s) seems like a decent task.  You need to run `export`,
at least, so stylo bindgen will work correctly, and possibly a couple of other
things that I don't know about because I haven't tried it.  We could leave that
for a followup?

Comment 70

[Xidorn Quan [:xidorn] UTC+11]

[Tracking Requested - why for this release]: I don't think we want to ship stylo without having the FFI tests running.

Comment 71

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8910930 [details] [diff] [review]
part 4 - mozharness and mozconfigs for rusttest builds

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

::: browser/config/mozconfigs/linux32/rusttests
@@ +1,3 @@
> +MOZ_AUTOMATION_BUILD_SYMBOLS=0
> +MOZ_AUTOMATION_PACKAGE_TESTS=0
> +MOZ_AUTOMATION_L10N_CHECK=0

We may want to set `MOZ_AUTOMATION_UPLOAD=0` and `MOZ_AUTOMATION_PACKAGE=0` in a follow up.

Comment 72

[Chris Peterson [:cpeterson]]

Nathan, are there any major blockers for uplifting these struct layout tests to Beta 57?

Comment 73

[Xidorn Quan [:xidorn] UTC+11]

FWIW, the part 0 (SchedulerGroup change) has been landed in bug 1403073. That bug also includes a fix to another newly-introduced struct layout issue from bindgen.

Comment 74

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson [:cpeterson] from comment #72)
> Nathan, are there any major blockers for uplifting these struct layout tests
> to Beta 57?

The patches that land in mozilla-central won't be able to be uplifted directly due to Cargo.lock issues.

I'm attempting to land this, but the issues mentioned in comment 58 are cropping up again, and iterating on fixing them takes a very long time, due to how slowly Rust compiles.  I would rather not land this and break everybody's `mach gtest` workflow.

Comment 75

[Chris Peterson [:cpeterson]]

(In reply to Nathan Froyd [:froydnj] from comment #74)
> I'm attempting to land this, but the issues mentioned in comment 58 are
> cropping up again, and iterating on fixing them takes a very long time, due
> to how slowly Rust compiles.  I would rather not land this and break
> everybody's `mach gtest` workflow.

OK. We will just need to be sure to uplift any fixes to Beta 57 for any issues found by the struct layout tests on Nightly.

Comment 76

[Nathan Froyd [:froydnj]]

Apparently, we have some debug-only changes for ComputedValues on Linux64:

thread 'size_of::test_size_of_cv' panicked at 'Your changes have increased the stack size of ComputedValues from 256 to 264. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update the size in /builds/worker/workspace/build/src/servo/tests/unit/stylo/size_of.rs.', /builds/worker/workspace/build/src/servo/tests/unit/stylo/size_of.rs:35

...but the opt Linux64 tests are OK.

Comment 77

[Manish Goregaokar [:manishearth]]

That's probably bug 1399228 (not yet uplifted, but will be soon)

Comment 78

[Nathan Froyd [:froydnj]]

That is what brief blame on size_of.rs suggests.  Not quite sure how to fix:

1) don't run the tests in debug on Linux;
2) conditionalize that particular test for linux64 and debug (ew);
3) something else.

I'm inclined to do #1, since we already don't run the tests in debug for Windows, and this would make Linux consistent with that.

Comment 79

[Manish Goregaokar [:manishearth]]

I'm actually surprised that size changes in debug mode. Any idea why?

Comment 80

[Nathan Froyd [:froydnj]]

My money is on:

http://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.h#352

and that mFrameRefCnt somehow packed with other members in nsStyleContext or ServoStyleContext.  But now nothing packs well with it, and so we have problems.

Comment 81

[Manish Goregaokar [:manishearth]]

Oh, right, that still exists.

IIRC that field is only used by GeckoStyleContext and can be moved as well, but we should do that separately.

Comment 82

[Manish Goregaokar [:manishearth]]

bug 1403808 removes the field

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3876cd293d
part 1 - add --enable-rust-tests configure option; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbb140bb423
part 2 - build system support for Rust tests; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0cf5897a62
part 3 - add stylo_tests as a RustTest; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/af661a8eb6c2
part 4 - mozharness and mozconfigs for rusttest builds; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/f179a112278d
part 5 - taskcluster configuration for rusttest builds; r=dustin

Comment 84

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e3876cd293d
https://hg.mozilla.org/mozilla-central/rev/2dbb140bb423
https://hg.mozilla.org/mozilla-central/rev/7d0cf5897a62
https://hg.mozilla.org/mozilla-central/rev/af661a8eb6c2
https://hg.mozilla.org/mozilla-central/rev/f179a112278d

Comment 85

[Mike Hommey [:glandium]]

Comment on attachment 8907752 [details] [diff] [review]
part 5 - taskcluster configuration for rusttest builds

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

::: taskcluster/ci/build/linux.yml
@@ +302,5 @@
> +    index:
> +        product: firefox
> +        job-name: linux-rusttests-opt
> +    treeherder:
> +        platform: linux32-rusttests/opt

I think there are already way too many platforms on treeherder, and you've added 6 more.

Comment 86

[Mike Hommey [:glandium]]

> and you've added 6 more.

for only 1 job each...

Comment 87

[Xidorn Quan [:xidorn] UTC+11]

I'm thinking about whether the rusttests task can run faster.

Some thoughts: Can we schedule this task after the corresponding build finish, so that it can reuse majority of the cached object? Do we actually need a full build of Gecko itself? I think bindgen only needs the include directory, so configure + moving headers to dist/include should be enough? It doesn't seem to me we are linking the tests against libxul anyway.

Comment 88

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add stylo_tests as a RustTest (for beta)

Rebased patch (particularly for Cargo.lock files) against Beta.

Comment 89

[Nathan Froyd [:froydnj]]

Comment on attachment 8907744 [details] [diff] [review]
part 0 - treat mozilla::SchedulerGroup as an opaque type during bindgen

This patch landed as bug 1403073, which has also already been uplifted.

Comment 90

[Nathan Froyd [:froydnj]]

Comment on attachment 8917500 [details] [diff] [review]
part 3 - add stylo_tests as a RustTest (for beta)

Approval Request Comment
[Feature/Bug causing the regression]: none
[User impact if declined]: none
[Is this code covered by automated tests?]: This code adds automated tests for detecting structure mismatches, which could cause weird crashes.
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1399228 would be required, so we would need to reconsider the uplift of that bug.  Note that it would only be uplifted to make the tests pass.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: No changes to code we actually ship.
[String changes made/needed]: None.

Comment 91

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Nate, I asked CPeterson why we need this change if there is no user benefit. He told me this will enable some memory corruption tests on Beta57 similar to those in Nightly58. If these are test-only changes, you do not need relman approval to land them in m-b. In the patch there are some rust changes, could those have an impact on the Firefox build?

Comment 92

[Nathan Froyd [:froydnj]]

(In reply to Ritu Kothari (:ritu) from comment #91)
> Hi Nate, I asked CPeterson why we need this change if there is no user
> benefit. He told me this will enable some memory corruption tests on Beta57
> similar to those in Nightly58. If these are test-only changes, you do not
> need relman approval to land them in m-b. In the patch there are some rust
> changes, could those have an impact on the Firefox build?

The Rust changes will have no material impact on the Firefox build.

We do need to consider whether to uplift bug 1399228 and bug 1403808 (sorry, missed that one in the uplift request) to be able to uplift this bug.

Comment 93

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8917500 [details] [diff] [review]
part 3 - add stylo_tests as a RustTest (for beta)

We need to find another way of porting these tests over to Beta57 :( since the decision to uplift the fix from https://bugzilla.mozilla.org/show_bug.cgi?id=1399228#c63 was already made and we are not going to uplift that change to 57.