Bug 201172 - Refactor to use VM& instead of VM* at as many places as possible.
Summary: Refactor to use VM& instead of VM* at as many places as possible.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-26 22:11 PDT by Mark Lam
Modified: 2019-08-28 08:37 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 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.
Comment 1 Mark Lam 2019-08-27 00:53:28 PDT
Created attachment 377324 [details]
work in progress for EWS testing.
Comment 2 EWS Watchlist 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.
Comment 3 Mark Lam 2019-08-27 01:33:41 PDT
Created attachment 377328 [details]
work in progress for EWS testing.
Comment 4 EWS Watchlist 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.
Comment 5 Mark Lam 2019-08-27 01:51:25 PDT
Created attachment 377330 [details]
work in progress for EWS testing.
Comment 6 EWS Watchlist 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.
Comment 7 EWS Watchlist 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
Comment 8 Mark Lam 2019-08-27 11:06:27 PDT
Created attachment 377353 [details]
proposed patch.
Comment 9 Yusuke Suzuki 2019-08-27 15:01:00 PDT
Comment on attachment 377353 [details]
proposed patch.

r=me
Comment 10 Mark Lam 2019-08-27 15:16:12 PDT
Thanks for the review.  Landed in r249175: <http://trac.webkit.org/r249175>.
Comment 11 Radar WebKit Bug Importer 2019-08-27 15:17:21 PDT
<rdar://problem/54765752>
Comment 12 Said Abou-Hallawa 2019-08-27 23:02:37 PDT
Followup patch is r249175.
Comment 13 Mark Lam 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.
Comment 14 Truitt Savell 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
Comment 15 Mark Lam 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>.