1602530 - Improve vm/Opcodes.h comments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jason Orendorff [:jorendorff]]

I'm documenting some of the bytecode invariants, as part of trying to make a safer BytecodeEmitter.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 1: Remove the "value" field from FOR_EACH_OPCODE. r?tcampbell.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 2: Remove JSOP_UNUSED* opcodes. r?tcampbell.

The baseline jit's OPCODE_LIST macro is removed at the same time, rather than
try to keep it in sync with Opcodes.h. This touches a bit more code than
expected because OPCODE_LIST treated JSOP_FORCEINTERPRETER as special. Happily
it turns out to be easy to work around that.

Depends on D57517

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 3: Rearrange opcodes. r?tcampbell.

I initially used a script to do this. I checked that sort(1) gives the same
output on the before and after versions of the file.

This does not follow the existing categorization exactly. The plan is to
re-categorize the opcodes in a later patch in this stack.

Depends on D57521

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 4: Insert two comments to get make_opcode_doc.py working again. r?tcampbell.

make_opcode_doc.py allows groups of related opcodes, but checks that they share
all relevant details (stack budget, etc). If they don't match, the script bails
out with an error. Rearranging the bytecodes had the side effect of breaking up
some of these groups, so this changeset simply copies a couple of existing
comments.

(I placed JSOP_INC and JSOP_DEC with JSOP_ADD and JSOP_SUB because they are
optimized versions of the same operation. They don't increment or decrement
variables. inc is always equivalent to one; add. The other group that was
broken up included both JSOP_EQ/NE and JSOP_LT/GT/LE/GE. The new ordering is
the ordering in the standard.)

Depends on D57522

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 5: General comments about bytecode invariants. r?tcampbell.

Depends on D57523

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 6: Revise comments about constants. r?tcampbell.

A few words on generally what I'm trying to do with these edits:

• I want to know what counts as valid bytecode, because I want to make
  a safe BytecodeEmitter type that, when used via its public methods,
  can't produce invalid bytecode that would lead to crashes.

  Therefore, I'm documenting all (!) the rules I an think of, using
  the word "must" to mark them as rules.

  Often the rules are unclear, because it's been pretty informal. This
  exercise therefore involves some judgment calls and other places
  where I still use the word "must" but on a deliberately vague
  requirement ("All paths into the span must establish that
  invariant.")

• I try to let the Stack: line do its job, and not spend prose words
  telling the story of what all values an instruction pushes or pops.

  Generally, I want to spend more words on why things are like this,
  and less words minutely describing stack traffic.

• I'm changing the verbs in the first sentence or two of each comment
  to the imperative: "Pushes" -> "Push". This amounts to a personal
  preference. I like the directness of "Push null." They are
  instructions, after all.

  There is a limit; I'll keep saying "This instruction never throws",
  not "Don't throw."

• The comments in later patches say "binding" a lot. That's because
  it's shorter than "local or global variable, function argument,
  function, class, import, export, or random other thing I
  forgot, or that has been added to the language in the thirty seconds
  since I wrote this".

Depends on D57524

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 7: Revise comments about operators. r?tcampbell.

Depends on D57525

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 8: Revise comments about conversion and "other expression" opcodes. r?tcampbell.

Depends on D57526

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 9: Revise comments about object opcodes. r?tcampbell.

Depends on D57527

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 10: Revise comments about opcodes for enumeration and iteration. r?tcampbell.

Depends on D57528

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 11: Revise comments about opcodes for functions. r?tcampbell.

Depends on D57529

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 12: Revise comments about jump instructions. r?tcampbell.

Depends on D57530

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 13: Revise comments about return instructions. r?tcampbell.

Depends on D57531

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 14: Revise comments about exception handling opcodes. r?tcampbell.

Depends on D57532

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 15: Revise comments about some opcodes for variables and scopes, including function prologue opcodes. r?tcampbell.

Depends on D57534

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 16: Revise comments about stack operations and other VM internals opcodes. r?tcampbell.

Depends on D57535

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 17: Recategorize the opcodes. r?tcampbell.

Depends on D57536

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 18: Remove unnecessary command-line argument to opcode documentation script. r?tcampbell.

Depends on D57538

Comment 19

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 19: Make make_opcode_doc.py run in python3. r?tcampbell.

filter() returned a list in Python 2 and returns an iterator in Python 3.

Depends on D57539

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 20: Add markdown support to make_opcode_doc.py. r?tcampbell.

Depends on D57540

Comment 21

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1602530 - Part 21: Do not sort opcodes by name in documentation. r?tcampbell.

Now that they're in a reasonable order in Opcodes.h, it's better to keep that
order for the docs.

Depends on D57541

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d67d92c7d208
Part 1: Remove the "value" field from FOR_EACH_OPCODE. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/80922e1c6330
Part 2: Remove JSOP_UNUSED* opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/029e060da554
Part 3: Rearrange opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/afac7a2e6a30
Part 4: Insert two comments to get make_opcode_doc.py working again. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/9b7d13e0fe06
Part 5: General comments about bytecode invariants. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/ad18731b2f15
Part 6: Revise comments about constants. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/0024aa0271c0
Part 7: Revise comments about operators. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/6e2c26f28375
Part 8: Revise comments about conversion and "other expression" opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/f128305cb5c0
Part 9: Revise comments about object opcodes. r=tcampbell.

Comment 23

[Raul Gurzau (:RaulG)]

Backed out 9 changesets (bug 1602530) for SM bustage at /gdb-tests.cpp on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/aa8ef4f721768ae09143dc6cfb932f1f67c6476e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=f128305cb5c0ab61124a142ee1a1a654edf290d9&selectedJob=282416675

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=282416675&repo=autoland&lineNumber=7141

Log snippet:
[task 2019-12-23T18:06:32.338Z] SpiderMonkey unwinder is disabled by default, to enable it type:
[task 2019-12-23T18:06:32.338Z] enable unwinder .* SpiderMonkey
[task 2019-12-23T18:06:32.338Z] Breakpoint 1: file /builds/worker/workspace/build/src/js/src/gdb/gdb-tests.cpp, line 53.
[task 2019-12-23T18:06:32.338Z] [Thread debugging using libthread_db enabled]
[task 2019-12-23T18:06:32.338Z] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[task 2019-12-23T18:06:32.338Z] [New Thread 0x7ffff6a71700 (LWP 7722)]
[task 2019-12-23T18:06:32.338Z] [Thread 0x7ffff6a71700 (LWP 7722) exited]
[task 2019-12-23T18:06:32.338Z] [New Thread 0x7ffff5eff700 (LWP 7723)]
[task 2019-12-23T18:06:32.338Z] [New Thread 0x7ffff5d01700 (LWP 7724)]
[task 2019-12-23T18:06:32.338Z] [New Thread 0x7ffff5b03700 (LWP 7725)]
[task 2019-12-23T18:06:32.338Z] [New Thread 0x7ffff5905700 (LWP 7726)]
[task 2019-12-23T18:06:32.338Z] [New Thread 0x7ffff5707700 (LWP 7727)]
[task 2019-12-23T18:06:32.338Z] [New Thread 0x7ffff5509700 (LWP 7728)]
[task 2019-12-23T18:06:32.338Z] [New Thread 0x7ffff530b700 (LWP 7729)]
[task 2019-12-23T18:06:32.338Z] [New Thread 0x7ffff510d700 (LWP 7730)]
[task 2019-12-23T18:06:32.338Z]
[task 2019-12-23T18:06:32.338Z] Thread 1 "gdb-tests" hit Breakpoint 1, breakpoint () at /builds/worker/workspace/build/src/js/src/gdb/gdb-tests.cpp:53
[task 2019-12-23T18:06:32.338Z] 53 fprintf(stderr, "Called " FILE ":breakpoint\n");
[task 2019-12-23T18:06:32.338Z] #1 Fragment_typedef_printers_one::run (this=<Fragment_typedef_printers_one::singleton>, cx=, argv=) at /builds/worker/workspace/build/src/js/src/gdb/tests/typedef-printers.cpp:8
[task 2019-12-23T18:06:32.338Z] 8 breakpoint();
[task 2019-12-23T18:06:32.338Z] (gdb) quit
[task 2019-12-23T18:06:32.338Z]
[task 2019-12-23T18:06:32.338Z] tests failed:
[task 2019-12-23T18:06:32.339Z] test-Interpreter.py
[task 2019-12-23T18:06:32.354Z] + BUILD_STATUS=2

Comment 24

[Ted Campbell [:tcampbell]]

The relevant portion of log is:

[task 2019-12-23T18:04:53.952Z] Unexpected result:
[task 2019-12-23T18:04:53.952Z] expected: '{ fp_ = , sp = fp_.slots() + 2, pc =  (JSOP_IFEQ) }'
[task 2019-12-23T18:04:53.952Z] actual:   '{ fp_ = , sp = fp_.slots() + 2, pc =  (-105) }'

Part 3 reordered opcodes and we hit a signed-vs-unsigned issue in the GDB tests.

If I can find a simple fix, I'll reland this, otherwise we can wait until Jason is back. This work isn't urgent.

Comment 25

[Ted Campbell [:tcampbell]]

Attachment: Bug 1602530 - Workaround gdb-tests issue. r?sfink

Once we re-order the opcodes, a gdb-test fails because JSOP_IFEQ is now >
128. This works around that problem by changing the test to look for a
different opcode.

Comment 26

[Ted Campbell [:tcampbell]]

Tweaked the testcase. Will check on try and then push Jason's patches.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a88c006da5fc
Workaround gdb-tests issue. r=sfink
https://hg.mozilla.org/integration/autoland/rev/9585633a4e58
Part 1: Remove the "value" field from FOR_EACH_OPCODE. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/d28fd3e593c7
Part 2: Remove JSOP_UNUSED* opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/275f83010d64
Part 3: Rearrange opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/aa0fdb572be9
Part 4: Insert two comments to get make_opcode_doc.py working again. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/752cb5e796ba
Part 5: General comments about bytecode invariants. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/cfbe639af441
Part 6: Revise comments about constants. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/6be587fe00dc
Part 7: Revise comments about operators. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/c2077ae6552a
Part 8: Revise comments about conversion and "other expression" opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/b57384d314f4
Part 9: Revise comments about object opcodes. r=tcampbell.

Comment 28

[Jason Orendorff [:jorendorff]]

Thanks.

Comment 29

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a88c006da5fc
https://hg.mozilla.org/mozilla-central/rev/9585633a4e58
https://hg.mozilla.org/mozilla-central/rev/d28fd3e593c7
https://hg.mozilla.org/mozilla-central/rev/275f83010d64
https://hg.mozilla.org/mozilla-central/rev/aa0fdb572be9
https://hg.mozilla.org/mozilla-central/rev/752cb5e796ba
https://hg.mozilla.org/mozilla-central/rev/cfbe639af441
https://hg.mozilla.org/mozilla-central/rev/6be587fe00dc
https://hg.mozilla.org/mozilla-central/rev/c2077ae6552a
https://hg.mozilla.org/mozilla-central/rev/b57384d314f4

Comment 30

[Ted Campbell [:tcampbell]]

Attachment: Bug 1602530 - Fixup escaping and links in vm/Opcodes.h. r?jorendorff

Comment 31

[Ted Campbell [:tcampbell]]

Attachment: Bug 1602530 - Fixup escaping and links in vm/Opcodes.h. r?jorendorff

Comment 32

[Ted Campbell [:tcampbell]]

Attachment: Bug 1602530 - Add more asserts about bytecode structure. r?jorendorff

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf4a4a4e82ba
Part 10: Revise comments about opcodes for enumeration and iteration. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/e3335a38dc67
Part 11: Revise comments about opcodes for functions. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/12cf8297599d
Part 12: Revise comments about jump instructions. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/0ea55d6ffa02
Part 13: Revise comments about return instructions. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/2093d0d74df8
Part 14: Revise comments about exception handling opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/623b2526fb4a
Part 15: Revise comments about some opcodes for variables and scopes, including function prologue opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/0202261dc26f
Part 16: Revise comments about stack operations and other VM internals opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/01562cd62979
Part 17: Recategorize the opcodes. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/8a218ed395e5
Part 18: Remove unnecessary command-line argument to opcode documentation script. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/1b3122a089d1
Part 19: Make make_opcode_doc.py run in python3. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/ff59ddb49234
Part 20: Add markdown support to make_opcode_doc.py. r=tcampbell.
https://hg.mozilla.org/integration/autoland/rev/d25bcced29f6
Part 21: Do not sort opcodes by name in documentation. r=tcampbell.

Comment 34

[Ted Campbell [:tcampbell]]

Attachment: Bug 1602530 - Fixup escaping and links in vm/Opcodes.h. r?jorendorff

Comment 35

[Ted Campbell [:tcampbell]]

Attachment: Bug 1602530 - Add more asserts about bytecode structure. r?jorendorff

Depends on D59061

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1cfad5c14c54
Fixup escaping and links in vm/Opcodes.h. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/d31d6754fce8
Add more asserts about bytecode structure. r=jorendorff

Comment 37

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/cf4a4a4e82ba
https://hg.mozilla.org/mozilla-central/rev/e3335a38dc67
https://hg.mozilla.org/mozilla-central/rev/12cf8297599d
https://hg.mozilla.org/mozilla-central/rev/0ea55d6ffa02
https://hg.mozilla.org/mozilla-central/rev/2093d0d74df8
https://hg.mozilla.org/mozilla-central/rev/623b2526fb4a
https://hg.mozilla.org/mozilla-central/rev/0202261dc26f
https://hg.mozilla.org/mozilla-central/rev/01562cd62979
https://hg.mozilla.org/mozilla-central/rev/8a218ed395e5
https://hg.mozilla.org/mozilla-central/rev/1b3122a089d1
https://hg.mozilla.org/mozilla-central/rev/ff59ddb49234
https://hg.mozilla.org/mozilla-central/rev/d25bcced29f6
https://hg.mozilla.org/mozilla-central/rev/1cfad5c14c54
https://hg.mozilla.org/mozilla-central/rev/d31d6754fce8

Comment 38

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jorendorff, maybe it's time to close this bug?