Bug 18338 – Support exceptions in SquirrelFish

• Status: CLOSED
• Resolution: FIXED
• Component: JavaScriptCore

[Oliver Hunt]

task tracking bug

[Oliver Hunt]

Created attachment 20375 [details]
Initial patch

[Oliver Hunt]

Created attachment 20376 [details]
Slightly improved version of gcc perf hack

Avoid using a global for the exceptionTarget -- curse gcc

[Geoffrey Garen]

-    printf("%lu instructions; %lu bytes at %p; %d locals (%d parameters); %d temporaries\n\n", instructionCount, instructions.size() * sizeof(Instruction), this, numParameters + numVars, numParameters, numTemporaries);
+    printf("%lu instructions; %lu bytes at %p; %lu exception handlers; %d locals (%d parameters); %d temporaries\n\n", instructionCount, instructions.size() * sizeof(Instruction), this, exceptionHandlers.size(), numParameters + numVars, numParameters, numTemporaries);

I don't think it's helpful to list the number of exception handlers in the code dump. We don't list other forms of control flow, like jumps and loops, and exception handlers aren't referenced by number in the disasembly. I'd suggest removing this.
     
     for (Vector<Instruction>::const_iterator it = begin; it != end; ++it)
         dump(exec, begin, it);
@@ -154,6 +154,13 @@ void CodeBlock::dump(ExecState* exec) const
         } while (i < jsValues.size());
     }
     
+    for (size_t i = 0; i < identifiers.size(); ++i)
+        printf("  id%u = %s\n", static_cast<unsigned>(i), identifiers[i].ascii());
+
+    printf("\nConstants:\n");
+    for (size_t i = 0; i < jsValues.size(); ++i)
+        printf("  k%u = %s\n", static_cast<unsigned>(i), valueToSourceString(exec, jsValues[i]).ascii());

This looks like an incorrect merge. Bad Ollie.

+    if (exceptionHandlers.size()) {
+        printf("\nException Handlers:\n");
+        unsigned i = 0;
+        do {
+            printf("\t%d: {start: [%4d] end: [%4d] handler: [%4d] }\n", i+1, exceptionHandlers[i].start, exceptionHandlers[i].end, exceptionHandlers[i].target);
+            ++i;
+        } while (i < exceptionHandlers.size());
+    }

"{start" is missing a space.

"handler:" prints a data member named "target". Should it say "target:", or should the data member be named "handler" instead? I think it should say "target".

+Instruction* CodeBlock::handlerForPC(const Instruction* const address, int& expectedDepth)

This function signature is a little confusing. The first parameter is called "PC", "vPC", or "address", depending on the contenxt, while the second parameter is
called "expectedDepth" or "scopeDepth", depending on context. Also, it returns an out parameter but doesn't have "get" in its name. How about
"bool getHandlerInfoForVPC(vPC, target, scopeDepth)", which returns false if no handler info is found?


+{
+    Vector<HandlerInfo>::iterator ptr = exceptionHandlers.begin(); 
+    Vector<HandlerInfo>::iterator end = exceptionHandlers.end();
+    unsigned addressOffset = address - instructions.begin();
+    ASSERT(addressOffset < instructions.size());
+    
+    for (; ptr != end && (ptr->start <= addressOffset || ptr->end >= addressOffset); ++ptr) {

As we discussed in person, please remove the second loop condition, since it complicates things, and is either wrong, or always true, and therefore irrelevent.
(Our heads ended up hurting too much to prove one way or the other.)

+        // Handlers are ordered innermost first, so the first handler we encounter
+        // that contains the source address is the correct handler to use.

Very helpful comment!

+RegisterID* CodeGenerator::emitCatch(RegisterID* targetRegister, LabelID* start, LabelID* end)
+{
+    HandlerInfo info = { start->offsetFrom(0), end->offsetFrom(0), instructions().size(), m_scopeDepth};

"m_scopeDepth};" is missing a space.

+NEVER_INLINE JSValue* prepareException(ExecState* exec, JSValue* exceptionValue) 
+{
+    if (exceptionValue->isObject()) {
+        JSObject* exception = static_cast<JSObject*>(exceptionValue);
+        if (!exception->hasProperty(exec, "line") && !exception->hasProperty(exec, "sourceURL")) {
+            // Need to set line and sourceURL properties on the exception, but that is not currently possible
+            ASSERT_NOT_REACHED();

This ASSERT is really going to get in the way of using exceptions! Maybe we should do something less destructive for now? How about just putting in the empty
string and the number 0?

+NEVER_INLINE Instruction* unwindFrame(CodeBlock*& codeBlock, JSValue**& k, ScopeChain*& scopeChain, Vector<Register>* registers, Register*& base)

The "dump" function calls a register frame a "callFrame". How about renaming this function to "unwindCallFrame", for consistency?

Also for consistency, can we call "base" "r" for now? If "r" is not so good, we should rename it everywhere, but giving different names in different places will
just cause heartburn.

+{
+    CodeBlock* oldCodeBlock = codeBlock;
+    
+    Register* returnInfo = base - oldCodeBlock->numVars - oldCodeBlock->numParameters - Machine::returnInfoSize;

Can this be a member function so we don't have to "Machine::"?

You need special handling for global code, since it has no call frame. You can detect global code by testing if CodeBlock::numParameters is 0. (It's only 0 for
global code.)

+    
+    if (oldCodeBlock->needsActivation) {
+        // Find the top of the scope chain

I think you mean "find this function's activation in the scope chain". The top of the scope chain is the first entry.

+        ScopeChainIterator iter = scopeChain->begin(); 
+        ScopeChainIterator end = scopeChain->end();
+        while (!((*iter)->isActivationObject())) {
+            ++iter;
+            if (iter == end)
+                break;
+        }

You can just ASSERT(itr != end). 

+
+        // Clean up the activation if'n it's necessary
+        if (iter != end && (*iter)->isActivationObject()) {
	
Same here.

+    codeBlock = returnInfo[0].u.codeBlock;
+    if (!codeBlock)
+        return 0;

Could use a comment here saying "0 means built-in function".

+NEVER_INLINE static bool throwException(CodeBlock*& codeBlock, JSValue**& k, ScopeChain*& scopeChain, Vector<Register>* registers, Register*& base, const Instruction* vPC)
+{
+    while (codeBlock) {
+        int expectedDepth;        
+        Instruction* handlerPC = codeBlock->handlerForPC(vPC, expectedDepth);
+        if (!handlerPC) {
+            vPC = unwindFrame(codeBlock, k, scopeChain, registers, base);
+            continue;
+        }
+        // Now unwind the scope chain
+        int scopeDepth = 0;
+        // Step 1) work out how deep the scope chain is
+        ScopeChainIterator iter = scopeChain->begin(); 
+        ScopeChainIterator end = scopeChain->end(); 
+        while (!((*iter)->isActivationObject())) {
+            ++iter;
+            ++scopeDepth;
+            if (iter == end) {
+                // reached the global object at the top of the scope chain
+                --scopeDepth;
+                break;
+            }
+        }

Can depth() just be a member function on ScopeChain?

+        // Step 3) Cry :-(
	
There's no crying in JavaScriptCore!

Nice work.

r=me

Geoff

[Oliver Hunt]

Landed 31701