Bug 193467

Summary: Refactor new bytecode structs so that the fields are prefixed with "m_".
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, 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:    
Bug Blocks: 193557    
Attachments:
Description Flags
proposed patch.
saam: review+, ews-watchlist: commit-queue-
patch for landing.
none
patch for landing. none

Mark Lam
Reported 2019-01-15 15:06:02 PST
This makes it easier to do a manual audit of type correctness of the LLInt instructions used to access these fields. Without this change, it would be difficult (and error prone) to distinguish the difference between field names and macro variables.
Attachments
proposed patch. (519.40 KB, patch)
2019-01-15 15:13 PST, Mark Lam 		saam: review+
ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
patch for landing. (520.74 KB, patch)
2019-01-15 17:16 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch for landing. (519.26 KB, patch)
2019-01-15 19:07 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2019-01-15 15:13:17 PST
Created attachment 359210 [details] proposed patch.
Saam Barati
Comment 2 2019-01-15 15:50:21 PST
Comment on attachment 359210 [details] proposed patch. r=me but we should probably wait for Tadeu to say it's ok too.
EWS Watchlist
Comment 3 2019-01-15 17:02:14 PST
Comment on attachment 359210 [details] proposed patch. Attachment 359210 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/10764813 New failing tests: stress/arith-abs-to-arith-negate-range-optimizaton.js.no-llint apiTests
Mark Lam
Comment 4 2019-01-15 17:14:19 PST
(In reply to Build Bot from comment #3) > Comment on attachment 359210 [details] > proposed patch. > > Attachment 359210 [details] did not pass jsc-ews (mac): > Output: https://webkit-queues.webkit.org/results/10764813 > > New failing tests: > stress/arith-abs-to-arith-negate-range-optimizaton.js.no-llint > apiTests Found the issue: I missed one place: the SFINAE condition for JIT::emitValueProfilingSiteIfProfiledOpcode which depends on Metadata::m_profile. Will upload patch for landing with this fix shortly.
Mark Lam
Comment 5 2019-01-15 17:16:21 PST
Created attachment 359230 [details] patch for landing.
Mark Lam
Comment 6 2019-01-15 19:07:57 PST
Created attachment 359243 [details] patch for landing. Rebased to ToT.
EWS Watchlist
Comment 7 2019-01-15 19:12:06 PST
Attachment 359243 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1307: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
Tadeu Zagallo
Comment 8 2019-01-15 23:56:09 PST
Comment on attachment 359243 [details] patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=359243&action=review Looks good to me, I think the rationale makes sense. Just a couple nits in the ruby code. > Source/JavaScriptCore/generator/Argument.rb:38 > + "#{@type.to_s} m_#{@name};" Just a nit, but this should probably use `field_name` here. You might also want to remove the `attr_reader :name` to avoid accidentally referring to the unprefixed name. > Source/JavaScriptCore/generator/Opcode.rb:83 > + def untyped_params This seems to be unused
Mark Lam
Comment 9 2019-01-16 10:35:25 PST
Thanks. (In reply to Tadeu Zagallo from comment #8) > Comment on attachment 359243 [details] > patch for landing. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359243&action=review > > Looks good to me, I think the rationale makes sense. Just a couple nits in > the ruby code. > > > Source/JavaScriptCore/generator/Argument.rb:38 > > + "#{@type.to_s} m_#{@name};" Fixed. > Just a nit, but this should probably use `field_name` here. You might also > want to remove the `attr_reader :name` to avoid accidentally referring to > the unprefixed name. Hmmm, I'm not too knowledgeable about ruby but we still need a name property, and it should be write once i.e. read only after the Argument is constructed. So, I think attr_reader :name is appropriate. > > Source/JavaScriptCore/generator/Opcode.rb:83 > > + def untyped_params > > This seems to be unused Removed.
Mark Lam
Comment 10 2019-01-16 10:45:39 PST
Landed in r240041: <http://trac.webkit.org/r240041>.
Radar WebKit Bug Importer
Comment 11 2019-01-16 10:48:56 PST
<rdar://problem/47321555>
Note You need to log in before you can comment on or make changes to this bug.