Bug 164192 – Add $vm.codeBlock() debugging utility.

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

[Mark Lam]

Sometimes, while debugging, it would be nice if we can get the codeBlock info for a function.  This can be useful for checking what optimization level the function is currently at.  With $vm.codeBlock(), we can now do this.  For example:

    print("test's codeBlock = " + $vm.classBlock(test)); // prints function test's codeBlock.

[Mark Lam]

Created attachment 293315 [details]
proposed patch.

[Mark Lam]

Comment on attachment 293315 [details]
proposed patch.

I have a better patch coming.

[Mark Lam]

Created attachment 293316 [details]
proposed patch.

[Saam Barati]

Comment on attachment 293316 [details]
proposed patch.

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

> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:295
> +        codeBlock = reinterpret_cast<CodeBlock*>(bitwise_cast<uint64_t>(value.asDouble()));

This could just be a single bitwise_cast to CodeBlock*. Why is this code even important. Why would a double JSValue ever be a CodeBlock? This seems sketchy

> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:323
> +    JSValue value = exec->uncheckedArgument(0);

Might as well use argument instead of unchrckedArgument here just to be friendly to incorrect usage.

> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:464
> +    addFunction(vm, globalObject, "codeBlock", functionCodeBlockFor, 1);

I think a better name is codeBlockFor in the exposed JS name

[Sam Weinig]

Comment on attachment 293316 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:13
> +            print("test's codeBlock = " + $vm.classBlock(test)); // prints function test's codeBlock.

I think you mean $vm.codeBlock(...), not $vm.classBlock(...).

> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:276
>      if (exec->argumentCount() < 1)
>          return nullptr;

Now that you are passing a value, it's weird that you are still checking the argument count.  If you want to keep this check, you should probably move it to the call sites.  That said, I'm not clear why you changed this to pass a value since it is always argument 0.

[Mark Lam]

Comment on attachment 293316 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:13
>> +            print("test's codeBlock = " + $vm.classBlock(test)); // prints function test's codeBlock.
> 
> I think you mean $vm.codeBlock(...), not $vm.classBlock(...).

This is from muscle memory of typing "classBlock" for many years in a pass career.  Fixed now.

>> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:276
>>          return nullptr;
> 
> Now that you are passing a value, it's weird that you are still checking the argument count.  If you want to keep this check, you should probably move it to the call sites.  That said, I'm not clear why you changed this to pass a value since it is always argument 0.

Thanks for catching this.  I was sloppy.  After thinking about this some more, I'm going to revert this function to be codeBlockFromArg() so that I don't have to duplicate the check in the 3 clients that calls this function.

>> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:295
>> +        codeBlock = reinterpret_cast<CodeBlock*>(bitwise_cast<uint64_t>(value.asDouble()));
> 
> This could just be a single bitwise_cast to CodeBlock*. Why is this code even important. Why would a double JSValue ever be a CodeBlock? This seems sketchy

Fixed to just use a single bitwise_cast.

The reason it can be a double is because $vm.codeBlockForFrame(...) returns the bits of a CodeBlock* encoded as a double JSValue cookie.  That's why there's a isValidCodeBlock() check below before we actually use it as a CodeBlock*.  This API is for debugging with $vm only where we get to look into C++ data structures.  In real world JS code, we'll never have a JSValue that is a CodeBlock*, let alone a CodeBlock* that is a double JSValue.

>> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:323
>> +    JSValue value = exec->uncheckedArgument(0);
> 
> Might as well use argument instead of unchrckedArgument here just to be friendly to incorrect usage.

I reverted to using codeBlockFromArg() which checks the argument count first.

>> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:464
>> +    addFunction(vm, globalObject, "codeBlock", functionCodeBlockFor, 1);
> 
> I think a better name is codeBlockFor in the exposed JS name

I was going for brevity, but I think you are right that codeBlockFor is more clear.  Fixed.

[Mark Lam]

Created attachment 293322 [details]
proposed patch.

[Mark Lam]

Comment on attachment 293322 [details]
proposed patch.

Can't just use a single bitwise_cast after all.  Also have a bug.  I think a JSFuntion can only have a FunctionExecutable.  I'll double check before uploading another patch.

[Saam Barati]

(In reply to comment #6)
> Comment on attachment 293316 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293316&action=review
> 
> >> Source/JavaScriptCore/ChangeLog:13
> >> +            print("test's codeBlock = " + $vm.classBlock(test)); // prints function test's codeBlock.
> > 
> > I think you mean $vm.codeBlock(...), not $vm.classBlock(...).
> 
> This is from muscle memory of typing "classBlock" for many years in a pass
> career.  Fixed now.
> 
> >> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:276
> >>          return nullptr;
> > 
> > Now that you are passing a value, it's weird that you are still checking the argument count.  If you want to keep this check, you should probably move it to the call sites.  That said, I'm not clear why you changed this to pass a value since it is always argument 0.
> 
> Thanks for catching this.  I was sloppy.  After thinking about this some
> more, I'm going to revert this function to be codeBlockFromArg() so that I
> don't have to duplicate the check in the 3 clients that calls this function.
> 
> >> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:295
> >> +        codeBlock = reinterpret_cast<CodeBlock*>(bitwise_cast<uint64_t>(value.asDouble()));
> > 
> > This could just be a single bitwise_cast to CodeBlock*. Why is this code even important. Why would a double JSValue ever be a CodeBlock? This seems sketchy
> 
> Fixed to just use a single bitwise_cast.
> 
> The reason it can be a double is because $vm.codeBlockForFrame(...) returns
> the bits of a CodeBlock* encoded as a double JSValue cookie.  That's why
> there's a isValidCodeBlock() check below before we actually use it as a
> CodeBlock*.  This API is for debugging with $vm only where we get to look
> into C++ data structures.  In real world JS code, we'll never have a JSValue
> that is a CodeBlock*, let alone a CodeBlock* that is a double JSValue.
I'm not still confused why this would ever return true because of our encoding. Maybe I'm missing something here? 

> >> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:323
> >> +    JSValue value = exec->uncheckedArgument(0);
> > 
> > Might as well use argument instead of unchrckedArgument here just to be friendly to incorrect usage.
> 
> I reverted to using codeBlockFromArg() which checks the argument count first.
> 
> >> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:464
> >> +    addFunction(vm, globalObject, "codeBlock", functionCodeBlockFor, 1);
> > 
> > I think a better name is codeBlockFor in the exposed JS name
> 
> I was going for brevity, but I think you are right that codeBlockFor is more
> clear.  Fixed.

[Saam Barati]

"not still" => "still"

(Sorry I'm typing on my phone.)

[Mark Lam]

(In reply to comment #9)
> I'm [still] confused why this would ever return true because of our
> encoding. Maybe I'm missing something here? 

You'll need to see the implementation of functionCodeBlockForFrame() in JSDollarVMPrototype.cpp.  When reading that, consider the following use case of $vm:

    function foo(i) { 
        if (i > 0)
            foo(i - 1);
        else {
            var cb = $vm.codeBlockForFrame(2);
            $vm.printByteCodeFor(cb);
        }
    }
    foo(5);

$vm.codeBlockForFrame(2) will return the CodeBlock* at callFrame 2 (0 == top frame, 2 == 2nd below top frame).  That CodeBlock* is returned encoded as a double.

You know what ... CodeBlock is a JSCell.  I wrote this code way back before I understood many things about the system.  I'll just fix it to return the CodeBlock* itself.

[Mark Lam]

Created attachment 293332 [details]
proposed patch.

[Mark Lam]

(In reply to comment #11)
> You know what ... CodeBlock is a JSCell.  I wrote this code way back before
> I understood many things about the system.  I'll just fix it to return the
> CodeBlock* itself.

I stand corrected.  While exploring this, I discover why I encoded the CodeBlock* as a double.  The reason is that JS code is not supposed to handle JSCells that are not JSObjects.  Hence, if we have $vm.codeBlockForFrame(...) return a CodeBlock*, the client will crash the VM if it uses the CodeBlock* as a JSObject*.  By first encoding it as a double, JS code will simply see it as a double which is safe.  The only way it can be used as a CodeBlock* is if we pass that double encoded CodeBlock* to $vm.codeBlockFor(...), $vm.printSourceCodeFor(...), and $vm.printByteCodeFor(...).

I added comments to the patch to document this.

[Saam Barati]

Comment on attachment 293332 [details]
proposed patch.

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

> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:270
> +    // Though CodeBlock is a JSCell, it is not safe to return a JSCell* that is not a JSObject*

This is not strictly true. There are other things that are cells but not objects, like JSString*.

> Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:273
>      return JSValue::encode(JSValue(bitwise_cast<double>(reinterpret_cast<uint64_t>(codeBlock))));

I think you want to make sure to pass the EncodeAsDouble tag to this. Also, I'm not sure your reinterpret_cast is going to work well on 32-bit platforms. You might want this:
bitwise_cast<double>(static_cast<uint64_t>(bitwise_cast<uintptr_t>(CodeBlock*)))

[Mark Lam]

Thanks for the review.

(In reply to comment #14)
> > Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:270
> > +    // Though CodeBlock is a JSCell, it is not safe to return a JSCell* that is not a JSObject*
> 
> This is not strictly true. There are other things that are cells but not
> objects, like JSString*.

I fixed the comment to say:
"Though CodeBlock is a JSCell, it is not safe to return it directly back to JS code as it is an internal type that the JS code cannot handle."

> > Source/JavaScriptCore/tools/JSDollarVMPrototype.cpp:273
> >      return JSValue::encode(JSValue(bitwise_cast<double>(reinterpret_cast<uint64_t>(codeBlock))));
> 
> I think you want to make sure to pass the EncodeAsDouble tag to this. Also,
> I'm not sure your reinterpret_cast is going to work well on 32-bit
> platforms. You might want this:
> bitwise_cast<double>(static_cast<uint64_t>(bitwise_cast<uintptr_t>(CodeBlock*
> )))

I changed this to JSValue(bitwise_cast<double>(static_cast<uint64_t>(reinterpret_cast<uintptr_t>(codeBlock)))) which adds the extra zero extension on 32-bit platforms.

Landed in r208189: <http://trac.webkit.org/r208189>.