[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 #7 from Mark Lam <mark.lam at apple.com>  2012-08-13 15:20:00 PST ---
(From update of attachment 157459)
View in context: https://bugs.webkit.org/attachment.cgi?id=157459&action=review

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

The usage cases for the VMInspector class is not baked yet.  But I think I concur that there isn't a use case for instantiating it.  I tend to prefer using classes as the smallest collection of functionality, and namespaces for arbitration between commonly named classes.  In this case, I think of using "the VMInspector" as it is an object although there may ever be one instance / pseudo instance.  But I don't mind changing it to be a namespace instead if I don't find any good reasons to keep it as a class when it's time to bake it.

>> Source/JavaScriptCore/jit/JITExceptions.cpp:82
>> +    }
> 
> 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.

It is an "executable address" in the lint loop.  What I'm trying to do here is represent it in a way that the C compiler won't complain about.  That said, I haven't tried really hard to make it real clean yet.  Have just been aiming at getting it to work so far.  Will revisit for applying a cleaner abstraction when things are fully functional.

>> Source/JavaScriptCore/llint/LLIntData.cpp:64
>> +    }
> 
> You're risking a race condition here.  Either use pthread_once (or similar) or move this initialization into JSC::initializeThreading().

OK, thanks for the tip.  Will look into it.

>> Source/JavaScriptCore/llint/LLIntData.h:111
>>  };
> 
> 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.

Having the opcodeMap as a global singleton allows me to get its info from anywhere without needing a globalData pointer, and I encountered a few of such cases as I was trying to do this work.  That is my main motivation.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.h:47
>> +    ExecState* b;
> 
> I believe we often return something other than Instruction* as the first word.

I was letting the C compiler complain if there was a type conflict.  So far, I didn't encounter any, but I will recheck to be sure before submitting my final patch.

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:92
>> +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
>         }
>     }
> }

Because there are local labels where a jump to it really means a jump as opposed to goto next opcode.  I need to be able to distinguish between the 2.  One alternative is to make the C loop backend aware that the branch target specifier is the vPC, and use that as the distinguishing factor.  I can see if I can make that work later.

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