Closed
Bug 471713
Opened 16 years ago
Closed 10 years ago
Document opcodes (stack conventions, preconditions, postconditions, other implicit assumptions) in jsopcode.tbl
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: Waldo, Assigned: arai)
References
()
Details
(Whiteboard: [good first bug])
Attachments
(10 files, 15 obsolete files)
1.33 KB,
patch
|
Waldo
:
review+
ehoogeveen
:
checkin+
|
Details | Diff | Splinter Review |
13.78 KB,
patch
|
Waldo
:
review+
ehoogeveen
:
checkin+
|
Details | Diff | Splinter Review |
79.61 KB,
text/html
|
Details | |
11.61 KB,
patch
|
Waldo
:
review+
ehoogeveen
:
checkin+
|
Details | Diff | Splinter Review |
16.04 KB,
patch
|
evilpie
:
review+
ehoogeveen
:
checkin+
|
Details | Diff | Splinter Review |
16.12 KB,
patch
|
djvj
:
review+
ehoogeveen
:
checkin+
|
Details | Diff | Splinter Review |
16.21 KB,
patch
|
luke
:
review+
ehoogeveen
:
checkin+
|
Details | Diff | Splinter Review |
907 bytes,
patch
|
Waldo
:
review+
ehoogeveen
:
checkin+
|
Details | Diff | Splinter Review |
15.00 KB,
patch
|
jorendorff
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
814 bytes,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
Something akin to what's at the given URL, basically.
I'll probably try to get a large batch done all at once, but past the simpler/easier cases I'll probably try to attack incrementally.
Comment 1•16 years ago
|
||
Cool! I would really like it if the docs showed the top-of-stack input contents and output contents, and for each value had the "type" (allowed jsval taggings, I guess, maybe with other constraints annotated if any) and "meaning". Diagrams would be the best, of course, but even a text format like:
ADD
input: v1 v2
output: (v1+v2)
would be beautiful.
Comment 2•16 years ago
|
||
Forth has concise stack effect notation, see Tamarin Tracing. Not good for casual readers, but it could be pretty-printed or elaborated ad nauseum.
Once again the literate programming approach seems better than a separate doc that can too easily bit-rot.
Best of all would be to encode more info into the opcode table and extract it mechanically. I've thought about regularizing the decompiler this way, but it wants a DSL of some sort.
/be
Reporter | ||
Comment 3•14 years ago
|
||
Recent discussion in bug 578517 paged this bug back into memory. In case it wasn't clear from this bug's neglect in the last year and a half, I don't think I have time to work on it, except piecemeal.
One point I would like to stress is that I would rather not see perfect made the enemy of good. A paragraph of prosaic documentation per opcode, plus some completely ad hoc description of stack pre- and post-conditions, is far preferable to nothing. Given the relative unimportance of this compared to feature and perf work, I sense the perfect will be the enemy of good if we hold out for some sort of mechanically-usable descriptive format.
(Luke, our discussion a few days back about good first bugs made me think: this is probably a very good first bug [perhaps only in a bite-sized piece, but I'd take whatever I can get]. You get to learn about how the SpiderMonkey stack works; read through how opcodes are implemented, both in the interpreter and in the tracer [and soon in the method JIT]; and likely examine how they're emitted during the AST walk to generate bytecode. It's the intersection of a lot of different core SpiderMonkey concepts, but it doesn't require much specific foreknowledge to tackle.)
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug]
Comment 4•14 years ago
|
||
As noted in the referred-to bug, I started a set of concise verbal descriptions in https://developer.mozilla.org/En/SpiderMonkey/Internals/Bytecode -- maybe we could break out another section for a notation-based convention.
Comment 5•14 years ago
|
||
FWIW, I agree with Brendan that inline comments in the file are far superior to a separate doc. I've gone this route with LIRopcode.tbl in Nanojit and my life has improved greatly as a result.
Comment 6•11 years ago
|
||
Jeff, are you still working on this?
I'm going through old "good first bugs" that have been inactive for a while. Is this a valid issue?
Flags: needinfo?(jwalden+bmo)
Reporter | ||
Comment 7•11 years ago
|
||
It's still valid. We still have a stack machine, jsopcode.tbl is still very under-documented, we have nothing that intelligently talks about how the stack size changes during evaluation of an opcode, etc.
For whatever reason most of the work I've been doing lately has been away from opcodes, the interpreter, etc. lately, so this hasn't been something I could do, or start, in passing while doing other things. I'm most definitely not working on this right now.
Assignee: jwalden+bmo → general
Status: ASSIGNED → NEW
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 8•11 years ago
|
||
Added documents in Opcodes.h as comments, and created script to generate a HTML document from Opcodes.h and Xdr.h (actually a HTML fragment, to paste in MDN).
If there is any wrong description, or bad styling in HTML, could you tell me?
By the way, the title of this bug sould be "... in Opcodes.h" instead of "... in jsopcode.tbl" now?.
I'll post an example output soon.
Attachment #8376804 -
Flags: feedback?(jwalden+bmo)
Assignee | ||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
Comment on attachment 8376804 [details] [diff] [review]
documents in comment, script to generate document HTML
Review of attachment 8376804 [details] [diff] [review]:
-----------------------------------------------------------------
This is great! I wonder if you could merge the comments for lots of the related ones. E.g. TRUE and FALSE are very similar, as are ADD/SUB/MUL/DIV/MOD, and various others.
::: js/src/vm/Opcodes.h
@@ +706,1 @@
> macro(JSOP_ENDINIT, 92, "endinit", NULL, 1, 0, 0, JOF_BYTE) \
Is this really a no-op? Huh.
::: js/src/vm/make_opcode_doc.py
@@ +184,5 @@
> + 'JSOP_SWAP',
> + ]),
> + ]),
> +
> + ('Litetals', [
Typo: should be 'Literals'.
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #10)
> This is great! I wonder if you could merge the comments for lots of the
> related ones. E.g. TRUE and FALSE are very similar, as are
> ADD/SUB/MUL/DIV/MOD, and various others.
Thank you for your comment!
What do you think is the best way to merge them?
1. Make a document template and extract them when generating HTML document.
So generated HTML will be same as now.
However the comment itself is hard to read.
> /*
> * @def ARITHMETIC(A,OP)
> * Pops the top two values on the stack, converts them into number, pushes
> * a {A} of them onto the stack.
> * Operands:
> * Stack: lval, rval => (lval {OP} rval)
> */ \
> /* @ARITHMETIC(subtraction, -) */ \
> macro(JSOP_SUB, 28, "sub", "-", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> /* @ARITHMETIC(multiplication, *) */ \
> macro(JSOP_MUL, 29, "mul", "*", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> /* @ARITHMETIC(division, /) */ \
> macro(JSOP_DIV, 30, "div", "/", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
2. Simplify description by referring other comment.
> /*
> * Pops the top two values on the stack, converts them into number, pushes
> * a subtraction of them onto the stack.
> * Operands:
> * Stack: lval, rval => (lval - rval)
> */ \
> macro(JSOP_SUB, 28, "sub", "-", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> /*
> * Like JSOP_SUB but a multiplication.
> * Operands:
> * Stack: lval, rval => (lval * rval)
> */ \
> macro(JSOP_MUL, 29, "mul", "*", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> /*
> * Like JSOP_MUL but a division.
> * Operands:
> * Stack: lval, rval => (lval / rval)
> */ \
> macro(JSOP_DIV, 30, "div", "/", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
3. Simplify description by referring other comment and removing Operands and Stack information.
So JSOP_MUL section does not have Operands and Stack Uses/Defs rows in generated HTML.
> /*
> * Pops the top two values on the stack, converts them into number, pushes
> * a subtraction of them onto the stack.
> * Operands:
> * Stack: lval, rval => (lval - rval)
> */ \
> macro(JSOP_SUB, 28, "sub", "-", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> /*
> * Like JSOP_SUB but a multiplication.
> */ \
> macro(JSOP_MUL, 29, "mul", "*", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> /*
> * Like JSOP_MUL but a division.
> */ \
> macro(JSOP_DIV, 30, "div", "/", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
4. Just merge comments, and generate single section for them in HTML.
This may be difficult when related opcodes are separated in the list (such like JSOP_GETPROP and JSOP_CALLPROP).
> /*
> * Pops the top two values on the stack, converts them into number, pushes
> * a result of arithmetic operation of them onto the stack.
> * Operands:
> * Stack: lval, rval => (lval OP rval)
> */ \
> macro(JSOP_SUB, 28, "sub", "-", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> macro(JSOP_MUL, 29, "mul", "*", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> macro(JSOP_DIV, 30, "div", "/", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
If you have any other idea, could you tell me?
> ::: js/src/vm/Opcodes.h
> @@ +706,1 @@
> > macro(JSOP_ENDINIT, 92, "endinit", NULL, 1, 0, 0, JOF_BYTE) \
>
> Is this really a no-op? Huh.
If I understand correctly, Interpreter/JIT do nothing with JSOP_ENDINIT itself
(there are only assertions in interpreter).
But it may be better to modify description to following:
> * A no-operation bytecode.
> *
> * Indicates the end of object/array initialization, and used for
> * Type-Inference, decompile, etc.
Is it okay?
> Typo: should be 'Literals'.
Thank you! I'll fix it.
Comment 12•11 years ago
|
||
> 4. Just merge comments, and generate single section for them in HTML.
> This may be difficult when related opcodes are separated in the list (such
> like JSOP_GETPROP and JSOP_CALLPROP).
> > /*
> > * Pops the top two values on the stack, converts them into number, pushes
> > * a result of arithmetic operation of them onto the stack.
> > * Operands:
> > * Stack: lval, rval => (lval OP rval)
> > */ \
> > macro(JSOP_SUB, 28, "sub", "-", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> > macro(JSOP_MUL, 29, "mul", "*", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> > macro(JSOP_DIV, 30, "div", "/", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
I'd do that one. I wouldn't worry about related opcodes that are separated... though if you really want to, you could renumber them so they're adjacent. But that might not be worth it.
> > * A no-operation bytecode.
> > *
> > * Indicates the end of object/array initialization, and used for
> > * Type-Inference, decompile, etc.
> Is it okay?
That looks good.
BTW, jwalden will be able to give you more detailed comments. These were just some things I noticed when looking quickly at the patch.
Reporter | ||
Comment 13•11 years ago
|
||
Okay, that this patch is happening is mind-blowingly awesome. Not even exaggerating a little bit. I think I'm out of time to look at this today, but it's happening for sure tomorrow first thing.
Reporter | ||
Comment 14•11 years ago
|
||
Comment on attachment 8376804 [details] [diff] [review]
documents in comment, script to generate document HTML
Review of attachment 8376804 [details] [diff] [review]:
-----------------------------------------------------------------
Wow, this is pretty great. Sorry I've been super-slow about getting back to you on it. :-(
I didn't review the Opcodes.h part of this patch, because it's going to take awhile to get through 150+ opcode descriptions and double-check their correctness. At least, not if one person is doing it. I think we should break up this burden somewhat, so that it can be incrementally reviewed and landed. Would you mind posting a single patch with just the make_opcode_doc.py addition (with requested minor changes), then perhaps five patches with roughly even splits of the additions to Opcodes.h? Flag me for review on all of them, and I'll direct them to appropriate people tomorrow. (By which I mean, inform people that this needs doing, then watch them all run like crazy to pick off parts so the entire set of all this can land. Because everyone will want that like crazy.)
Merging would be nice for the similar ones. But again. MUST NOT STOP-ENERGY. Followup work. It's not the worst thing in the world to have duplication. (I agree with njn that when we try to reduce that, we should only allow reused commenting on consecutive opcodes.)
::: js/src/vm/make_opcode_doc.py
@@ +2,5 @@
> +
> +""" Usage: make_opcode_doc.py [path_to_mozilla_central]
> +
> + This script generats SpiderMonkey Bytecode document
> + from js/src/vm/Opcodes.h and js/src/vm/Xdr.h.
This script generates SpiderMonkey bytecode documentation from js/src/vm/Opcodes.h and js/src/vm/Xdr.h.
@@ +6,5 @@
> + from js/src/vm/Opcodes.h and js/src/vm/Xdr.h.
> +
> + This script outputs document to stdout,
> + which is intended to paste into following MDN page:
> + https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
Output is written to stdout and should be pasted into the following MDN page...
@@ +14,5 @@
> +import re, sys
> +from xml.sax.saxutils import escape
> +
> +# Index for document.
> +# The opcode which is not contained in list will be shown in "Other" section.
# An opcode not contained in this list will be shown...
@@ +16,5 @@
> +
> +# Index for document.
> +# The opcode which is not contained in list will be shown in "Other" section.
> +index = [
> + ('Statements', [
Totally not intending to bikeshed. We can fix this in a separate patch later. I do not want to delay this patch any more than I absolutely must. But I would probably prefer if this group info were included in Opcodes.h. Maybe Category: Statements, Type: Jumps or something in the doc comments, with a list of all categories/types in a comment at the top of the file.
@@ +238,5 @@
> + ]),
> +]
> +
> +def get_xdr_version(dir):
> + version_pat = re.compile('XDR_BYTECODE_VERSION = ([^;]+);')
Use this instead, so the version number is linearly increasing:
'XDR_BYTECODE_VERSION = uint32_t\(0xb973c0de - (\d+)\);'
I suppose technically one could argue the whole thing is the version, which it technically is. But in practice I have to believe people will convert any value into the corresponding subtrahend, so we might as well save them the time. (Or show both values in UI here -- maybe that's the clear best choice!)
@@ +251,5 @@
> +
> + return version
> +
> +quoted_pat = re.compile('([^A-Za-z0-9]|^)\'([^\']+)\'')
> +js_pat = re.compile('([^A-Za-z0-9]|^)(JS[A-Z0-9_\*]+)')
Differently quoting those strings (as raw strings) to avoid needing to escape embedded apostrophes seems useful here:
quoted_pat = re.compile(r"([^A-Za-z0-9]|^)'([^']+)'")
js_pat = re.compile(r"([^A-Za-z0-9]|^)(JS[A-Z0-9_\*]+)")
@@ +259,5 @@
> +
> + return text
> +
> +def get_opcodes(dir):
> + iter_pat = re.compile('/\*(.*?)\*/|macro\(([^,]+),\s*([0-9]+),\s*[^,]+,\s*[^,]+,\s*([0-9\-]+),\s*([0-9\-]+),\s*([0-9\-]+),\s*([^\)]+)\)', re.S)
This would be far more readable spread across several lines:
iter_pat = re.compile(r"/\*(.*?)\*/" # either a documentation comment...
r"|"
r"macro\(" # or a macro(...) call, currently alternating
r"([^,]+),\s*" # op
r"([0-9]+),\s*" # val
r"[^,]+,\s*" # name
r"[^,]+,\s*" # image
r"([0-9\-]+),\s*" # length
r"([0-9\-]+),\s*" # nuses
r"([0-9\-]+),\s*" # ndefs
r"([^\)]+)" # format
r"\)"
@@ +260,5 @@
> + return text
> +
> +def get_opcodes(dir):
> + iter_pat = re.compile('/\*(.*?)\*/|macro\(([^,]+),\s*([0-9]+),\s*[^,]+,\s*[^,]+,\s*([0-9\-]+),\s*([0-9\-]+),\s*([0-9\-]+),\s*([^\)]+)\)', re.S)
> + format_pat = re.compile('^\s*\* ?', re.M)
space_star_space_pat would be a better name. Longer, for sure, but much clearer about its use/purpose.
@@ +401,5 @@
> +
> +def print_doc(version, opcodes):
> + print("""<h2 id="Bytecode_Listing">Bytecode Listing</h2>
> +
> +<p>Following document is automatically generated from <a href="http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Opcodes.h">Opcodes.h</a>, <a href="http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Xdr.h">Xdr.h</a>, and <a href="http://mxr.mozilla.org/mozilla-central/source/js/src/vm/make_opcode_doc.py">make_opcode_doc.py</a>.</p>
Add in some line breaks to this line, please. :-) Not violating an 80ch limit would be nice, but feel free to overflow if there's no good opportunity to break earlier.
Maybe instead of "automatically generated from X, Y, and Z" you should have "automatically generated from X and Y by Z".
@@ +403,5 @@
> + print("""<h2 id="Bytecode_Listing">Bytecode Listing</h2>
> +
> +<p>Following document is automatically generated from <a href="http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Opcodes.h">Opcodes.h</a>, <a href="http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Xdr.h">Xdr.h</a>, and <a href="http://mxr.mozilla.org/mozilla-central/source/js/src/vm/make_opcode_doc.py">make_opcode_doc.py</a>.</p>
> +
> +<p><code>XDR_BYTECODE_VERSION</code> is <code>{version}</code>.</p>
Bytecode version: {number that's the 0xb973c0de subtrahend} (hex magic value {actual XDR_BYTECODE_VERSION number})
@@ +408,5 @@
> +""".format(version=version))
> +
> + for (name, subsections) in index:
> + print('<h3 id="{id}">{name}</h3>'.format(name=name, id=name.replace(' ', '-')))
> + for (subname, list) in subsections:
Inferring this structure from the parsed data would be preferable. But not in the first-pass patch. MUST NOT STOP-ENERGY. We are way way way way way more than happy to take this as it is and change that later. :-)
Attachment #8376804 -
Flags: feedback?(jwalden+bmo) → feedback+
Assignee | ||
Comment 15•11 years ago
|
||
Thank you for reviewing!
> I think we should
> break up this burden somewhat, so that it can be incrementally reviewed and
> landed. Would you mind posting a single patch with just the
> make_opcode_doc.py addition (with requested minor changes), then perhaps
> five patches with roughly even splits of the additions to Opcodes.h?
Okay, I've split the patch into following series:
Part 1: Add a script to generate documentation for SpiderMonkey opcodes
add make_opcode_doc.py
Part 2: Add documentation for control statements related opcodes
Part 3: Add documentation for function call related opcodes
Part 4: Add documentation for variables and constants related opcodes
Part 5: Add documentation for operators and stack related opcodes
Part 6: Add documentation for object and array related opcodes
add documentation comments, about 30 opcodes per each patch
Part 7: Add index for documentation
to minify the impact when category/type changed in Part 2...6,
add index at last
> Merging would be nice for the similar ones. But again. MUST NOT
> STOP-ENERGY. Followup work. It's not the worst thing in the world to have
> duplication. (I agree with njn that when we try to reduce that, we should
> only allow reused commenting on consecutive opcodes.)
Added support for merging.
Consecutive opcodes without documentation comment between them (such like 4th
example in comment #11) are merged and displayed as single section.
Rules are following:
* length, ndefs and nuses should be same for all opcodes in merged section
(of course, if not same, it implies documentation is wrong)
* flags can be different (to merge JSOP_EQ and JSOP_LT)
Already merged some opcodes (e.g. SUB/MUL/DIV/MOD),
but there might be more opcodes which can be merged.
> Totally not intending to bikeshed. We can fix this in a separate patch
> later. I do not want to delay this patch any more than I absolutely must.
> But I would probably prefer if this group info were included in Opcodes.h.
> Maybe Category: Statements, Type: Jumps or something in the doc comments,
> with a list of all categories/types in a comment at the top of the file.
Added "Category: " and "Type: " for each opcode,
and index is written in a comment which contains "[Index]"
at a top of Opcodes.h (in patch Part 7).
Categories and Types are displayed in same order as written in the comment.
Each opcodes are displayed in alphabetical order inside each Type,
if there are merged opcodes, smallest name of them is used for ordering.
If Type is not specified for an opcode, it is displayed under the Category.
(e.g. "Other" Category)
Attachment #8376804 -
Attachment is obsolete: true
Attachment #8400604 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #8400605 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8400606 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #8400607 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #8400608 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #8400609 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8400610 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 22•11 years ago
|
||
Here is an example output embedded in MDN page.
Attachment #8376805 -
Attachment is obsolete: true
Reporter | ||
Comment 23•11 years ago
|
||
Comment on attachment 8400604 [details] [diff] [review]
Part 1: Add a script to generate documentation for SpiderMonkey opcodes
Review of attachment 8400604 [details] [diff] [review]:
-----------------------------------------------------------------
Moving all the grouping stuff into the comments? Such an over-achiever. ;-) This is awesome! Thanks!
This script isn't actually quite runnable until we have all the ops documented, but probably it's worth landing this as soon as you've made these changes. Then further improvements to it won't require rereading the entire patch. :-) Interdiffs help on that front, but not as much as would be ideal.
::: js/src/vm/make_opcode_doc.py
@@ +31,5 @@
> + version = int(m.group(1))
> +
> + return version
> +
> +quoted_pat = re.compile(r"([^A-Za-z0-9]|^)'([^']+)'")
It occurs to me that this won't play well with comments containing contractions. I guess we can deal with that issue when we reach it.
@@ +58,5 @@
> + current_types = []
> + index.append((category_name, current_types))
> + else:
> + type_name = line.strip()
> + if type_name and current_types != None:
PEP8 says to use |is not| instead of != here.
@@ +95,5 @@
> + return None
> +
> +def add_to_index(index, opcode):
> + types = find_by_name(index, opcode.category_name)
> + if types == None:
is not
@@ +97,5 @@
> +def add_to_index(index, opcode):
> + types = find_by_name(index, opcode.category_name)
> + if types == None:
> + warn("Category is not listed in index: "
> + "{name}".format(name=opcode.category_name))
I'm not entirely convinced this should be allowed -- perhaps better to error out entirely than to warn and let typos (the most likely possibility) slide. Same for most of the other warn() calls in this patch. But it should be fine enough to run with this for now, and maybe change later if we want.
@@ +99,5 @@
> + if types == None:
> + warn("Category is not listed in index: "
> + "{name}".format(name=opcode.category_name))
> + index.append((opcode.category_name, [(opcode.type_name, [opcode])]))
> +
Get rid of this blank line, not convinced it usefully elucidates structure.
@@ +103,5 @@
> +
> + return
> +
> + opcodes = find_by_name(types, opcode.type_name)
> + if opcodes == None:
is not
@@ +109,5 @@
> + warn("Type is not listed in {category}: "
> + "{name}".format(category=opcode.category_name,
> + name=opcode.type_name))
> + types.append((opcode.type_name, [opcode]))
> +
Also kill this blank line.
@@ +138,5 @@
> +
> +def get_opcodes(dir):
> + iter_pat = re.compile(r"/\*(.*?)\*/" # either a documentation comment...
> + r"|"
> + r"macro\(" # or a macro(...) call, currently alternating
The ", currently alternating" part should be removed, if you actually allowed ops to share documentation. :-)
@@ +282,5 @@
> +
> + return value
> +
> +def print_opcode(opcode):
> + names_template = '{name} [-{nuses}, +{ndefs}{flags}]'
'{name} [-{nuses}, +{ndefs}] ({flags})' perhaps. Blending the flags into the stack effects seems not super-readable to me.
@@ +292,5 @@
> + flags=escape(code.flags)),
> + sorted([opcode] + opcode.group,
> + key=lambda opcode: opcode.name))
> + if len(opcode.group) == 0:
> + values = ['{value}(0x{value:02x})'.format(value=opcode.value)]
Add a space between {value} and the (...) hex display version of it, for readability.
@@ +294,5 @@
> + key=lambda opcode: opcode.name))
> + if len(opcode.group) == 0:
> + values = ['{value}(0x{value:02x})'.format(value=opcode.value)]
> + else:
> + values_template = '{name}: {value}(0x{value:02x})'
Same here.
@@ +321,5 @@
> + stack_uses=escape(opcode.stack_uses),
> + stack_defs=escape(opcode.stack_defs),
> + desc=opcode.desc)) # desc is already escaped
> +
> +def idfy(name):
Rename to make_element_id, please.
@@ +327,5 @@
> +
> +def print_doc(version, index):
> + print("""<h2 id="Bytecode_Listing">Bytecode Listing</h2>
> +
> +<p>Following document is automatically generated from
Actually: "This document is...", please.
@@ +336,5 @@
> +<p>Bytecode version: <code>{version}</code>
> +(<code>0x{actual_version:08x}</code>).</p>
> +""".format(source_base=SOURCE_BASE,
> + version=version,
> + actual_version=0xb973c0de-version))
Spaces around the - in the actual_version line.
Attachment #8400604 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 24•11 years ago
|
||
Thank you again!
I didn't know about PEP8, I'll read related documents.
Applied "Imports" rule in it (separated "import re, sys" line.)
> > +quoted_pat = re.compile(r"([^A-Za-z0-9]|^)'([^']+)'")
>
> It occurs to me that this won't play well with comments containing
> contractions. I guess we can deal with that issue when we reach it.
Yeah, it may be better to search safer, powerful and famous markup.
I don't know much about such markup, but Markdown may be nice for this purpose.
If you have any ideas about markup or its Python library, please tell me :)
Anyway, I think the problematic case about contractions is very rare,
(if I understand correctly, contractions rarely appear inside quoted code,
and almost all English contractions have leading alphabet just before
quotation mark, so the pattern does not match),
and it's not so hard to replace markup in the comment later.
> I'm not entirely convinced this should be allowed -- perhaps better to error
> out entirely than to warn and let typos (the most likely possibility) slide.
I agree.
Changed warn to error.
Attachment #8400604 -
Attachment is obsolete: true
Attachment #8401161 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 25•11 years ago
|
||
updated example HTML
Attachment #8400611 -
Attachment is obsolete: true
Reporter | ||
Comment 26•11 years ago
|
||
Comment on attachment 8400610 [details] [diff] [review]
Part 7: Add index for documentation
Review of attachment 8400610 [details] [diff] [review]:
-----------------------------------------------------------------
I'm going to try to review this, cherry-pick the comment for JSOP_NOP (as soon as I find which patch it's in), review the Python script, then land those. Once those are in place, the rest of this is much more leisurely, because anyone who adds an opcode can run the script to double-check their work. (I'm leery of doing too much here before there's a script people can use to check their work.) I don't have too many comments on these disjoint components, so I'll just push them as one patch once I've done the reviewing.
Attachment #8400610 -
Flags: review?(jwalden+bmo) → review+
Reporter | ||
Comment 27•11 years ago
|
||
Comment on attachment 8401161 [details] [diff] [review]
Part 1: Add a script to generate documentation for SpiderMonkey opcodes
Review of attachment 8401161 [details] [diff] [review]:
-----------------------------------------------------------------
No need to post a new patch with this one tweak, I'll do it when I land the first few bits here. :-)
::: js/src/vm/make_opcode_doc.py
@@ +284,5 @@
> +
> + return ' ({flags})'.format(flags=flags)
> +
> +def print_opcode(opcode):
> + names_template = '{name} [-{nuses}, +{ndefs}]{flags}'
Space between "]" and "{" at the end there.
Attachment #8401161 -
Flags: review?(jwalden+bmo) → review+
Reporter | ||
Comment 28•11 years ago
|
||
Oh bleh, I can't just comment JSOP_NOP and have the script continue, because we changed warn() to error(). :-) I commented out the sys.exit() in error() for now, to keep going til we have all the docs landed.
Script, index comment, and JSOP_NOP documentation landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e27520610ef
I'm halfway through one of these, and I'll look into parceling out the other reviews over the weekend.
Reporter | ||
Updated•11 years ago
|
Whiteboard: [good first bug] → [good first bug][leave open]
Assignee | ||
Comment 29•11 years ago
|
||
Thank you!
I noticed that new opcode JSOP_LAMBDA_ARROW is added, so I'll update patch Part3 soon.
> > + names_template = '{name} [-{nuses}, +{ndefs}]{flags}'
>
> Space between "]" and "{" at the end there.
Space and parens are already included in the return value of format_flags function,
to avoid outputting empty parens on no flags.
Assignee | ||
Comment 30•11 years ago
|
||
Added documantation for JSOP_LAMBDA_ARROW.
Attachment #8400606 -
Attachment is obsolete: true
Attachment #8400606 -
Flags: review?(jwalden+bmo)
Attachment #8402185 -
Flags: review?(jwalden+bmo)
Comment 31•11 years ago
|
||
Reporter | ||
Comment 32•11 years ago
|
||
Comment on attachment 8400608 [details] [diff] [review]
Part 5: Add documentation for operators and stack related opcodes
Review of attachment 8400608 [details] [diff] [review]:
-----------------------------------------------------------------
Mostly a bunch of little English nits and such. Looks pretty reasonable, tho!
My head hurts verifying the ordering of operands to JSOP_IN and JSOP_INSTANCEOF. Maybe it makes sense from a source/execution-order perspective, but verifying that ordering as what's actually used took more time than I really care to admit. :-\
::: js/src/vm/Opcodes.h
@@ +138,5 @@
> macro(JSOP_ARGUMENTS, 9, "arguments", NULL, 1, 0, 1, JOF_BYTE) \
> \
> + /*
> + * Swaps the top two values on the stack. This is useful for things like
> + * post- increment/decrement.
post-increment/decrement
@@ +148,3 @@
> macro(JSOP_SWAP, 10, "swap", NULL, 1, 2, 2, JOF_BYTE) \
> + /*
> + * Pops the top 'n' values on the stack.
*from* the stack
@@ +157,5 @@
> macro(JSOP_POPN, 11, "popn", NULL, 3, -1, 0, JOF_UINT16) \
> \
> /* More long-standing bytecodes. */ \
> + /*
> + * Duplicates the top of stack value.
Pushes a copy of the top value on the stack.
@@ +182,5 @@
> */ \
> macro(JSOP_SETCONST, 14, "setconst", NULL, 5, 1, 1, JOF_ATOM|JOF_NAME|JOF_SET) \
> + /*
> + * Pops the top two values on the stack, converts them into signed 32-bit
> + * integer, pushes a result of bitwise operation of them onto the stack.
Pops the top two values 'lval' and 'rval' from the stack, then pushes the result of the operation applied to the two operands, converting both to 32-bit signed integers if necessary.
@@ +193,5 @@
> macro(JSOP_BITXOR, 16, "bitxor", "^", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> macro(JSOP_BITAND, 17, "bitand", "&", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> + /*
> + * Pops the top two values on the stack, pushes a result of comparison of
> + * them onto the stack.
Pops the top two values from the stack and pushes the result of comparing them.
@@ +207,5 @@
> macro(JSOP_GT, 22, "gt", ">", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> macro(JSOP_GE, 23, "ge", ">=", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> + /*
> + * Pops the top two values on the stack, converts them into signed 32-bit
> + * integer, pushes signed left/right shift operation of them onto the stack.
Pops the top two values 'lval' and 'rval' from the stack, then pushes the result of the operation applied to the operands.
@@ +217,5 @@
> macro(JSOP_LSH, 24, "lsh", "<<", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> macro(JSOP_RSH, 25, "rsh", ">>", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> + /*
> + * Pops the top two values on the stack, converts them into unsigned 32-bit
> + * integer, pushes unsigned right shift operation of them onto the stack.
Pops the top two values 'lval' and 'rval' from the stack, then pushes 'lval >>> rval'.
@@ +227,5 @@
> macro(JSOP_URSH, 26, "ursh", ">>>", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> + /*
> + * Pops the top two values on the stack as 'rval' and 'lval', if both of
> + * their types are number, pushes a sum of them onto the stack, if one of
> + * their types is string, pushes a concatenation of them onto the stack.
Pops the top two values 'lval' and 'rval' from the stack, then pushes the result of 'lval + rval'.
@@ +237,4 @@
> macro(JSOP_ADD, 27, "add", "+", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> + /*
> + * Pops the top two values on the stack, converts them into number, pushes
> + * a result of arithmetic operation of them onto the stack.
Pops the top two values 'lval' and 'rval' from the stack, then pushes the result of applying the arithmetic operation to them.
@@ +248,5 @@
> macro(JSOP_DIV, 30, "div", "/", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> macro(JSOP_MOD, 31, "mod", "%", 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \
> + /*
> + * Pops the top of stack value, converts it into a boolean, pushes a
> + * negation of it onto the stack.
Pops the value 'val' from the stack, then pushes '!val'.
@@ +258,4 @@
> macro(JSOP_NOT, 32, "not", "!", 1, 1, 1, JOF_BYTE|JOF_ARITH|JOF_DETECTING) \
> + /*
> + * Pops the top of stack value, converts it into a signed 32-bit integer,
> + * pushes bitwise NOT of it onto the stack.
Pops the value 'val' from the stack, then pushes '~val'.
@@ +267,4 @@
> macro(JSOP_BITNOT, 33, "bitnot", "~", 1, 1, 1, JOF_BYTE|JOF_ARITH) \
> + /*
> + * Pops the top of stack value, converts it into a number and negates its
> + * sign, pushes it onto the stack.
Pops the value 'val' from the stack, then pushes '-val'.
@@ +276,4 @@
> macro(JSOP_NEG, 34, "neg", "- ", 1, 1, 1, JOF_BYTE|JOF_ARITH) \
> + /*
> + * Pops the top of stack value, converts it into a number, pushes it onto
> + * the stack.
Pops the value 'val' from the stack, then pushes '+val'. ('+val' is the value converted to a number.)
@@ +297,5 @@
> macro(JSOP_DELPROP, 37, "delprop", NULL, 5, 1, 1, JOF_ATOM|JOF_PROP) \
> macro(JSOP_DELELEM, 38, "delelem", NULL, 1, 2, 1, JOF_BYTE |JOF_ELEM) \
> + /*
> + * Pops the top of stack value, pushes string representation of typeof it
> + * onto the stack.
Pops the value 'val' from the stack, then pushes 'typeof val'.
@@ +307,3 @@
> macro(JSOP_TYPEOF, 39, js_typeof_str,NULL, 1, 1, 1, JOF_BYTE|JOF_DETECTING) \
> + /*
> + * Pops the top of stack value, pushes 'undefined' onto the stack.
Pops the top value on the stack and pushes 'undefined'.
@@ +352,5 @@
> */ \
> macro(JSOP_SPREADEVAL,43, "spreadeval", NULL, 1, 3, 1, JOF_BYTE|JOF_INVOKE|JOF_TYPESET) \
> \
> + /*
> + * Duplicates the Nth value from the top.
...onto the stack.
@@ +506,5 @@
> \
> /* New, infallible/transitive identity ops. */ \
> + /*
> + * Pops the top two values on the stack, pushes strict equality ('===') or
> + * a negation of it ('!==') of them onto the stack.
Pops the top two values from the stack, then pushes the result of applying the operator to the two values.
We could almost unify the various binary opcodes into one documented range, if we're willing to be very slightly less precise, more reliant on (lval OP rval) being interpreted to use JS semantics. Not sure if worthwhile. Let's leave that to a possible second pass and just get them all documented to start.
@@ +588,5 @@
> /* Push object initializer literal. */ \
> macro(JSOP_OBJECT, 80, "object", NULL, 5, 0, 1, JOF_OBJECT) \
> \
> + /*
> + * Pops the top of stack value and discard it.
Pops the top value off the stack.
@@ +760,5 @@
> macro(JSOP_THROW, 112,js_throw_str, NULL, 1, 1, 0, JOF_BYTE) \
> \
> + /*
> + * Pops the top two values on the stack as 'rval' and 'id', pushes
> + * 'true' onto the stack if 'rval' has 'id' property, 'false' if not.
Pops the top two values 'id' and 'obj' from the stack, then pushes 'id in obj'. This will throw a 'TypeError' if 'obj' is not an object.
Then of course update all rval=>obj in this entire comment.
The id/obj ordering here is possibly a little surprising, if one isn't thinking in terms of the parse/execution ordering of the |in| operator. I wonder if it's worth saying something like "Note that |obj| is the top value." to make the ordering stick in the reader's mind better.
@@ +772,4 @@
> macro(JSOP_IN, 113,js_in_str, js_in_str, 1, 2, 1, JOF_BYTE|JOF_LEFTASSOC) \
> + /*
> + * Pops the top two values on the stack as 'rval' and 'obj', pushes
> + * 'true' onto the stack if 'obj' is an instance of 'rval', 'false' if not.
Pops the top two values 'obj' and 'ctor' from the stack, then pushes 'obj instanceof ctor'. This will throw a 'TypeError' if 'obj' is not an object.
@@ +777,5 @@
> + * Throws TypeError if 'obj' is not an object.
> + * Category: Operator
> + * Type: Special Operators
> + * Operands:
> + * Stack: obj, rval => (obj instanceof rval)
obj, ctor => (obj instanceof ctor)
@@ +934,5 @@
> \
> macro(JSOP_UNUSED132, 132, "unused132", NULL, 1, 0, 0, JOF_BYTE) \
> \
> + /*
> + * Picks 'n'-th element from the stack.
Picks the nth element from the stack and moves it to the top of the stack.
(nth is perfectly fine English. Also a valid word when playing Scrabble. :-) And we want to be clear that the value is removed from its previous position in the stack -- on first skim I was thinking of a DUPAT opcode rather than a rotate-n opcode.)
@@ +1194,5 @@
> macro(JSOP_UNUSED196, 196,"unused196", NULL, 1, 0, 0, JOF_BYTE) \
> \
> + /*
> + * Specialized JSOP_TYPEOF to avoid reporting undefined for
> + * 'typeof(0, undef)'.
Pops the top stack value as 'val' and pushes 'typeof val'. Note that this opcode isn't used when, in the original source code, 'val' is a name -- see 'JSOP_TYPEOF' for that. (This is because 'typeof undefinedName === "undefined"'.)
Attachment #8400608 -
Flags: review?(jwalden+bmo) → feedback+
Assignee | ||
Comment 33•11 years ago
|
||
Thank you for your feedback!
Attachment #8400608 -
Attachment is obsolete: true
Attachment #8403284 -
Flags: review?(jwalden+bmo)
Reporter | ||
Updated•11 years ago
|
Attachment #8400605 -
Flags: review?(jwalden+bmo) → review?(jorendorff)
Reporter | ||
Updated•11 years ago
|
Attachment #8400607 -
Flags: review?(jwalden+bmo) → review?(luke)
Reporter | ||
Updated•11 years ago
|
Attachment #8400609 -
Flags: review?(jwalden+bmo) → review?(evilpies)
Reporter | ||
Updated•11 years ago
|
Attachment #8402185 -
Flags: review?(jwalden+bmo) → review?(kvijayan)
Reporter | ||
Comment 34•11 years ago
|
||
Comment on attachment 8403284 [details] [diff] [review]
Part 5: Add documentation for operators and stack related opcodes
Review of attachment 8403284 [details] [diff] [review]:
-----------------------------------------------------------------
Will push later today. Thanks again!
Attachment #8403284 -
Flags: review?(jwalden+bmo) → review+
Comment 35•11 years ago
|
||
Comment on attachment 8400605 [details] [diff] [review]
Part 2: Add documentation for control statements related opcodes
Review of attachment 8400605 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
::: js/src/vm/Opcodes.h
@@ +53,5 @@
> + * Pops the top of stack value, turns it into a member of the scope chain.
> + *
> + * The fact that the new scope chain object gets pushed is a bit scary,
> + * because that's an engine-only internal data structure, and it ends up on
> + * the stack with all the user data.
The second paragraph of this comment is not really correct.
The new scope chain object (DynamicWithObject) isn't pushed to the operand stack. It's never exposed to scripts as a value. I don't think we need to specify that this is "scary". How about:
* Pops the top of stack value, converts it to an object, and adds a
* DynamicWithObject wrapping that object to the scope chain.
*
* There is a matching JSOP_LEAVEWITH instruction later. All name
* lookups between the two that may need to consult the With object
* are deoptimized.
@@ +192,4 @@
> macro(JSOP_AND, 69, "and", NULL, 5, 1, 1, JOF_JUMP|JOF_DETECTING|JOF_LEFTASSOC) \
> \
> + /*
> + * Pops the top of stack value as 'i', if 'i' is between 'low' and 'high',
More precisely: if 'low <= i <= high', ...
@@ +229,5 @@
> + * with an iterator for it.
> + * Category: Statements
> + * Type: For-In Statement
> + * Operands: uint8_t flags
> + * Stack: val => (iterator of val)
For the stack balance, please write:
val => iter
then use 'iter' again subsequently when the value on top of the stack is this iterator:
@@ +242,5 @@
> + * JSITER_FOREACH flag.
> + * Category: Statements
> + * Type: For-In Statement
> + * Operands:
> + * Stack: obj => obj, cond
iter => iter, cond
@@ +248,4 @@
> macro(JSOP_MOREITER, 76, "moreiter", NULL, 1, 1, 2, JOF_BYTE) \
> + /*
> + * Pushes the iterated value of top of stack value ('cx->iterValue') onto
> + * the stack.
"Pushes the value produced by the preceding JSOP_MOREITER operation (cx->iterValue) onto the stack."
@@ +254,5 @@
> + * JSITER_FOREACH flag.
> + * Category: Statements
> + * Type: For-In Statement
> + * Operands:
> + * Stack: obj => obj, res
iter => iter, val
@@ +260,4 @@
> macro(JSOP_ITERNEXT, 77, "iternext", "<next>", 1, 0, 1, JOF_BYTE) \
> + /*
> + * Pops the top of stack value, cleans up after the loop for it. It uses the
> + * slot above the iterator for temporary GC rooting.
"Exits a for-in loop by popping the iterator object from the stack and closing it."
The second sentence is wrong; that hasn't been true for some time. Please delete it.
@@ +263,5 @@
> + * slot above the iterator for temporary GC rooting.
> + * Category: Statements
> + * Type: For-In Statement
> + * Operands:
> + * Stack: obj =>
iter =>
@@ +328,5 @@
> macro(JSOP_UNUSED104, 104, "unused104", NULL, 1, 0, 0, JOF_BYTE) \
> macro(JSOP_UNUSED105, 105, "unused105", NULL, 1, 0, 0, JOF_BYTE) \
> \
> + /*
> + * 'offset' is the offset to the next statement and is used by IonMonkey.
* This opcode precedes every labeled statement. It's a no-op.
*
* 'offset' is the offset to the next instruction after this statement,
* the one 'break LABEL;' would jump to. IonMonkey uses this.
@@ +342,5 @@
> /* Like JSOP_FUNAPPLY but for f.call instead of f.apply. */ \
> macro(JSOP_FUNCALL, 108,"funcall", NULL, 3, -1, 1, JOF_UINT16|JOF_INVOKE|JOF_TYPESET) \
> \
> + /*
> + * This opcode is the target of the backwards jump for some loop.
Add: "Another no-op."
@@ +398,5 @@
> macro(JSOP_RETSUB, 117,"retsub", NULL, 1, 2, 0, JOF_BYTE) \
> \
> /* More exception handling ops. */ \
> + /*
> + * Pushes pending exception onto the stack, and clears pending exception.
Pushes the current pending exception onto the stack and clears the pending exception. This is only emitted at the beginning of code for a catch-block, so it is known that an exception is pending. It is used to implement catch-blocks and 'yield*'.
@@ +409,5 @@
> \
> /* Embedded lineno to speedup pc->line mapping. */ \
> macro(JSOP_LINENO, 119,"lineno", NULL, 3, 0, 0, JOF_UINT16) \
> \
> + /* ECMA-compliant switch statement ops. */ \
This line ("ECMA-compliant switch statement ops") is not necessary.
@@ +414,2 @@
> /*
> + * A no-operation bytecode.
* This no-op appears after the bytecode for EXPR in 'switch (EXPR) {...}'
* if the switch cannot be optimized using JSOP_TABLESWITCH.
* For a non-optimized switch statement like this:
*
* switch (EXPR) {
* case V0:
* C0;
* ...
* default:
* D;
* }
*
* the bytecode looks like this:
*
* (EXPR)
* condswitch
* (V0)
* case ->C0
* ...
* default ->D
* (C0)
* ...
* (D)
*
* Note that code for all case-labels is emitted first, then code for
* the body of each case clause.
@@ +431,4 @@
> macro(JSOP_CASE, 121,"case", NULL, 5, 2, 1, JOF_JUMP) \
> + /*
> + * Pops the top of stack value ('lval' of JSOP_CASE), jumps to a 32-bit
> + * offset from the current bytecode.
This appears after all cases in a JSOP_CONDSWITCH, whether there is a 'default:' label in the switch statement or not. Pop the switch operand from the stack and jump to a 32-bit offset from the current bytecode.
@@ +467,3 @@
> /*
> * Exception handling no-op, for more economical byte-coding than SRC_TRYFIN
> * srcnote-annotated JSOP_NOPs and to simply stack balance handling.
The existing comment here is stale.
* This no-op appears at the top of the bytecode for a TryStatement.
@@ +468,5 @@
> * Exception handling no-op, for more economical byte-coding than SRC_TRYFIN
> * srcnote-annotated JSOP_NOPs and to simply stack balance handling.
> + *
> + * Starting positions of 'catch'es and 'finally' are stored in Exception
> + * table ('script->trynotes()').
* Location information for catch/finally blocks is stored in a
* side table, 'script->trynotes()'.
@@ +524,5 @@
> macro(JSOP_UNUSED147, 147,"unused147", NULL, 1, 0, 0, JOF_BYTE) \
> macro(JSOP_UNUSED148, 148,"unused148", NULL, 1, 0, 0, JOF_BYTE) \
> \
> + /*
> + * Placeholders for a real jump opcode set during backpatch chain fixup.
* Placeholder opcode used during bytecode generation. This never
* appears in a finished script. FIXME: bug 473671.
@@ +528,5 @@
> + * Placeholders for a real jump opcode set during backpatch chain fixup.
> + *
> + * Emit a backpatch op with offset pointing to the previous jump of this
> + * type, so that we can walk back up the chain fixing up the op and jump
> + * offset.
Drop this paragraph.
Attachment #8400605 -
Flags: review?(jorendorff) → review+
Comment 36•11 years ago
|
||
Comment on attachment 8400609 [details] [diff] [review]
Part 6: Add documentation for object and array related opcodes
Review of attachment 8400609 [details] [diff] [review]:
-----------------------------------------------------------------
Nice work. I found some of the sentences somewhat strange, but I am not the best person to judge it and I don't want to slow anything down.
::: js/src/vm/Opcodes.h
@@ +294,5 @@
> * Stack: => succeeded
> */ \
> macro(JSOP_DELNAME, 36, "delname", NULL, 5, 0, 1, JOF_ATOM|JOF_NAME) \
> + /*
> + * Pops the top of stack value, deletes propetry from it, pushes 'true' onto
property
@@ +401,5 @@
> + * 'obj' as 'val', pushes 'obj' onto the stack.
> + * Category: Literals
> + * Type: Object
> + * Operands: uint32_t nameIndex
> + * Stack: obj, val => obj
I think the result here is => val
@@ +420,5 @@
> + * stack.
> + * Category: Literals
> + * Type: Object
> + * Operands:
> + * Stack: obj, propval, val => obj
=> val as well
@@ +634,5 @@
> */ \
> macro(JSOP_FUNAPPLY, 79, "funapply", NULL, 3, -1, 1, JOF_UINT16|JOF_INVOKE|JOF_TYPESET) \
> \
> + /*
> + * Pushes object initializer literal onto the stack.
"object initializer literal" Looking at the code I would rather talk about "deep-cloned object literal or singleton"
@@ +663,5 @@
> */ \
> macro(JSOP_NEW, 82, js_new_str, NULL, 3, -1, 1, JOF_UINT16|JOF_INVOKE|JOF_TYPESET) \
> + /*
> + * Pops the top three values on the stack as 'iterable', 'index' and 'obj',
> + * iterates over 'iterable' and store them into 'index + i' properties of
"and stores the iteration values as 'index + i' elements" or something along that line
@@ +729,5 @@
> + * This opcode takes the kind of initializer (JSProto_Array or
> + * JSProto_Object).
> + *
> + * This opcode has an extra byte so it can be exchanged with JSOP_NEWOBJECT
> + * during emit.
I didn't even know that.
@@ +732,5 @@
> + * This opcode has an extra byte so it can be exchanged with JSOP_NEWOBJECT
> + * during emit.
> + * Category: Literals
> + * Type: Object
> + * Operands: uint8_t kind, uint24_t extra
I would put extra in parentheses.
@@ +739,5 @@
> macro(JSOP_NEWINIT, 89, "newinit", NULL, 5, 0, 1, JOF_UINT8) \
> + /*
> + * Pushes newly created array onto the stack.
> + *
> + * This opcode takes the final length, which can be filled in at the start
This opcode takes the final length, which is preallocated.
@@ +772,2 @@
> macro(JSOP_ENDINIT, 92, "endinit", NULL, 1, 0, 0, JOF_BYTE) \
> + /*
I think you missed a short description here.
Initialize a named property in an object literal, like '{a: x}'.
@@ +772,4 @@
> macro(JSOP_ENDINIT, 92, "endinit", NULL, 1, 0, 0, JOF_BYTE) \
> + /*
> + * Pops the top two values on the stack as 'val' and 'obj', defines
> + * property of 'obj' as 'val', pushes 'obj' onto the stack.
There is seems to be something missing before 'property', telling us which it is. i.e. nameIndex.
@@ +1311,5 @@
> macro(JSOP_UNUSED159, 159, "unused159", NULL, 1, 0, 0, JOF_BYTE) \
> \
> + /*
> + * Pushes a regular expression literal onto the stack.
> + * It requires special "fork on exec" handling.
fork -> clone?
@@ +1314,5 @@
> + * Pushes a regular expression literal onto the stack.
> + * It requires special "fork on exec" handling.
> + *
> + * In the ES3 spec, regexp literals had their (mutable) 'RegExp' objects
> + * aliased together, but this is no longer the case in ES5.
Drop reference to ES3
Attachment #8400609 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 37•11 years ago
|
||
Thank you for reviewing!
Here is fixed patch.
Maybe converting "bug XXXXXX" into "{{Bug(XXXXXX)}}" in make_opcode_doc.py will be nice.
I'll do it after markup method is decided.
Attachment #8400605 -
Attachment is obsolete: true
Attachment #8404858 -
Flags: review?(jorendorff)
Assignee | ||
Comment 38•11 years ago
|
||
Thank you for reviewing!
Here is fixed patch.
I started learning English, to correct my strange English :)
Attachment #8400609 -
Attachment is obsolete: true
Attachment #8404867 -
Flags: review?(evilpies)
Comment 39•11 years ago
|
||
Comment on attachment 8402185 [details] [diff] [review]
Part 3: Add documentation for function call related opcodes
Review of attachment 8402185 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/Opcodes.h
@@ +222,5 @@
> + * return value onto the stack.
> + * Category: Statements
> + * Type: Function
> + * Operands:
> + * Stack: =>
Stack: callee, this, args => rval
Attachment #8402185 -
Flags: review?(kvijayan) → review+
Assignee | ||
Comment 40•11 years ago
|
||
Thank you for reviewing!
Here is fixed patch.
Attachment #8402185 -
Attachment is obsolete: true
Attachment #8404999 -
Flags: review?(kvijayan)
Comment 41•11 years ago
|
||
Comment on attachment 8400607 [details] [diff] [review]
Part 4: Add documentation for variables and constants related opcodes
Review of attachment 8400607 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/Opcodes.h
@@ +143,5 @@
> /* More long-standing bytecodes. */ \
> macro(JSOP_DUP, 12, "dup", NULL, 1, 1, 2, JOF_BYTE) \
> macro(JSOP_DUP2, 13, "dup2", NULL, 1, 2, 4, JOF_BYTE) \
> + /*
> + * Sets propetry of current scope as the top of stack value.
s/propetry/property/
You could elaborate a bit with: "Defines a readonly property on the frame's current variables-object (the scope object on the scope chain designated to receive new variables)."
@@ +173,5 @@
> macro(JSOP_NEG, 34, "neg", "- ", 1, 1, 1, JOF_BYTE|JOF_ARITH) \
> macro(JSOP_POS, 35, "pos", "+ ", 1, 1, 1, JOF_BYTE|JOF_ARITH) \
> + /*
> + * Lookups name in the scope chain and deletes it, pushes 'true' onto the
> + * stack if succeeded, 'false' if not.
s/Lookups name in/Looks up name on/
You might append on to "if succeeded" "if the property was present and deleted or if the property wasn't present in the first place".
@@ +254,1 @@
> macro(JSOP_CALLNAME, 57, "callname", NULL, 5, 0, 1, JOF_ATOM|JOF_NAME|JOF_TYPESET) \
Note: this is going away in bug 994937 (along with most of the other JSOP_CALL* ops).
@@ +262,5 @@
> * nuses: (argc+2)
> */ \
> macro(JSOP_CALL, 58, "call", NULL, 3, -1, 1, JOF_UINT16|JOF_INVOKE|JOF_TYPESET) \
> + /*
> + * Lookups name in the scope chain and pushes its value onto the stack.
s/Lookups name in/Looks up name on/
@@ +493,3 @@
> macro(JSOP_GETLOCAL, 86,"getlocal", NULL, 4, 0, 1, JOF_LOCAL|JOF_NAME) \
> + /*
> + * Pops the top of stack value, sets the value of local variable as it.
Technically, this op leaves the operand on the stack, it doesn't pop. So I'd say: "Stores the top stack value the given local."
@@ +495,5 @@
> + * Pops the top of stack value, sets the value of local variable as it.
> + * Category: Variables and Scopes
> + * Type: Local Variables
> + * Operands: uint32_t localno
> + * Stack: v =>
v => v
@@ +582,5 @@
> \
> /* ECMA-compliant assignment ops. */ \
> + /*
> + * Lookups name in the scope chain and pushes the scope which contains name
> + * onto the stack.
s/Lookups name in/Looks up name on/
@@ +593,4 @@
> macro(JSOP_BINDNAME, 110,"bindname", NULL, 5, 0, 1, JOF_ATOM|JOF_NAME|JOF_SET) \
> + /*
> + * Pops the top two values on the stack as 'val' and 'scope', sets
> + * property of 'scope' as 'val'.
Technically, this op leaves 'val' on the stack, so I'd phrase this as: "Pops a scope and and value from the stack, assigns value to the given name, and pushes the value back on the stack".
@@ +714,5 @@
> macro(JSOP_UNUSED125, 125, "unused125", NULL, 1, 0, 0, JOF_BYTE) \
> macro(JSOP_UNUSED126, 126, "unused126", NULL, 1, 0, 0, JOF_BYTE) \
> \
> + /*
> + * Prolog bytecode for defining function name.
Given that "prolog" doesn't mean much outside the frontend, I'd leave off this part of the comment (here and 3x below).
@@ +716,5 @@
> \
> + /*
> + * Prolog bytecode for defining function name.
> + *
> + * Defines function in the scope chain.
I'd phrase this: "Defines the given function on the current scope." I'd also add "This is used for global scripts and also in some cases for function scripts where use of dynamic scoping inhibits optimization." (here and 3x below)
@@ +933,5 @@
> */ \
> macro(JSOP_RETRVAL, 153,"retrval", NULL, 1, 0, 0, JOF_BYTE) \
> \
> + /*
> + * Lookups name in global scope and pushes its value onto the stack.
s/Lookups name in/Looks up name on/
@@ +946,4 @@
> macro(JSOP_GETGNAME, 154,"getgname", NULL, 5, 0, 1, JOF_ATOM|JOF_NAME|JOF_TYPESET|JOF_GNAME) \
> + /*
> + * Pops the top two values on the stack as 'val' and 'scope', sets property
> + * of 'scope' as 'val'.
I'd add "and pushes 'val' back on the stack.
Attachment #8400607 -
Flags: review?(luke) → review+
Comment 42•11 years ago
|
||
Comment on attachment 8404999 [details] [diff] [review]
Part 3: Add documentation for function call related opcodes
Review of attachment 8404999 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry, had some more changes I wanted to note. Should have done so in earlier review. My apologies.
::: js/src/vm/Opcodes.h
@@ +115,5 @@
> */ \
> macro(JSOP_LEAVEWITH, 4, "leavewith", NULL, 1, 0, 0, JOF_BYTE) \
> + /*
> + * Pops the top of stack value as 'rval', stops interpretation and
> + * returns 'rval'.
", stops interpretation of current script and returns 'rval'"
@@ +155,5 @@
> macro(JSOP_IFNE, 8, "ifne", NULL, 5, 1, 0, JOF_JUMP) \
> \
> + /*
> + * Pushes the 'arguments' object for the current, lightweight function
> + * activation.
Just "Pushes the 'arguments' object for the current function activation" is enough. JSOP_ARGUMENTS is used for both lightweight and heavyweight functions.
@@ +157,5 @@
> + /*
> + * Pushes the 'arguments' object for the current, lightweight function
> + * activation.
> + *
> + * If arguments object is not needed immediately, JS_OPTIMIZED_ARGUMENTS
"If JSScript is not marked needsArgsObj, then a JS_OPTIMIZED_ARGUMENTS magic value is pushed. Otherwise, a proper arguments object is constructed and pushed."
No need to reference lazy argsobj creation in the op description. That's more specifics of runtime turning.
@@ +499,5 @@
> macro(JSOP_UNUSED107, 107,"unused107", NULL, 1, 0, 0, JOF_BYTE) \
> \
> + /*
> + * Invokes 'callee' with 'this' and 'args', pushes return value onto the
> + * stack.
Add: If 'callee' is determined to be the canonical |Function.prototype.call| function, then this operation is optimized to directly call 'callee' with args[0] as |this|, and the remaining arguments as formal args to 'callee'.
@@ +956,5 @@
> macro(JSOP_UNUSED223, 223,"unused223", NULL, 1, 0, 0, JOF_BYTE) \
> \
> + /*
> + * Creates rest parameter object for current function call, and pushes it
> + * onto the stack.
The rest parameter is a proper array. So say "Creates rest parameter array" instead.
Attachment #8404999 -
Flags: review?(kvijayan) → review+
Assignee | ||
Comment 43•11 years ago
|
||
Thank you for reviewing!
I left the documentation for JSOP_CALL* for now (in Part 3 and 4).
I'll update them when the patch for bug 994937 is landed.
If they should be removed soon, please tell me.
Attachment #8400607 -
Attachment is obsolete: true
Attachment #8405100 -
Flags: review?(luke)
Assignee | ||
Comment 44•11 years ago
|
||
Thank you again!
Attachment #8404999 -
Attachment is obsolete: true
Attachment #8405103 -
Flags: review?(kvijayan)
Comment 45•11 years ago
|
||
Comment on attachment 8405100 [details] [diff] [review]
Part 4: Add documentation for variables and constants related opcodes
Review of attachment 8405100 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/Opcodes.h
@@ +534,3 @@
> macro(JSOP_GETLOCAL, 86,"getlocal", NULL, 4, 0, 1, JOF_LOCAL|JOF_NAME) \
> + /*
> + * Stores the top stack value the given local.
*to the given local.
@@ +633,5 @@
> \
> /* ECMA-compliant assignment ops. */ \
> + /*
> + * Looks up name on the scope chain and pushes the scope which contains
> + * name onto the stack.
*contains the name. Also I'd remove the \n before "If not found..."
@@ +807,3 @@
> macro(JSOP_DEFFUN, 127,"deffun", NULL, 5, 0, 0, JOF_OBJECT) \
> + /*
> + * Defines the given name on the current scope with 'READONLY' attribute.
I just noticed that JSOP_DEFVAR/CONST (but not JSOP_DEFFUN) define the new binding on the variables-object, not the current-scope. Can you copy over the variables-object blurb you added for JSOP_SETCONST?
Attachment #8405100 -
Flags: review?(luke) → review+
Assignee | ||
Comment 46•11 years ago
|
||
Thank you again!
Attachment #8405100 -
Attachment is obsolete: true
Attachment #8405437 -
Flags: review?(luke)
Comment 47•11 years ago
|
||
Comment on attachment 8405103 [details] [diff] [review]
Part 3: Add documentation for function call related opcodes
Review of attachment 8405103 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. Nice work!
Attachment #8405103 -
Flags: review?(kvijayan) → review+
Updated•11 years ago
|
Attachment #8405437 -
Flags: review?(luke) → review+
Comment 48•11 years ago
|
||
Comment on attachment 8404867 [details] [diff] [review]
Part 6: Add documentation for object and array related opcodes
Review of attachment 8404867 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you.
Attachment #8404867 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 49•11 years ago
|
||
Wow, the the patch in bug 994937 was landed so quickly.
I removed JSOP_CALLARG from Part3 and all JSOP_CALL* from Part4.
Attachment #8405103 -
Attachment is obsolete: true
Attachment #8405726 -
Flags: review?(kvijayan)
Assignee | ||
Comment 50•11 years ago
|
||
Removed all JSOP_CALL* from Part4.
Attachment #8405437 -
Attachment is obsolete: true
Attachment #8405727 -
Flags: review?(luke)
Assignee | ||
Comment 51•11 years ago
|
||
Removed duplicated parens around flags.
JOF_BYTE flag is not shown, and no parens are shown if there is no other flag:
For example:
> JSOP_NOP [-0, +0]
> JSOP_REST [-0, +1] (TYPESET)
If JOF_BYTE flag should be shown, please tell me.
Attachment #8405731 -
Flags: review?(jwalden+bmo)
Updated•11 years ago
|
Attachment #8405727 -
Flags: review?(luke) → review+
Updated•11 years ago
|
Attachment #8405726 -
Flags: review?(kvijayan) → review+
Reporter | ||
Comment 52•11 years ago
|
||
Landed the part 5 docs: https://hg.mozilla.org/integration/mozilla-inbound/rev/a79661f16f41
Comment 53•11 years ago
|
||
Reporter | ||
Comment 54•11 years ago
|
||
Comment on attachment 8405731 [details] [diff] [review]
Remove duplicated parens around flags
Review of attachment 8405731 [details] [diff] [review]:
-----------------------------------------------------------------
Ah, right. This isn't the obviously most intuitive way to make this work, but I'm not sure offhand what would be better. :-\ Will push when I get a few patches to go with it.
Attachment #8405731 -
Flags: review?(jwalden+bmo) → review+
Reporter | ||
Comment 55•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2cfd08ae677
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2df996c19b
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e8c9fc98fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a723b73c03
We're rapidly homing in on fully fixing this. So good!
Assignee: general → arai_a
Status: NEW → ASSIGNED
Comment 56•10 years ago
|
||
Updated•10 years ago
|
Attachment #8404858 -
Flags: review?(jorendorff) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 57•10 years ago
|
||
We're now requesting (fairly-minimal, but appropriate) Try pushes for bugs wishing to make use of sheriff checkin-neededs (regardless of the diff) - removing keyword since there isn't one pasted in the bug for the latest version of the patch. [Generic message & note I'm not CCed to the bug]
Keywords: checkin-needed
Comment 59•10 years ago
|
||
Part 2 needs rebasing. Also, can we *please* set checkin+ on the patches that have already landed here?
Keywords: checkin-needed
Assignee | ||
Comment 60•10 years ago
|
||
I'm sorry to be so much trouble.
Here is rebased patch.
Attachment #8404858 -
Attachment is obsolete: true
Attachment #8415387 -
Flags: review?(jorendorff)
Updated•10 years ago
|
Attachment #8401161 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8405726 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8405727 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8403284 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8404867 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8405731 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8400610 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8415387 -
Flags: review?(jorendorff) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 61•10 years ago
|
||
Comment on attachment 8415387 [details] [diff] [review]
Part 2: Add documentation for control statements related opcodes
https://hg.mozilla.org/integration/mozilla-inbound/rev/024225990643
Attachment #8415387 -
Flags: checkin+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 62•10 years ago
|
||
Assignee | ||
Comment 63•10 years ago
|
||
Updated documentation on MDN page:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Internals/Bytecode
Assignee | ||
Comment 64•10 years ago
|
||
All opcodes were documented, so let's uncomment sys.exit().
Attachment #8420199 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 65•10 years ago
|
||
Comment on attachment 8420199 [details] [diff] [review]
uncomment the sys.exit() in error()
Review of attachment 8420199 [details] [diff] [review]:
-----------------------------------------------------------------
\o/
Sorry for the delay here, bogged down in super-nitpicky, complex, critical security bugs. :-(
Will push shortly. And I ran the script and updated the wiki page just now, for whatever changed between bytecode 172/173 revs.
Attachment #8420199 -
Flags: review?(jwalden+bmo) → review+
Reporter | ||
Comment 66•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef421d919f1
Woo, done! Thanks so much for the work on this! Opcode documentation is something we've wanted for a long time but never had.
Whiteboard: [good first bug][leave open] → [good first bug]
Target Milestone: --- → mozilla32
Comment 67•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•