Run stylo struct layout tests on automation

RESOLVED FIXED in Firefox 58

Status

defect
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: emilio, Assigned: froydnj)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [Stylo])

Attachments

(6 attachments, 3 obsolete attachments)

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.
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?
Flags: needinfo?(cpeterson)
Bug 1323140 is probably related? Maybe a dupe of that?
Blocks: stylo-release
No longer blocks: stylo
Flags: needinfo?(cpeterson)
Priority: -- → P3
What is the mechanism for running these tests? i.e. what command do we need to run?
./mach test-stylo 

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

Comment 5

2 years ago
(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.
Yep. It builds the test crate servo/tests/unit/stylo/Cargo.toml.
Running the Stylo struct layout tests should block enabling Stylo on Nightly to avoid crash bugs.
Blocks: stylo-nightly
No longer blocks: stylo-release
The actual command that needs to be run is `cargo test -p stylo_tests`, perhaps with a `--features testing`
Nathan said he could look at this bug. We need to run these tests before we can safely enable Stylo by default.
Assignee: nobody → nfroyd
Summary: Run stylo struct layout tests on automation. → Run stylo struct layout tests on automation
bug 1331022 covers the more general "run `cargo test` tests".
Assignee

Comment 11

2 years ago
(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?
Flags: needinfo?(ted)
Flags: needinfo?(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?
Flags: needinfo?(manishearth)
Assignee

Comment 13

2 years ago
(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/)?
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).
Flags: needinfo?(ted)
> 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.
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.
>  `$ 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.
Duplicate of this bug: 1323140
Assignee

Comment 19

2 years ago
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...
(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?
(though I guess maybe there are also advantages to running separate test builds on automation anyway?)
Assignee

Comment 22

2 years ago
(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.
Assignee

Comment 23

2 years ago
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. =/
Assignee

Comment 24

2 years ago
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?
Flags: needinfo?(gps)
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?
It should be reasonably fast to compile.
Assignee

Comment 27

2 years ago
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.
Oh, right. Hm. Can we continue using panic=abort and link in a backtrace handler the way we already do for libxul?
Assignee

Comment 29

2 years ago
(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.
Oh, never mind. I was talking about the way we get backtraces on abort in Firefox but that won't help here. Hm.
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.
Flags: needinfo?(gps)
My concern is that we'll end up running bindgen twice, which can't really be eliminated by sccache.
Assignee

Comment 33

2 years ago
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.
The size_of tests are only meant to be run on 64-bit platforms. We can just turn them off on 32-bit.
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.
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.
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...
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?
(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).
Assignee

Comment 40

2 years ago
(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
Can we probably pass -OPT:REF for debug build as well in this special task so that we don't hit this link error?
Assignee

Comment 42

2 years ago
(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...
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.
Assignee

Comment 44

2 years ago
(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?
Flags: needinfo?(bobbyholley)
Ryan did most of the linux32 stuff, so he's probably the right person to field this one.
Flags: needinfo?(bobbyholley) → needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
Depends on: 1397937
Filed bug 1397937 to prune the unused types causing issues for Linux 32-bit.
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?
Flags: needinfo?(nfroyd)
We want to stand up these tests in Nightly 57.
Assignee

Comment 50

2 years ago
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?
Flags: needinfo?(nfroyd) → needinfo?(jryans)
I think we can. Just add this type to opaque-types list in ServoBindings.toml should work.
Assignee

Comment 52

2 years ago
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.
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?)
Flags: needinfo?(jryans)
Assignee

Comment 55

2 years ago
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.
Attachment #8907744 - Flags: review?(jryans)
Assignee

Comment 56

2 years ago
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).
Attachment #8907745 - Flags: review?(giles)
Assignee

Comment 57

2 years ago
Attachment #8907746 - Flags: review?(giles)
Assignee

Comment 58

2 years ago
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.)
Attachment #8907747 - Flags: review?(giles)
Assignee

Comment 59

2 years ago
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.
Attachment #8907751 - Flags: review?(cmanchester)
Attachment #8907745 - Flags: review?(giles) → review+
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.
Attachment #8907747 - Flags: review?(giles) → review+
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?
Attachment #8907746 - Flags: review?(giles) → review+
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! :)
Attachment #8907744 - Flags: review?(jryans) → review+
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?
Attachment #8907752 - Flags: review?(dustin) → review+
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 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,"
Attachment #8907751 - Flags: review?(cmanchester)
Assignee

Comment 67

2 years ago
(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.
Assignee

Comment 68

2 years ago
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.
Attachment #8910928 - Flags: review?(giles)
Assignee

Updated

2 years ago
Attachment #8907746 - Attachment is obsolete: true
Assignee

Comment 69

2 years ago
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?
Attachment #8910930 - Flags: review?(cmanchester)
Assignee

Updated

2 years ago
Attachment #8907751 - Attachment is obsolete: true
[Tracking Requested - why for this release]: I don't think we want to ship stylo without having the FFI tests running.
Attachment #8910928 - Flags: review?(giles) → review+
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.
Attachment #8910930 - Flags: review?(cmanchester) → review+
Nathan, are there any major blockers for uplifting these struct layout tests to Beta 57?
Flags: needinfo?(nfroyd)
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.
Depends on: 1403073
Assignee

Comment 74

2 years ago
(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.
Flags: needinfo?(nfroyd)
(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.
Assignee

Comment 76

2 years ago
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.
That's probably bug 1399228 (not yet uplifted, but will be soon)
Assignee

Comment 78

2 years ago
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.
I'm actually surprised that size changes in debug mode. Any idea why?
Assignee

Comment 80

2 years ago
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.
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.
bug 1403808 removes the field

Comment 83

2 years ago
Pushed by nfroyd@mozilla.com:
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 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.
> and you've added 6 more.

for only 1 job each...
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.
Depends on: 1407687
Assignee

Comment 88

2 years ago
Rebased patch (particularly for Cargo.lock files) against Beta.
Attachment #8917500 - Flags: review+
Assignee

Comment 89

2 years ago
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.
Attachment #8907744 - Attachment is obsolete: true
Assignee

Comment 90

2 years ago
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.
Attachment #8917500 - Flags: approval-mozilla-beta?
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?
Flags: needinfo?(nfroyd)
Assignee

Comment 92

2 years ago
(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.
Flags: needinfo?(nfroyd)
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.
Attachment #8917500 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Blocks: 1426292

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.