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)
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.
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Comment 2•7 years ago
|
||
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.
Reporter | ||
Comment 3•7 years ago
|
||
(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.
Reporter | ||
Comment 4•7 years ago
|
||
(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.
Comment 5•7 years ago
|
||
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
Comment 6•7 years ago
|
||
Err, shouldn't be hard, I mean, ofc
Comment 7•7 years ago
|
||
(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.
Comment 8•7 years ago
|
||
Assigning to Shing because Astley said he would look into this bug.
Assignee: nobody → shing.lyu
Reporter | ||
Comment 9•7 years ago
|
||
(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...
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
(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)
Updated•7 years ago
|
Summary: enable Stylo for 32-bit Linux by default on automation → Build Stylo for 32-bit Linux on automation
Updated•7 years ago
|
No longer blocks: stylo-nightly-build
Updated•7 years ago
|
Comment 12•7 years ago
|
||
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)
Comment 13•7 years ago
|
||
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)
Comment 14•7 years ago
|
||
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)
Comment 15•7 years ago
|
||
More errors after I rebase to bindgen 0.26.1 https://treeherder.mozilla.org/logviewer.html#?job_id=110881911&repo=try&lineNumber=6981
Comment 16•7 years ago
|
||
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
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
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, ...
Assignee | ||
Comment 19•7 years ago
|
||
(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.
Comment 20•7 years ago
|
||
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)
Comment 21•7 years ago
|
||
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)
Comment 22•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
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. :)
Comment 24•7 years ago
|
||
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)
Comment 25•7 years ago
|
||
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)
Comment 26•7 years ago
|
||
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)
Comment 27•7 years ago
|
||
(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.
Comment 28•7 years ago
|
||
(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.
Comment 29•7 years ago
|
||
(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.
Comment 30•7 years ago
|
||
(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.
Comment 31•7 years ago
|
||
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?
Comment 32•7 years ago
|
||
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.
Comment 33•7 years ago
|
||
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)
Comment 34•7 years ago
|
||
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)
Comment 35•7 years ago
|
||
(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.
Comment 36•7 years ago
|
||
(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!
Comment 37•7 years ago
|
||
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.)
Comment 38•7 years ago
|
||
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)
Comment 39•7 years ago
|
||
(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)
Comment 40•7 years ago
|
||
(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)
Assignee | ||
Comment 41•7 years ago
|
||
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.
Assignee | ||
Comment 42•7 years ago
|
||
:bholley suspects this would be another bindgen / struct alignment issue. :manishearth, is this something you can investigate?
Flags: needinfo?(shing.lyu) → needinfo?(manishearth)
Comment 43•7 years ago
|
||
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)
Assignee | ||
Comment 44•7 years ago
|
||
I think I am approaching a usable Linux32 build environment. Hope to have more details soon.
Assignee: shing.lyu → jryans
Assignee | ||
Comment 45•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Attachment #8885101 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889250 -
Attachment is obsolete: true
Assignee | ||
Comment 46•7 years ago
|
||
: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)
Comment 47•7 years ago
|
||
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)
Assignee | ||
Comment 48•7 years ago
|
||
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.)
Assignee | ||
Comment 49•7 years ago
|
||
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.
Assignee | ||
Comment 50•7 years ago
|
||
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.
Assignee | ||
Comment 51•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8894695 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8895608 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 53•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 54•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f4e47e5c7f82b0fc6775414572e8c6b550cc686
Assignee | ||
Comment 55•7 years ago
|
||
(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)
Comment 56•7 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/338bc44c5e21 Build with Stylo for Linux32. r=gps
Comment 57•7 years ago
|
||
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)
Comment 58•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/338bc44c5e21
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 59•7 years ago
|
||
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/
You need to log in
before you can comment on or make changes to this bug.
Description
•