This is a meta bug for moving the Rust bindings to SpiderMonkey (rust-mozjs) into mozilla-central and everything that entails. For background discussion, see this thread on dev-tech-js-engine-internals: https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine.internals/fVLDl-QWWW0 +CC folks from that thread.

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #7) > Created attachment 8795546 [details] [diff] [review] > Part 6: Vendor third party rust crates used by mozjs_sys and SpiderMonkey > Rust bindings This vendoring doesn't seem to be working correctly in the tackcluster tasks added in part 5. AFAICT, it is still trying to hit the network (and failing in automation) when resolving dependencies. In the logs for the task[0] from the try push[1], we get: > + cargo build --verbose --features debugmozjs > (B Updating(B registry `https://github.com/rust-lang/crates.io-index` Ted, any idea why the top level .cargo/config might not be respected here? [0] https://tools.taskcluster.net/task-inspector/#AUmD6BTUTE6lm084AWnItw/0 [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8d11ca14e73&group_state=expanded

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #8) > (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #7) > > Created attachment 8795546 [details] [diff] [review] > > Part 6: Vendor third party rust crates used by mozjs_sys and SpiderMonkey > > Rust bindings > > This vendoring doesn't seem to be working correctly in the tackcluster tasks > added in part 5. AFAICT, it is still trying to hit the network (and failing > in automation) when resolving dependencies. > > In the logs for the task[0] from the try push[1], we get: > > > + cargo build --verbose --features debugmozjs > > (B Updating(B registry `https://github.com/rust-lang/crates.io-index` > > Ted, any idea why the top level .cargo/config might not be respected here? We don't have a .cargo/config in the topsrcdir, we write it out to $objdir/.cargo/config: https://dxr.mozilla.org/mozilla-central/rev/66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd/moz.build#87 This only affects cargo invocations run from the objdir. Also that task isn't running configure at all, so it wouldn't be generating that file. You could have the task copy the source file to .cargo/config in any directory that's an ancestor of the directory where you run cargo: https://dxr.mozilla.org/mozilla-central/source/.cargo/config.in You'd just need to s/@top_srcdir@/$topsrcdir/ on it. Also, if you want to ensure that your tasks will fail if cargo would hit the network you can run `cargo build --frozen`.

The last patch missed the taskcluster script.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7814b0c94b9a&selectedJob=29068018

Comment on attachment 8800469 [details] [diff] [review] Part 0: Vendor third party crates needed for the mozjs-sys crate Review of attachment 8800469 [details] [diff] [review]: ----------------------------------------------------------------- For posterity and clarification: this code isn't actually shipping anywhere; we're importing this so we can turn on automation builds that test Rust bindings for spidermonkey. So I don't have a problem with all of this code landing. Presumably if we start integrating all of this into Firefox, we can have a conversation about that integration when the time comes (size hit, etc.).

For the record, these two patches only build the "mozjs_sys" crate, which does *not* include any of the rust bindings to spidermonkey. It is basically just building spidermonkey from cargo. This is not shipping in Firefox, and is about putting a damper on breakage.

^ yes what froydnj said :)

Comment on attachment 8800471 [details] [diff] [review] Part 1: Turn js/src into the mozjs-sys crate Review of attachment 8800471 [details] [diff] [review]: ----------------------------------------------------------------- I think I understand enough to r+ with a clear conscience. ::: taskcluster/scripts/builder/build-sm-mozjs-crate.sh @@ +5,5 @@ > +source $(dirname $0)/sm-tooltool-config.sh > + > +cd "$SRCDIR/.cargo" > +cp config.in config > +sed -i -- 's|@top_srcdir@|'"$SRCDIR"'|g' config Hm, it feels weird to be copying and then editing (makes me worry about the script failing midway and leaving the unreplaced file). Why not replace the cp; sed with: sed -e "s|@top_srcdir@|$SRCDIR|" < config.in > config or heck, you could replace the cat too: sed -e "s|@top_srcdir@|$SRCDIR|" < config.in | tee config

Pushed by nfitzgerald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/711480e99c8c Part 0: Vendor third party crates needed for the mozjs-sys crate; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2bcc67e907 Part 1: Turn js/src into the mozjs-sys crate; r=sfink

Pushed by nfitzgerald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a15521c1e84f Part 2: Fix SM(pkg) bustage; r=Waldo

https://hg.mozilla.org/mozilla-central/rev/711480e99c8c https://hg.mozilla.org/mozilla-central/rev/7c2bcc67e907 https://hg.mozilla.org/mozilla-central/rev/a15521c1e84f

Are there plans to make the vendored third-party crates (pkg-config, etc.) be part of the gkrust dependency tree? Because right now running |mach vendor rust| on a vanilla mozilla-central tree results in the removal of those third-party crates, which I presume is undesirable.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21) > Are there plans to make the vendored third-party crates (pkg-config, etc.) > be part of the gkrust dependency tree? Because right now running |mach > vendor rust| on a vanilla mozilla-central tree results in the removal of > those third-party crates, which I presume is undesirable. What is gkrust? IIRC, the initial patches here landed before `mach vendor rust` existed, so I'm not really sure how that stuff works or if we need to do more integration of SpiderMonkey's crate usage with that system. Would love some guidance. Maybe Ted or Nathan know?

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #22) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21) > > Are there plans to make the vendored third-party crates (pkg-config, etc.) > > be part of the gkrust dependency tree? Because right now running |mach > > vendor rust| on a vanilla mozilla-central tree results in the removal of > > those third-party crates, which I presume is undesirable. > > What is gkrust? gkrust is the top-level crate that gets linked into libxul: https://hg.mozilla.org/mozilla-central/file/tip/toolkit/library/rust/Cargo.toml > IIRC, the initial patches here landed before `mach vendor rust` existed, so > I'm not really sure how that stuff works or if we need to do more > integration of SpiderMonkey's crate usage with that system. Would love some > guidance. Maybe Ted or Nathan know? If SpiderMonkey has an appropriate top-level crate, all we should need to do is add it here: http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/vendor_rust.py#77 and |mach vendor rust| will pick it up automagically.

Right now, the "mozjs_sys" crate builds js/src but without any actual Rust FFI bindings. I'm working on improving bindgen and rebasing the Rust-y wrappers onto m-c tip. The FFI bindings generated by bindgen and the wrappers on top of the raw FFI form a new crate "js". That *will* be the top level crate, but it isn't ready yet. So I guess the "mozjs_sys" crate is our temporary top level crate. I can add that crate to the list and replace it with the "js" crate once that lands.

Thanks! FYI the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1324462 also touches the same code.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea6dd08eb1fd089cc6456cd9fc6b4ba7c4707fa2

Comment on attachment 8820350 [details] [diff] [review] Add the "mozjs_sys" crate as a root crate Review of attachment 8820350 [details] [diff] [review]: ----------------------------------------------------------------- Did you make sure that |mach vendor rust| on a current tree with your changes is more-or-less idempotent? I doubt libc/libz-sys have been updated recently, but it'd be good to make sure. r=me with that.

It's idempotent only when both this patch and the patch in bug 1324462 are applied. Either patch by itself is "more idempotent" than the current state of affairs, but both patches are needed to make it fully idempotent. (Nathan, feel free to steal the review on bug 1324462, btw - I'm not sure who's supposed to be reviewing what when it comes to the rust build stuff).

Pushed by nfitzgerald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a89365ded0b Add the "mozjs_sys" crate as a root crate; r=froydnj

Quick update for those following along at home: * After a lot of trial, error, and creduce, bindgen can now generate correct-enough Rust FFI bindings for all of SpiderMonkey's public API that I can successfully compile the js bindings crate and link it with SpiderMonkey. (The crowd goes wild) * When I run the js crate's tests, the callback.rs test promptly segfaults. (The crowd falls silent) I am a bit confused what is going on with this test. I am underneath a Rust `lazy_static!` to create the global parent JSContext. This calls JS::detail::InitWithFailureDiagnostic, which calls mozilla::TimeStamp::ProcessCreation, from which I end up in ld.so, which does some runtime sse detection/resolution stuff (maybe due to jumping to a bad pointer?? because this is when I start to lose useful stack traces) and from there jump to r11, but r11 is 0x0 :-/ > (rr) si > 0x00007f00403655e0 in _dl_runtime_resolve_sse_vex () from /lib64/ld-linux-x86-64.so.2 > 3: x/5i $pc > => 0x7f00403655e0 <_dl_runtime_resolve_sse_vex+320>: bnd jmpq *%r11 > 0x7f00403655e4: nopw %cs:0x0(%rax,%rax,1) > 0x7f00403655ee: xchg %ax,%ax > 0x7f00403655f0 <_dl_cache_libcmp>: movsbl (%rdi),%eax > 0x7f00403655f3 <_dl_cache_libcmp+3>: movsbl (%rsi),%edx > (rr) p $r11 > $5 = 0 > (rr) si > 0x0000000000000000 in ?? () > 3: x/5i $pc > => 0x0: <error: Cannot access memory at address 0x0> And here is my last useful stack trace, which is right before calling mozilla::TimeStamp::ProcessCreation: > (rr) bt > #0 0x000055befb010107 in JS::detail::InitWithFailureDiagnostic (isDebugBuild=<optimized out>) > at /home/fitzgen/mozilla-central/js/src/vm/Initialization.cpp:89 > #1 0x000055befb003f78 in js::rust::{{impl}}::new::{{impl}}::deref::__static_ref_initialize () > at /home/fitzgen/mozilla-central/js/rust/src/rust.rs:92 > #2 lazy_static::lazy::{{impl}}::get::{{closure}}<js::rust::{{impl}}::new::TopContext,fn() -> js::rust::{{impl}}::new::TopContext> () > at /home/fitzgen/.cargo/registry/src/github.com-1ecc6299db9ec823/lazy_static-0.2.2/src/lazy.rs:16 > #3 0x000055befb0033a2 in std::sync::once::{{impl}}::call_once::{{closure}}<closure> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/sync/once.rs:212 > #4 0x000055befbd82e13 in std::sync::once::Once::call_inner () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/sync/once.rs:288 > #5 0x000055befb003300 in std::sync::once::Once::call_once<closure> ( > self=0x55befcea6790 <_$LT$js..rust..Runtime..new..PARENT$u20$as$u20$core..ops..Deref$GT$::deref::__stability::LAZY::h60e9d6810455d076+8>, f=...) > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/sync/once.rs:212 > #6 0x000055befb00bbf6 in lazy_static::lazy::Lazy<js::rust::{{impl}}::new::TopContext>::get<js::rust::{{impl}}::new::TopContext,fn() -> js::rust::{{impl}}::new::TopContext> (self=<optimized out>) > at /home/fitzgen/.cargo/registry/src/github.com-1ecc6299db9ec823/lazy_static-0.2.2/src/lazy.rs:15 > #7 js::rust::{{impl}}::new::{{impl}}::deref::__stability () > at /home/fitzgen/mozilla-central/js/rust/src/rust.rs:92 > #8 js::rust::{{impl}}::new::{{impl}}::deref (self=0x55befbdb95a4) > at /home/fitzgen/mozilla-central/js/rust/src/rust.rs:92 > #9 0x000055befb00a5c7 in js::rust::Runtime::new () > at /home/fitzgen/mozilla-central/js/rust/src/rust.rs:115 > #10 0x000055befb00099c in callback::callback () > at /home/fitzgen/mozilla-central/js/rust/tests/callback.rs:28 > #11 0x000055befbd56d2f in test::run_test::{{closure}} () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libtest/lib.rs:1366 > #12 test::{{impl}}::call_box<(),closure> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libtest/lib.rs:140 > #13 0x000055befbd8fa9b in panic_unwind::__rust_maybe_catch_panic () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libpanic_unwind/lib.rs:98 > #14 0x000055befbd4b11b in std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:436 > #15 std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panic.rs:361 > #16 test::run_test::run_test_inner::{{closure}} () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libtest/lib.rs:1311 > #17 std::panic::{{impl}}::call_once<(),closure> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panic.rs:296 > #18 std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:460 > #19 0x000055befbd8fa9b in panic_unwind::__rust_maybe_catch_panic () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libpanic_unwind/lib.rs:98 > #20 0x000055befbd51d47 in std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:436 > #21 std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panic.rs:361 > #22 std::thread::{{impl}}::spawn::{{closure}}<closure,()> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/thread/mod.rs:357 > #23 alloc::boxed::{{impl}}::call_box<(),closure> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/liballoc/boxed.rs:605 > #24 0x000055befbd879f5 in alloc::boxed::{{impl}}::call_once<(),()> () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/liballoc/boxed.rs:615 > #25 std::sys_common::thread::start_thread () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/sys_common/thread.rs:21 > #26 std::sys::imp::thread::{{impl}}::new::thread_start () > at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/sys/unix/thread.rs:84 > #27 0x00007f003f3206ca in start_thread () from /lib64/libpthread.so.0 > #28 0x00007f003ee43f7f in clone () from /lib64/libc.so.6 Additionally, I feel like maybe I am not getting all the debugging symbols here. A lot of stepping is funky and symbols optimized out, despite a debug cargo build and "plaindebug" autospider.py variant.

One question Nick, does the InitWithFailureDiagnostic function execute correctly? i.e., is the symbol correct, and you enter there with the registers properly set up? That <optimized out> looks suspicious. Note that the js crate is releas by default, and has a debugmozjs feature, are you building with that? May that explain the failure?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #33) > One question Nick, does the InitWithFailureDiagnostic function execute > correctly? i.e., is the symbol correct, and you enter there with the > registers properly set up? That <optimized out> looks suspicious. Investigating. > Note that the js crate is releas by default, and has a debugmozjs feature, > are you building with that? May that explain the failure? When you say "release by default" you mean that it is SpiderMonkey that is built without DEBUG by default, right? I don't think you can make `cargo build` imply the `--release` flag can you? Anyways, I am building with `cargo build --features debugmozjs`. It's worth noting that I overhauled a lot of the build glue between cargo and spidermonkey, too.

Aha, I wasn't linking mozglue! I think doing so should fix these crashes.

Yes, that did fix those crashes, and the js crate's hand written tests pass! But now getting a few assertion failure's in the size and alignment tests that bindgen emits into the bindings. Progress!

All of bindgen's layout, size, and alignment tests are passing, as well as the Rust API's integration tests! \o/ This currently is using a python script to configure and run bindings generation because that was the state of the art approach when I began working on this. Since then, it has become clear that using build.rs to generate the bindings is a superior approach. I need to port the bindings generation script over to build.rs. This also needs to be rebased, since I think my m-c checkout is at least a month old or so. Furthermore, I need to split this one huge commit up into nice little parts. Finally, there has been a few PRs landing in the servo/rust-mozjs repository that need to be merged into this. I don't suspect this will be too difficult but a few bits of JSAPI have changed enough that they are probably broken and there are probably conflicts. I think this can be follow up work, and doesn't need to block this patch series from landing.

Rebased once again. This rebase caught the recent JSContext/JSRuntime refactoring, and so I had to ensure that the parent context was in its own thread. Unfortunately, there are new uses of C++ features we don't understand, which is causing bindgen to generate incorrect layouts, and tests to fail assertions: > failures: > > ---- jsapi::root::__bindgen_test_layout_UniquePtr_instantiation_59710 stdout ---- > thread 'jsapi::root::__bindgen_test_layout_UniquePtr_instantiation_59710' panicked at 'assertion failed: `(left == right)` (left: `1`, right: `8`): Size of template specialization: root :: mozilla :: UniquePtr', src/jsapi_linux_64.rs:5846 > > ---- jsapi::root::__bindgen_test_layout_PersistentRooted_instantiation_56262 stdout ---- > thread 'jsapi::root::__bindgen_test_layout_PersistentRooted_instantiation_56262' panicked at 'assertion failed: `(left == right)` (left: `32`, right: `40`): Size of template specialization: root :: JS :: PersistentRooted < * mut :: std :: os :: raw :: c_void >', src/jsapi_linux_64.rs:5789 > note: Run with `RUST_BACKTRACE=1` for a backtrace. > > ---- jsapi::root::bindgen_test_layout_JSErrorReport stdout ---- > thread 'jsapi::root::bindgen_test_layout_JSErrorReport' panicked at 'assertion failed: `(left == right)` (left: `60`, right: `64`): Alignment of field: JSErrorReport::flags', src/jsapi_linux_64.rs:3496 > > > failures: > jsapi::root::__bindgen_test_layout_PersistentRooted_instantiation_56262 > jsapi::root::__bindgen_test_layout_UniquePtr_instantiation_59710 > jsapi::root::bindgen_test_layout_JSErrorReport > > test result: FAILED. 95 passed; 3 failed; 0 ignored; 0 measured

Ok, rebased + using bindgen in build.rs instead of running it via a python script. TODO: * Cut a new release of bindgen with the features necessary to support SpiderMonkey bindings. * Split this up into small, logical commits.

Comment on attachment 8800469 [details] [diff] [review] Part 0: Vendor third party crates needed for the mozjs-sys crate This landed.

Comment on attachment 8800471 [details] [diff] [review] Part 1: Turn js/src into the mozjs-sys crate This landed.

Comment on attachment 8820350 [details] [diff] [review] Add the "mozjs_sys" crate as a root crate This landed.

Zero-sized base classes are a particular pain point for bindgen. When used as a base class they can be truly zero sized, but when used directly they have to have a byte inserted to enable C++'s distinct-objects-have-distinct-addresses rule. Bindgen could generate two different struct definitions for such cases, but then users need to know which to use at which time and its simpler to just avoid zero sized base classes.

They were previously using duplicate definitions and this DRYs them up. This is needed because bindgen can't understand `mozilla::Conditional`, and so we want to replace `MaybeWrapped` with something a little simpler when doing bindings generation, and its easier if we don't have to repeat our desired replacement as well.

This is not a new external Rust crate dependency for m-c since Servo already depends on `num_cpus`.

I'm not entirely sure why this wasn't failing loudly before (weak symbols?) but once we add the Rust FFI calls that actually use JS stuff, the linker starts complaining about missing symbols from nspr if we don't have this.

This is mostly just importing existing code from the github.com/servo/mozjs repository and giving it a new home in js/rust. The difference from that repository that is introduced here is that FFI bindings are generated on-the-fly at compile time by bindgen. See the js/rust/README.md and js/rust/build.rs files for details.

This adds a new SpiderMonkey taskcluster test task that builds and tests the js/rust crate.

This commit ensures that we copy the js/rust crate into the resulting source tarball whenever we make standalone JS releases.

Comment on attachment 8856728 [details] [diff] [review] Part 9: Move the servo/mozjs crate providing bindings to SpiderMonkey to js/rust Review of attachment 8856728 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/rust/Cargo.toml @@ +5,5 @@ > +authors = ["The Servo Project Developers"] > +build = "build.rs" > + > +[build-dependencies] > +bindgen = { path = "/home/fitzgen/rust-bindgen" } # TODO FITZGEN This is temporary, waiting on https://github.com/servo/rust-bindgen/pull/627 to merge to servo/rust-bindgen and a `cargo publish`.

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #55) > This is temporary, waiting on https://github.com/servo/rust-bindgen/pull/627 > to merge to servo/rust-bindgen and a `cargo publish`. The new bindgen version has been published on crates.io, and I've locally updated this patch to point to the newly published version rather than a local path.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73d7c78d8ccc

Comment on attachment 8856725 [details] [diff] [review] Part 7: In JS standalone builds, always statically link mozglue This is causing JS_STANDALONE builds to lose jemalloc, which in turn causes a bunch of size-related tests fail. I think something more is needed here, but I don't really know what.

This patch series also depends on Servo updating its bindgen dependency, so we get one correct version of bindgen in third_party/rust. https://github.com/servo/servo/pull/16392

Comment on attachment 8856720 [details] [diff] [review] Part 2: Refactor CallArgs to avoid zero-sized base classes Review of attachment 8856720 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/CallArgs.h @@ +141,5 @@ > + void setUsedRval() const {} > + void clearUsedRval() const {} > + void assertUnusedRval() const {} > +#endif > + whitespace

Comment on attachment 8856723 [details] [diff] [review] Part 5: Stop doing the old `typedef struct` C thing in jsapi.h Review of attachment 8856723 [details] [diff] [review]: ----------------------------------------------------------------- Every one of these was very slightly different. But all equally stupid.

Comment on attachment 8856724 [details] [diff] [review] Part 6: Turn various macro definitions into proper constants Review of attachment 8856724 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.h @@ +885,5 @@ > +/* native that can be called as a ctor */ > +static const uintptr_t JSFUN_CONSTRUCTOR = 0x400; > + > +/* | of all the JSFUN_* flags */ > +static const uintptr_t JSFUN_FLAGS_MASK = 0x600; alignment (since you preserved it for everything else)

Comment on attachment 8856727 [details] [diff] [review] Part 8: Tell cargo to tell rustc to link nspr when building SpiderMonkey Review of attachment 8856727 [details] [diff] [review]: ----------------------------------------------------------------- I'll take your word for the syntax.

Comment on attachment 8856728 [details] [diff] [review] Part 9: Move the servo/mozjs crate providing bindings to SpiderMonkey to js/rust Review of attachment 8856728 [details] [diff] [review]: ----------------------------------------------------------------- Ok, I'll assume you don't want me to review the Rust code. ::: js/rust/Cargo.toml @@ +5,5 @@ > +authors = ["The Servo Project Developers"] > +build = "build.rs" > + > +[build-dependencies] > +bindgen = { path = "/home/fitzgen/rust-bindgen" } # TODO FITZGEN This is done now? ::: js/rust/build.rs @@ +109,5 @@ > + "JS::AutoObjectVector", > + "JS::CallArgs", > + "js::Class", > + "JS::CompartmentOptions", > + "js::ContextFriendFields", dead @@ +111,5 @@ > + "js::Class", > + "JS::CompartmentOptions", > + "js::ContextFriendFields", > + "JS::ContextOptions", > + "js::ESClass", dead @@ +145,5 @@ > + "JSType", > + "JSValueTag", > + "JSValueType", > + "jsid", > + "jsval_layout", jsval_layout is gone @@ +147,5 @@ > + "JSValueType", > + "jsid", > + "jsval_layout", > + "JS::Latin1Char", > + "JS::detail::MaybeWrapped", dead @@ +153,5 @@ > + "JS::MutableHandleObject", > + "JS::MutableHandleValue", > + "JS::NativeImpl", > + "JS::ObjectOpResult", > + "JS::ForOfIterator::NonIterableBehavior", dead @@ +163,5 @@ > + "JS::SourceBufferHolder", > + "JS::Symbol", > + "JS::TraceKind", > + "JS::Value", > + "JS::Value::PayloadType", dead ::: js/rust/src/conversions.rs @@ +1,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/. */ > + > +//! Conversions of Rust values to and from `JSVal`. To what degree should I review this file? I read through it. It was interesting. I can't say I'd be able to say if something was wrong. ::: js/rust/src/jsglue.cpp @@ +7,5 @@ > + > +#include "js-config.h" > + > +#ifdef JS_DEBUG > +// A hack for MFBT. Guard objects need this to work. The guard objects that prevent things from being used as temporaries? From mfbt/GuardObjects.h? I wonder if we can kill those yet, in favor of static analysis annotations. Never mind for now. ::: js/rust/src/lib.rs @@ +70,5 @@ > + // don't end up getting the global definitions for mozglue's symbols after > + // linking unless we have a direct call to one of them. No, the transitive > + // calls through the JS engine's C++ code don't do the trick. I have no idea > + // why. I'm really sorry if you're reading this in anger. I empathize with > + // your frustration, and you have my pity :( Ugh. I don't understand any of that either. But I guess if it works, it works. @@ +74,5 @@ > + // your frustration, and you have my pity :( > + > + extern "C" { > + #[link_name="_ZN7mozilla9TimeStamp3NowEb"] > + pub fn mozilla_TimeStamp_Now(high_res: bool) -> u64; Is this restricted to gcc+clang somehow? I would expect the name mangling to differ on Windows. ::: js/rust/src/rust.rs @@ +55,5 @@ > +// between system code and trusted script is a very unscientific 10k. > +const SYSTEM_CODE_BUFFER: usize = 10 * 1024; > + > +// Gecko's value on 64-bit. > +const TRUSTED_SCRIPT_BUFFER: usize = 8 * 12800; Ouch. I'm sorry you had to deal with this craziness. @@ +87,5 @@ > + use std::sync::{Once, ONCE_INIT}; > + use std::sync::atomic::{AtomicPtr, Ordering}; > + > + unsafe { > + #[derive(Debug)] whitespace at end @@ +288,5 @@ > + } > + > + unsafe fn get_rooting_context(cx: *mut JSContext) -> *mut JS::RootingContext { > + mem::transmute(cx) > + } Why does this need to happen? For Rust, a JSContext isn't already a RootingContext somehow? @@ +473,5 @@ > + } > +} > + > +impl Default for jsid { > + fn default() -> jsid { whitespace at eol

Comment on attachment 8856729 [details] [diff] [review] Part 10: Add the SM-tc(rust) taskcluster task Review of attachment 8856729 [details] [diff] [review]: ----------------------------------------------------------------- ::: taskcluster/scripts/builder/sm-tooltool-config.sh @@ +51,5 @@ > + > +# Add all the tooltool binaries to our $PATH. > +for bin in ls $TOOLTOOL_CHECKOUT/*/bin; do > + export PATH="$bin:$PATH" > +done there's a stray 'ls' here that's getting added to $PATH. I think you can just s/ls // and be good.

Thanks _so much_ for the reviews, Steve! I've found myself in transitive version dependency hell between Servo, Stylo, and these JS bindings. Also, mozglue linking. So there's a few open blocking issues surrounding those things. In the meantime, I'll try and land all the little fixups that help bindgen generate better bindings for JSAPI. Thanks again! (In reply to Steve Fink [:sfink] [:s:] from comment #64) > Comment on attachment 8856728 [details] [diff] [review] > Part 9: Move the servo/mozjs crate providing bindings to SpiderMonkey to > js/rust > > Review of attachment 8856728 [details] [diff] [review]: > ----------------------------------------------------------------- > > Ok, I'll assume you don't want me to review the Rust code. > > ::: js/rust/Cargo.toml > @@ +5,5 @@ > > +authors = ["The Servo Project Developers"] > > +build = "build.rs" > > + > > +[build-dependencies] > > +bindgen = { path = "/home/fitzgen/rust-bindgen" } # TODO FITZGEN > > This is done now? 0.23.0 is published, but still waiting on Stylo to bump their version dependency so we don't get two copies of bindgen vendored in tree. > > ::: js/rust/build.rs > @@ +109,5 @@ > > + "JS::AutoObjectVector", > > + "JS::CallArgs", > > + "js::Class", > > + "JS::CompartmentOptions", > > + "js::ContextFriendFields", > > dead Thanks for pointing all these out! > @@ +147,5 @@ > > + "JSValueType", > > + "jsid", > > + "jsval_layout", > > + "JS::Latin1Char", > > + "JS::detail::MaybeWrapped", > > dead Actually, introduced in an earlier patch, so I imagine if you were checking based on searchfox or dxr, that's why you'd miss it ;) > ::: js/rust/src/conversions.rs > @@ +1,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/. */ > > + > > +//! Conversions of Rust values to and from `JSVal`. > > To what degree should I review this file? I read through it. It was > interesting. I can't say I'd be able to say if something was wrong. This is all previously reviewed code from servo/rust-mozjs just being imported, so I think review is appreciated and useful, but not absolutely necessary. It is nice to get JS engine hackers' eyes on it too, though :) > ::: js/rust/src/jsglue.cpp > @@ +7,5 @@ > > + > > +#include "js-config.h" > > + > > +#ifdef JS_DEBUG > > +// A hack for MFBT. Guard objects need this to work. > > The guard objects that prevent things from being used as temporaries? From > mfbt/GuardObjects.h? I wonder if we can kill those yet, in favor of static > analysis annotations. Never mind for now. Yeah, those RAII guard objects. > ::: js/rust/src/lib.rs > @@ +70,5 @@ > > + // don't end up getting the global definitions for mozglue's symbols after > > + // linking unless we have a direct call to one of them. No, the transitive > > + // calls through the JS engine's C++ code don't do the trick. I have no idea > > + // why. I'm really sorry if you're reading this in anger. I empathize with > > + // your frustration, and you have my pity :( > > Ugh. I don't understand any of that either. But I guess if it works, it > works. Actually I think I can rm this code ever since I gave up on shared linking of mozglue and weak symbols and all that craziness in the patch for part 7. However, that patch accidentally removes jemalloc from JS_STANDALONE builds too, so this isn't a closed case yet :( > @@ +74,5 @@ > > + // your frustration, and you have my pity :( > > + > > + extern "C" { > > + #[link_name="_ZN7mozilla9TimeStamp3NowEb"] > > + pub fn mozilla_TimeStamp_Now(high_res: bool) -> u64; > > Is this restricted to gcc+clang somehow? I would expect the name mangling to > differ on Windows. D'oh of course. Luckily I think this code can go away, and we'll end up with other (hopefully nicer) hacks. > ::: js/rust/src/rust.rs > @@ +55,5 @@ > > +// between system code and trusted script is a very unscientific 10k. > > +const SYSTEM_CODE_BUFFER: usize = 10 * 1024; > > + > > +// Gecko's value on 64-bit. > > +const TRUSTED_SCRIPT_BUFFER: usize = 8 * 12800; > > Ouch. I'm sorry you had to deal with this craziness. Luckily this code already existed, and I didn't have to personally deal with it :) > @@ +288,5 @@ > > + } > > + > > + unsafe fn get_rooting_context(cx: *mut JSContext) -> *mut JS::RootingContext { > > + mem::transmute(cx) > > + } > > Why does this need to happen? For Rust, a JSContext isn't already a > RootingContext somehow? IIRC, JSContext is only forward declared in the JSAPI headers, ever since the remove of the context friend fields stuff.

Comment on attachment 8856725 [details] [diff] [review] Part 7: In JS standalone builds, always statically link mozglue Review of attachment 8856725 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to punt this to glandium since he understands our mozglue linkage way better than I do.

The `js` crate at `js/rust` depends on the `mozjs_sys` crate at `js/src`, so it makes sense to make it the top level crate.

Comment on attachment 8858070 [details] [diff] [review] Part 12: Make js/rust a top level crate instead of js/src Review of attachment 8858070 [details] [diff] [review]: ----------------------------------------------------------------- And so the JS engine part of Rewrite It In Rust(tm) commences. :p

Pushed by nfitzgerald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1501025c8e6b Part 2: Refactor CallArgs to avoid zero-sized base classes; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/eac68716afa7 Part 3: Make JS::Rooted and JS::PersistentRooted share the same MaybeWrapped<T> definition; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc525439e46 Part 4: Tell autospider.py to use as many parallel jobs as we have logical cores; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f50bab2fc6 Part 5: Stop doing the old `typedef struct` C thing in jsapi.h; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/718d18978ec4 Part 6: Turn various macro definitions into proper constants; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/20aa2f8284e0 Part 8: Tell cargo to tell rustc to link nspr when building SpiderMonkey; r=sfink

https://hg.mozilla.org/mozilla-central/rev/1501025c8e6b https://hg.mozilla.org/mozilla-central/rev/eac68716afa7 https://hg.mozilla.org/mozilla-central/rev/1fc525439e46 https://hg.mozilla.org/mozilla-central/rev/a4f50bab2fc6 https://hg.mozilla.org/mozilla-central/rev/718d18978ec4 https://hg.mozilla.org/mozilla-central/rev/20aa2f8284e0

Comment on attachment 8856725 [details] [diff] [review] Part 7: In JS standalone builds, always statically link mozglue Review of attachment 8856725 [details] [diff] [review]: ----------------------------------------------------------------- You're lucky if it works somehow. But I don't remember the details of how this was failing for you without this (from irc) and I can't find it written down here, so I can't tell much more.

(In reply to Mike Hommey [:glandium] from comment #72) > Comment on attachment 8856725 [details] [diff] [review] > Part 7: In JS standalone builds, always statically link mozglue > > Review of attachment 8856725 [details] [diff] [review]: > ----------------------------------------------------------------- > > You're lucky if it works somehow. But I don't remember the details of how > this was failing for you without this (from irc) and I can't find it written > down here, so I can't tell much more. `rustc` always passes `--gc-sections` to the linker, and there isn't a way for an individual crate to turn that off. All of the mozglue symbol definitions end up getting stripped. Ideally, we would be able to tell `rustc`/`cargo` not to do this, but that requires RFCs and coming to agreement, implementation, and then waiting 12 weeks before it is in `rustc` beta. AFAICT, the alternative to avoiding the weak symbols and statically linking both SM and mozglue (which is what this patch is trying to do, although ended up changing jemalloc too) is: * Running `nm` (and `dumpbin.exe` on windows) on `libmozglue.a` * Parsing the output to find all the symbols whose definitions are in mozglue * And generating fake uses of each symbol on the Rust side so that they don't get stripped Which seems very hacky... Is there a way I can statically link mozglue into libmozjs.so? Then `rustc` wouldn't participate in SM/mozglue linking at all. That would actually be the most ideal scenario, I think.

Building Firefox also always passes --gc-sections to the linker, so this is a red herring. Why is rustc involved in linking? That's not how we build things in mozilla-central. Is this about building a rust crate that links libmozjs? Then I bet the problem is the lack of the -export-dynamic option on the linker command.

(In reply to Mike Hommey [:glandium] from comment #74) > Why is rustc involved in linking? ... Is this about building a rust crate that links > libmozjs? Correct, this is about building the `js` crate, which is how Servo uses SpiderMonkey. > Then I bet the problem is the lack of the -export-dynamic option > on the linker command. Ok, it sounds like I misunderstood which linker flag (or lack thereof) is the problem here. What I am observing is: * mozglue's symbols' have global definitions in libmozglue.a, * libmozjs.so and libjs_static.a have weak symbols to reference mozglue things, * the .rlib files (which are just archives) that rustc produces have the global definitions from libmozglue.a in them, * the final linked executables emitted from rustc do not end up with the global definitions from libmozglue.a in them, * and therefore when I run the executable, ld ends up resolving mozglue's symbols' @plt to 0x0 and segfaulting. The first three points are all well and dandy, the last two are not. There is no way for an individual crate to customize linker flags, at the moment. Which brings me back to my last question: (In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #73) > Is there a way I can statically link mozglue into libmozjs.so? Then `rustc` > wouldn't participate in SM/mozglue linking at all. That would actually be > the most ideal scenario, I think.

^ ping, Glandium :)

I tried building rust-mozjs, and it doesn't build libmozjs.so, so I'm puzzled, now.

This rollup patch applies on this github.com/mozilla/gecko-dev commit: commit 7e1e3260bdd2bcfdb82fd2882b366c21e7b4bfea Merge: ff46e3c b223516 Author: Carsten "Tomcat" Book <cbook@mozilla.com> Date: Mon May 8 10:07:27 2017 +0200 merge mozilla-inbound to mozilla-central a=merge Right now, the rollup patch is configured to statically link libjs_static.a. I was able to get this working if I manually edit rustc's linker invocation to add `-Wl,--whole-archive` for the mozjs_sys.rlib (which is an archive containing both libmozglue.a and libjs_static.a). You can switch it to dynamically linking libmozjs.so by uncommenting lines 56-67 in js/src/build.rs, and commenting out line 54. After applying the rollup patch, you can see where libmozjs.so (and libjs_static.a etc) get built like this: $ cd js/rust $ cargo build <snip> $ find target/debug -type f -name 'libmozjs*.so' target/debug/build/mozjs_sys-d55de22c161d797b/out/js/src/build/libmozjs-55a1.so

Is there anything else I can do to help resolve these linking questions? Would a meeting with synchronous communication between us be of benefit? Let me know -- thanks!

This makes sure that: * We don't define `MOZ_GLUE_IN_PROGRAM` so that everything in mozglue gets defined. * `MFBT_API`'s symbol export rules match `JS_PUBLIC_API` and `EXPORT_JS_API`. * We add mozglue to SpiderMonkey's `USE_LIBS` when jemalloc is disabled. I verified that all the Rust bindings tests that were failing on undefined symbols are now passing without further build modifications \o/

Comment on attachment 8889661 [details] [diff] [review] Part 7: Export mozglue symbols when building JS_STANDALONE without jemalloc Review of attachment 8889661 [details] [diff] [review]: ----------------------------------------------------------------- Also, sfink for the autospider changes :)

Try push for part 7: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67de4021bf9b

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #83) > Try push for part 7: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=67de4021bf9b Hm... but this seems to have broken every other kind of build :( Glandium, I think I need your review of the patch to continue to make progress here.

Comment on attachment 8889661 [details] [diff] [review] Part 7: Export mozglue symbols when building JS_STANDALONE without jemalloc Review of attachment 8889661 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/moz.build @@ +57,5 @@ > 'js-confdefs.h', > ] > > +if CONFIG['JS_STANDALONE'] and not CONFIG['MOZ_MEMORY']: > + USE_LIBS += ["mozglue"] Please move this to js/src/build/moz.build ::: js/src/old-configure.in @@ +1507,5 @@ > dnl ======================================================== > dnl = Enable jemalloc > dnl ======================================================== > > +if test "$JS_STANDALONE" -a ! "$MOZ_MEMORY"; then -z instead of ! ::: mfbt/Types.h @@ +77,5 @@ > * Consistent with the above comment, the MFBT_API and MFBT_DATA macros expose > * export mfbt declarations when building mfbt, and they expose import mfbt > * declarations when using mfbt. > */ > +#if defined(IMPL_MFBT) || defined(EXPORT_JS_API) || (defined(JS_STANDALONE) && !defined(MOZ_MEMORY)) The STATIC_EXPORTABLE_JS_API and STATIC_JS_API cases are not handled.

This makes sure that: * We don't define `MOZ_GLUE_IN_PROGRAM` so that everything in mozglue gets defined. * `MFBT_API`'s symbol export rules match `JS_PUBLIC_API` and `EXPORT_JS_API`. * We add mozglue to SpiderMonkey's `USE_LIBS` when jemalloc is disabled.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0606609fa761

Comment on attachment 8895071 [details] [diff] [review] Part 7: Export mozglue symbols when building JS_STANDALONE without jemalloc Review of attachment 8895071 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Types.h @@ +77,5 @@ > * Consistent with the above comment, the MFBT_API and MFBT_DATA macros expose > * export mfbt declarations when building mfbt, and they expose import mfbt > * declarations when using mfbt. > */ > +#if defined(IMPL_MFBT) || defined(EXPORT_JS_API) || defined(STATIC_EXPORTABLE_JS_API) || defined(STATIC_JS_API) || (defined(JS_STANDALONE) && !defined(MOZ_MEMORY)) STATIC_JS_API needs its own branch, as JS_PUBLIC_API in that case is neither MOZ_EXPORT nor what's in the #else.

Forgot to `#endif` in the last patch. D'oh. Verified that this patch still works with the `js` crate that Servo uses.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc2f538d3456

It seems like this latest patch results in undefined symbols when building SM as a shared library: > ../Unified_cpp_js_src6.o: In function `ToSeconds': > /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:98: undefined reference to `mozilla::BaseTimeDurationPlatformUtils::ToSeconds(long)' > /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:98: undefined reference to `mozilla::BaseTimeDurationPlatformUtils::ToSeconds(long)' > ../Unified_cpp_js_src6.o: In function `js::gcstats::Statistics::endSlice()': > /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:98: undefined reference to `mozilla::BaseTimeDurationPlatformUtils::ToSeconds(long)' > ../Unified_cpp_js_src6.o: In function `Now': > /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:463: undefined reference to `mozilla::TimeStamp::Now(bool)' > ../Unified_cpp_js_src6.o: In function `js::gcstats::Statistics::endSCC(unsigned int, mozilla::TimeStamp)': > /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:463: undefined reference to `mozilla::TimeStamp::Now(bool)' > collect2: error: ld returned 1 exit status > /home/worker/workspace/build/src/config/rules.mk:721: recipe for target 'libmozjs-57a1.so' failed > make[3]: *** [libmozjs-57a1.so] Error 1 What do we need to do to fix this?

Comment on attachment 8902008 [details] [diff] [review] Part 7: Export mozglue symbols when building JS_STANDALONE without jemalloc Review of attachment 8902008 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Types.h @@ +77,5 @@ > * Consistent with the above comment, the MFBT_API and MFBT_DATA macros expose > * export mfbt declarations when building mfbt, and they expose import mfbt > * declarations when using mfbt. > */ > +#if defined(IMPL_MFBT) || defined(EXPORT_JS_API) || defined(STATIC_EXPORTABLE_JS_API) || (defined(JS_STANDALONE) && !defined(MOZ_MEMORY)) I think all your problems stem from the fact this condition and the condition for STATIC_JS_API below have a flawed logic. What you want is something like: #if defined(IMPL_MFBT) || (defined(JS_STANDALONE) && !defined(MOZ_MEMORY) && (defined(EXPORT_JS_API) || defined(STATIC_EXPORTABLE_JS_API))) #else # if defined(JS_STANDALONE) && !defined(MOZ_MEMORY) && defined(STATIC_JS_API)

This finally results in a green try run, and at least here locally, a mozjs crate build works, too. Mike, I'm not sure if you can review the change to check_vanilla_allocations.py to exclude mozalloc. If not, can you forward the review for that file to someone who can?

Oh, and the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58fa1ce94df3a5538308fb4bfb5ec0b1c3bfa824

Comment on attachment 8902716 [details] [diff] [review] Part 7: Export mozglue symbols when building JS_STANDALONE without jemalloc Review of attachment 8902716 [details] [diff] [review]: ----------------------------------------------------------------- r=me on check_vanilla_allocations.py, just to cover you. ::: config/check_vanilla_allocations.py @@ +131,5 @@ > filename = m.group(1) > + > + # mozalloc contains calls to memalign. These are ok, so we whitelist them. > + if string.find(filename, "mozalloc") > -1: > + continue Why do you need to import the string module? Isn't this the same as if filename.find("mozalloc") > -1: ?

(In reply to Steve Fink [:sfink] [:s:] (PTO Aug 16-23) from comment #97) > Why do you need to import the string module? Isn't this the same as > if filename.find("mozalloc") > -1: Absolutely no reason whatsoever - beyond how rarely I use Python. Fitzgen, can you fix this nit before landing?

Comment on attachment 8902716 [details] [diff] [review] Part 7: Export mozglue symbols when building JS_STANDALONE without jemalloc Review of attachment 8902716 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/check_vanilla_allocations.py @@ +130,5 @@ > > filename = m.group(1) > + > + # mozalloc contains calls to memalign. These are ok, so we whitelist them. > + if string.find(filename, "mozalloc") > -1: Can be written as `if "mozalloc" in filename:`.

Updated the python mozalloc check based on review feedback. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4774dbaaa34

Mis-applied parts of the last patch which messed up indentation. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76724b2d69c0

Pushed by nfitzgerald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a17a6af7ff93 Part 7: Export mozglue when JS_STANDALONE && !jemalloc; r=glandium,sfink

Rebased.

Rebased.

Rebased.

Rebased.

Fix minor yaml format changes.

More minor yaml format tweaks...

Comment on attachment 8903799 [details] [diff] [review] Part 13: Update vendored crates for newer `js` crate Review of attachment 8903799 [details] [diff] [review]: ----------------------------------------------------------------- rs=me

Small tweaks to remove nightly features that weren't necessary. Taskcluster doesn't run with nightly rust, so it wasn't going to work out.

Comment on attachment 8903834 [details] [diff] [review] Part 13: Update vendored crates for newer `js` crate Woops, data race

Green push! https://treeherder.mozilla.org/#/jobs?repo=try&revision=4acf23ef1cfa

Pushed by nfitzgerald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee31bf579fc Part 9: Move the servo/rust-mozjs crate providing bindings to SpiderMonkey to js/rust; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/ec92904a27cf Part 10: Add the SM-tc(rust) taskcluster task; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/40d0eefb1b5a Part 11: Add js/rust to standalone JS source packages; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/b00ca2e7bda4 Part 12: Make js/rust a top level crate instead of js/src; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ae266cd61e Part 13: Update vendored crates for newer `js` crate; r=sfink

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/59ea29d58ab0 because we are indeed anal enough to have a tier-1 job which fails for a single line with more than 99 chars, https://treeherder.mozilla.org/logviewer.html#?job_id=127899842&repo=mozilla-inbound

Pushed by nfitzgerald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b984dd3cda3 Part 9: Move the servo/rust-mozjs crate providing bindings to SpiderMonkey to js/rust; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/81ef74814f55 Part 10: Add the SM-tc(rust) taskcluster task; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/4904af8ae3ec Part 11: Add js/rust to standalone JS source packages; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/ee603f7b673f Part 12: Make js/rust a top level crate instead of js/src; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/ef1033c0be43 Part 13: Update vendored crates for newer `js` crate; r=sfink

https://hg.mozilla.org/mozilla-central/rev/8b984dd3cda3 https://hg.mozilla.org/mozilla-central/rev/81ef74814f55 https://hg.mozilla.org/mozilla-central/rev/4904af8ae3ec https://hg.mozilla.org/mozilla-central/rev/ee603f7b673f https://hg.mozilla.org/mozilla-central/rev/ef1033c0be43

Err... this checked in another bindgen in tree :(

Actually, it duplicates quite a bit of third party crates...

I may be dumb, but shouldn't this ticket be about mozjs-sys? That way we still own the higher-level bindings, can fix them faster (no need for making Bugzilla tickets, it stays on GH), and all that project needs to care is to be able to produce FFI glue to the SpiderMonkey engine, with the bundled C++ source code. At the very least, this project does not intend to merge mozjs-sys and rust-mozjs, right?

At the very least, jsapi_linux_32.rs and friends, and jsval.rs should be owned by SM.

Follow-up about my previous comment: I should probably have raised that concern earlier; but I think my confusion for all this time stems from the fact that our current rust-mozjs crate contains both the lower-level glue and the higher-level bindings. All that time, whenever we discussed about this project, I was operating under the assumption that we were talking about moving only the lower-level glue. I don't think it would be a good idea to move the higher-level bindings in m-c too.

At the very least, even if we move the higher-level bindings to m-c, the lower-level bindings should still live in a different crate, and we should strive to make them not leak at all from the crate of higher-level bindings. If we don't do that, any breaking change in the lower-level bindings will result in a major bump of the higher-level bindings.