Closed Bug 1366050 Opened 7 years ago Closed 7 years ago

Build Stylo for 32-bit Linux on automation

Categories

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

All
Linux
enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: froydnj, Assigned: jryans)

References

Details

Attachments

(1 file, 4 obsolete files)

Stylo for 32-bit Linux should Just Work in a normal cross-compile-from-linux64 situation, but with our automation, something is preventing clang from getting the correct headers in a cross-compile situation.  We should fix that if possible.
Blocks: stylo-nightly-build
No longer blocks: stylo
Priority: -- → P2
We aren't going to block Stylo built-but-disabled on linux32 support.
Blocks: stylo-nightly
No longer blocks: stylo-nightly-build
The things using bindgen probably just need to call like `clang_arg("-m32")`:
https://docs.rs/bindgen/0.25.1/bindgen/struct.Builder.html#method.clang_arg

I poked around at bindgen/libclang recently. It would be nice if this was something bindgen could handle automatically, like if you could pass it the target and it could detect when you're targeting x86 but building on x86-64.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> The things using bindgen probably just need to call like `clang_arg("-m32")`:
> https://docs.rs/bindgen/0.25.1/bindgen/struct.Builder.html#method.clang_arg

We do this, and it still does not fix the problem.  clang is still picking up the 64-bit headers, rather than the 32-bit ones.
(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> > The things using bindgen probably just need to call like `clang_arg("-m32")`:
> > https://docs.rs/bindgen/0.25.1/bindgen/struct.Builder.html#method.clang_arg
> 
> We do this, and it still does not fix the problem.  clang is still picking
> up the 64-bit headers, rather than the 32-bit ones.

I should note here that clang itself includes the necessary 64-bit headers, but it's only our gcc package that includes the 32-bit headers.  clang has some logic to figure out where the gcc include paths are if it's installed alongside gcc, but it's not finding the gcc include directories and/or not understanding that it needs to look in the 32-bit directories as well.
So, I bet this is because this line[1] doesn't pass our custom clang flags.

Shouldn't be easy to add support for it, should I go for it and see if it fixes the problem? A different alternative is telling bindgen to not do the path fixup and pass the paths manually, but that can be cumbersome.

[1]: https://github.com/KyleMayes/clang-sys/blob/master/src/support.rs#L183
Err, shouldn't be hard, I mean, ofc
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> So, I bet this is because this line[1] doesn't pass our custom clang flags.
> 
> Shouldn't be easy to add support for it, should I go for it and see if it
> fixes the problem? A different alternative is telling bindgen to not do the
> path fixup and pass the paths manually, but that can be cumbersome.

In the meantime, Shing is also looking into this issue.
That would be great if you could quickly verify your fix or Shing is able to do that as well.
Assigning to Shing because Astley said he would look into this bug.
Assignee: nobody → shing.lyu
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> So, I bet this is because this line[1] doesn't pass our custom clang flags.
>
> Shouldn't be easy to add support for it, should I go for it and see if it
> fixes the problem? A different alternative is telling bindgen to not do the
> path fixup and pass the paths manually, but that can be cumbersome.

Why do we even need to fixup include paths in the first place?  That seems like a bad idea at first glance.  And we pass -m32 to the clang instance anyway, which should find the appropriate include paths too...
Emilio, so you are suggesting we pass our custom clang argument at [1]?
And could you also point me to the include path fixup code in bindgen? Thank you.

[1]: https://github.com/KyleMayes/clang-sys/blob/master/src/support.rs#L183
Flags: needinfo?(emilio+bugs)
(In reply to Nathan Froyd [:froydnj] from comment #9)
> Why do we even need to fixup include paths in the first place?  That seems
> like a bad idea at first glance.

Well, yeah, I agree a priori it's not a great idea, but long story short, libclang doesn't properly find system headers without it. See [1] for example, where other alternatives are discussed.

> And we pass -m32 to the clang instance
> anyway, which should find the appropriate include paths too...

We pass -m32 to bindgen's libclang instantiation, but not to the clang-sys code. That'd be the missing piece to make this work AFAICT.

(In reply to Shing Lyu [:shinglyu] from comment #10)
> Emilio, so you are suggesting we pass our custom clang argument at [1]?

Yes, we need to pass the BindgenContext's clang_argument options to that line of code.

> And could you also point me to the include path fixup code in bindgen? Thank
> you.

The fixup lives pretty much here[2], but now I see the code, I see we avoid it for builds that use `--target`. Presumably we should do the same for other arch-dependent flags? Or just adding an option to bindgen to not mess with the include path may be enough? We already have in ServoBindings.toml a line that reads:

    # To disable the fixup bindgen applies which adds search
    # paths from clang command line in order to avoid potential
    # conflict with -stdlib=libc++.
    "--target=x86_64-apple-darwin",

For the Mac builds.

[1]: http://lists.llvm.org/pipermail/cfe-dev/2017-June/054088.html
[2]: https://github.com/servo/rust-bindgen/blob/cabfac71fc454196656ff63e5cc761294896831d/src/lib.rs#L1041
Flags: needinfo?(emilio+bugs)
Summary: enable Stylo for 32-bit Linux by default on automation → Build Stylo for 32-bit Linux on automation
No longer blocks: stylo-nightly-build
I passed the clang_args down but got this error: 

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

Looks like it's the problem with Servo's style code, is this a known issue?
Flags: needinfo?(emilio+bugs)
Looks like a bindgen bug (we should be generating nsTArray<nsString> instead.

Could you try to get a reduced version of it? It's trying to generate this function: http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/layout/style/ServoBindings.cpp#1517

Also, if you have a build locally and can attach the bindings, that'd be awesome.

Also, does it happen if you remove this line? http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/layout/style/ServoBindings.toml#482
Flags: needinfo?(emilio+bugs)
Removing the line in ServoBindings.toml doesn't seem to fix it: https://treeherder.mozilla.org/logviewer.html#?job_id=110840620&repo=try&lineNumber=8815

Building locally is a little bit tricky because I can't faithfully reproduce the linux32 cross compiling environment easily.

How would you suggest we reduce the issue? Are there ways we can test it without compiling everything?

Thanks :)
Flags: needinfo?(emilio+bugs)
Update: 

Since my patch seems to fix the header issue, I landed the clang-sys patch [0]. KyleMayes kindly bump the version number to 0.19.0, so we are ready to land the bindgen change [1]. Then after we update bindgen on m-c everyone can jump in and debug the bindgen bug mentioned in Comment 15.

[0]: https://github.com/KyleMayes/clang-sys/pull/57
[1]: https://github.com/servo/rust-bindgen/pull/781
Emilio bumped rust-bindgen to version 0.26.3 [1] to include Shing's Linux32 fix. (btw, neither v0.26.3 nor v0.26.2 are listed in rust-bindgen's releases page [2].)

Emilio or Shing, can you please update Servo's crate dependencies to use the new rust-bindgen v0.26.3 version? Servo is currently using v0.26.1.

[1] https://github.com/servo/rust-bindgen/commit/1bf0b8a40bbca6ffdfb1cdd116a3f3fa0cdc8f10
[2] https://github.com/servo/rust-bindgen/releases
[3] https://github.com/servo/servo/commit/e8db09766dd70ec0562da3e954f65fb4311962b9
Looks like is not released yet? 

error: no matching version `^0.26.3` found for package `bindgen` (required by `style`)
location searched: registry https://github.com/rust-lang/crates.io-index
versions found: 0.26.2, 0.26.1, 0.26.0, ...
(In reply to Shing Lyu [:shinglyu] from comment #18)
> Looks like is not released yet? 
> 
> error: no matching version `^0.26.3` found for package `bindgen` (required
> by `style`)
> location searched: registry https://github.com/rust-lang/crates.io-index
> versions found: 0.26.2, 0.26.1, 0.26.0, ...

Seems to now be present, looks like it was published 6 hours after your comment here.
Hmm, we can't merge it yet, initially I believe the version bump will unblock the 32-bit header issue, and enable us to fix another bindgen issue on Linux32, keeping Linux64 unharmed. But it seems this version bump will also break Linux64 build in a different way: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4c621093543f6a97fe2bad6ad97906797c36648

I'm investigating.
Flags: needinfo?(emilio+bugs)
Attached patch bindings.rs.patch (obsolete) — Splinter Review
This is a diff of the generated bindings.rs from bindgen 0.26.1 v.s. 0.26.3, It seems to generate a few functions but didn't include the `use` for them. For example there are many functions that takes `nsStringRepr_*` as parameters. 

Any idea on why 0.26.1 didn't include the function but 0.26.3 does?
Flags: needinfo?(emilio+bugs)
So, there aren't many commits in that version range:

 * https://github.com/servo/rust-bindgen/compare/v0.26.1...v0.26.3

The new functions generated seem C++ methods that end up being marked as children of some module and whitelisted. I suspect a lot from:

 * https://github.com/servo/rust-bindgen/commit/979f5aae9a078a54aec18d4a6cfe38f34322f534

Could you try reverting that and see if that stops generating those functions?

I'm less sure about the mishandling of nsTArray, but could either be that or https://github.com/servo/rust-bindgen/commit/d9ede218489e0bf5330d0005676f7687025e44c8, I guess... Any chance you could bisect it?

Thanks!
Flags: needinfo?(emilio+bugs)
I reverted back to 0.26.1 the error goes away, everything compiles OK.

I'll try to bisect the rust-bindgen code, it might take a while. :)
first BAD  979f5aa Ensure that every item is in some module's children list
last GOOD  26094ea Allow path separators in test-one.sh

Looks like this is the one causing the problem: 

commit 979f5aae9a078a54aec18d4a6cfe38f34322f534
Author: Nick Fitzgerald <fitzgen@gmail.com>
Date:   Mon Jun 19 14:01:46 2017 -0700

    Ensure that every item is in some module's children list
Flags: needinfo?(emilio+bugs)
Thanks for looking Shing!

Nick, could you investigate this one if you have the time?

Let me know if not, I can try to take a closer look, but if it was blocking SM the trivial way to fix it by backout may not be the optimal :P
Flags: needinfo?(emilio+bugs) → needinfo?(nfitzgerald)
To be clear, the issue is that we're getting compilation errors in the emitted bindings like

> error[E0412]: cannot find type `nsPresContext` in this scope
>      --> /home/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-a3e69c1fe576a835/out/gecko/bindings.rs:20750:54
>       |
> 20750 |                                     aContext: *const nsPresContext);
>       |                                                      ^^^^^^^^^^^^^ not found in this scope
>       |
>       = help: possible candidate is found in another module, you can import it into scope:
>                 `use gecko_bindings::structs::root::nsPresContext;`

after updating to the latest bindgen release?

A little bit of an aside, but it would be really helpful if Stylo generated self-contained, standalone bindings, instead of generating structs and functions separately with one depending on the other. Then, we could have the compile-stylo-bindings smoke test in bindgen's test suite not just test that bindings can be emitted, but also that the emitted bindings compile and pass the generated layout tests. If we could do all that, then we wouldn't regress Stylo bindings generation with new bindgen releases in the first place. It would also make tracking down and fixing Stylo + bindgen bugunderstands easier because we could properly use C-Reduce, which is difficult to do with the Stylo bindings right now. I'm able to diagnose and fix SpiderMonkey bindings issues *much* faster than Stylo bindings issues, because the SpiderMonkey bindings are standalone and I can easily use C-Reduce on them.

I empathize with the desire to only emit bindings for precisely the types and functions required, but that needs to be weighed against the above drag on productivity. Finally, we can and should also add more options/features to bindgen to cut down on emitting bindings to things that weren't intended, so that this isn't a good excuse not to have standalone bindings for Stylo.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #25)
> if it was blocking SM the trivial way to fix it by backout may not be
> the optimal :P

That commit fixed bogus codegen that resulted in failure to compile the emitted bindings. We were running into this with SM. Definitely not something that would be wise to back out.
Flags: needinfo?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #26)
> A little bit of an aside, but it would be really helpful if Stylo generated
> self-contained, standalone bindings, instead of generating structs and
> functions separately with one depending on the other. Then, we could have
> the compile-stylo-bindings smoke test in bindgen's test suite not just test
> that bindings can be emitted, but also that the emitted bindings compile and
> pass the generated layout tests. If we could do all that, then we wouldn't
> regress Stylo bindings generation with new bindgen releases in the first
> place. It would also make tracking down and fixing Stylo + bindgen
> bugunderstands easier because we could properly use C-Reduce, which is
> difficult to do with the Stylo bindings right now. I'm able to diagnose and
> fix SpiderMonkey bindings issues *much* faster than Stylo bindings issues,
> because the SpiderMonkey bindings are standalone and I can easily use
> C-Reduce on them.

Yeah, this way updates to the generated bindings are less noisy though... I'd be super-happy to get rid of the bindings.rs file when we don't have to upstream them to servo.

> I empathize with the desire to only emit bindings for precisely the types
> and functions required, but that needs to be weighed against the above drag
> on productivity. Finally, we can and should also add more options/features
> to bindgen to cut down on emitting bindings to things that weren't intended,
> so that this isn't a good excuse not to have standalone bindings for Stylo.

So there are two problems here, first is the extra methods generated, which is probably workaroundable/acceptable.

But there's also the template parameter of nsTArray issue, which seems way more serious. In particular, that commit apparently switched us from generating nsTArray<Foo> to nsTArray in the functions, which I'm still not quite sure why, and requires debugging.

> (In reply to Emilio Cobos Álvarez [:emilio] from comment #25)
> > if it was blocking SM the trivial way to fix it by backout may not be
> > the optimal :P
> 
> That commit fixed bogus codegen that resulted in failure to compile the
> emitted bindings. We were running into this with SM. Definitely not
> something that would be wise to back out.

Sure, I agree, but the nsTArray regression also seems scary.
(In reply to Shing Lyu [:shinglyu] from comment #24)
> last GOOD  26094ea Allow path separators in test-one.sh

I'm seeing `nsTArray` without template parameters even with this commit.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #28)
> (In reply to Shing Lyu [:shinglyu] from comment #24)
> > last GOOD  26094ea Allow path separators in test-one.sh
> 
> I'm seeing `nsTArray` without template parameters even with this commit.

Nevermind, it turns out that `clang++ -save-temps` has two issues that just bit me:

1. It doesn't include headers that have been included on the command line like `-include` into the preprocessed .ii file. I've filed https://github.com/servo/rust-bindgen/issues/811 as a place to figure out how to improve the "dump the preprocessed input that bindgen is working with" workflow

2. It strips comments, so any `<div rust-bindgen replaces="..."/>` annotations get lost. nsTArray_Simple replaces nsTArray in this fashion, so I just wasn't performing an equivalent bindgen run.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #29)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #28)
> > (In reply to Shing Lyu [:shinglyu] from comment #24)
> > > last GOOD  26094ea Allow path separators in test-one.sh
> > 
> > I'm seeing `nsTArray` without template parameters even with this commit.
> 
> Nevermind, it turns out that `clang++ -save-temps` has two issues that just
> bit me:
> 
> 1. It doesn't include headers that have been included on the command line
> like `-include` into the preprocessed .ii file. I've filed
> https://github.com/servo/rust-bindgen/issues/811 as a place to figure out
> how to improve the "dump the preprocessed input that bindgen is working
> with" workflow

This is now fixed.

> 2. It strips comments, so any `<div rust-bindgen replaces="..."/>`
> annotations get lost. nsTArray_Simple replaces nsTArray in this fashion, so
> I just wasn't performing an equivalent bindgen run.

This is fixed in https://github.com/servo/rust-bindgen/pull/817

We can now call `bindgen::Builder::dump_preprocessed_input` to get a single C++ file that contains everything we are generating bindings for, with the source annotations, and without any ifdefs or includes.

It might make sense to call `bindgen::Builder::dump_preprocessed_input` in taskcluster stylo builds so that if/when we hit a bindgen bug, we already have a standalone test case to investigate with, feed into C-Reduce, etc.
I'll give it a try as soon as it lands. 

> It might make sense to call `bindgen::Builder::dump_preprocessed_input` in taskcluster stylo builds so that if/when we hit a bindgen bug, we already have a standalone test case to investigate with, feed into C-Reduce, etc.

Can you elaborate on this? So the function will dump into a C++ file, and we need to add it to the artifact list? Or does it trigger some form of unit test?
It dumps a `__bindgen.i` or `__bindgen.ii` file (for C or C++ respectively) that contains the complete input that bindgen is processing.

We would add it to the artifacts list.

Then, when we get bindgen bugs in CI, anyone should be able to simply download that file and run bindgen on it with the correct flags to reproduce the bug.

Perhaps we also want to add `println!("{:?}", builder.command_line_flags());` to stylo's build script, which dumps the equivalent command line flags for a bindgen builder, if we're going to go down this route.
I still got the extra methods and nsTArray errors while building stylo, what else do we need to do to fix this?
Flags: needinfo?(nfitzgerald)
There is ongoing work in https://github.com/servo/rust-bindgen/pull/827 and https://github.com/servo/rust-bindgen/issues/826 for the the unwanted methods.

The nsTarray errors are regarding self_type, right? That has a PR that is r+ with comments (https://github.com/servo/rust-bindgen/pull/823) and which I am landing in a minute.
Flags: needinfo?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #34)
> There is ongoing work in https://github.com/servo/rust-bindgen/pull/827 and
> https://github.com/servo/rust-bindgen/issues/826 for the the unwanted
> methods.
> 
> The nsTarray errors are regarding self_type, right? That has a PR that is r+
> with comments (https://github.com/servo/rust-bindgen/pull/823) and which I
> am landing in a minute.

All of these things have merged.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #34)
> There is ongoing work in https://github.com/servo/rust-bindgen/pull/827 and
> https://github.com/servo/rust-bindgen/issues/826 for the the unwanted
> methods.
> 
> The nsTarray errors are regarding self_type, right? That has a PR that is r+
> with comments (https://github.com/servo/rust-bindgen/pull/823) and which I
> am landing in a minute.

FWIW, the nsTArray errors weren't re. self_type. We were just failing to generate the template specialization in function arguments, for some reason... But I guess it's worth a try with the new fixes, thanks for working on them btw!
Attached file 2017-07-24-build-error.log (obsolete) —
I built stylo with the latest bindgen, but got the attached error. There seems to be some struct/trait missing in gecko_bindings. And the nsTArray issue is still there. 

(There are some clang path config issue on try, so no try build link for now.)
Depends on: 1385147
Is there a known way to workaround the current bindgen issue (getting internal compiler error while building stylo)? I was told that using stable rustc for bindgen might work, but it might be hard to maintain two versions of rustc on CI just for linux 32 builds.
Flags: needinfo?(nfitzgerald)
(In reply to Shing Lyu [:shinglyu] from comment #38)
> Is there a known way to workaround the current bindgen issue (getting
> internal compiler error while building stylo)? I was told that using stable
> rustc for bindgen might work, but it might be hard to maintain two versions
> of rustc on CI just for linux 32 builds.

I haven't experienced any rustc ICEs when compiling bindgen nor stylo. I don't think I'm the right person to ask about that.

-----------------

Regarding this bug in general: we've landed fixes for all the bindgen regressions impacting Stylo that I'm aware of.
Flags: needinfo?(nfitzgerald)
(In reply to Shing Lyu [:shinglyu] from comment #38)
> Is there a known way to workaround the current bindgen issue (getting
> internal compiler error while building stylo)? I was told that using stable
> rustc for bindgen might work, but it might be hard to maintain two versions
> of rustc on CI just for linux 32 builds.

Shing, nightly rustc is known to panic when compiling mozilla-central, but this shouldn't an issue in CI automation because I thought we were using stable rustc there.

The nightly rustc panic was reported in bug 1386414. Here is the rustc bug report:

https://github.com/rust-lang/rust/issues/43567
Flags: needinfo?(shing.lyu)
Here's an updated try run to check the latest status here:

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

Builds complete successfully, but Stylo panics at runtime with "already immutably borrowed" inside Servo_StyleSet_AppendStyleSheet.

Full panic backtrace attached.
:bholley suspects this would be another bindgen / struct alignment issue.  :manishearth, is this something you can investigate?
Flags: needinfo?(shing.lyu) → needinfo?(manishearth)
This probably means that raw_data has a bad pointer.

I've had a hard time getting a cross build environment up though, so I can't investigate this more.

I tried running the treeherder binary and I got a crash here:

(gdb) bt
#0  0xf191ff0c in RefPtr<nsDeviceContext>::operator->() const () from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#1  0xf195d15e in nsPresContext::AppUnitsPerDevPixel() const () from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#2  0xf188992b in nsStyleBorder::nsStyleBorder(nsPresContext const*) () from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#3  0xf3aaaadb in style::gecko_properties::_$LT$impl$u20$style..gecko_bindings..structs..root..mozilla..GeckoBorder$GT$::default::hefbd
14373aa50243 () from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#4  0xf3893acc in style::gecko_properties::ComputedValues::default_values::hea0673cbab89e96f ()
   from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#5  0xf38e366f in style::gecko::media_queries::Device::new::hbf18b1e0b1ca2891 ()
   from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#6  0xf37f55aa in style::gecko::data::PerDocumentStyleData::new::hff3a94fa571bd336 ()
   from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#7  0xf35ed71a in Servo_StyleSet_Init () from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#8  0xf18249d1 in mozilla::ServoStyleSet::Init(nsPresContext*, nsBindingManager*) ()
   from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#9  0xf190b40d in mozilla::PresShell::Init(nsIDocument*, nsPresContext*, nsViewManager*, mozilla::StyleSetHandle) ()
   from /home/manishearth/mozilla/treeherder/firefox/libxul.so
#10 0xf077b6a9 in nsDocument::CreateShell(nsPresContext*, nsViewManager*, mozilla::StyleSetHandle) ()
   from /home/manishearth/mozilla/treeherder/firefox/libxul.so


Are the layout tests still passing on linux32?
Flags: needinfo?(manishearth)
I think I am approaching a usable Linux32 build environment.  Hope to have more details soon.
Assignee: shing.lyu → jryans
Okay, by running the struct layout tests in my linux32 environment (with bindings replaced by those generated in linux32 automation), I can see there are a few layout test failures.

:Manishearth helped me isolate one of them related to Maybe<T> on 32-bit:

https://github.com/rust-lang-nursery/rust-bindgen/issues/898
Attached file ./mach test-stylo failures (obsolete) —
:emilio has offered to take a look as well.

Here are the linux32 binding files:

https://queue.taskcluster.net/v1/task/buXEOZMEQFW4St9vPOZhVg/runs/0/artifacts/public/build/target.stylo-bindings.zip

I have attached the failures from running `./mach test-stylo` on linux32.  I assume the test_size_of failures don't really matter (they are different because of pointer size), but the 5 bindgen failures seem relevant.
Flags: needinfo?(emilio+bugs)
So, from those tests, the only layout test that affects us is ComputedTiming. And I don't think that explains the crash described here.

I would look into issues with alignment and what not on AtomicRefCell, or our uses of it, it looks suspiciously similar to https://github.com/servo/servo/pull/16833

I'm trying to get a reduced version of the bad bindings for ComputedTiming too.
Flags: needinfo?(emilio+bugs)
I have extended my linux64 host, linux32 target build environment to work for a full Firefox build (previously it was only enough for the layout tests in Servo).  This should hopefully make it easier to investigate issues and test fixes here.

(It involved a variety of terrible things...  I'll try to describe it in a blog post once we've gotten past the issues here.)
Bug 1390691 covers a set of ABI issues related to struct return types.  With that fixed, regular styling works, but animation things still fail.

The Nullable / Maybe issues :Manishearth helped with above (comment 45) still seems suspect for animations, so I'll keep looking into that.
I filed https://github.com/rust-lang-nursery/rust-bindgen/issues/917 about the alignment issue with Nullable / Maybe types on Linux 32-bit.

We'll either need a bindgen fix or workaround for these types.
Depends on: 1391103
I can confirm via local testing that the patch in bug 1391103 fixes up the animation layout issues on Linux 32-bit, and should be ready to enable build and testing the platform once that lands.
Comment on attachment 8899587 [details]
Bug 1366050 - Build with Stylo for Linux32.

https://reviewboard.mozilla.org/r/170880/#review176050

At this point, mozconfig.stylo only exports LLVM_CONFIG. So I /think/ we can roll its content into ``build/unix/mozconfig.linux``. I'm not 100% sure about that given the state of toolchain build configurations. Best to ask glandium and deal with it as a follow-up, if needed.
Attachment #8899587 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #53)
> Comment on attachment 8899587 [details]
> Bug 1366050 - Build with Stylo for Linux32.
> 
> https://reviewboard.mozilla.org/r/170880/#review176050
> 
> At this point, mozconfig.stylo only exports LLVM_CONFIG. So I /think/ we can
> roll its content into ``build/unix/mozconfig.linux``. I'm not 100% sure
> about that given the state of toolchain build configurations. Best to ask
> glandium and deal with it as a follow-up, if needed.

:glandium, any thoughts about the right way to handle `monconfig.stylo` for the future?  Should it be folded into the platform default mozconfigs like :gps suggests?

It is used on more that just Linux (it applies to most desktop Firefox builds now).
Flags: needinfo?(mh+mozilla)
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/338bc44c5e21
Build with Stylo for Linux32. r=gps
Whatever floats your boat, but I'll note that mozconfig.stylo has the nice properly that if we ever need to change the path, we'll be able to do it in one place, vs. multiple if we kill it. OTOH, we already have multiple locations for other toolchain paths, so, it's only a small benefit.
Flags: needinfo?(mh+mozilla)
Depends on: 1392977
https://hg.mozilla.org/mozilla-central/rev/338bc44c5e21
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
I wrote a blog post about my experience setting up a Linux 32-bit build environment to resolve the issues in this bug:

https://convolv.es/blog/2017/08/25/building-firefox-for-linux-32-bit/
Depends on: 1401093
No longer depends on: 1401093
See Also: → 1401093
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: