Bug 193423 - JSFunction::canUseAllocationProfile() should account for builtin functions with no own prototypes.
Summary: JSFunction::canUseAllocationProfile() should account for builtin functions wi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-14 18:25 PST by Mark Lam
Modified: 2019-01-16 10:12 PST (History)
9 users (show)

See Also:


Attachments
proposed patch. (5.21 KB, patch)
2019-01-14 18:49 PST, Mark Lam
sbarati: review-
Details | Formatted Diff | Diff
proposed patch. (6.81 KB, patch)
2019-01-14 22:24 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
patch for landing. (8.74 KB, patch)
2019-01-15 18:44 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-01-14 18:25:54 PST
<rdar://problem/46209355>
Comment 1 Mark Lam 2019-01-14 18:49:03 PST
Created attachment 359116 [details]
proposed patch.
Comment 2 Saam Barati 2019-01-14 18:50:40 PST
Comment on attachment 359116 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:125
> +        if (prototype.isCell()) {
> +            JSCell* cell = prototype.asCell();
> +            if (cell->isGetterSetter() || cell->isCustomGetterSetter() || cell->isProxy())
> +                return false;
> +        }

Not a fan of this. Can you do the structure lookup along w/ and inspect the bits that say if this is a getter/setter?
Comment 3 Mark Lam 2019-01-14 22:24:17 PST
Created attachment 359137 [details]
proposed patch.
Comment 4 Saam Barati 2019-01-15 12:19:54 PST
Comment on attachment 359137 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/ChangeLog:10
> +        because the majority of them has no prototype property.  The only exception to

s/has/have

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:120
> +        if (!prototype || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue))

Can we add a test case for this? Like define a getter as the prototype of a random builtin function.
Comment 5 Mark Lam 2019-01-15 18:44:10 PST
Created attachment 359239 [details]
patch for landing.

Thanks for the review.
Comment 6 Mark Lam 2019-01-16 10:12:02 PST
Landed in r240040: <http://trac.webkit.org/r240040>.