| Summary: | Audit bytecode fields and ensure that LLInt instructions for accessing them are appropriate. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
| Component: | JavaScriptCore | Assignee: | 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
Mark Lam
2019-01-17 16:35:17 PST
Created attachment 359424 [details]
proposed patch.
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 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 (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. 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. Created attachment 359432 [details]
patch for landing.
Landed in r240138: <http://trac.webkit.org/r240138>. 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? |