950822 - js/src/vm/Runtime.cpp: remove obsolete code where JS_STACK_GROWTH_DIRECTION > 0

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeroen Roovers]

Attachment: firefox-26.0-stack_grows_up.patch

User Agent: Mozilla/5.0 (X11; Linux parisc64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131216175841

Steps to reproduce:

I filed Gentoo bug https://bugs.gentoo.org/show_bug.cgi?id=494162 with a patch attached.


Actual results:

/var/tmp/portage/www-client/firefox-26.0/work/mozilla-release/js/src/vm/Runtime.cpp: In constructor 'JSRuntime::JSRuntime(JSUseHelperThreads)':
/var/tmp/portage/www-client/firefox-26.0/work/mozilla-release/js/src/vm/Runtime.cpp:292:33: error: 'kind' was not declared in this scope
distcc[24822] ERROR: compile /var/tmp/portage/www-client/firefox-26.0/work/mozilla-release/js/src/vm/Runtime.cpp on localhost failed
make[4]: *** [Runtime.o] Error 1


Expected results:

The code in question should have been removed before the 26 release. It has already been replaced elsewhere.

Comment 1

[Jory A. Pratt [:anarchy]]

This code is introduced by Gentoo. Leave bugs downstream unless myself or another gentoo mozilla dev ask you to open one upstream please.

Comment 2

[Jeroen Roovers]

Attachment: firefox-26.0-remove-obsolete-code.patch

The previous patch applied against code already patched. This one should do it right, though. And no, this bug is not invalid.

Comment 3

[Jory A. Pratt [:anarchy]]

Attachment: bug-950822-fix.patch

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8348369 [details] [diff] [review]
bug-950822-fix.patch

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

I don't understand what this goal of the patch is, and why this change is safe.  It doesn't look obsolete to me.

Comment 5

[Jeroen Roovers]

Most of that code was moved to
mozilla-release/js/src/jsapi.cpp:[2274-]: void js::RecomputeStackLimit(JSRuntime *rt, StackKind kind)

What remains is obsolete and causes a compilation failure on systems where the stack grows up.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8348369 [details] [diff] [review]
bug-950822-fix.patch

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

bholley made those changes, so I'll defer to him.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8348369 [details] [diff] [review]
bug-950822-fix.patch

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

Hm, so the idea here was to have a default in case the embedding never calls JS_SetNativeStackQuota.

It looks like this member moved from the JSRuntime onto PerThreadData at some point, and now that initialization happens in the PerThreadDataFriendFields. Shouldn't the code in the constructor do something different in the upward-growth case?

What platforms does this affect, by the way? Has compilation been busted on those platforms since all this happened? Does Mozilla build/run on those platforms with this patch?

Comment 8

[Ian Stakenvicius]

Attachment: bug-950822-fix-by-updating-instead-of-removing.patch

Primary platform is hppa, but it affects any platform (ia64 maybe?) that has an upward-growing stack.  And yes, compilation has been busted for a long time.

In case removal is not deemed safe, the following patch will at least actually allow compilation and operate in the spirit of what the old/obsolete code used to do.  However, this would only be necessary if js::RecalculateStackLimit() (via SetNativeStackQuota() or some other call) isn't done early enough after the JSRuntime object is created.

Comment 9

[Ian Stakenvicius]

Note, attachment #8416663 [details] [diff] [review] was reviewed on irc by sfink and mjrosenb already.  Note also that attachment #8334171 [details] [diff] [review] from bug #940061 also still needs landing to support compilation/operation on hppa.

Also, I haven't received a run-time test report from this bug, yet; still working on that.  Would like to run it through try after r+ to get confirmation that it doesn't break any of the other systems.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8416663 [details] [diff] [review]
bug-950822-fix-by-updating-instead-of-removing.patch

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

This should go in PerThreadDataFriendFields::PerThreadDataFriendFields in jsfriendapi.cpp to get this on all threads, right?

Comment 11

[Ian Stakenvicius]

FYI, attachment 8348369 [details] [diff] [review] is confirmed runtime-tested and working on HPPA (when the other patch from bug 940061 is used) -- firefox-29 built successfully and loading html5test.com without issue: "Your browser scores 418 out of 555 points".

The reason for this would be because, within a few operations of a JSRuntime being created, SetNativeStackQuota (or a variant of it) is being called, which sets the nativeStackLimit appropriately.

This code should be safe to just drop.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Ian Stakenvicius from comment #11)
> This code should be safe to just drop.

But wouldn't it be safer to just do what I suggest in comment 10?

Comment 13

[Jeroen Roovers]

(In reply to Bobby Holley (:bholley) from comment #12)
> (In reply to Ian Stakenvicius from comment #11)
> > This code should be safe to just drop.
> 
> But wouldn't it be safer to just do what I suggest in comment 10?

Because all of the rest of the code that assigned a value to nativeStackLimit at that spot was removed, but not this code path (an oversight). Nothing in the entire file sets nativeStackLimit now, except some code that doesn't even compile. After removing the offending code, FF compiles and runs fine (if slow, for other reasons) on HPPA/Linux.

I don't see what you're trying to achieve here. We had bad code that didn't compile, and that's probably not going to change soon. Now my patch makes it compile and run as badly as it ever did, but that's not good enough? Don't assume for a second that on HPPA/Linux you get a proper "Firefox Experience". It takes hours to compiles and it runs too slow to be usable. Improving that is going to take a lot more, but first we need to compile something that runs at all, preferably somewhere in December 2013...

Comment 14

[Ian Stakenvicius]

Attachment: bad code removal, UINTPTR_MAX initiailization

better?  note, patch cleaned up to apply cleanly against m-c as of an hour ago.  Please attribute patch to Jeroen

Comment 15

[Bobby Holley (:bholley)]

(In reply to Jeroen Roovers from comment #13)
> > But wouldn't it be safer to just do what I suggest in comment 10?
> 
> Because all of the rest of the code that assigned a value to
> nativeStackLimit at that spot was removed, but not this code path (an
> oversight).

Yes, and the code that assigned to nativeStackLimit moved to jsfriendapi.cpp, without support for upward growing stacks.

> I don't see what you're trying to achieve here.

I'm trying to help you avoid a potential bug in the platform you're trying to support?

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8416734 [details] [diff] [review]
bad code removal, UINTPTR_MAX initiailization

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

r=bholley with that nit fixed.

::: js/src/jsfriendapi.cpp
@@ +39,5 @@
>  PerThreadDataFriendFields::PerThreadDataFriendFields()
>  {
>      PodArrayZero(nativeStackLimit);
> +#if JS_STACK_GROWTH_DIRECTION > 0
> +    for (int i=0;i<StackKindCount;i++)

Need spaces after each semicolon.

Comment 17

[Ian Stakenvicius]

Attachment: bad code removal, UINTPTR_MAX initiailization, nit-free

nit-free ; apparently it's been too long since i've submitted to try though, so I can't get confirmation that the patch breaks nothing else (although its pretty obvious that it won't just from reading it)..  Could someone else run try on this and land it please?

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Ian Stakenvicius]

Could this get backported into mozilla-31 too?  It'd be nice for the patch to be carried through the next ESR (and the next spidermonkey release), and it only missed the deadline by what, 48h?

Comment 21

[Bobby Holley (:bholley)]

(In reply to Ian Stakenvicius from comment #20)
> Could this get backported into mozilla-31 too?  It'd be nice for the patch
> to be carried through the next ESR (and the next spidermonkey release), and
> it only missed the deadline by what, 48h?

You can flag the patch for approval-mozilla-aurora via the 'details' link, and state your case. :-)

Comment 22

[Ian Stakenvicius]

Comment on attachment 8416940 [details] [diff] [review]
bad code removal, UINTPTR_MAX initiailization, nit-free

[Approval Request Comment]
Bug caused by (feature/regressing bug #): rafactoring of code during mozilla-26
User impact if declined: compile failures on HPPA / up-growing-stack+big-endian platforms
Testing completed (on m-c, etc.): testing (success) on hppa platforms on Gentoo/Linux, #ifdef's ensure on effect on any other platform
Risk to taking this patch (and alternatives if risky): none, except compilation will continue to fail on hppa and sources will need to be externally patched
String or IDL/UUID changes made by this patch: none

It would be very helpful if this could be carried back to mozilla-31 , so that the coebase for the next ESR and the next spidermonkey release will build on HPPA.

Comment 23

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

Attachment: Add an informational SIGSEGV for JS_DISABLE_SLOW_SCRIPT_SIGNALS

Is there a good web link that describes what's going on here that I could use instead of this verbose comment? I don't think this bug is good enough. Maybe one of the other bugs. It'd be good to point to something that discusses both the env var and the gdb script (and how it gets autoinstalled for some uses.)

Comment 24

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

Doh! Wrong bug. Sorry.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f253c8cf8d2