Bug 193467 - Refactor new bytecode structs so that the fields are prefixed with "m_".
Summary: Refactor new bytecode structs so that the fields are prefixed with "m_".
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:
Blocks: 193557
  Show dependency treegraph
 
Reported: 2019-01-15 15:06 PST by Mark Lam
Modified: 2019-01-17 16:39 PST (History)
9 users (show)

See Also:


Attachments
proposed patch. (519.40 KB, patch)
2019-01-15 15:13 PST, Mark Lam
saam: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
patch for landing. (520.74 KB, patch)
2019-01-15 17:16 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing. (519.26 KB, patch)
2019-01-15 19:07 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 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.
Comment 1 Mark Lam 2019-01-15 15:13:17 PST
Created attachment 359210 [details]
proposed patch.
Comment 2 Saam Barati 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.
Comment 3 EWS Watchlist 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
Comment 4 Mark Lam 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.
Comment 5 Mark Lam 2019-01-15 17:16:21 PST
Created attachment 359230 [details]
patch for landing.
Comment 6 Mark Lam 2019-01-15 19:07:57 PST
Created attachment 359243 [details]
patch for landing.

Rebased to ToT.
Comment 7 EWS Watchlist 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.
Comment 8 Tadeu Zagallo 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
Comment 9 Mark Lam 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.
Comment 10 Mark Lam 2019-01-16 10:45:39 PST
Landed in r240041: <http://trac.webkit.org/r240041>.
Comment 11 Radar WebKit Bug Importer 2019-01-16 10:48:56 PST
<rdar://problem/47321555>