Bug 125979 - Clarified stack maintenance code
Summary: Clarified stack maintenance code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-18 22:10 PST by Geoffrey Garen
Modified: 2013-12-19 09:19 PST (History)
3 users (show)

See Also:


Attachments
Patch (24.80 KB, patch)
2013-12-18 22:25 PST, Geoffrey Garen
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2013-12-18 22:10:11 PST
Clarified stack maintainence code
Comment 1 Geoffrey Garen 2013-12-18 22:25:57 PST
Created attachment 219616 [details]
Patch
Comment 2 Mark Lam 2013-12-18 23:26:12 PST
Comment on attachment 219616 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219616&action=review

Nice. r=me too with unused code removed.

> Source/JavaScriptCore/llint/LLIntEntrypoint.cpp:137
> +int stackPointerOffsetFor(CodeBlock* codeBlock)
> +{
> +    return virtualRegisterForLocal(frameRegisterCountFor(codeBlock) - 1).offset();
> +}
> +

Is this used anywhere?  If not, it’s better to remove it.

> Source/JavaScriptCore/llint/LLIntEntrypoint.h:45
> +int stackPointerOffsetFor(CodeBlock*);

Ditto ... not used.
Comment 3 Mark Lam 2013-12-18 23:53:18 PST
I see that DFGOSREntry.cpp and FTLOSREntry.cpp still uses JSStack::grow() with the expectation that it takes a "past the end" pointer, but that is inconsequential considering they should be checking against the C stack limits instead of the JSStack (via grow()).  These appear to be part of several stack checking issues which still remains broken in the jsCStack branch.  I'll take care of these in https://bugs.webkit.org/show_bug.cgi?id=125928 when I fix the other stack issues I reported there later.
Comment 4 Geoffrey Garen 2013-12-19 09:00:31 PST
> > Source/JavaScriptCore/llint/LLIntEntrypoint.cpp:137
> > +int stackPointerOffsetFor(CodeBlock* codeBlock)
> > +{
> > +    return virtualRegisterForLocal(frameRegisterCountFor(codeBlock) - 1).offset();
> > +}
> > +
> 
> Is this used anywhere?  If not, it’s better to remove it.

Oops! Will remove.
Comment 5 Geoffrey Garen 2013-12-19 09:19:23 PST
<http://trac.webkit.org/changeset/160835>