1273639 - Add a nonunified spidermonkey build

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

A fair number of people are affected by unified breakage within spidermonkey, so RyanVM suggested that it'd be worth having a build to keep it from breaking. I agree.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Add a nonunified spidermonkey build

You're a fine reviewer for the code part of this, not necessarily for the adding an additional (lightweight) job's worth of load part, but this is such a small thing that it seems worth going the forgiveness route. (Plus, I marked the blocking metabug, so the right people will see it.)

Note to readers: the build is currently broken right now, so I will coordinate with the sheriffs about turning it on and its visibility tier.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8753515 [details] [diff] [review]
Add a nonunified spidermonkey build

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

Thanks for whipping this up!

::: js/src/devtools/automation/autospider.sh
@@ +56,5 @@
>  fi
>  
> +if [[ "$VARIANT" = "nonunified" ]]; then
> +    # Hack the moz.build files to turn off unified compilation.
> +    find "$SOURCE/js/src" -name moz.build -exec perl -i.autospider -pe 's/UNIFIED_SOURCES/SOURCES/g' '{}' ';'

Is perl required by our build still? I'd hate to add more dependencies on it when sed would work just as well. How about:

find ... -name moz.build -exec sed -i 's/UNIFIED_SOURCES/SOURCES/' '{}' ';'

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #0)
Thanks for doing this!

Comment 4

[Steve Fink [:sfink] [:s:]]

For some reason, TH is not showing the result status of these builds, even though TC knows and displays it properly. I'm waiting to land until that is figured out.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eab16f90bea7

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efa156b65838

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Fix nonunified spidermonkey builds

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8754119 [details] [diff] [review]
Fix nonunified spidermonkey builds

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

Thanks!

::: js/src/gdb/tests/test-unwind.cpp
@@ +9,2 @@
>  {
> +    JS::CallArgs args = CallArgsFromVp(argc, vp);

CallArgs needs a namespace but CallArgsFromVp doesn't?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eab16f90bea7
https://hg.mozilla.org/mozilla-central/rev/efa156b65838

Comment 10

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #8)
> Comment on attachment 8754119 [details] [diff] [review]
> Fix nonunified spidermonkey builds
> 
> Review of attachment 8754119 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: js/src/gdb/tests/test-unwind.cpp
> @@ +9,2 @@
> >  {
> > +    JS::CallArgs args = CallArgsFromVp(argc, vp);
> 
> CallArgs needs a namespace but CallArgsFromVp doesn't?

Of course not! Any idiot can see that! (I just spent 20 minutes minimizing the preprocessed file to figure it out because it intrigued me. It seemed to make no sense at all.)

CallArgsFromVp doesn't need qualification here because it has a parameter in the JS namespace. And parameter namespaces are searched for function calls. The only reason I can come up with for that to not be utter lunacy would be if it's needed for operator overloading, so that something like this can work:

namespace VectorStuff {
struct Vector { int x; int y; };
bool operator==(const Vector& v1, const Vector& v2) { return v1.x == v2.x && v1.y == v2.y; }
}

VectorStuff::Vector v;
if (v == v) { ... }

but that's just speculation on my part; I haven't googled it or looked up the spec.

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #10)
Looks like your speculation is correct:

https://en.wikipedia.org/wiki/Argument-dependent_name_lookup

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c0e9e19a08

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0c0e9e19a08