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

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jer, Assigned: axs)

Tracking

26 Branch
mozilla32
Other
Linux
Points:
---

Firefox Tracking Flags

(firefox30 wontfix, firefox31 fixed, firefox32 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 6 obsolete attachments)

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.
This code is introduced by Gentoo. Leave bugs downstream unless myself or another gentoo mozilla dev ask you to open one upstream please.
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
The previous patch applied against code already patched. This one should do it right, though. And no, this bug is not invalid.
Attachment #8348222 - Attachment is obsolete: true
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Posted patch bug-950822-fix.patch (obsolete) — Splinter Review
Attachment #8348264 - Attachment is obsolete: true
Attachment #8348369 - Flags: review?(n.nethercote)
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
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.
Attachment #8348369 - Flags: review?(n.nethercote) → review-
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 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.
Attachment #8348369 - Flags: review- → review?(bobbyholley+bmo)
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?
Attachment #8348369 - Flags: review?(bobbyholley+bmo) → review-
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.
Attachment #8416663 - Flags: review?(bobbyholley)
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 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?
Attachment #8416663 - Flags: review?(bobbyholley) → review-
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.
(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?
(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...
better?  note, patch cleaned up to apply cleanly against m-c as of an hour ago.  Please attribute patch to Jeroen
Attachment #8416663 - Attachment is obsolete: true
Attachment #8416734 - Flags: review?(bobbyholley)
(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 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.
Attachment #8416734 - Flags: review?(bobbyholley) → review+
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?
Attachment #8348369 - Attachment is obsolete: true
Attachment #8416734 - Attachment is obsolete: true
Attachment #8416940 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c54f91eb0cfe
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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?
(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 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.
Attachment #8416940 - Flags: approval-mozilla-aurora?
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.)
Attachment #8418281 - Flags: review?(luke)
Assignee: axs → sphink
Assignee: sphink → axs
Attachment #8418281 - Attachment is obsolete: true
Attachment #8418281 - Flags: review?(luke)
Doh! Wrong bug. Sorry.
Attachment #8416940 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.