If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use "enum class" for ExitFrame tokens.

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine: JIT
P5
normal
RESOLVED FIXED
2 years ago
11 days ago

People

(Reporter: nbp, Unassigned, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox57 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
As suggestted by jandem in Bug 1184965 comment 20, ExitFrameToken should use "enum class", instead of the current prefixes that we always have.

This should be an easy bug which aims at compiling SpiderMonkey and getting your first patch reviewed.
Assignee: nobody → evilpies
This enum is called ExitFrameTokenValues. I would suggested renaming it back ExitFrameToken and shortening all the enum values by at least removing the trailing Token from the name.
Assignee: evilpies → nobody
Mentor: nicolas.b.pierron@mozilla.com
Priority: -- → P5

Comment 2

19 days ago
Created attachment 8904355 [details] [diff] [review]
0001-enum-class-ExitFrameToken.patch
Attachment #8904355 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 3

19 days ago
Comment on attachment 8904355 [details] [diff] [review]
0001-enum-class-ExitFrameToken.patch

Review of attachment 8904355 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/JitFrames.h
@@ +493,5 @@
>  class IonOOLPropertyOpExitFrameLayout;
>  class IonOOLProxyExitFrameLayout;
>  class IonDOMExitFrameLayout;
>  
> +enum class ExitFrameToken : int32_t

nit: any reason to not use uint8_t ?

@@ +498,2 @@
>  {
> +    CallNativeExitFrameLayout        = 0x0,

This patch sounds good, but as Tom mentioned before, now that we are using enum calss, the names are quite redundant.
    *ExitFrame*Token::CallNative*ExitFrame*Layout

I suggest to remove all the  ExitFrame  from the enumerated value names, as well as the  Layout  , as these are tokens and not a layout of frames.

    ExitFrameToken::CallNative
    ExitFrameToken::IonDOMMethod
    ExitFrameToken::Bare
Attachment #8904355 - Flags: review?(nicolas.b.pierron) → feedback+

Comment 4

19 days ago
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Comment on attachment 8904355 [details] [diff] [review]
> 0001-enum-class-ExitFrameToken.patch
> 
> Review of attachment 8904355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/JitFrames.h
> @@ +493,5 @@
> >  class IonOOLPropertyOpExitFrameLayout;
> >  class IonOOLProxyExitFrameLayout;
> >  class IonDOMExitFrameLayout;
> >  
> > +enum class ExitFrameToken : int32_t
> 
> nit: any reason to not use uint8_t ?
js::jit::Imm32 has a ctor that takes (int32_t)ExitFrameToken, that is the only reason. Changing to uint8_t in the new patch.

> 
> @@ +498,2 @@
> >  {
> > +    CallNativeExitFrameLayout        = 0x0,
> 
> This patch sounds good, but as Tom mentioned before, now that we are using
> enum calss, the names are quite redundant.
>     *ExitFrame*Token::CallNative*ExitFrame*Layout
> 
> I suggest to remove all the  ExitFrame  from the enumerated value names, as
> well as the  Layout  , as these are tokens and not a layout of frames.
> 
>     ExitFrameToken::CallNative
>     ExitFrameToken::IonDOMMethod
>     ExitFrameToken::Bare
Done.

Comment 5

19 days ago
Created attachment 8904653 [details] [diff] [review]
0002-enum-class-ExitFrameToken.patch
Attachment #8904355 - Attachment is obsolete: true
Attachment #8904653 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 6

18 days ago
Comment on attachment 8904653 [details] [diff] [review]
0002-enum-class-ExitFrameToken.patch

Review of attachment 8904653 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this contribution, it is really nice to have shorter names while using an enum class!
Do you have commit access, or you need someone else to push the patch for you?

::: js/src/jit/MacroAssembler-inl.h
@@ +305,3 @@
>  {
>      linkExitFrame(cxreg, scratch);
> +    Push(Imm32((int32_t)token));

nit: use C++ constructor when possible:  int32_t(token)
Attachment #8904653 - Flags: review?(nicolas.b.pierron) → review+

Comment 7

18 days ago
I do not have commit nor try access, thanks.
(Reporter)

Comment 8

17 days ago
Comment on attachment 8904653 [details] [diff] [review]
0002-enum-class-ExitFrameToken.patch

Review of attachment 8904653 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix the nits, and I will send it to try once it compiles successfully locally.

::: js/src/jit/mips64/Trampoline-mips64.cpp
@@ +271,5 @@
>          masm.storePtr(zero, Address(StackPointer, 0)); // fake return address
>  
>          // No GC things to mark, push a bare token.
>          masm.loadJSContext(scratch);
> +        masm.enterFakeExitFrame(scratch, scratch, ExitFrameToken::ExitFrameLayoutBare);

nit: ExitFrameToken::Bare

Comment 9

16 days ago
Created attachment 8905668 [details] [diff] [review]
0003-enum-class-ExitFrameToken.patch
Attachment #8904653 - Attachment is obsolete: true
Attachment #8905668 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 10

16 days ago
Comment on attachment 8905668 [details] [diff] [review]
0003-enum-class-ExitFrameToken.patch

Review of attachment 8905668 [details] [diff] [review]:
-----------------------------------------------------------------

Fix js/src/jit/x64/Trampoline-x64.cpp, or include the change, and fix the nit mentioned in comment 6.
Attachment #8905668 - Flags: review?(nicolas.b.pierron) → feedback+

Comment 11

12 days ago
Created attachment 8906754 [details] [diff] [review]
0004-enum-class-ExitFrameToken.patch

Fixed the nit and included Trampoline-x64.cpp changes.
Attachment #8905668 - Attachment is obsolete: true
Attachment #8906754 - Flags: review?(nicolas.b.pierron)

Comment 12

12 days ago
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b550eb6afd78
ExitFrameTokenValues enum is refactored into enum class ExitFrameToken. r=nbp
(Reporter)

Comment 13

12 days ago
Comment on attachment 8906754 [details] [diff] [review]
0004-enum-class-ExitFrameToken.patch

Review of attachment 8906754 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good to me.

Tested on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58e5c610b041e36e39ccac7f1ed8496215c510f1
( and landed before publishing the review :/ )
Attachment #8906754 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/b550eb6afd78
Status: NEW → RESOLVED
Last Resolved: 11 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.