Bug 193557 - Audit bytecode fields and ensure that LLInt instructions for accessing them are appropriate.
Summary: Audit bytecode fields and ensure that LLInt instructions for accessing them a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 193467
Blocks:
  Show dependency tree / graph
 
Reported: 2019-01-17 16:35 PST by Mark Lam
Modified: 2019-01-20 12:33 PST (History)
8 users (show)

See Also:

Attachments
proposed patch. (119.38 KB, patch)
2019-01-17 16:41 PST, Mark Lam 		ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing. (119.83 KB, patch)
2019-01-17 17:34 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) 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 Mark Lam 2019-01-17 16:35:17 PST 
Patch coming.
Comment 1 Radar WebKit Bug Importer 2019-01-17 16:35:49 PST 
<rdar://problem/47369125>
Comment 2 Mark Lam 2019-01-17 16:41:42 PST 
Created attachment 359424 [details]
proposed patch.
Comment 3 Yusuke Suzuki 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
Comment 4 Saam Barati 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
Comment 5 Mark Lam 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.
Comment 6 Mark Lam 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.
Comment 7 Mark Lam 2019-01-17 17:34:16 PST 
Created attachment 359432 [details]
patch for landing.
Comment 8 Mark Lam 2019-01-17 18:12:28 PST 
Landed in r240138: <http://trac.webkit.org/r240138>.
Comment 9 Saam Barati 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?