baldrdash: running wasm with Cranelift

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: sunfish, Assigned: bbouvier)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(6 attachments, 14 obsolete attachments)

10.00 KB, patch
luke
: review+
Details | Diff | Splinter Review
17.73 KB, patch
luke
: review+
Details | Diff | Splinter Review
104.92 KB, patch
luke
: review+
sunfish
: review+
Details | Diff | Splinter Review
2.67 MB, patch
Details | Diff | Splinter Review
14.24 KB, patch
mhoye
: review+
Details | Diff | Splinter Review
23.57 KB, patch
luke
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 months ago
Attached is the current "baldrdash" patch, which adds a nightly-only option for running wasm with Cretonne. Cretonne is in development and doesn't yet have all its planned optimization features, however it is able to pass the testsuite on x64. The patch has the following known issues:

 - Currently JS shell builds require the environment variable MOZ_CLANG_PATH to be set to a clang binary. This appears to be needed by the bindgen infrastructure. Is there a better way to handle this?

 - The JS shell builds, but full firefox builds currently fail with "mozbuild.configure.ConfigureError: Cannot include `[...]/build/moz.configure/rust.configure` because it was included already.". It appears rust.configure is being included by both toolkit/moz.configure and js/moz.configure. Is there a way to make it conditional on whether the JS shell is being built?

 - Cretonne depends on the failure crate, which isn't yet in firefox. Is this a problem? If so, it'd be possible to remove this dependency or make it optional.

 - wasmparser.rs currently includes 7.4M of test binaries that will get copied into the tree by the vendoring script. Is that too much?

 - Cretonne depends on target-lexicon (which I also maintain), which is licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license, however crates.io is still using an older SPDX, so target-lexicon currently uses a license-file. I added it to the whitelist in python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary workaround.
(In reply to Dan Gohman [:sunfish] from comment #0)
>  - Cretonne depends on target-lexicon (which I also maintain), which is
> licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license,
> however crates.io is still using an older SPDX, so target-lexicon currently
> uses a license-file. I added it to the whitelist in
> python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary
> workaround.

Is this tracked in a GitHub issue? It seems easy enough to do to have it block this. The alternative would probably be to reach out to licensing people (i.e. mhoye), which might be more effort. (OTOH, I don't know if reaching out to the licensing people would still be needed.)
(Reporter)

Comment 2

10 months ago
(In reply to Till Schneidereit [:till] from comment #1)
> Is this tracked in a GitHub issue? It seems easy enough to do to have it
> block this. The alternative would probably be to reach out to licensing
> people (i.e. mhoye), which might be more effort. (OTOH, I don't know if
> reaching out to the licensing people would still be needed.)

It's being tracked here:

https://github.com/rust-lang-nursery/license-exprs/issues/12

Comment 3

10 months ago
(In reply to Dan Gohman [:sunfish] from comment #0)
> Attached is the current "baldrdash" patch, which adds a nightly-only option
> for running wasm with Cretonne.

A+ name
Component: JavaScript Engine: JIT → Javascript: Web Assembly
(In reply to Dan Gohman [:sunfish] from comment #0)
>  - Currently JS shell builds require the environment variable MOZ_CLANG_PATH
> to be set to a clang binary. This appears to be needed by the bindgen
> infrastructure. Is there a better way to handle this?

We can move the logic for determining where bindgen bits live:

https://searchfox.org/mozilla-central/source/toolkit/moz.configure#666-809

somewhere where JS and Gecko can see it.

>  - The JS shell builds, but full firefox builds currently fail with
> "mozbuild.configure.ConfigureError: Cannot include
> `[...]/build/moz.configure/rust.configure` because it was included
> already.". It appears rust.configure is being included by both
> toolkit/moz.configure and js/moz.configure. Is there a way to make it
> conditional on whether the JS shell is being built?

Can you have JS-only builds without building the shell?  If so, we have a couple of different cases:

- Gecko builds, Rust required
- JS-only builds, shell built, Rust required
- JS-only builds, shell not built, Rust...not required (yet?)?

I think this is doable, but there will be some configure bits to sort out.  I'm assuming we'd like to keep whatever Rust bits the JS engine requires Nightly-only at this point?  Alternatively, do we want to take a hard dependency on Rust in the JS engine now, or wait until ESR 69/70?

>  - Cretonne depends on the failure crate, which isn't yet in firefox. Is
> this a problem? If so, it'd be possible to remove this dependency or make it
> optional.

Assuming the failure crate is reasonable (no huge files, no obvious badness), I don't see why this would be a problem.

>  - wasmparser.rs currently includes 7.4M of test binaries that will get
> copied into the tree by the vendoring script. Is that too much?

I think so.  Is it possible to separate those out into a test-only crate?  I'm not sure if cargo-vendor will pull those in or not...

>  - Cretonne depends on target-lexicon (which I also maintain), which is
> licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license,
> however crates.io is still using an older SPDX, so target-lexicon currently
> uses a license-file. I added it to the whitelist in
> python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary
> workaround.

You can ask for license review from mhoye and we can add that as a permanent exception.

I assume we'll also want some sort of configure flag --enable-cretonne or somesuch, defaulted to on in Nightly and off everywhere else, rather than the hardcoded defines the patch currently has.
(Reporter)

Updated

9 months ago
Depends on: 1476388
(Reporter)

Comment 5

9 months ago
(In reply to Nathan Froyd [:froydnj] from comment #4)
> (In reply to Dan Gohman [:sunfish] from comment #0)
> >  - wasmparser.rs currently includes 7.4M of test binaries that will get
> > copied into the tree by the vendoring script. Is that too much?
> 
> I think so.  Is it possible to separate those out into a test-only crate? 
> I'm not sure if cargo-vendor will pull those in or not...

I've now filed https://github.com/yurydelendik/wasmparser.rs/issues/55 about moving the tests out.

> >  - Cretonne depends on target-lexicon (which I also maintain), which is
> > licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license,
> > however crates.io is still using an older SPDX, so target-lexicon currently
> > uses a license-file. I added it to the whitelist in
> > python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary
> > workaround.
> 
> You can ask for license review from mhoye and we can add that as a permanent
> exception.

I've now filed bug 1476388 to request a license review.
Depends on: 1476423
Depends on: 1476427
(Reporter)

Comment 6

9 months ago
Posted patch baldrdash.patch (obsolete) — Splinter Review
Here's an updated patch. Major changes:

 - Update to the latest Cretonne/Cranelift
 - Merge with trunk, and implement realm switching
 - Add the "Apache-2.0 WITH LLVM-exception" license to the whitelist, now that bug 1476388 is resolved.

Also note that in this patch, as well as the previous one, I've excluded the Cargo.lock and third_part/rust changes from this patch, as they're mechanically generated from "mach vendor rust".
Attachment #8985677 - Attachment is obsolete: true
(Reporter)

Updated

9 months ago
Depends on: 1476798
Alias: cranelift
Summary: baldrdash: running wasm with Cretonne → baldrdash: running wasm with Cranelift
(Reporter)

Comment 7

9 months ago
Posted patch baldrdash.patch (obsolete) — Splinter Review
This version adds Cranelift's license to the about:license page, as requested in bug 1476388.
Attachment #8993114 - Attachment is obsolete: true
Depends on: 1444141
(Assignee)

Comment 8

9 months ago
Posted patch baldrdash.patch (obsolete) — Splinter Review
(rebased and updated to use cranelift 0.17.0)
Attachment #8994362 - Attachment is obsolete: true
(Assignee)

Comment 9

9 months ago
Posted patch baldrdash.patch (obsolete) — Splinter Review
(rebased, without Cargo.lock changes this time)
Attachment #8995990 - Attachment is obsolete: true
(Assignee)

Comment 10

9 months ago
Posted patch 2-valtype.patch (obsolete) — Splinter Review
More changes since the ValType refactoring.
(Assignee)

Comment 11

8 months ago
As agreed on IRC with Dan, I'll take over this bug to get it landed. I'm currently rebasing this patch, updating Cranelift and adapting the new APIs, while ensuring tests still pass.
Assignee: sunfish → bbouvier
Status: NEW → ASSIGNED
(Assignee)

Comment 12

8 months ago
Posted patch wip.patch (obsolete) — Splinter Review
Rolled up wip.

- updated to cranelift 0.20;
- tried using bindgen for the ModuleEnvironment; failed b/o mozilla::Vector and other data structures in there (which use templates etc., which bindgen doesn't seem to support out of the box);
- started to remove all the calls between Spidermonkey and Cranelift; the code and metadata is now stored in a Rust structure that's passed out to C++ afterwards, so code generation entirely happens in C++. (no more need for the Emitter structure either) Did remove a lot of bindings.
Attachment #8996306 - Attachment is obsolete: true
Attachment #8996307 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Alias: cranelift
(Assignee)

Updated

8 months ago
Blocks: cranelift
(Assignee)

Comment 13

8 months ago
Posted patch rolledup.patch (obsolete) — Splinter Review
Rolled up patch (applies cleanly on top of mozilla-inbound 345269b39c6c), with vendoring etc. Tried to simplify a few things, attempting some different designs, etc.

There are cranelift register allocation failures when trying this patch on AngryBots/Tanks, but I'd like to make sure I didn't introduce them first.
Attachment #9005714 - Attachment is obsolete: true
(Assignee)

Comment 14

8 months ago
Posted patch 1.cranelift-nightly.patch (obsolete) — Splinter Review
We'd like to always include Cranelift in Nightly builds, or on demand for non-Nightly. This will increase the binary's size, compilation time (both Spidermonkey and full Gecko); I haven't measured by how much though.

Nathan, does it make sense? Is it the right way to do this with the configure machinery?
Attachment #9006604 - Attachment is obsolete: true
Attachment #9007273 - Flags: review?(nfroyd)
(Assignee)

Comment 15

8 months ago
Adds a --wasm-force-cranelift shell flag, which does nothing at the moment but to set the option in the JS options struct.
Attachment #9007274 - Flags: review?(luke)
(Assignee)

Comment 16

8 months ago
Posted patch 3.tier::cranelift.patch (obsolete) — Splinter Review
Introduces the Tier::Cranelift, which can be used as a second tier and binds it to the wasm-force-cranelift shell flag. With these patches, it will just compile with IonMonkey; later patches will make use of Cranelift is Tier is Tier::Cranelift.
Attachment #9007275 - Flags: review?(luke)
(Assignee)

Comment 17

8 months ago
This is a preparatory patch for the next one, mostly code motion. It introduces a new header called WasmBasicTypes.h, containing types that are then included in WasmTypes.h or bindgen'd for use in Baldrdash.

The changes are the following:
- code motion (for some types)
- the FuncWithTypeId::Kind is extracted to its own enum class
- a new CPU feature detection of SSE4.2 is added to the Assembler class.
Attachment #9007278 - Flags: review?(luke)
(Assignee)

Comment 18

8 months ago
This is part of the main patch (the rest is automatic generated output, license updates and moz.build goo).

Lars, flagged you for feedback since you wrote this code in the first place. If you'd like to review it too, feel free to do so; the more eyes on this big patch, the better.

This builds on all platforms, and passes tests with --wasm-force-cranelift on x64. On other platforms, since SSE2 is marked as not present (see #ifdef in CraneliftStaticEnvironment ctor), the cranelift_compiler_create function call will fail in make_isa(); so ARM / x86 / others will gracefully fail with a Rust error message ("SSE2 is not present"), reported as an OOM (better error reporting will come in subsequent bugs).

My plan is to try to land this as soon as possible, then have many other bugs to make the integration better/faster, rename things, harmonize names/casing in FFIs, etc.
Attachment #9007289 - Flags: review?(sunfish)
Attachment #9007289 - Flags: review?(luke)
Attachment #9007289 - Flags: feedback?(lhansen)
(Assignee)

Comment 19

8 months ago
Posted patch 5b-build-support.patch (obsolete) — Splinter Review
All the build bits, mostly copied from what was in previous patches.
Attachment #9007290 - Flags: review?(nfroyd)
(Assignee)

Comment 20

8 months ago
The output of ./mach vendor rust.

Nathan, I assume this needs to be reviewed/approved? Is there a documented process, if approval is needed?
Flags: needinfo?(nfroyd)
(Assignee)

Comment 21

8 months ago
Mark, this patch contains the license changes that have been discussed in bug 1476388 and bug 1476798. This will be folded with the rest of the patches numbered 5, which introduce the code the new license applies to.
Attachment #9007294 - Flags: review?(mhoye)
Comment on attachment 9007273 [details] [diff] [review]
1.cranelift-nightly.patch

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

I believe this is correct.
Attachment #9007273 - Flags: review?(nfroyd) → review+
Comment on attachment 9007294 [details] [diff] [review]
5d-license-changes.patch

R+, thank you.
Attachment #9007294 - Flags: review?(mhoye) → review+
Comment on attachment 9007290 [details] [diff] [review]
5b-build-support.patch

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

r=me with the RUST_STATIC_LIB_FOR_SHARED_LIB change mentioned below, and assuming there's a reasonable answer to the necessity of the .hgignore change.

::: .hgignore
@@ +43,5 @@
>  _DBG\.OBJ/
>  _OPT\.OBJ/
>  ^js/src/.*-obj/
>  ^js/src/obj-.*/
> +^js/src/wasm/cranelift/target

Please add this to .gitignore as well.

But wait, why is this necessary?  Shouldn't the build be putting the output files in the objdir, not the srcdir?  Or is this an attempt to ease local development for people who just want to `cargo build` cranelift and nothing else?

::: config/rules.mk
@@ +552,5 @@
>  #
>  # PROGRAM = Foo
>  # creates OBJS, links with LIBS to create Foo
>  #
> +$(PROGRAM): $(PROGOBJS) $(RUST_STATIC_LIB_FOR_SHARED_LIB) $(STATIC_LIBS) $(EXTRA_DEPS) $(RESFILE) $(GLOBAL_DEPS) $(call mkdir_deps,$(FINAL_TARGET))

I think it would also be a good idea to rename RUST_STATIC_LIB_FOR_SHARED_LIB at this point, since we're now using Rust static libraries in other places than shared libraries. :)

::: js/src/gdb/moz.build
@@ +46,5 @@
> +
> +# The Rust standard library references libresolv on macOS, so we need to link
> +# it as a workaround. See also bug 1367932.
> +if CONFIG['OS_ARCH'] == 'Darwin':
> +    OS_LIBS += ['-lresolv']

I don't think you have to fix this here, but maybe we should just start linking in dependencies for Rust libraries in the build system itself, rather than scattering this stanza around in a bunch of different places.  Please file a followup bug?
Attachment #9007290 - Flags: review?(nfroyd) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #20)
> Created attachment 9007292 [details] [diff] [review]
> 5c-mach-vendor-rust.patch
> 
> The output of ./mach vendor rust.
> 
> Nathan, I assume this needs to be reviewed/approved? Is there a documented
> process, if approval is needed?

It does need to be approved, by me or by someone who has experience with the crates being modified.  AFAICT, this is just trivially updating `log` and then adding a bunch of cranelift packages.  Assuming that doesn't change, r=me.
Flags: needinfo?(nfroyd)

Updated

8 months ago
Attachment #9007274 - Flags: review?(luke) → review+
Comment on attachment 9007275 [details] [diff] [review]
3.tier::cranelift.patch

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

Question: do we want Ion and Cranelift to be separate tiers or could we instead:
 - rename Tier::Ion to Tier::Optimized
 - have a bool that indicates whether to use Cranelift for the optimized tier

This would make sense under the assumption that most code that is asking about tier doesn't care about the ion vs. cranelift distinction (since they produce code with the same ABI / metadata).  It seems like mostly a ModuleGenerator-specific concern, but I could be wrong.  What's nice is that this isn't a long-lived distinction; once cranelift "ships", it can go away so the bool that decides which to use could be #ifdef CRANELIFT.
Comment on attachment 9007278 [details] [diff] [review]
4.bindgen-d-types.patch

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

Thanks for breaking out these patches.

::: js/src/wasm/WasmBasicTypes.h
@@ +19,5 @@
> +#ifndef wasm_basic_types_h
> +#define wasm_basic_types_h
> +
> +// This header defines basic enums and structs that don't depend on other header files being
> +// included. It is included by "wasm/WasmTypes.h" AND by bindgen when making bindings for Cranelift.

WasmBinaryConstants.h also doesn't #include anything, could it serve this purpose instead?  It currently contains some structs that maybe trip up bindgen (OpBytes), so perhaps those could be moved to WasmTypes.h?  Also, the "Binary" in "WasmBinaryConstants.h" seems superfluous/confusing, in retrospect, perhaps rename to just WasmConstants.h (which makes it's role here a bit more clear)?
Attachment #9007278 - Flags: review?(luke) → review+
Comment on attachment 9007289 [details] [diff] [review]
5a-cranelift-integration.patch

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

Seems OK to me, for a first cut.

::: js/src/wasm/cranelift/src/lib.rs
@@ +93,5 @@
> +        error!("Cranelift compilation error: {}\n{}", e, compiler);
> +        return false;
> +    };
> +
> +    // TODO(bbouvier) if destroy is called while one of these objects is alive, you're gonna have a

"going to have"
Attachment #9007289 - Flags: feedback?(lhansen) → feedback+
(Assignee)

Comment 29

7 months ago
(In reply to Luke Wagner [:luke] from comment #26)
> Comment on attachment 9007275 [details] [diff] [review]
> 3.tier::cranelift.patch
> 
> Review of attachment 9007275 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Question: do we want Ion and Cranelift to be separate tiers or could we
> instead:
>  - rename Tier::Ion to Tier::Optimized
>  - have a bool that indicates whether to use Cranelift for the optimized tier
> 
> This would make sense under the assumption that most code that is asking
> about tier doesn't care about the ion vs. cranelift distinction (since they
> produce code with the same ABI / metadata).  It seems like mostly a
> ModuleGenerator-specific concern, but I could be wrong.  What's nice is that
> this isn't a long-lived distinction; once cranelift "ships", it can go away
> so the bool that decides which to use could be #ifdef CRANELIFT.

That's how it was done in the patch previously, but there was one thing that troubled me: Tier::Serialized == Tier::Ion (and thus would be equal to Tier::Cranelift), so just having Tier::Optimized could mean that Cranelift code gets serialized/deserialized. Since Ion is still generating better code at the moment, I thought not fusing the two would prevent performance issues for people testing cranelift in a browser build.

Also, we could have different heuristics for tiering baseline with cranelift, which is the only other interest.

Reverting to the previous situation would be just a bit bad in my opinion, but I don't care too strongly here.
Comment on attachment 9007273 [details] [diff] [review]
1.cranelift-nightly.patch

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

::: js/moz.configure
@@ +487,5 @@
>            help='Enable Cranelift code generator for wasm')
>  
> +@depends('--enable-cranelift', milestone)
> +def enable_cranelift(value, milestone):
> +    return value or milestone.is_nightly

This makes --disable-cranelift *not* work on central. We avoid doing things like this. If you want the default to be enabled for central, use default=milestone.is_nightly on the js_option().
Attachment #9007273 - Flags: review+ → review-
Comment on attachment 9007290 [details] [diff] [review]
5b-build-support.patch

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

::: js/src/wasm/cranelift/moz.build
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +RustLibrary('baldrdash')

Adding if CONFIG['OS_ARCH'] == 'Darwin': OS_LIBS += ['-lresolv'] here should make USE_LIBS += ['baldrdash'] imply it, removing the need to it in all the other files.
(Assignee)

Comment 32

7 months ago
Posted patch 1.cranelift-nightly.patch (obsolete) — Splinter Review
Thanks! (Also incorporated the change you suggested on the moz.build files in the moz.build patch)
Attachment #9007273 - Attachment is obsolete: true
Attachment #9007771 - Flags: review?(mh+mozilla)
(Assignee)

Updated

7 months ago
Keywords: leave-open

Comment 33

7 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9347f5a5365d
Add a --wasm-force-cranelift flag to the shell; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7954f956259
Move wasm types shared with Cranelift in a WasmBinaryConstants.h; r=luke
(Assignee)

Comment 34

7 months ago
Posted patch 1.cranelift-nightly.patch (obsolete) — Splinter Review
Attachment #9007771 - Attachment is obsolete: true
Attachment #9007771 - Flags: review?(mh+mozilla)
Attachment #9007795 - Flags: review?(mh+mozilla)
(Assignee)

Updated

7 months ago
Blocks: 1490048
Comment on attachment 9007275 [details] [diff] [review]
3.tier::cranelift.patch

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

Ah, Tier::Serialized is a good point.  Moreover, when cranelift ships, we can delete Tier::Ion and so this is only a transitional state.
Attachment #9007275 - Flags: review?(luke) → review+

Updated

7 months ago
Attachment #9007289 - Flags: review?(luke) → review+
Attachment #9007795 - Flags: review?(mh+mozilla) → review+
(Reporter)

Comment 37

7 months ago
Comment on attachment 9007289 [details] [diff] [review]
5a-cranelift-integration.patch

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

Looks good to me :-).
Attachment #9007289 - Flags: review?(sunfish) → review+
(Assignee)

Comment 38

7 months ago
Thanks all for the reviews!

Alright, some status update here: I'm fighting with the build gods at the moment, and have succeeded at solving a few issues. Another bunch of issues needs to be solved before landing.

- build issues: to be linked with libmozjs, baldrdash must have "staticlib" (static library) as its crate-type; to be linked with libxul, it must be a "rlib" (rust object lib) due to the way we link rust code in gecko. These requirements are conflicting: in particular, rust static libs are built with LTO, while rlib can't be. The solution, as suggested by Nathan, was to split baldrdash into two libs:
  - baldrdash, used by Gecko (and baldrdash_lib below), which is an rlib
  - baldrdash_lib, used only by the shell, and defined as a static library, which just references baldrdash.
Thus, baldrdash_lib needs its own Cargo.toml file, and needs to reference the source of baldrdash in its dependencies, so its root directory must be a parent of the baldrdash lib. As a matter of fact, all the code that was in /js/src/wasm/cranelift is now in /js/src/wasm/cranelift/lib; baldrdash_lib's Cargo.toml and dummy lib.rs files live by themselves in /js/src/wasm/cranelift. (Will upload a patch for all the build changes) Happy to change the `lib` name to something else, if people have better suggestions.

- more build issues: the hazard analysis builds don't reference libclang, which is needed by bindgen. Nathan and Steve gave me useful help on IRC to fix this.

- even more build issues: Android builds need more metadata for bindgen. Emilio on irc pointed what needed to be done for the style system, which should be helpful.

- rust-bindgen doesn't support bindings generation for C++ methods attributes on windows: I'll revert to function calls that take what was the value of "this" as the first argument, on all platforms.

- in Firefox, env_logger::init() was called by JS after it was already initialized in Gecko; replaced by a call to try_init() which doesn't fail if a logger has already been set.
If you create a jsrust staticlib library similar to gkrust, you can then have baldrdash as a rlib linked into either gtkrust or jsrust.
I should add that linking baldrdash into gkrust is probably not something we can do, not as long as we support building firefox with spidermonkey as a shared library.
(Assignee)

Updated

7 months ago
Depends on: 1490948
(In reply to Mike Hommey [:glandium] from comment #40)
> I should add that linking baldrdash into gkrust is probably not something we
> can do, not as long as we support building firefox with spidermonkey as a
> shared library.

Why do we need to support that?

That is, is there a reason why we can't aim for a setup where SpiderMonkey-relevant Rust code is built as jsrust in standalone SpiderMonkey builds and as part of gkrust when SpiderMonkey is built as part of Firefox without support for SpiderMonkey as a shared library in Firefox?
Flags: needinfo?(mh+mozilla)
(In reply to Henri Sivonen (:hsivonen) from comment #41)
> (In reply to Mike Hommey [:glandium] from comment #40)
> > I should add that linking baldrdash into gkrust is probably not something we
> > can do, not as long as we support building firefox with spidermonkey as a
> > shared library.
> 
> Why do we need to support that?

People have been using that to make incremental builds faster. Now that there are fast linking options, this might be less necessary, but it's not a given.

> That is, is there a reason why we can't aim for a setup where
> SpiderMonkey-relevant Rust code is built as jsrust in standalone
> SpiderMonkey builds and as part of gkrust when SpiderMonkey is built as part
> of Firefox without support for SpiderMonkey as a shared library in Firefox?

As mentioned in another bug, there's actually another problem, that e.g. the js shell, that is still built for Firefox, is not linking gecko for good reason, and still needs the spidermonkey-specific rust bits.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #42)
> (In reply to Henri Sivonen (:hsivonen) from comment #41)
> > (In reply to Mike Hommey [:glandium] from comment #40)
> > > I should add that linking baldrdash into gkrust is probably not something we
> > > can do, not as long as we support building firefox with spidermonkey as a
> > > shared library.
> > 
> > Why do we need to support that?
> 
> People have been using that to make incremental builds faster. Now that
> there are fast linking options, this might be less necessary, but it's not a
> given.

That doesn't seems like a great reason to prevent SpiderMonkey from reusing code that Gecko has access to.

> > That is, is there a reason why we can't aim for a setup where
> > SpiderMonkey-relevant Rust code is built as jsrust in standalone
> > SpiderMonkey builds and as part of gkrust when SpiderMonkey is built as part
> > of Firefox without support for SpiderMonkey as a shared library in Firefox?
> 
> As mentioned in another bug, there's actually another problem, that e.g. the
> js shell, that is still built for Firefox, is not linking gecko for good
> reason, and still needs the spidermonkey-specific rust bits.

I looked at a couple of obvious dependencies and didn't find a discussion of js shell issues. Which bug do you mean?
Flags: needinfo?(mh+mozilla)
bug 1490948, I think, but maybe the discussion was only on irc.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #44)
> bug 1490948, I think, but maybe the discussion was only on irc.

Does anyone remember the details? (Asking in order to understand the issues facing bug 1490601.)
(Assignee)

Comment 46

7 months ago
Sure, will report there, since I've moved all the build work in this bug.
(Assignee)

Updated

7 months ago
Blocks: 1491770
(Assignee)

Comment 47

7 months ago
Posted patch 3.new-tier.patch (obsolete) — Splinter Review
Just wanted a double-check of this patch after the serialization tests landed.

Mostly the same, except we need to explicitly check that the serialized tier isn't the second tier before calling the serialization listener.
Attachment #9007275 - Attachment is obsolete: true
Attachment #9011768 - Flags: review?(luke)
(In reply to Benjamin Bouvier [:bbouvier] from comment #29)
> > Question: do we want Ion and Cranelift to be separate tiers or could we
> > instead:
> >  - rename Tier::Ion to Tier::Optimized
> >  - have a bool that indicates whether to use Cranelift for the optimized tier

> That's how it was done in the patch previously, but there was one thing that
> troubled me: Tier::Serialized == Tier::Ion (and thus would be equal to
> Tier::Cranelift), so just having Tier::Optimized could mean that Cranelift
> code gets serialized/deserialized. Since Ion is still generating better code
> at the moment, I thought not fusing the two would prevent performance issues
> for people testing cranelift in a browser build.

Actually, thinking about this a bit more, it seems totally fine (and beneficial for testing purposes) to serialize cranelift code.  First of all, it'll only get serialized when the force-cranelift option is set which will only be set for testing purposes.  Second, even if someone sets it and caches some suboptimal cranelift code (before we enable cranelift by default), the code is build-id keyed so as soon as the browser updates we'll compile new code.  So I don't think there is any reason to distinguish Tier::Ion from Tier::Cranelift for serialization purposes.

> Also, we could have different heuristics for tiering baseline with
> cranelift, which is the only other interest.

It's probably fine to just use the Ion constants and then tweak those constants based on re-measurements before cranelift release.

So based on that, how about having just Tier::Baseline and Tier::Optimized?
(Assignee)

Comment 49

7 months ago
Here's the patch, which is a conceptual regression I think (more conditions to check to know if we're compiling for Cranelift, meaningless OptimizedBackend enum field in more cases in the CompilerEnv, etc.), but, oh well, I don't want to fight over this.
Attachment #9011768 - Attachment is obsolete: true
Attachment #9011768 - Flags: review?(luke)
Attachment #9012112 - Flags: review?(luke)
Comment on attachment 9012112 [details] [diff] [review]
3.new-switch.patch

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

Looks great!
Attachment #9012112 - Flags: review?(luke) → review+

Comment 51

7 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df169d72b50
Introduce a new switch to determine if we should compile with Cranelift; r=luke

Comment 54

7 months ago
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1c85d872cc
Backed out changeset 7df169d72b50 for assertion failures on env.optimizedBackend() == OptimizedBackend::Ion. CLOSED TREE
(Assignee)

Comment 55

7 months ago
Duh, optimized build + uninitialized memory in !ENABLE_WASM_CRANELIFT build = kaboom. Thanks for the backout.
Flags: needinfo?(bbouvier)

Comment 56

7 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2456b585ef87
Introduce a new switch to determine if we should compile with Cranelift; r=luke
(Assignee)

Comment 57

7 months ago
Comment on attachment 9007290 [details] [diff] [review]
5b-build-support.patch

This work has moved to bug 1490948.
Attachment #9007290 - Attachment is obsolete: true
(Assignee)

Comment 58

7 months ago
Comment on attachment 9007795 [details] [diff] [review]
1.cranelift-nightly.patch

This work has moved to bug 1490948.
Attachment #9007795 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Keywords: leave-open

Comment 60

7 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7864f125bbcd
Integrate Cranelift in Spidermonkey; r=sunfish, r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a644dda1dc3
Vendor in Rust dependencies for Cranelift; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/66a1434be89e
License changes; r=mhoye
(Assignee)

Comment 61

7 months ago
Last try build went all green (except for tier2 Btup, to be fixed in bug 1494139): https://treeherder.mozilla.org/#/jobs?repo=try&revision=282c9c7435616db59cd4ad672ea876c883ea3333

Comment 62

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7864f125bbcd
https://hg.mozilla.org/mozilla-central/rev/8a644dda1dc3
https://hg.mozilla.org/mozilla-central/rev/66a1434be89e
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Hey all,

I'm trying to compile Firefox for the first time on my new MacBook, and am running into the following error:
 9:45.75    Compiling baldrdash v0.1.0 (/Users/bwinton/Programming/mozilla/central/js/src/wasm/cranelift)
 9:46.99 error: failed to run custom build command for `baldrdash v0.1.0 (/Users/bwinton/Programming/mozilla/central/js/src/wasm/cranelift)`
 9:46.99 process didn't exit successfully: `/Users/bwinton/Programming/mozilla/central/obj-x86_64-apple-darwin18.0.0/toolkit/library/release/build/baldrdash-4511ac70b523b040/build-script-build` (exit code: 101)
 9:46.99 --- stdout
 9:46.99 cargo:rerun-if-changed=baldrapi.h
 9:46.99 cargo:rerun-if-changed=/Users/bwinton/Programming/mozilla/central/obj-x86_64-apple-darwin18.0.0/js/src/rust/extra-bindgen-flags
 9:46.99 --- stderr
 9:47.00 /usr/local/Cellar/llvm/7.0.0/lib/clang/7.0.0/include/inttypes.h:30:15: fatal error: 'inttypes.h' file not found
 9:47.00 /usr/local/Cellar/llvm/7.0.0/lib/clang/7.0.0/include/inttypes.h:30:15: fatal error: 'inttypes.h' file not found, err: true
 9:47.00 thread 'main' panicked at 'Unable to generate baldrapi.h bindings: ()', libcore/result.rs:1009:5
 9:47.00 stack backtrace:
 9:47.00    0:        0x105b64e0f - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::heb6afa683932dedf
 9:47.00    1:        0x105b44c7d - std::sys_common::backtrace::print::h766ca70084cc68f0
 9:47.00    2:        0x105b6dae3 - std::panicking::default_hook::{{closure}}::hf3402f90591bf7bb
 9:47.00    3:        0x105b6d86c - std::panicking::default_hook::hb3b7f3a59e2ecd5a
 9:47.00    4:        0x105b6e217 - std::panicking::rust_panic_with_hook::h359374823d95520e
 9:47.00    5:        0x105b6dd7c - std::panicking::continue_panic_fmt::hf199d5eb60536380
 9:47.00    6:        0x105b6dc68 - rust_begin_unwind
 9:47.00    7:        0x105bd20f1 - core::panicking::panic_fmt::he16a30f6c103fffa
 9:47.00    8:        0x104ee97be - core::result::unwrap_failed::h9d03696a608dbad4
 9:47.00    9:        0x104ef0fd5 - build_script_build::main::h0a86b4b58e0c8b60
 9:47.00   10:        0x104ee99f5 - std::rt::lang_start::{{closure}}::he5e0228125c8f22e
 9:47.00   11:        0x105b6dbe7 - std::panicking::try::do_call::h2bd103f24173fc3c
 9:47.00   12:        0x105b7b5fe - __rust_maybe_catch_panic
 9:47.00   13:        0x105b5192c - std::rt::lang_start_internal::h6fb67bdf1de01d4a
 9:47.00   14:        0x104ee99e7 - std::rt::lang_start::h0f8cdf02aa015a6e

It seems like it might be related to this bug, so I wonder if there's something in the bootstrap.py or build setup instructions that needs to be changed to get it compiling?

Thanks!
(Oh, I should probably also mention that I'm on macOS Mojave, which could very well be the problem.)
Chiming in that that I'm getting identical output to Blake's after the update to macOS Mojave.
You are both running into bug 1487522.
Wrong bug number? "Request for Jenkins X VA"
Flags: needinfo?(nfroyd)
Whoops, bug 1487552.  There we go.
Flags: needinfo?(nfroyd)
Depends on: 1498966
Depends on: 1505559
You need to log in before you can comment on or make changes to this bug.