Bug 144545 - Add backed intrinsics to private functions exposed with private symbols in global object
Summary: Add backed intrinsics to private functions exposed with private symbols in gl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-03 06:20 PDT by Yusuke Suzuki
Modified: 2015-05-04 17:39 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.86 KB, patch)
2015-05-03 06:21 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-05-03 06:20:04 PDT
Add backed intrinsics to private functions exposed with private symbols in global object
Comment 1 Yusuke Suzuki 2015-05-03 06:21:34 PDT
Created attachment 252267 [details]
Patch
Comment 2 Darin Adler 2015-05-04 08:56:35 PDT
Comment on attachment 252267 [details]
Patch

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

Looks fine.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:430
> -    JSFunction* privateFuncAbs = JSFunction::create(vm, this, 0, String(), globalPrivateFuncAbs);
> -    JSFunction* privateFuncFloor = JSFunction::create(vm, this, 0, String(), globalPrivateFuncFloor);
> +    JSFunction* privateFuncAbs = JSFunction::create(vm, this, 1, String(), mathProtoFuncAbs, AbsIntrinsic);
> +    JSFunction* privateFuncFloor = JSFunction::create(vm, this, 1, String(), mathProtoFuncFloor, FloorIntrinsic);

There are irritating inconsistencies here. Why do these functions use String(), but log uses vm.propertyNames->emptyIdentifier.string()? Why are some lengths 0 and other 1 for otherwise identical functions?

Besides the change mentioned in change log, this also changes length from 0 to 1. Can we add a test case covering this? Or just wait and do it in a different patch? If it’s right for abs, then it must be right for isFinite as well, for example.
Comment 3 Yusuke Suzuki 2015-05-04 15:47:56 PDT
Comment on attachment 252267 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:430
>> +    JSFunction* privateFuncFloor = JSFunction::create(vm, this, 1, String(), mathProtoFuncFloor, FloorIntrinsic);
> 
> There are irritating inconsistencies here. Why do these functions use String(), but log uses vm.propertyNames->emptyIdentifier.string()? Why are some lengths 0 and other 1 for otherwise identical functions?
> 
> Besides the change mentioned in change log, this also changes length from 0 to 1. Can we add a test case covering this? Or just wait and do it in a different patch? If it’s right for abs, then it must be right for isFinite as well, for example.

Yeah! Strictly speaking, these length should be 1. However, fixing it would have no effect on the rest of the JSC. Because nobody looks up the length of private functions.
In this patch, I've just fixed the length of touched private functions. But seeing the other functions, there is a lot of inconsistency about it.

So in this patch, to keep the patch simle, I'll just fix intrinsic related issues.
Comment 4 Yusuke Suzuki 2015-05-04 17:39:05 PDT
Committed r183785: <http://trac.webkit.org/changeset/183785>