[Webkit-unassigned] [Bug 91052] JSC: LLInt should auto-generate our cross-platform C++ interpreter

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=91052





--- Comment #6 from Filip Pizlo <fpizlo at apple.com>  2012-08-13 14:17:15 PST ---
(From update of attachment 157459)
View in context: https://bugs.webkit.org/attachment.cgi?id=157459&action=review

Below are some comments so far.

> Source/JavaScriptCore/interpreter/VMInspector.h:11
> +class VMInspector {

I like this, but would prefer a VMInspector namespace over a class - unless you plan to make it instantiable, which doesn't seem to be the case..

> Source/JavaScriptCore/jit/JITExceptions.cpp:82
> +#if ENABLE(LLINT_C_LOOP)
> +        catchRoutine = reinterpret_cast<Instruction*>(catchPCForInterpreter->u.opcode);
> +#else
> +        catchRoutine = handler->nativeCode.executableAddress();
> +#endif
> +        // mlam BEGIN
> +        #if mlam_PRINTF
> +        printf("\n    mlam :: genericThrow() SET handler %p | catchPCForInterpreter = %p catchRoutine %p\n", handler, catchPCForInterpreter, catchRoutine);
> +        #endif // mlam_PRINTF
> +        // mlam END
> +    } else {
> +#if ENABLE(LLINT_C_LOOP)
> +        // mlam orig catchRoutine = LLINT_ADDRESS_OF_GLUE(llint_ctiOpThrowNotCaught);
> +        // mlam COMPUTED_GOTO catchRoutine = const_cast<void*>(LLInt::Data::getOpcode(llint_ctiOpThrowNotCaught));
> +        catchRoutine = LLInt::Data::getOpcodePtr(llint_ctiOpThrowNotCaught);
> +#else
>          catchRoutine = FunctionPtr(ctiOpThrowNotCaught).value();
> +#endif
> +        // mlam BEGIN
> +        #if mlam_PRINTF
> +        printf("\n    mlam :: genericThrow() SET NO handler | catchRoutine = %p\n", catchRoutine);
> +        #endif // mlam_PRINTF
> +        // mlam END
> +    }

Why can't the catchRoutine be an "executable address" in the LLInt C loop as well?  By "executable address" I of course mean a number that will cause the switch statement to take you to the relevant piece of code.

> Source/JavaScriptCore/llint/LLIntData.cpp:64
> +    if (!s_isInited) {
> +        s_exceptionInstructions = new Instruction[maxOpcodeLength + 1];
> +        s_opcodeMap = new LLIntOpcode[numLLIntOpcodeIDs];
> +#if !ENABLE(LLINT_C_LOOP)
>      for (int i = 0; i < maxOpcodeLength + 1; ++i)
> -        m_exceptionInstructions[i].u.pointer = bitwise_cast<void*>(&llint_throw_from_slow_path_trampoline);
> -#define OPCODE_ENTRY(opcode, length) m_opcodeMap[opcode] = bitwise_cast<void*>(&llint_##opcode);
> +        s_exceptionInstructions[i].u.pointer = bitwise_cast<void*>(&llint_throw_from_slow_path_trampoline);
> +#define OPCODE_ENTRY(opcode, length) s_opcodeMap[opcode] = bitwise_cast<void*>(&llint_##opcode);
>      FOR_EACH_OPCODE_ID(OPCODE_ENTRY);
>  #undef OPCODE_ENTRY
> +        s_isInited = true;
> +#endif // !ENABLE(LLINT_C_LOOP)
> +    }

You're risking a race condition here.  Either use pthread_once (or similar) or move this initialization into JSC::initializeThreading().

> Source/JavaScriptCore/llint/LLIntData.h:107
> +    static bool s_isInited;

isInited should be isInitialized.  We generally prefer not to abbreviate.

> Source/JavaScriptCore/llint/LLIntData.h:111
> +    */
> +    static bool s_isInited;
> +    static Instruction* s_exceptionInstructions;
> +    static LLIntOpcode* s_opcodeMap;
> +    // mlam END */
>  };

Why is this change necessary?  What's wrong with this being per-LLIntData-instance?  Having it be per-LLIntData-instance means that you completely avoid initialization race conditions.

> Source/JavaScriptCore/llint/LLIntSlowPaths.h:47
> -    void* a;
> -    void* b;
> +    Instruction* a;
> +    ExecState* b;

I believe we often return something other than Instruction* as the first word.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:92
> +macro jumpToPC()
> +    if C_LOOP
> +        cxxNextOpcode
> +    else
> +        jmp [PC]                    // goto NEXT_INSTRUCTION;
> +    end
> +end

Why do you need this?

Why isn't the translation of jmp <address> just:

// the function you generate from the offlineasm
enum LLIntGlobalLabels {
    _llint_op_something,
    // ... lots more
};
void cLoop() {
    LLIntGlobalLabels nextPC;
    for (;;) {
        switch (nextPC) {
        case _llint_op_something:
            // translation of jmp [PC]
            nextPC = *bitwise_cast<LLIntGlobalLabels*>(PC);
            continue;
        // more things
        }
    }
}

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:126
> +        cxxCallSlowPath function, arg1, arg2    // call C function(arg1, arg2);

For the record, I concur that having a special opcode for calls to C functions is right.  I just don't agree that it's right for jmp, or for call when it's used for JS->JS calls.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.