Bug 129181 – Auto generate bytecode information for bytecode parser and LLInt

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

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.

[Michael Saboff]

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.

[WebKit Commit Bot]

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.

[Mark Lam]

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.

[Mark Lam]

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.

[Michael Saboff]

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.

[WebKit Commit Bot]

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.

[Michael Saboff]

Created attachment 224949 [details]
This time without the temp changes to Platform.h to build with the C_LOOP enabled

[WebKit Commit Bot]

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.

[Michael Saboff]

Created attachment 224974 [details]
Speculative build update

[WebKit Commit Bot]

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.

[Michael Saboff]

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.

[WebKit Commit Bot]

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.

[Michael Saboff]

Created attachment 225203 [details]
Updated to fix EFL and Windows builds

[WebKit Commit Bot]

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.

[Mark Lam]

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.

[Csaba Osztrogonác]

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. ;)

[Michael Saboff]

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.

[Michael Saboff]

Committed r164732: <http://trac.webkit.org/changeset/164732>

[Mark Lam]

(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. =(

[Csaba Osztrogonác]

(In reply to comment #18)
> Committed r164732: <http://trac.webkit.org/changeset/164732>

bdash rolled it out by http://trac.webkit.org/changeset/164746

[Michael Saboff]

Created attachment 225307 [details]
Patch for landing with build fix for Mac internal builds

[Michael Saboff]

Created attachment 225309 [details]
This time with the added files actually added to the patch

[Michael Saboff]

Committed r164814: <http://trac.webkit.org/changeset/164814>