191468 – LLInt VectorSizeOffset should be based on offset extraction

RESOLVED FIXED 191468

LLInt VectorSizeOffset should be based on offset extraction

https://bugs.webkit.org/show_bug.cgi?id=191468

Summary LLInt VectorSizeOffset should be based on offset extraction

Keith Miller

Reported 2018-11-09 06:45:10 PST

LLInt VectorSizeOffset should be based on PtrSize

Attachments
Patch (1.97 KB, patch) 2018-11-09 06:45 PST, Keith Miller	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (1.52 MB, application/zip) 2018-11-09 07:15 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews102 for mac-sierra (527.55 KB, application/zip) 2018-11-09 07:29 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews113 for mac-sierra (1.54 MB, application/zip) 2018-11-09 07:59 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (780.71 KB, application/zip) 2018-11-09 08:03 PST, EWS Watchlist	no flags	Details
Patch (4.96 KB, patch) 2018-11-09 08:22 PST, Keith Miller	ysuzuki: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Keith Miller

Comment 1 2018-11-09 06:45:31 PST

Created attachment 354337 [details] Patch

EWS Watchlist

Comment 2 2018-11-09 07:15:10 PST

Comment on attachment 354337 [details] Patch Attachment 354337 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9923489 Number of test failures exceeded the failure limit.

EWS Watchlist

Comment 3 2018-11-09 07:15:11 PST

Created attachment 354339 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Yusuke Suzuki

Comment 4 2018-11-09 07:22:16 PST

Comment on attachment 354337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354337&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:524 > +const VectorSizeOffset = PtrSize * 2 I think the size offset is `PtrSize + 4`, 4 for `unsigned` capacity. How about using Vector<XXX>::m_size instead?

Yusuke Suzuki

Comment 5 2018-11-09 07:27:12 PST

Comment on attachment 354337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354337&action=review >> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:524 >> +const VectorSizeOffset = PtrSize * 2 > > I think the size offset is `PtrSize + 4`, 4 for `unsigned` capacity. > How about using Vector<XXX>::m_size instead? BTW, VectorBufferOffset is wrongly used in some places of LLInt. It is also used for RefCountedArray (m_argumentValueProfiles). I think we should use RefCountedArray<XXX>::m_data instead.

EWS Watchlist

Comment 6 2018-11-09 07:29:53 PST

Comment on attachment 354337 [details] Patch Attachment 354337 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9923694 Number of test failures exceeded the failure limit.

EWS Watchlist

Comment 7 2018-11-09 07:29:54 PST

Created attachment 354342 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6

Keith Miller

Comment 8 2018-11-09 07:36:43 PST

Comment on attachment 354337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354337&action=review >>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:524 >>> +const VectorSizeOffset = PtrSize * 2 >> >> I think the size offset is `PtrSize + 4`, 4 for `unsigned` capacity. >> How about using Vector<XXX>::m_size instead? > > BTW, VectorBufferOffset is wrongly used in some places of LLInt. It is also used for RefCountedArray (m_argumentValueProfiles). > I think we should use RefCountedArray<XXX>::m_data instead. Ah, whoops. I don't think the LLInt supports using <> in the object offsets syntax, though.

EWS Watchlist

Comment 9 2018-11-09 07:59:13 PST

Comment on attachment 354337 [details] Patch Attachment 354337 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9923755 Number of test failures exceeded the failure limit.

EWS Watchlist

Comment 10 2018-11-09 07:59:14 PST

Created attachment 354343 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 11 2018-11-09 08:03:12 PST

Comment on attachment 354337 [details] Patch Attachment 354337 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9923747 Number of test failures exceeded the failure limit.

EWS Watchlist

Comment 12 2018-11-09 08:03:13 PST

Created attachment 354344 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Yusuke Suzuki

Comment 13 2018-11-09 08:05:00 PST

Comment on attachment 354337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354337&action=review >>>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:524 >>>> +const VectorSizeOffset = PtrSize * 2 >>> >>> I think the size offset is `PtrSize + 4`, 4 for `unsigned` capacity. >>> How about using Vector<XXX>::m_size instead? >> >> BTW, VectorBufferOffset is wrongly used in some places of LLInt. It is also used for RefCountedArray (m_argumentValueProfiles). >> I think we should use RefCountedArray<XXX>::m_data instead. > > Ah, whoops. I don't think the LLInt supports using <> in the object offsets syntax, though. How about `const VectorSizeOffset = SimpleJumpTable::branchOffsets::m_size`?

Keith Miller

Comment 14 2018-11-09 08:09:29 PST

Comment on attachment 354337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354337&action=review >>>>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:524 >>>>> +const VectorSizeOffset = PtrSize * 2 >>>> >>>> I think the size offset is `PtrSize + 4`, 4 for `unsigned` capacity. >>>> How about using Vector<XXX>::m_size instead? >>> >>> BTW, VectorBufferOffset is wrongly used in some places of LLInt. It is also used for RefCountedArray (m_argumentValueProfiles). >>> I think we should use RefCountedArray<XXX>::m_data instead. >> >> Ah, whoops. I don't think the LLInt supports using <> in the object offsets syntax, though. > > How about `const VectorSizeOffset = SimpleJumpTable::branchOffsets::m_size`? Ohh! Clever! I think I'd need to make a typedef for branchOffsets but I like the idea!

Keith Miller

Comment 15 2018-11-09 08:22:44 PST

Created attachment 354346 [details] Patch

Yusuke Suzuki

Comment 16 2018-11-09 08:25:56 PST

Comment on attachment 354346 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354346&action=review r=me, nice! > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:523 > +const VectorSizeOffset = Vector::m_size Nice!

Keith Miller

Comment 17 2018-11-09 08:40:16 PST

Committed r238031: <https://trac.webkit.org/changeset/238031>

Radar WebKit Bug Importer

Comment 18 2018-11-09 08:41:26 PST

<rdar://problem/45944924>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Keith Miller

Reported

2018-11-09 06:45 PST

Modified

2018-11-09 08:41 PST History

CC List

11 users Show

URL

Keywords InRadar

Depends on

Blocks