Bug 131205 – LLInt interpreter code should be generated as part of one function

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

Currently each LLInt opcode as well as the various relate LLInt helpers are generated as separate global functions.  Instead, all of the opcode functions and related helpers should be in one global function.  This would help with stack traces and eliminate issues caused by the linker relocating the functions.  There should be one entry point, say _llint_entry.

[Michael Saboff]

Created attachment 228574 [details]
Compiles and runs X86 (32 & 64)

Builds and runs on X86 (32 & 64).

ToDo:
 * Add ARM support to LowLevelInterpreter.asm possibly with offline assembler support.
 * Change opcode labels to true locals.  lldb still thinks these are entry points even though they are exported.
 * Add more safeguards to expressions involving labels to offline assembler.

[Michael Saboff]

Created attachment 228649 [details]
Updated patch

Builds and runs on X86 (32 & 64), ARMv7 and ARM64.

ToDo:
 * Add more safeguards to expressions involving labels to offline assembler.
 * Full testing of ARM variants
 * Windows changes & build
 * C Loop
 * Makefile changes for non-Apple platforms

[Michael Saboff]

Created attachment 228891 [details]
Patch

Builds and tested on Mac 32 & 64, iOS 32 & 64, C Loop.  Builds on Windows 32 (with LLInt).

Speculative work for Arm Traditional.  Specifically does not run on MIPS and SH4.  Contact me for pointers how to get those platforms working again.

[Mark Lam]

Comment on attachment 228891 [details]
Patch

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

> Source/JavaScriptCore/llint/LLIntData.h:131
> +#if 1

Why #if 1?  Is this superfluous?

> Source/JavaScriptCore/llint/LowLevelInterpreter.h:52
> +#if 0

Why #if 0?  Why not just remove this section of code?  Do you anticipate it being used again in the near future?

[Michael Saboff]

(In reply to comment #4)
> (From update of attachment 228891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228891&action=review
> 
> > Source/JavaScriptCore/llint/LLIntData.h:131
> > +#if 1
> 
> Why #if 1?  Is this superfluous?
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.h:52
> > +#if 0
> 
> Why #if 0?  Why not just remove this section of code?  Do you anticipate it being used again in the near future?

I'll remove these.  They are holdovers from developing the patch.

[Mark Lam]

Comment on attachment 228891 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:40
> +        well as including file from other than the local director via a newly added -I

/file/files/
/director/directory/

> Source/JavaScriptCore/ChangeLog:43
> +        source.  This is used both choosing the correct macro for external references as

/both choosing/both for choosing/

> Source/JavaScriptCore/ChangeLog:45
> +        of the masm only .sym file to be written once at the end of online assembling.

/online assembling/offline assembly/

> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:297
> -    static MacroAssemblerCodePtr createLLIntCodePtr(LLIntCode codeId)
> +    static MacroAssemblerCodePtr createLLIntCodeRefForHelper(unsigned id)

Should this be named “createLLIntCodePtrForHelper" instead because you’re returning a MacroAssemblerCodePtr (which is distinct from the createLLIntCodeRefForHelper below which actually returns a MacroAssemblerCodeRef)?

> Source/JavaScriptCore/llint/LLIntData.h:98
> +ALWAYS_INLINE void* getHelperCodePtr(unsigned id)

Michael, you’ve mentioned offline that you plan to change some #defines to enums.  I presume you were referring to the helper id here.  Please make it so.

> Source/JavaScriptCore/llint/LLIntData.h:108
> +ALWAYS_INLINE void(*getHelperFunctionPtr(unsigned id))()

This is hard to read.  You should use a typedef, and there is already one that you can use.  See LLIntCode in llint/LLIntData.h.  Hence, you can change this to:

ALWAYS_INLINE LLIntCode getHelperFunctionPtr(unsigned id)

Feel free to rename LLIntCode to a better name is appropriate.

> Source/JavaScriptCore/llint/LLIntData.h:110
> +    return reinterpret_cast<void (*)()>(getHelperCodePtr(id));

Ditto.  Change to:
    return reinterpret_cast<LLIntCode>(getHelperCodePtr(id));

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:752
> +include InitBytecodes

nit: I’d prefer to indent this include directive, but other folks may (or may not) have a different opinion.  Just thought I’d mention it for your consideration.

> Source/JavaScriptCore/offlineasm/asm.rb:193
> +        end            

Looks like this “end” needs to be indented.

[Michael Saboff]

Created attachment 229066 [details]
Patch addressing issues raised in review

[Michael Saboff]

Created attachment 229067 [details]
Generated LLIntAssembly.h before this change

[Michael Saboff]

Created attachment 229068 [details]
Generated LLIntAssembly.h after this change

[Michael Saboff]

Per an offline discussion with Mark, I added samples of the generated LLIntAssembly.h files before and after this change.

[Mark Lam]

Comment on attachment 229066 [details]
Patch addressing issues raised in review

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

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:40
> +        as well as  including files from other than the local directory via a newly

Extra space between "as  including".

> Source/JavaScriptCore/ChangeLog:45
> +        Updated the generationo f the masm only .sym file to be written once at the end

/generationo f/generation of/.

> Source/JavaScriptCore/ChangeLog:46
> +        of online assembling.

/online assembling/offline assembly/.

> Source/JavaScriptCore/llint/LLIntData.h:38
> +const int opcodeMapSize = NUMBER_OF_BYTECODE_IDS + NUMBER_OF_CLOOP_BYTECODE_HELPER_IDS + NUMBER_OF_BYTECODE_HELPER_IDS;

This looks identical to the definition of numOpcodeIDs.  I think once upon a time, these 2 values are different, but now they are redundant.  Let's get rid of opcodeMapSize and just use numOpcodeIDs everywhere instead.

> Source/JavaScriptCore/llint/LLIntData.h:41
> +const int opcodeMapSize = NUMBER_OF_BYTECODE_IDS + NUMBER_OF_BYTECODE_HELPER_IDS;

Ditto.

> Source/JavaScriptCore/llint/LLIntData.h:64
> +    friend void* getCodePtr(unsigned);
> +    friend void* getHelperCodePtr(unsigned);

I think these 2 are now obsolete and not referenced anywhere.  Please remove.

> Source/JavaScriptCore/llint/LLIntData.h:118
>  ALWAYS_INLINE void* getCodePtr(JSC::EncodedJSValue glueHelper())

I presume this is still needed for "LLInt::getCodePtr(getHostCallReturnValue))" or something.  Please confirm that it is still needed.  If not, please remove.

> Source/JavaScriptCore/llint/LowLevelInterpreter.h:50
>  #endif // !ENABLE(LLINT_C_LOOP)

/!ENABLE(LLINT_C_LOOP)/ENABLE(LLINT_C_LOOP)/ since there's no longer a !ENABLE(LLINT_C_LOOP) case.

[Michael Saboff]

(In reply to comment #11)
> (From update of attachment 229066 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229066&action=review
> 
> r=me with fixes.
> 
> > Source/JavaScriptCore/ChangeLog:40
> > +        as well as  including files from other than the local directory via a newly
> 
> Extra space between "as  including".
> 
> > Source/JavaScriptCore/ChangeLog:45
> > +        Updated the generationo f the masm only .sym file to be written once at the end
> 
> /generationo f/generation of/.
> 
> > Source/JavaScriptCore/ChangeLog:46
> > +        of online assembling.
> 
> /online assembling/offline assembly/.
> 
> > Source/JavaScriptCore/llint/LLIntData.h:38
> > +const int opcodeMapSize = NUMBER_OF_BYTECODE_IDS + NUMBER_OF_CLOOP_BYTECODE_HELPER_IDS + NUMBER_OF_BYTECODE_HELPER_IDS;
> 
> This looks identical to the definition of numOpcodeIDs.  I think once upon a time, these 2 values are different, but now they are redundant.  Let's get rid of opcodeMapSize and just use numOpcodeIDs everywhere instead.
> 
> > Source/JavaScriptCore/llint/LLIntData.h:41
> > +const int opcodeMapSize = NUMBER_OF_BYTECODE_IDS + NUMBER_OF_BYTECODE_HELPER_IDS;
> 
> Ditto.
> 
> > Source/JavaScriptCore/llint/LLIntData.h:64
> > +    friend void* getCodePtr(unsigned);
> > +    friend void* getHelperCodePtr(unsigned);
> 
> I think these 2 are now obsolete and not referenced anywhere.  Please remove.
> 
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.h:50
> >  #endif // !ENABLE(LLINT_C_LOOP)
> 
> /!ENABLE(LLINT_C_LOOP)/ENABLE(LLINT_C_LOOP)/ since there's no longer a !ENABLE(LLINT_C_LOOP) case.

Made the suggested changes except:

> > Source/JavaScriptCore/llint/LLIntData.h:118
> >  ALWAYS_INLINE void* getCodePtr(JSC::EncodedJSValue glueHelper())
> 
> I presume this is still needed for "LLInt::getCodePtr(getHostCallReturnValue))" or something.  Please confirm that it is still needed.  If not, please remove.

Yes, this is why this is needed.

[Michael Saboff]

Committed r167094: <http://trac.webkit.org/changeset/167094>