Bug 203282 - [JSC] Remove LLInt's Callee size assumption
Summary: [JSC] Remove LLInt's Callee size assumption
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 203260
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-22 18:11 PDT by Yusuke Suzuki
Modified: 2019-10-24 13:28 PDT (History)
7 users (show)

See Also:


Attachments
Patch (29.61 KB, patch)
2019-10-24 00:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (910.67 KB, application/zip)
2019-10-24 04:27 PDT, EWS Watchlist
no flags Details
Patch (29.72 KB, patch)
2019-10-24 11:22 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (29.67 KB, patch)
2019-10-24 11:43 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-10-22 18:11:30 PDT
331     loadp Callee[cfr], t3
   332     andp MarkedBlockMask, t3
   333     loadp MarkedBlockFooterOffset + MarkedBlock::Footer::m_vm[t3], t3

This will be wrong.
Comment 1 Yusuke Suzuki 2019-10-24 00:48:22 PDT
Created attachment 381784 [details]
Patch
Comment 2 EWS Watchlist 2019-10-24 04:24:36 PDT
Comment on attachment 381784 [details]
Patch

Attachment 381784 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13173398

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
Comment 3 EWS Watchlist 2019-10-24 04:27:36 PDT
Comment on attachment 381784 [details]
Patch

Attachment 381784 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13173450

Number of test failures exceeded the failure limit.
Comment 4 EWS Watchlist 2019-10-24 04:27:38 PDT
Created attachment 381799 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 5 Yusuke Suzuki 2019-10-24 11:22:41 PDT
Created attachment 381826 [details]
Patch
Comment 6 Yusuke Suzuki 2019-10-24 11:43:04 PDT
Created attachment 381829 [details]
Patch
Comment 7 Mark Lam 2019-10-24 13:10:29 PDT
Comment on attachment 381829 [details]
Patch

r=me
Comment 8 Keith Miller 2019-10-24 13:20:18 PDT
Comment on attachment 381829 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:15
> +           exception unwinding happens, and it is already taking some time.

Nit: maybe change "and it is already taking some time" => "which is an expensive operation anyway"

> Source/JavaScriptCore/heap/LargeAllocation.h:155
> +    static constexpr unsigned headerSize()
> +    {
> +        return ((sizeof(LargeAllocation) + halfAlignment - 1) & ~(halfAlignment - 1)) | halfAlignment;
> +    }

Nit: Can we put this on one line?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2045
> +    if X86 or X86_WIN
> +        addp 8, sp
> +    elsif MIPS
> +        addp 24, sp
> +    else
> +        addp 8, sp
> +    end

Can this just be:
if MIPS 
    addp 24, sp
else
    addp 8, sp
end
Comment 9 Yusuke Suzuki 2019-10-24 13:22:22 PDT
Comment on attachment 381829 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:15
>> +           exception unwinding happens, and it is already taking some time.
> 
> Nit: maybe change "and it is already taking some time" => "which is an expensive operation anyway"

OK, changed.

>> Source/JavaScriptCore/heap/LargeAllocation.h:155
>> +    }
> 
> Nit: Can we put this on one line?

Done.

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2045
>> +    end
> 
> Can this just be:
> if MIPS 
>     addp 24, sp
> else
>     addp 8, sp
> end

Sounds nice, fixed.
Comment 10 Yusuke Suzuki 2019-10-24 13:27:42 PDT
Committed r251556: <https://trac.webkit.org/changeset/251556>
Comment 11 Radar WebKit Bug Importer 2019-10-24 13:28:16 PDT
<rdar://problem/56591933>