Bug 193557

Summary: Audit bytecode fields and ensure that LLInt instructions for accessing them are appropriate.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, keith_miller, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 193467    
Bug Blocks:    
Attachments:
Description Flags
proposed patch.
ysuzuki: review+
patch for landing. none

Mark Lam
Reported 2019-01-17 16:35:17 PST
Patch coming.
Attachments
proposed patch. (119.38 KB, patch)
2019-01-17 16:41 PST, Mark Lam 		ysuzuki: review+
DetailsFormatted DiffDiff
patch for landing. (119.83 KB, patch)
2019-01-17 17:34 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-01-17 16:35:49 PST
<rdar://problem/47369125>
Mark Lam
Comment 2 2019-01-17 16:41:42 PST
Created attachment 359424 [details] proposed patch.
Yusuke Suzuki
Comment 3 2019-01-17 17:17:38 PST
Comment on attachment 359424 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=359424&action=review r=me > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1331 > + loadp OpGetByIdDirect::Metadata::m_structureID[t5], t1 I prefer using loadi for StructureID, but either is OK to me if it is consistent. > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:918 > + loadi CodeBlock::m_numParameters[temp1], temp1 Nice
Saam Barati
Comment 4 2019-01-17 17:22:51 PST
Comment on attachment 359424 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=359424&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:737 > - loadi JSCallee::m_scope[t0], t0 > + loadp JSCallee::m_scope[t0], t0 we should really have some static analysis for this where we'd just fail to compile
Mark Lam
Comment 5 2019-01-17 17:25:21 PST
(In reply to Saam Barati from comment #4) > Comment on attachment 359424 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359424&action=review > > > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:737 > > - loadi JSCallee::m_scope[t0], t0 > > + loadp JSCallee::m_scope[t0], t0 > > we should really have some static analysis for this where we'd just fail to > compile Totally agree. That's what I'm hoping to do as a feature for the offlineasm later.
Mark Lam
Comment 6 2019-01-17 17:33:12 PST
Thanks for the review. (In reply to Yusuke Suzuki from comment #3) > > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1331 > > + loadp OpGetByIdDirect::Metadata::m_structureID[t5], t1 > > I prefer using loadi for StructureID, but either is OK to me if it is > consistent. I think I'll switch back to loadi, storei, and bi* instructions for structureID on 32-bit.
Mark Lam
Comment 7 2019-01-17 17:34:16 PST
Created attachment 359432 [details] patch for landing.
Mark Lam
Comment 8 2019-01-17 18:12:28 PST
Landed in r240138: <http://trac.webkit.org/r240138>.
Saam Barati
Comment 9 2019-01-20 12:33:22 PST
Comment on attachment 359432 [details] patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=359432&action=review > Source/JavaScriptCore/ChangeLog:52 > + 4. The following should be read with loadi (not loadis) because they are Does loadis sign extend to 64-bit?
Note You need to log in before you can comment on or make changes to this bug.