Move rust-mozjs bindings into mozilla-central

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Depends on 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(16 attachments, 28 obsolete attachments)

3.38 MB, patch
froydnj
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
10.56 KB, patch
sfink
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
1.54 KB, patch
froydnj
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
3.17 KB, patch
sfink
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
3.54 KB, patch
sfink
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
1.51 KB, patch
sfink
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
2.53 KB, patch
sfink
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
15.82 KB, patch
sfink
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
1.35 KB, patch
sfink
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
1.63 KB, text/markdown
Details
9.32 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
1.14 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
1.45 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
9.27 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
218.86 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
4.87 MB, patch
fitzgen
: review+
Details | Diff | Splinter Review
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.
Depends on: 1277341
Depends on: 1277343
Depends on: 1275639
Blocks: 1263289
Summary: [meta] Move rust-mozjs bindings into mozilla-central → [meta-html] Move rust-mozjs bindings into mozilla-central
Whiteboard: [devtools-html] → [meta-html] [devtools-html]

Updated

3 years ago
Depends on: 1273917
Whiteboard: [meta-html] [devtools-html] → [meta-html]
Severity: normal → enhancement
Depends on: 1280064
Depends on: 1280089
Depends on: 1280104
Depends on: 1280107
Depends on: 956899
Summary: [meta-html] Move rust-mozjs bindings into mozilla-central → Move rust-mozjs bindings into mozilla-central
Whiteboard: [meta-html]
(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
Flags: needinfo?(ted)
(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`.
Flags: needinfo?(ted)
Attachment #8795540 - Attachment is obsolete: true
Attachment #8795541 - Attachment is obsolete: true
Attachment #8795542 - Attachment is obsolete: true
Attachment #8795543 - Attachment is obsolete: true
Attachment #8795544 - Attachment is obsolete: true
Attachment #8795545 - Attachment is obsolete: true
Attachment #8795546 - Attachment is obsolete: true
The last patch missed the taskcluster script.
Attachment #8800471 - Flags: review?(sphink)
Attachment #8800470 - Attachment is obsolete: true
Attachment #8800470 - Flags: review?(sphink)
Assignee: nobody → nfitzgerald
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.).
Attachment #8800469 - Flags: review?(nfroyd) → review+
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
Attachment #8800471 - Flags: review?(sphink) → review+

Comment 18

3 years ago
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
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.
Flags: needinfo?(nfitzgerald)
(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?
Flags: needinfo?(ted)
Flags: needinfo?(nfroyd)
Flags: needinfo?(nfitzgerald)
(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.
Flags: needinfo?(nfroyd)
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.
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.
Attachment #8820350 - Flags: review?(nfroyd) → review+
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).
Flags: needinfo?(ted)

Comment 30

2 years ago
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!
Posted patch WIP WIP WIP roll up (obsolete) — Splinter Review
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.
Attachment #8848619 - Attachment is obsolete: true
Posted patch WIP WIP WIP roll up (obsolete) — Splinter Review
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
Attachment #8850081 - Attachment is obsolete: true
Posted patch WIP WIP WIP roll up (obsolete) — Splinter Review
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.
Attachment #8854127 - Attachment is obsolete: true
Duplicate of this bug: 1277343
Comment on attachment 8800469 [details] [diff] [review]
Part 0: Vendor third party crates needed for the mozjs-sys crate

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

This landed.
Attachment #8800471 - Flags: checkin+
Comment on attachment 8820350 [details] [diff] [review]
Add the "mozjs_sys" crate as a root crate

This landed.
Attachment #8820350 - Flags: checkin+
Attachment #8855482 - Attachment is obsolete: true
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.
Attachment #8856720 - Flags: review?(sphink)
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.
Attachment #8856721 - Flags: review?(sphink)
This is not a new external Rust crate dependency for m-c since Servo already
depends on `num_cpus`.
Attachment #8856722 - Flags: review?(sphink)
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.
Attachment #8856727 - Flags: review?(sphink)
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.
Attachment #8856728 - Flags: review?(sphink)
This adds a new SpiderMonkey taskcluster test task that builds and tests the
js/rust crate.
Attachment #8856729 - Flags: review?(sphink)
This commit ensures that we copy the js/rust crate into the resulting source
tarball whenever we make standalone JS releases.
Attachment #8856730 - Flags: review?(sphink)
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.
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
Attachment #8856720 - Flags: review?(sphink) → review+
Attachment #8856721 - Flags: review?(sphink) → review+
Attachment #8856722 - Flags: review?(sphink) → review+
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.
Attachment #8856723 - Flags: review?(sphink) → review+
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)
Attachment #8856724 - Flags: review?(sphink) → review+
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.
Attachment #8856727 - Flags: review?(sphink) → review+
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
Attachment #8856728 - Flags: review?(sphink) → review+
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.
Attachment #8856729 - Flags: review?(sphink) → review+
Attachment #8856730 - Flags: review?(sphink) → review+
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.
Attachment #8856725 - Flags: review?(ted) → review?(mh+mozilla)
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.
Attachment #8858070 - Flags: review?(nfroyd)
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
Attachment #8858070 - Flags: review?(nfroyd) → review+

Comment 70

2 years ago
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
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.
Attachment #8856725 - Flags: review?(mh+mozilla) → review-
(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.
Flags: needinfo?(mh+mozilla)
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.
Flags: needinfo?(mh+mozilla)
(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.
Flags: needinfo?(mh+mozilla)
^ ping, Glandium :)
I tried building rust-mozjs, and it doesn't build libmozjs.so, so I'm puzzled, now.
Flags: needinfo?(mh+mozilla)
Posted patch Rollup patch (obsolete) — Splinter Review
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
Flags: needinfo?(mh+mozilla)
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!
Depends on: 1369536
Attachment #8869536 - Attachment is obsolete: true
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/
Attachment #8889661 - Flags: review?(mh+mozilla)
Attachment #8856725 - Attachment is obsolete: true
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 :)
Attachment #8889661 - Flags: review?(sphink)
Attachment #8889661 - Flags: review?(sphink) → review+
(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.
Attachment #8889661 - Flags: review?(mh+mozilla) → review-
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.
Attachment #8895071 - Flags: review?(mh+mozilla)
Attachment #8889661 - Attachment is obsolete: true
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.
Attachment #8895071 - Flags: review?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Comment hidden (obsolete)
Attachment #8895071 - Attachment is obsolete: true
Forgot to `#endif` in the last patch. D'oh.

Verified that this patch still works with the `js` crate that Servo uses.
Attachment #8902008 - Flags: review?(mh+mozilla)
Attachment #8901972 - Attachment is obsolete: true
Attachment #8901972 - Flags: review?(mh+mozilla)
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?
Flags: needinfo?(mh+mozilla)
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)
Attachment #8902008 - Flags: review?(mh+mozilla)
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?
Attachment #8902008 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
Attachment #8902716 - Flags: review?(mh+mozilla)
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:`.
Attachment #8902716 - Flags: review?(mh+mozilla) → review+
Updated the python mozalloc check based on review feedback.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4774dbaaa34
Attachment #8903222 - Flags: review+
Attachment #8902716 - Attachment is obsolete: true
Mis-applied parts of the last patch which messed up indentation.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76724b2d69c0
Attachment #8903235 - Flags: review+
Attachment #8903222 - Attachment is obsolete: true

Comment 102

2 years ago
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
Attachment #8856728 - Attachment is obsolete: true
Attachment #8856729 - Attachment is obsolete: true
Attachment #8856730 - Attachment is obsolete: true
Attachment #8858070 - Attachment is obsolete: true
Fix minor yaml format changes.
Attachment #8903782 - Flags: review+
Attachment #8903779 - Attachment is obsolete: true
More minor yaml format tweaks...
Attachment #8903798 - Flags: review+
Attachment #8903782 - Attachment is obsolete: true
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
Attachment #8903799 - Flags: review?(sphink) → review+
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.
Attachment #8903832 - Flags: review+
Attachment #8903778 - Attachment is obsolete: true
Attachment #8903799 - Attachment is obsolete: true
Comment on attachment 8903834 [details] [diff] [review]
Part 13: Update vendored crates for newer `js` crate

Woops, data race
Attachment #8903834 - Flags: review?(sphink) → review+

Comment 116

2 years ago
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

Comment 118

2 years ago
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
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?
Flags: needinfo?(nfitzgerald)
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.
Flags: needinfo?(nfitzgerald)
You need to log in before you can comment on or make changes to this bug.