Bug 193557 – Audit bytecode fields and ensure that LLInt instructions for accessing them are appropriate.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/47369125>

[Mark Lam]

Created attachment 359424 [details]
proposed patch.

[Yusuke Suzuki]

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 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]

(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]

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]

Created attachment 359432 [details]
patch for landing.

[Mark Lam]

Landed in r240138: <http://trac.webkit.org/r240138>.

[Saam Barati]

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?