Bug 186947 – Add more debugging features to $vm.

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

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 343406 [details]
proposed patch.

[Keith Miller]

Comment on attachment 343406 [details]
proposed patch.

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

r=me with some comments.

> Source/JavaScriptCore/ChangeLog:12
> +            // We now have println in addition to print.
> +            // println automatically adds a '\n' at the end.
> +            $vm.println("Hello");

Nit: I think this is what print should do by default... it's also what print does in jsc.cpp

> Source/JavaScriptCore/tools/JSDollarVM.cpp:103
> +            addProperty(vm, "codeBlock", codeBlock);
> +            addProperty(vm, "unlinkedCodeBlock", codeBlock->unlinkedCodeBlock());
> +            addProperty(vm, "executable", codeBlock->ownerExecutable());

I think this will crash for Wasm. Can we add these only if codeBlock is non-null.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:1351
> +    return JSValue::encode(codeBlock);

I think you need a null check here for Wasm.

[Mark Lam]

Comment on attachment 343406 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/ChangeLog:12
>> +            $vm.println("Hello");
> 
> Nit: I think this is what print should do by default... it's also what print does in jsc.cpp

Per our offline discussion, I'll rename print to dataLog, and println to print with the justification that most people would probably expect print to append the \n.  I still think it's sad that we have to do this, but I'll go with it only because most the scripting languages (as you pointed out) like adopts this (bad, IMHO) convention of making print append \n and make their users jump thru hoops to print without the \n.

>> Source/JavaScriptCore/tools/JSDollarVM.cpp:103
>> +            addProperty(vm, "executable", codeBlock->ownerExecutable());
> 
> I think this will crash for Wasm. Can we add these only if codeBlock is non-null.

I'll add a null check and not populate these (which makes them undefined).

>> Source/JavaScriptCore/tools/JSDollarVM.cpp:1351
>> +    return JSValue::encode(codeBlock);
> 
> I think you need a null check here for Wasm.

Will do and return undefined if codeBlock is null.

[Mark Lam]

(In reply to Mark Lam from comment #3)
> Comment on attachment 343406 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343406&action=review
> 
> Thanks for the review.
> 
> >> Source/JavaScriptCore/ChangeLog:12
> >> +            $vm.println("Hello");
> > 
> > Nit: I think this is what print should do by default... it's also what print does in jsc.cpp
> 
> Per our offline discussion, I'll rename print to dataLog, and println to
> print with the justification that most people would probably expect print to
> append the \n.  I still think it's sad that we have to do this, but I'll go
> with it only because most the scripting languages (as you pointed out) like
> adopts this (bad, IMHO) convention of making print append \n and make their
> users jump thru hoops to print without the \n.

On second thought, I'll defer this to a follow up patch to go with other renaming that I think should be done for consistency.

[Mark Lam]

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

[Radar WebKit Bug Importer]

<rdar://problem/41398544>