SpiderMonkey upgrade

[Ms2ger]

Recreated the PR to aid collaboration.

---

This change is

[Ms2ger]

Funny story, that. The presence of invalid Cargo.tomls in m-c make cargo fail to compile here.

[metajack]

Why are there invalid Cargo.tomls in m-c?

cc @larsbergstrom

[fitzgen]

./mozjs/python/mozbuild/mozbuild/test/frontend/data/crate-build-dependency-absolute-path/Cargo.toml:14:bogus = { version = "0.1.0", path = "/path/to/somewhere" }
./mozjs/python/mozbuild/mozbuild/test/frontend/data/crate-dependency-absolute-path/Cargo.toml:12:bogus = { version = "0.1.0", path = "/path/to/somewhere" }

[fitzgen]

I'm just deleting all of m-c's test Cargo.toml files in servo/mozjs

[fitzgen]

By deleting the test Cargo.toml files, and commit d0b0f5a, rust-mozjs builds successfully for me. We'll see what CI thinks...

[fitzgen]

Travis CI OSX builds failing due to having an old clang/llvm? Do we need to update the .travis.yml config to use new OSX images?

ERROR: Only clang/llvm 3.6 or newer is supported.

---

Just pushed linux 64 bindings as well. Anyone care to help out with linux 32 and windows?

[Ms2ger]

CC @nox @vvuk: could you generate the missing bindings?

[fitzgen]

Actually hold up a minute. This upgrade caught the big cx/rt refactoring that jandem did, so cargo test is rather unhappy at the moment -- kind of surprised that cargo build didn't blow up too.

[fitzgen]

Looks like this upgrade also caught the move of roots from the runtime to zones...

[fitzgen]

Ok, linux64 is passing in Travis CI.

I updated the macos64 bindings locally and tested them and they are all good, but I expect them to fail on Travis CI again due to the llvm issues I mentioned before. Anyone have any ideas? I can choose a random OSX image that is new enough, but I'm not sure if there is a more principled approach you;d like to take...

@nox @vvuk I think we are ready for windows and linux32 bindings updates now!

[fitzgen]

Rebased.

I updated the macos64 bindings locally and tested them and they are all good, but I expect them to fail on Travis CI again due to the llvm issues I mentioned before. Anyone have any ideas? I can choose a random OSX image that is new enough, but I'm not sure if there is a more principled approach you;d like to take...

@Ms2ger @nox any opinion on this ^?

[bors-servo]

:umbrella: The latest upstream changes (presumably #298) made this pull request unmergeable. Please resolve the merge conflicts.

[fitzgen]

Ok, I think all the bindings need to be regenerated again, now that we aren't using unsafe_no_drop_flag anymore.

[nox]

Ok, I think all the bindings need to be regenerated again, now that we aren't using unsafe_no_drop_flag anymore.

You can just do /unsafe_no_drop_flag/d on them.

[Ms2ger]

@metajack do you have opinions on the llvm thing mentioned above?

[metajack]

If recent OS X images provided by Travis have the correct llvm, I think it's fine to udpate to those. Otherwise, it should be possible to install the required llvm version via brew.

[fitzgen]

Nice, looks like OSX is passing on Travis now with the xcode 7 image :)

[fitzgen]

I regenerated bindings for linux64 and now cargo test is segfaulting in a very strange way. It seems like the DWARF debugging info is garbage when I compare what gdb reports for variable values vs looking at the disassembly, so digging in is more annoying than usual.

[fitzgen]

I regenerated bindings for linux64 and now cargo test is segfaulting in a very strange way. It seems like the DWARF debugging info is garbage when I compare what gdb reports for variable values vs looking at the disassembly, so digging in is more annoying than usual.

I am 95% sure this is a miscompilation by gdb. It is passing the first argument to the current function as the this parameter in a method call on what should be a stack allocated object that never is actually allocated. Completely nonsensical.

I'll bet this works on Travis with a (hopefully) different compiler.

[fitzgen]

FWIW, filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77448 for the gcc codegen issues I was seeing.

[fitzgen]

Either rustc or g++ is not following the x86-64 ABI here. They're disagreeing on whether JS::CallArgs should be classified as MEMORY and therefore have the stack address of the return value passed as an implicit first parameter in rdi or not. g++ is classifying it as MEMORY and using the implicit stack pointer, while rustc is classifying it as INTEGER and splitting it across rax and rdx.

[fitzgen]

I tried to make a reduced test case, however now rustc is treating CallArgs as MEMORY in this example and passing the stack pointer in through rdi!

[fitzgen]

From the most up to date draft of the x86-64 ABI I could find: AMD64 ABI Draft 0.99.7, Section 3.2.3 Parameter Passing, p. 19:

If the size of the aggregate exceeds two eightbytes and the first eight- byte isn’t SSE or any other eightbyte isn’t SSEUP, the whole argument is passed in memory

That seems fairly damning to rustc's behavior, here. (sizeof(JS::CallArgs) is 24)

[Ms2ger]

File an issue on rustc?

[nox]

File an issue on rustc?

Nah the bindings were just badly generated, right @fitzgen?

[fitzgen]

Nah the bindings were just badly generated, right @fitzgen?

Yes, I managed to put non-DEBUG bindings in the _debug.rs file. I'm writing a patch to make it so that no one will repeat this mistake because it was a huge time sink figuring out what was going on.

Travis CI is now green for OSX and linux 64. Just need linux 32 and windows now.

[vvuk]

I'll work on win32 right now.

[vvuk]

Trying to build this version (smup branch of mozjs-sys, commit b5b7c84990325cde5226) with MSVC I get:

Unified_cpp_js_src3.cpp
c:/proj/r/mozjs/mozjs/js/src/builtin/Intl.cpp(859): error C2971: 'ScopedICUObject': template parameter 'Delete': 'uenum_close': a variable with non-static storage duration cannot be used as a non-type argument
c:/proj/r/mozjs/mozjs/js/src/builtin/Intl.cpp(615): note: see declaration of 'ScopedICUObject'
c:/proj/r/mozjs/mozjs/js/src/builtin/Intl.cpp(128): note: see declaration of 'uenum_close'

This is pretty strange, since that uenum_close on line 128 is a stub (because intl is disabled) declared as static void uenum_close() { ... }

[vvuk]

I can fix this if I change

static void
uenum_close(UEnumeration* en)
{
    MOZ_CRASH("uenum_close: Intl API disabled");
}

to

namespace {
void
uenum_close(UEnumeration* en)
{
    MOZ_CRASH("uenum_close: Intl API disabled");
}
}

(and for other similar stub functions).

I'm guessing that this was never tested with --without-intl-api on Windows. I'll prepare a patch and put up a PR for the smup branch.

[vvuk]

The other issue is that currently, the SM/Gecko build is broken with anything but MSVC or clang on Windows, because of https://bugzilla.mozilla.org/show_bug.cgi?id=1288313#c12 (configure fails). I'm working on a workaround.

[vvuk]

Okay, for windows: rust-bindgen fix: https://github.com/servo/rust-bindgen/pull/54 mozjs-sys fixes: https://github.com/servo/mozjs/pull/99 updated bindings: https://github.com/servo/rust-mozjs/pull/303

Note that without servo/mozjs/pull/99, mozjs will not be buildable on Windows (gcc has the WINNT issue, msvc has the anon namespace static issue).

[nox]

Proper union support landed in rust-bindgen. Should we account for this in that smup?

[fitzgen]

Okay, for windows:

So does this mean we need to rebase this PR and then the appveyor CI tests should start passing?

Also, I think the docopt changes got backed out of bindgen and haven't relanded so we may need to update some scripts...

[fitzgen]

The etc/bindings.sh script no longer works with the latest servo/rust-bindgen.

[vvuk]

Oh wow, didn't realize this hadn't landed yet. What's wrong with the bindings.sh script?

[fitzgen]

What's wrong with the bindings.sh script?

bindgen's backout of docopt made it so that the args aren't getting parsed correctly anymore for some reason. Still digging in. Additionally, I don't think the SpiderMonkey bindings have been regenerated since bindgen was completely rewritten, so maybe some fallout from there.

[fitzgen]

Biggest issue with the bindings.sh script atm: tons of --match, which is no longer a supported flag. Working on it.

[vvuk]

Oh wow, okay. Give me a shout on irc if you need help with any Windows stuff.

On Oct 12, 2016 5:03 PM, "Nick Fitzgerald" notifications@github.com wrote:

Biggest issue with the bindings.sh script atm: tons of --match, which is no longer a supported flag. Working on it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/servo/rust-mozjs/pull/291#issuecomment-253338233, or mute the thread https://github.com/notifications/unsubscribe-auth/AAL5lTMBPoByFoMpGo39fOEDPLSi6Gkcks5qzUrtgaJpZM4JqtfP .

[fitzgen]

My current plan of action:

• Land the mozjs-sys crate in m-c now because that is working and green in taskcluster
• Redo bindings generation from a clean slate because our current setup is completely hosed
• Land that in m-c once its fixed + building in taskcluster
• Then, whenever we want to smup, take known-working snapshots of spidermonkey and bindings into servo/mozjs or servo/rust-mozjs (wouldn't really need separate repos anymore)

This stuff will mostly happen in https://bugzilla.mozilla.org/show_bug.cgi?id=1277338

[nox]

@fitzgen Don't know if I told you that before, but if we could use the new support for unions in our bindgen, we can remove a few ifdef RUST_BINDGEN stuff from our SM fork.

[bors-servo]

:umbrella: The latest upstream changes (presumably #310) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

Closing since the activity is happening elsewhere.