1477621 - Add constant or enum for source note offsets for each source note type.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

Currently we're using magic number like 0, 1, 2 for setSrcNoteOffset arguments.
at least we should add constant or enum, to make it clear which offset it is.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add source note field constants for switch.

Applied to source notes for switch/case
(also, fixed SwitchEmitter::emitCaseOrDefaultJump not to add unnecessary source note, which is found while preparing this patch)
Can I have some feedback?

The plan is following:
  * create classes for each source note type (TableSwitch/CondSwitch/NextCase here),
    and add enum for each offset (I used "Fieilds" as enum name since it can hold non-offset)
  * use the enum when calling BytecodeEmitter::setSrcNoteOffset or GetSrcNoteOffset
  * put everything in SrcNote class (or maybe namespace)
  * move other related code to those classes in followup bugs
    * maybe we could make them accessor, instead of public enum
    * add wrapper for GetSrcNote for each type, which asserts opcode and SN_TYPE
    * move SN_* macros/functions into SrcNote or inner classes

I'm about to fix each source note type separately, at the same time as adding BytecodeEmitter helper classes (bug 1456404 etc)

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add source note field constants for try.

also applied to try.
(simpler than Part 1)

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8994116 [details] [diff] [review]
Part 1: Add source note field constants for switch.

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

Fantastic! Definitely an improvement.

::: js/src/frontend/SourceNotes.h
@@ +49,5 @@
>      M(SRC_CONTINUE,     "continue",    0)  /* JSOP_GOTO is a continue. */                          \
>      M(SRC_BREAK,        "break",       0)  /* JSOP_GOTO is a break. */                             \
>      M(SRC_BREAK2LABEL,  "break2label", 0)  /* JSOP_GOTO for 'break label'. */                      \
>      M(SRC_SWITCHBREAK,  "switchbreak", 0)  /* JSOP_GOTO is a break in a switch. */                 \
> +    M(SRC_TABLESWITCH,  "tableswitch", 1) \

Your plan is to derive this last column automatically, right? (Each enum could have "Count" (or something) as the last value.)

@@ +82,5 @@
> +  public:
> +    // SRC_TABLESWITCH: Source note for JSOP_TABLESWITCH.
> +    class TableSwitch {
> +      public:
> +        enum Fields {

This class + enum could be |enum class TableSwitch|, but I assume you'll add more fields to the classes :)

::: js/src/frontend/SwitchEmitter.cpp
@@ +229,5 @@
>              return false;
>          return true;
>      }
>  
> +    if (state_ == State::Case) {

Why is it okay to move this code? I don't think isDefault == true implies state_ != State::Case?

@@ +398,5 @@
>      }
>  
>      // Set the SRC_SWITCH note's offset operand to tell end of switch.
> +    // This code is shared between table switch and cond switch.
> +    MOZ_ASSERT(unsigned(SrcNote::TableSwitch::EndOffset) == unsigned(SrcNote::CondSwitch::EndOffset));

static_assert should work

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Why is it okay to move this code? I don't think isDefault == true implies
> state_ != State::Case?

Oops, sorry, I missed your comment about this.

Comment 5

[Jan de Mooij [:jandem]]

One thing we've talked about before is to encode most source notes as bytecode immediates (except for the line/column number information; it would have to be stored separately).

However that's a bigger change and these patches really clean up and document what we have now, so I still think this is a good idea.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Remove unnecessary note from JSOP_DEFAULT.

Separated the fix for SwitchEmitter's default case.
this bug was introduced by bug 1456006, where it wasn't adding source note for JSOP_DEFAULT, but I misunderstood it.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add source note field constants for switch.

Added Count to enum, and replaced all consumers to use enum.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add source note field constants for try.

also added Count to enum

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add source note field constants for colspan.

For now, I'm going to apply the same change for source notes which I'm not going to touch in other bugs (bug 1456404 etc)
here's the patch for SRC_COLSPAN.

the long line in BytecodeUtil-inl.h suggests that we should add static accessor method to the SrcNote::ColSpan class,
but I'd like to leave it to other bug, for simplicity.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Add source note field constants for line.

same change for SRC_SETLINE.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8994396 [details] [diff] [review]
Part 1: Add source note field constants for switch.

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

::: js/src/frontend/SourceNotes.h
@@ +43,5 @@
> +        enum Fields {
> +            // The offset of the end of switch (the first non-JumpTarget op
> +            // after switch) from JSOP_TABLESWITCH.
> +            EndOffset,
> +            Count,

Nit: I would remove the trailing comma after Count here and below. It normally makes sense: when you add a new value you don't have to add a comma to the previous line, but here we always want Count as last value I think :)

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8994398 [details] [diff] [review]
Part 3: Add source note field constants for colspan.

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

::: js/src/frontend/SourceNotes.h
@@ +86,5 @@
> +        enum Fields {
> +            // The column span (the diff between the column corresponds to the
> +            // current op and last known column).
> +            Span,
> +            Count,

We can use the Count value in the "table" below.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8994399 [details] [diff] [review]
Part 4: Add source note field constants for line.

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

Beautiful.

Comment 14

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8e23bff248c7500302deb4b68804bbafbc462b
Bug 1477621 - Part 0: Remove unnecessary note from JSOP_DEFAULT. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7b22fe0124b332ea410e44b94afa11d29b254a
Bug 1477621 - Part 1: Add source note field constants for switch. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cabc189feb9c32b870183b8096ca8702f518f61
Bug 1477621 - Part 2: Add source note field constants for try. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/d18e67f75339c713ce1aadc54631898b5af5df3d
Bug 1477621 - Part 3: Add source note field constants for colspan. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d328a1be52ce31ef56c4fe7e4155b4a6f2d35fc
Bug 1477621 - Part 4: Add source note field constants for line. r=jandem

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4f8e23bff248
https://hg.mozilla.org/mozilla-central/rev/5f7b22fe0124
https://hg.mozilla.org/mozilla-central/rev/7cabc189feb9
https://hg.mozilla.org/mozilla-central/rev/d18e67f75339
https://hg.mozilla.org/mozilla-central/rev/9d328a1be52c