Bug 159321 - Update JSC_functionOverrides to handle the new SourceCode strings that have params.
Summary: Update JSC_functionOverrides to handle the new SourceCode strings that have p...
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:
Depends on:
Blocks:
 
Reported: 2016-06-30 16:01 PDT by Mark Lam
Modified: 2016-07-01 09:39 PDT (History)
3 users (show)

See Also:


Attachments
proposed patch. (18.70 KB, patch)
2016-06-30 16:07 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch 2: fixed EWS complaint. (18.72 KB, patch)
2016-06-30 17:58 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-06-30 16:01:30 PDT
And add tests so that this won't fail silently and bit rot anymore.  Patch coming.
Comment 1 Mark Lam 2016-06-30 16:07:06 PDT
Created attachment 282487 [details]
proposed patch.
Comment 2 Mark Lam 2016-06-30 17:58:35 PDT
Created attachment 282498 [details]
proposed patch 2: fixed EWS complaint.
Comment 3 Geoffrey Garen 2016-07-01 09:24:57 PDT
Comment on attachment 282498 [details]
proposed patch 2: fixed EWS complaint.

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

> Source/JavaScriptCore/tools/FunctionOverrides.cpp:153
> +    String sourceString = origCode.view().toString();
> +    size_t sourceBodyStart = sourceString.find('{');
> +    if (sourceBodyStart == notFound)
> +        return false;
> +    String sourceBodyString = sourceString.substring(sourceBodyStart);
> +
> +    auto it = overrides.m_entries.find(sourceBodyString);

This is OK -- but I think it might be better to allow the author to override parameter declaration too.
Comment 4 Mark Lam 2016-07-01 09:32:11 PDT
(In reply to comment #3)
> Comment on attachment 282498 [details]
> proposed patch 2: fixed EWS complaint.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282498&action=review
> 
> > Source/JavaScriptCore/tools/FunctionOverrides.cpp:153
> > +    String sourceString = origCode.view().toString();
> > +    size_t sourceBodyStart = sourceString.find('{');
> > +    if (sourceBodyStart == notFound)
> > +        return false;
> > +    String sourceBodyString = sourceString.substring(sourceBodyStart);
> > +
> > +    auto it = overrides.m_entries.find(sourceBodyString);
> 
> This is OK -- but I think it might be better to allow the author to override
> parameter declaration too.

I'll leave overriding parameters to another patch.  As far as I can tell, the uses of overriding parameters are:

1. Ability to use different argument names for better readability (especially if we're replacing an obfuscated function).
2. Ability to force stack arity adjustments.

I think (2) is the more meaningful reason for overriding parameters, but the need for that is probably rare.  I filed https://bugs.webkit.org/show_bug.cgi?id=159355 to track this for future work.
Comment 5 Mark Lam 2016-07-01 09:39:32 PDT
Thanks for the review.  Landed in r202737: <http://trac.webkit.org/r202737>.