Bug 129181 - Auto generate bytecode information for bytecode parser and LLInt
Summary: Auto generate bytecode information for bytecode parser and LLInt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 129178
  Show dependency treegraph
 
Reported: 2014-02-21 15:54 PST by Michael Saboff
Modified: 2014-02-27 10:43 PST (History)
13 users (show)

See Also:


Attachments
Patch (46.31 KB, patch)
2014-02-21 16:06 PST, Michael Saboff
mark.lam: review-
Details | Formatted Diff | Diff
Patch with build fixes (47.20 KB, patch)
2014-02-21 18:00 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
This time without the temp changes to Platform.h to build with the C_LOOP enabled (46.74 KB, patch)
2014-02-21 22:56 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Speculative build update (46.73 KB, patch)
2014-02-22 10:02 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Reworked patch (55.44 KB, patch)
2014-02-25 16:21 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Updated to fix EFL and Windows builds (56.28 KB, patch)
2014-02-25 17:28 PST, Michael Saboff
mark.lam: review+
Details | Formatted Diff | Diff
Patch for landing (58.73 KB, patch)
2014-02-26 10:23 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch for landing with build fix for Mac internal builds (40.67 KB, patch)
2014-02-26 15:25 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
This time with the added files actually added to the patch (59.22 KB, patch)
2014-02-26 15:29 PST, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2014-02-21 15:54:23 PST
Currently, byte codes are defined in bytecode/Opcode.h listed in a macro.  That macro is used in various places to initialize byte code related tables.  It is used by the LLInt to create LLIntData::Data::s_opcodeMap.  We are planning on having the LLInt populate a vector of entry points since we are moving to the llint making all current entry points as locals.  This bug is to generate the byte code list currently in Opcode.h and what is needed by the llint assembly language from a coming source.
Comment 1 Michael Saboff 2014-02-21 16:06:31 PST
Created attachment 224922 [details]
Patch

Tested building for MacOSX and iOS.  Made speculative changes in makefiles for other platforms.  Will look at EWS build results.
Comment 2 WebKit Commit Bot 2014-02-21 16:09:26 PST
Attachment 224922 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/Opcode.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/Opcode.h:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2014-02-21 16:31:48 PST
Comment on attachment 224922 [details]
Patch

Please also do a C loop build and ensure that it does build and run.  We now have a LLINT C Loop build and test bot.  So, we need to keep that green.

I was thinking that you can also remove FOR_EACH_LLINT_OPCODE_EXTENTION and therefore LLIntOpcode.h.  However, when I  took a look at LLIntOpcode.h, I saw synthetic llint_cloop_did_return_from_js_X opcodes which tells me that I don't think you can remove FOR_EACH_LLINT_OPCODE_EXTENTION.  This also means that you can't remove the distinction between FOR_EACH_OPCODE_ID and FOR_EACH_CORE_OPCODE_ID.

If you do a C loop LLINT build, you should be able to resolve what you can and cannot do here.
Comment 4 Mark Lam 2014-02-21 16:46:35 PST
Comment on attachment 224922 [details]
Patch

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

> Source/JavaScriptCore/bytecode/Opcode.h:45
>  #define OPCODE_ID_ENUM(opcode, length) opcode,
> -    typedef enum { FOR_EACH_OPCODE_ID(OPCODE_ID_ENUM) } OpcodeID;
> +    typedef enum { FOR_EACH_CORE_OPCODE_ID(OPCODE_ID_ENUM) } OpcodeID;
>  #undef OPCODE_ID_ENUM

After talking with Michael in person, I think most of the changes from using FOR_EACH_OPCODE_ID() TO FOR_EACH_CORE_OPCODE_ID() will work out.  However, I think this one with the OpcodeID enum will still cause problems when building the C loop LLINT.

With the C loop LLINT, please also try a build with HAVE_COMPUTED_GOTO set to 0 in Platform.h.  I think this change above will break that case.
Comment 5 Michael Saboff 2014-02-21 18:00:58 PST
Created attachment 224937 [details]
Patch with build fixes

Made 2 fixes:
    Restored line inadvertantly split in JavaScriptCore/CMakeLists.txt
    Changed JavaScriptCore/generate-bytecode-files to use optparse instead of argparse given that Windows uses Python 2.6.8

This doesn't have needed changes for C_LOOP builds, posting for EWS checks.
Comment 6 WebKit Commit Bot 2014-02-21 18:03:37 PST
Attachment 224937 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/Opcode.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/Opcode.h:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Michael Saboff 2014-02-21 22:56:27 PST
Created attachment 224949 [details]
This time without the temp changes to Platform.h to build with the C_LOOP enabled
Comment 8 WebKit Commit Bot 2014-02-22 07:51:38 PST
Attachment 224949 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/Opcode.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/Opcode.h:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Michael Saboff 2014-02-22 10:02:13 PST
Created attachment 224974 [details]
Speculative build update
Comment 10 WebKit Commit Bot 2014-02-22 10:03:18 PST
Attachment 224974 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/Opcode.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/Opcode.h:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Michael Saboff 2014-02-25 16:21:56 PST
Created attachment 225194 [details]
Reworked patch

Reworked to have more details in the byte code list file.  Changed the file from a simple text file to a JSON file.  The generator now builds not only the main byte code "macro()" list, but the lists used by LLInt code.

Tested on Mac both the standard build as well as the two C Loop flavors.  It also builds on windows.
Comment 12 WebKit Commit Bot 2014-02-25 16:24:09 PST
Attachment 225194 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/Opcode.h:66:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Michael Saboff 2014-02-25 17:28:26 PST
Created attachment 225203 [details]
Updated to fix EFL and Windows builds
Comment 14 WebKit Commit Bot 2014-02-25 17:31:20 PST
Attachment 225203 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/Opcode.h:66:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Mark Lam 2014-02-26 02:47:31 PST
Comment on attachment 225203 [details]
Updated to fix EFL and Windows builds

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

Looks good.  r=me with issues addressed.  Please also fix the EFL build.  Looks like it’s failing to generate Bytecodes.h.  Or send email to webkit-dev to notify EFL engineers of a need for some attention here.

> Source/JavaScriptCore/CMakeLists.txt:614
> +    # changed the command will always be called because the mtime of .h files will

“.h files” => “the .h files”.

> Source/JavaScriptCore/CMakeLists.txt:616
> +    # Additionally, setting the OBJECT_DEPENDS property will make .h files a Makefile

“.h files” ==> “the .h files”.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:43
> +		65788A9D18B409EB00C189FF /* Offline Assembler */ = {
> +			isa = PBXAggregateTarget;
> +			buildConfigurationList = 65788AA218B409EB00C189FF /* Build configuration list for PBXAggregateTarget "Offline Assembler" */;
> +			buildPhases = (
> +				65788AA018B409EB00C189FF /* Generate Derived Sources */,
> +			);
> +			dependencies = (
> +				65788A9E18B409EB00C189FF /* PBXTargetDependency */,
> +			);
> +			name = "Offline Assembler";
> +			productName = "Derived Sources";
> +		};

Is this needed?  I’m no Xcode expert, but this blob suggests that it is for generating the offline assembler.  If so, how is it that we didn’t need this before?

Also, it looks suspicious to me that the buildPhases comment says "/* Generate Derived Sources */“ and the productName says "Derived Sources”.  Are these cut and paste errors?

> Source/JavaScriptCore/bytecode/BytecodeList.json:61
> +            { "name" : "op_get_by_id", "length" : 9  },
> +            { "name" : "op_get_by_id_out_of_line", "length" : 9  },
> +            { "name" : "op_get_array_length", "length" : 9 },

The original definitions of these bytecodes in Opcode.h has a comment /* has value profiling */ that is now lost.  If it’s not too much effort, would it be possible to add support for trailing // comments so that we can preserve those comments?  If it’s too much effort, we can leave adding comment support for a later time, and just file a bug to track it so that we don’t forget.

> Source/JavaScriptCore/bytecode/BytecodeList.json:112
> +            { "name" : "op_get_from_scope", "length" : 8 },

Ditto with the /* has value profiling */ comment.

> Source/JavaScriptCore/bytecode/Opcode.h:65
> +#if ENABLE(LLINT) && ENABLE(LLINT_C_LOOP)
> +    + NUMBER_OF_BYTECODE_HELPER_IDS + NUMBER_OF_CLOOP_BYTECODE_HELPER_IDS
> +#endif

ENABLE(LLINT_C_LOOP) implies ENABLE(LLINT_C_LOOP).  You don’t need to specify ENABLE(LLINT) here.
Comment 16 Csaba Osztrogonác 2014-02-26 09:28:53 PST
To fix the EFL build you should add "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}"
to Source/WebKit/CMakeLists.txt and Source/WebKit2/CMakeLists.txt as
include directory. I tested, build works and jsc tests pass. ;)
Comment 17 Michael Saboff 2014-02-26 10:23:23 PST
Created attachment 225265 [details]
Patch for landing

Already reviewed by Mark Lam.

(In reply to comment #15)
> (From update of attachment 225203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225203&action=review
> 
> Looks good.  r=me with issues addressed.  Please also fix the EFL build.  Looks like it’s failing to generate Bytecodes.h.  Or send email to webkit-dev to notify EFL engineers of a need for some attention here.
> 
> > Source/JavaScriptCore/CMakeLists.txt:614
> > +    # changed the command will always be called because the mtime of .h files will
> 
> “.h files” => “the .h files”.

Done.

> > Source/JavaScriptCore/CMakeLists.txt:616
> > +    # Additionally, setting the OBJECT_DEPENDS property will make .h files a Makefile
> 
> “.h files” ==> “the .h files”.

Done.

> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:43
> > +		65788A9D18B409EB00C189FF /* Offline Assembler */ = {
> > +			isa = PBXAggregateTarget;
> > +			buildConfigurationList = 65788AA218B409EB00C189FF /* Build configuration list for PBXAggregateTarget "Offline Assembler" */;
> > +			buildPhases = (
> > +				65788AA018B409EB00C189FF /* Generate Derived Sources */,
> > +			);
> > +			dependencies = (
> > +				65788A9E18B409EB00C189FF /* PBXTargetDependency */,
> > +			);
> > +			name = "Offline Assembler";
> > +			productName = "Derived Sources";
> > +		};
>
> Is this needed?  I’m no Xcode expert, but this blob suggests that it is for generating the offline assembler.  If so, how is it that we didn’t need this before?
> 
> Also, it looks suspicious to me that the buildPhases comment says "/* Generate Derived Sources */“ and the productName says "Derived Sources”.  Are these cut and paste errors?

As I said in the ChangeLog, I broke out the Offline Assembler as a separate step.  I did this from within Xcode using a copy operation.  Clearly it didn't change all of the old "Derived Sources" references when I changed the name.  I manually made these changes and built JSC from within Xcode.

> > Source/JavaScriptCore/bytecode/BytecodeList.json:61
> > +            { "name" : "op_get_by_id", "length" : 9  },
> > +            { "name" : "op_get_by_id_out_of_line", "length" : 9  },
> > +            { "name" : "op_get_array_length", "length" : 9 },
> 
> The original definitions of these bytecodes in Opcode.h has a comment /* has value profiling */ that is now lost.  If it’s not too much effort, would it be possible to add support for trailing // comments so that we can preserve those comments?  If it’s too much effort, we can leave adding comment support for a later time, and just file a bug to track it so that we don’t forget.
> 
> > Source/JavaScriptCore/bytecode/BytecodeList.json:112
> > +            { "name" : "op_get_from_scope", "length" : 8 },
> 
> Ditto with the /* has value profiling */ comment.

Oh that we could add comments to JSON, but the spec has no such provision.

> > Source/JavaScriptCore/bytecode/Opcode.h:65
> > +#if ENABLE(LLINT) && ENABLE(LLINT_C_LOOP)
> > +    + NUMBER_OF_BYTECODE_HELPER_IDS + NUMBER_OF_CLOOP_BYTECODE_HELPER_IDS
> > +#endif
> 
> ENABLE(LLINT_C_LOOP) implies ENABLE(LLINT_C_LOOP).  You don’t need to specify ENABLE(LLINT) here.

I cleaned this up to:
#if ENABLE(LLINT_C_LOOP)
const int numOpcodeIDs = NUMBER_OF_BYTECODE_IDS + NUMBER_OF_BYTECODE_HELPER_IDS + NUMBER_OF_CLOOP_BYTECODE_HELPER_IDS;
#else
const int numOpcodeIDs = NUMBER_OF_BYTECODE_IDS;
#endif

This change also eliminated the style warning.

(In reply to comment #16)
> To fix the EFL build you should add "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}"
> to Source/WebKit/CMakeLists.txt and Source/WebKit2/CMakeLists.txt as
> include directory. I tested, build works and jsc tests pass. ;)

Thanks for the heads up and test build.  I made these changes.
Comment 18 Michael Saboff 2014-02-26 11:40:18 PST
Committed r164732: <http://trac.webkit.org/changeset/164732>
Comment 19 Mark Lam 2014-02-26 12:32:49 PST
(In reply to comment #17)
> > > Source/JavaScriptCore/bytecode/BytecodeList.json:112
> > > +            { "name" : "op_get_from_scope", "length" : 8 },
> > 
> > Ditto with the /* has value profiling */ comment.
> 
> Oh that we could add comments to JSON, but the spec has no such provision.

Darn. =(
Comment 20 Csaba Osztrogonác 2014-02-26 14:11:47 PST
(In reply to comment #18)
> Committed r164732: <http://trac.webkit.org/changeset/164732>

bdash rolled it out by http://trac.webkit.org/changeset/164746
Comment 21 Michael Saboff 2014-02-26 15:25:01 PST
Created attachment 225307 [details]
Patch for landing with build fix for Mac internal builds
Comment 22 Michael Saboff 2014-02-26 15:29:23 PST
Created attachment 225309 [details]
This time with the added files actually added to the patch
Comment 23 Michael Saboff 2014-02-27 10:43:11 PST
Committed r164814: <http://trac.webkit.org/changeset/164814>