Bug 238918 - [JSC] DFG / FTL should be aware of JSString's String replacement
Summary: [JSC] DFG / FTL should be aware of JSString's String replacement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-06 19:08 PDT by Yusuke Suzuki
Modified: 2022-04-10 22:07 PDT (History)
8 users (show)

See Also:

Attachments
Patch (25.97 KB, patch)
2022-04-06 19:09 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (51.70 KB, patch)
2022-04-07 12:38 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (62.41 KB, patch)
2022-04-07 19:30 PDT, Yusuke Suzuki 		saam: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2022-04-06 19:08:35 PDT 
[JSC] JSString::value should return String instead of reference
Comment 1 Yusuke Suzuki 2022-04-06 19:09:30 PDT 
Created attachment 456881 [details]
Patch
Comment 2 Yusuke Suzuki 2022-04-07 12:38:34 PDT 
Created attachment 456955 [details]
Patch
Comment 3 Zolkarnain bin hashim 2022-04-07 18:53:01 PDT 
Slick
Comment 4 Yusuke Suzuki 2022-04-07 19:30:05 PDT 
Created attachment 457004 [details]
Patch
Comment 5 Saam Barati 2022-04-10 16:57:05 PDT 
Comment on attachment 457004 [details]
Patch

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

Nice. r=me

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3795
> +        ASSERT(node->arrayMode().type() != Array::String);

Can we put this in DFGValidate? That is the more canonical place to put such things.

> Source/JavaScriptCore/dfg/DFGClobberize.h:1341
> +        ASSERT(node->arrayMode().type() != Array::String);

ditto. I think just putting it in Validate is the right way

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2524
> +    string->value(globalObject);

why not call JSRopeString->resolveRope here?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4775
> +        ASSERT(m_node->arrayMode().type() != Array::String);

ditto about validation
Comment 6 Yusuke Suzuki 2022-04-10 21:50:50 PDT 
Comment on attachment 457004 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3795
>> +        ASSERT(node->arrayMode().type() != Array::String);
> 
> Can we put this in DFGValidate? That is the more canonical place to put such things.

Done. Nice

>> Source/JavaScriptCore/dfg/DFGClobberize.h:1341
>> +        ASSERT(node->arrayMode().type() != Array::String);
> 
> ditto. I think just putting it in Validate is the right way

Ditto.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2524
>> +    string->value(globalObject);
> 
> why not call JSRopeString->resolveRope here?

Sounds good. Changed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4775
>> +        ASSERT(m_node->arrayMode().type() != Array::String);
> 
> ditto about validation

Nice, done.
Comment 7 Yusuke Suzuki 2022-04-10 21:57:45 PDT 
Committed r292697 (249489@trunk): <https://commits.webkit.org/249489@trunk>
Comment 8 Radar WebKit Bug Importer 2022-04-10 22:07:04 PDT 
<rdar://problem/91547350>