Bug 238918

Summary: [JSC] DFG / FTL should be aware of JSString's String replacement
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, zolkarnainhashim73
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch saam: review+, ews-feeder: commit-queue-

Yusuke Suzuki
Reported 2022-04-06 19:08:35 PDT
[JSC] JSString::value should return String instead of reference
Attachments
Patch (25.97 KB, patch)
2022-04-06 19:09 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (51.70 KB, patch)
2022-04-07 12:38 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (62.41 KB, patch)
2022-04-07 19:30 PDT, Yusuke Suzuki 		saam: review+
ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2022-04-06 19:09:30 PDT
Created attachment 456881 [details] Patch
Yusuke Suzuki
Comment 2 2022-04-07 12:38:34 PDT
Created attachment 456955 [details] Patch
Zolkarnain bin hashim
Comment 3 2022-04-07 18:53:01 PDT
Slick
Yusuke Suzuki
Comment 4 2022-04-07 19:30:05 PDT
Created attachment 457004 [details] Patch
Saam Barati
Comment 5 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
Yusuke Suzuki
Comment 6 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.
Yusuke Suzuki
Comment 7 2022-04-10 21:57:45 PDT
Committed r292697 (249489@trunk): <https://commits.webkit.org/249489@trunk>
Radar WebKit Bug Importer
Comment 8 2022-04-10 22:07:04 PDT
<rdar://problem/91547350>
Note You need to log in before you can comment on or make changes to this bug.