Bug 242579

Summary:	VM should embed Interpreter.
Product:	WebKit	Reporter:	Mark Lam <mark.lam>
Component:	JavaScriptCore	Assignee:	Michael Catanzaro <mcatanzaro>
Status:	RESOLVED FIXED
Severity:	Normal	CC:	mcatanzaro, webkit-bug-importer
Priority:	P2	Keywords:	InRadar
Version:	WebKit Nightly Build
Hardware:	Unspecified
OS:	Unspecified

Description Mark Lam 2022-07-10 15:00:13 PDT

1. The Interpreter class is empty for release non-cloop builds.
2. There is always a 1:1 pairing between VM and Interpreter.

So, there's no point in malloc'ing the Interpreter separately.

Comment 1 Mark Lam 2022-07-10 15:04:49 PDT

Pull request: https://github.com/WebKit/WebKit/pull/2281

Comment 2 EWS 2022-07-11 03:31:10 PDT

Committed 252338@main (16e1928aae2e): <https://commits.webkit.org/252338@main>

Reviewed commits have been landed. Closing PR #2281 and removing active labels.

Comment 3 Radar WebKit Bug Importer 2022-07-11 03:32:17 PDT

<rdar://problem/96817609>

Comment 4 Michael Catanzaro 2022-07-12 15:38:59 PDT

fyi, this broke the cloop build. It's a shame we don't have an EWS for it. Follow-up incoming.

Comment 5 Mark Lam 2022-07-12 15:40:31 PDT

(In reply to Michael Catanzaro from comment #4)
> fyi, this broke the cloop build. It's a shame we don't have an EWS for it.
> Follow-up incoming.

How did this break the CLoop?  I deliberately tested for it.  The jsc-i386 bot builds CLoop AFAIK.  When I checked (before landing), all was well.

Please share your details about what actually broke.

Comment 6 Michael Catanzaro 2022-07-12 15:51:40 PDT

(In reply to Michael Catanzaro from comment #4)
> Follow-up incoming.

This will probably be tomorrow. It should just be:

diff --git a/Source/JavaScriptCore/CMakeLists.txt b/Source/JavaScriptCore/CMakeLists.txt
index aef0678e66f3..ee5d8a509284 100644
--- a/Source/JavaScriptCore/CMakeLists.txt
+++ b/Source/JavaScriptCore/CMakeLists.txt
@@ -840,6 +840,7 @@ set(JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS
     interpreter/CallFrame.h
     interpreter/CallFrameInlines.h
     interpreter/CalleeBits.h
+    interpreter/CLoopStack.h
     interpreter/EntryFrame.h
     interpreter/FrameTracers.h
     interpreter/Interpreter.h

but I need to confirm, and am testing something else right now.

Comment 7 Michael Catanzaro 2022-07-12 15:52:11 PDT

(In reply to Mark Lam from comment #5)
> Please share your details about what actually broke.

In file included from /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/PrivateHeaders/JavaScriptCore/VM.h:41,
                 from /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/PrivateHeaders/JavaScriptCore/ExceptionScope.h:28,
                 from /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/PrivateHeaders/JavaScriptCore/CatchScope.h:28,
                 from /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/PrivateHeaders/JavaScriptCore/JSCJSValueInlines.h:28,
                 from /home/mcatanzaro/Projects/WebKit/Tools/TestWebKitAPI/Tests/JavaScriptCore/PropertySlot.cpp:28:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/PrivateHeaders/JavaScriptCore/Interpreter.h:39:10: fatal error: CLoopStack.h: No such file or directory
   39 | #include "CLoopStack.h"
      |          ^~~~~~~~~~~~~~
compilation terminated.

Comment 8 Michael Catanzaro 2022-07-12 15:52:54 PDT

(In reply to Mark Lam from comment #5)
> How did this break the CLoop?  I deliberately tested for it.  The jsc-i386
> bot builds CLoop AFAIK.  When I checked (before landing), all was well.

Maybe it doesn't build TestWebKitAPI?

Comment 9 Mark Lam 2022-07-12 15:56:00 PDT

(In reply to Michael Catanzaro from comment #8)
> (In reply to Mark Lam from comment #5)
> > How did this break the CLoop?  I deliberately tested for it.  The jsc-i386
> > bot builds CLoop AFAIK.  When I checked (before landing), all was well.
> 
> Maybe it doesn't build TestWebKitAPI?

Yeah, it's a jsc build bot.  Locally, I also only tested building jsc.  Is this build failure manifesting when building TestWebKitAPI?

Comment 10 Michael Catanzaro 2022-07-12 15:56:45 PDT

Yes, the problem is the header is not available to TestWebKitAPI.

My patch works... I'll submit a PR.

Comment 11 Michael Catanzaro 2022-07-12 16:00:31 PDT

Re-opening for pull request https://github.com/WebKit/WebKit/pull/2349

Comment 12 EWS 2022-07-12 16:58:00 PDT

Committed 252400@main (40e49bb53218): <https://commits.webkit.org/252400@main>

Reviewed commits have been landed. Closing PR #2349 and removing active labels.