Bug 180653 - LLInt: reserve 16 bytes of stack on MIPS for native calls
Summary: LLInt: reserve 16 bytes of stack on MIPS for native calls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-11 10:00 PST by Guillaume Emont
Modified: 2017-12-12 11:03 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.13 KB, patch)
2017-12-11 10:07 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Guillaume Emont 2017-12-11 10:00:52 PST
The calling convention on MIPS is that the caller reserves 16 bytes (4 words) of stack space for all calls (regardless of the number of arguments). The callee can use that space. in LLInt's nativeCallTrampoline (which is only used if JIT is disabled), we effectively only save 8 bytes when adjusting the alignment. This crashes many date-related tests when running in llint-only mode when compiling in -Os.
Comment 1 Guillaume Emont 2017-12-11 10:02:41 PST
Note that it does not crash when the JIT is enabled, because it seems we use the thunk generated by JSC::nativeForGenerator() even when calling from llint. This thunk correctly allocates the 16 bytes of stack.
Comment 2 Guillaume Emont 2017-12-11 10:07:09 PST
Created attachment 328994 [details]
Patch

Patch fixing the issue.
Comment 3 Adrian Perez 2017-12-11 12:23:23 PST
Comment on attachment 328994 [details]
Patch

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

Informal review: r+, with a comment. Please check it before landing.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2086
> +        # calling convention says to save stack space for 4 first registers in

Wow, good find. A similar scheme is used in the
“internalFunctionCallTrampoline()” macro but this patch does NOT
modify it accordingly. Is that intended?
Comment 4 Guillaume Emont 2017-12-11 12:48:53 PST
> 
> Wow, good find. A similar scheme is used in the
> “internalFunctionCallTrampoline()” macro but this patch does NOT
> modify it accordingly. Is that intended?

My understanding is that nativeCallTrampoline() is used to call C++ (native) functions, and internalFunctionCallTrampoline() is used to call js (internal) functions. Neither LLInt nor the JIT use that extra space, so we can save some memory by not allocating it.
Comment 5 Adrian Perez 2017-12-11 13:54:41 PST
(In reply to Guillaume Emont from comment #4)
> > 
> > Wow, good find. A similar scheme is used in the
> > “internalFunctionCallTrampoline()” macro but this patch does NOT
> > modify it accordingly. Is that intended?
> 
> My understanding is that nativeCallTrampoline() is used to call C++ (native)
> functions, and internalFunctionCallTrampoline() is used to call js
> (internal) functions. Neither LLInt nor the JIT use that extra space, so we
> can save some memory by not allocating it.

That was also my suspicion, okay then!
Comment 6 Carlos Alberto Lopez Perez 2017-12-12 08:48:39 PST
Comment on attachment 328994 [details]
Patch

r=me (trusting Adrian review)
Comment 7 WebKit Commit Bot 2017-12-12 10:40:33 PST
Comment on attachment 328994 [details]
Patch

Clearing flags on attachment: 328994

Committed r225788: <https://trac.webkit.org/changeset/225788>
Comment 8 WebKit Commit Bot 2017-12-12 10:40:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-12-12 11:03:39 PST
<rdar://problem/35998684>