Bug 201172

Summary:

Refactor to use VM& instead of VM* at as many places as possible.

Product:

WebKit

Reporter:

Mark Lam <mark.lam>

Component:

JavaScriptCore

Assignee:

Mark Lam <mark.lam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, sabouhallawa, tsavell, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
work in progress for EWS testing.	none
work in progress for EWS testing.	none
work in progress for EWS testing.	ews-watchlist: commit-queue-
proposed patch.	ysuzuki: review+

Mark Lam

Reported 2019-08-26 22:11:57 PDT

This better documents that the VM pointer is expected to almost never be null. There are very few places where it cane null. Those will be left using a VM*. Also converted some uses of ExecState* to using VM& instead since those ExecState* is only there to fetch the VM pointer. Doing this also reduces the number of times we have to compute the VM* from ExecState*. This patch is not exhaustive in converting to use VM&, but applies the change to many commonly used pieces of code for a start.

Attachments
work in progress for EWS testing. (1.45 MB, patch) 2019-08-27 00:53 PDT, Mark Lam	no flags	Details Formatted Diff Diff
work in progress for EWS testing. (1.45 MB, patch) 2019-08-27 01:33 PDT, Mark Lam	no flags	Details Formatted Diff Diff
work in progress for EWS testing. (1.45 MB, patch) 2019-08-27 01:51 PDT, Mark Lam	ews-watchlist: commit-queue-	Details Formatted Diff Diff
proposed patch. (1.45 MB, patch) 2019-08-27 11:06 PDT, Mark Lam	ysuzuki: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Mark Lam

Comment 1 2019-08-27 00:53:28 PDT

Created attachment 377324 [details] work in progress for EWS testing.

EWS Watchlist

Comment 2 2019-08-27 00:57:28 PDT

Attachment 377324 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSCast.h:36: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/JavaScriptCore/runtime/JSCast.h:44: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:108: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/CodeBlock.h:1066: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:588: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:140: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 6 in 411 files If any of these errors are false positives, please file a bug against check-webkit-style.

Mark Lam

Comment 3 2019-08-27 01:33:41 PDT

Created attachment 377328 [details] work in progress for EWS testing.

EWS Watchlist

Comment 4 2019-08-27 01:37:01 PDT

Attachment 377328 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSCast.h:36: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/JavaScriptCore/runtime/JSCast.h:44: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:108: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/CodeBlock.h:1066: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:588: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:140: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 6 in 412 files If any of these errors are false positives, please file a bug against check-webkit-style.

Mark Lam

Comment 5 2019-08-27 01:51:25 PDT

Created attachment 377330 [details] work in progress for EWS testing.

EWS Watchlist

Comment 6 2019-08-27 01:55:40 PDT

Attachment 377330 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSCast.h:36: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/JavaScriptCore/runtime/JSCast.h:44: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:108: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/CodeBlock.h:1066: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:588: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:140: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 6 in 413 files If any of these errors are false positives, please file a bug against check-webkit-style.

EWS Watchlist

Comment 7 2019-08-27 04:09:19 PDT

Comment on attachment 377330 [details] work in progress for EWS testing. Attachment 377330 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12972553 New failing tests: stress/regress-189132.js.ftl-eager stress/regress-189132.js.ftl-no-cjit-b3o0 stress/regress-189132.js.ftl-no-cjit-small-pool stress/regress-189132.js.ftl-no-cjit-no-inline-validate stress/regress-189132.js.no-cjit-validate-phases stress/regress-189132.js.mini-mode stress/regress-189132.js.ftl-eager-no-cjit stress/regress-189132.js.dfg-eager-no-cjit-validate stress/regress-189132.js.no-cjit-collect-continuously stress/regress-189132.js.bytecode-cache stress/regress-189132.js.ftl-no-cjit-no-put-stack-validate stress/regress-189132.js.default stress/regress-189132.js.no-ftl stress/regress-189132.js.eager-jettison-no-cjit stress/regress-189132.js.ftl-no-cjit-validate-sampling-profiler stress/regress-189132.js.no-llint stress/regress-189132.js.dfg-eager stress/regress-189132.js.ftl-eager-no-cjit-b3o1

Mark Lam

Comment 8 2019-08-27 11:06:27 PDT

Created attachment 377353 [details] proposed patch.

Yusuke Suzuki

Comment 9 2019-08-27 15:01:00 PDT

Comment on attachment 377353 [details] proposed patch. r=me

Mark Lam

Comment 10 2019-08-27 15:16:12 PDT

Thanks for the review. Landed in r249175: <http://trac.webkit.org/r249175>.

Radar WebKit Bug Importer

Comment 11 2019-08-27 15:17:21 PDT

<rdar://problem/54765752>

Said Abou-Hallawa

Comment 12 2019-08-27 23:02:37 PDT

Followup patch is r249175.

Mark Lam

Comment 13 2019-08-27 23:17:02 PDT

(In reply to Said Abou-Hallawa from comment #12) > Followup patch is r249175. I think you meant build fix in http://trac.webkit.org/r249187. Thanks for the fix.

Truitt Savell

Comment 14 2019-08-28 08:28:29 PDT

It looks like the changes in https://trac.webkit.org/changeset/249175/webkit broke builtins-generator-tests. Build: https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/6206 I was able to reproduce this failure on 249175 but not on 249174

Mark Lam

Comment 15 2019-08-28 08:37:42 PDT

(In reply to Truitt Savell from comment #14) > broke builtins-generator-tests. > I was able to reproduce this failure on 249175 but not on 249174 Rebased test results in r249199: <http://trac.webkit.org/r249199>.

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