Closed Bug 1273639 Opened 4 years ago Closed 4 years ago

Add a nonunified spidermonkey build

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

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.
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.
Attachment #8753515 - Flags: review?(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/' '{}' ';'
Attachment #8753515 - Flags: review?(terrence) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #0)
Thanks for doing this!
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.
Attachment #8754119 - Flags: review?(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?
Attachment #8754119 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/eab16f90bea7
https://hg.mozilla.org/mozilla-central/rev/efa156b65838
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(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.
(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
Duplicate of this bug: 1274861
You need to log in before you can comment on or make changes to this bug.