Closed Bug 1602530 Opened 11 months ago Closed 4 months ago

Improve vm/Opcodes.h comments

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(24 files, 3 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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

Priority: -- → P3
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED

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

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

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

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

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

Depends on D57539

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

Depends on D57541

Pushed by jorendorff@mozilla.com:
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.

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

Flags: needinfo?(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.

Flags: needinfo?(tcampbell)

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.

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

Keywords: leave-open
Pushed by tcampbell@mozilla.com:
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.

Thanks.

Attachment #9118176 - Attachment is obsolete: true
Pushed by jorendorff@mozilla.com:
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.
Attachment #9119274 - Attachment is obsolete: true
Attachment #9119275 - Attachment is obsolete: true
Pushed by tcampbell@mozilla.com:
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

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

Flags: needinfo?(jorendorff)
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Flags: needinfo?(jorendorff)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.