Closed Bug 1343417 Opened 3 years ago Closed 3 years ago

Refactor make_opcode_doc.py and verify bytecode documentation comment in make check

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: arai, Assigned: arai)

Details

Attachments

(1 file, 1 obsolete file)

Separated from bug 1322019.

there was a patch that refactors make_opcode_doc.py, and extracts more information from Opcodes.h.
we can use the script to verify the documentation comment in make check.
Moved get_opcodes function from js/src/vm/make_opcode_doc.py into js/src/vm/opcode.py, and added some more verification,
also changed it to raise exception instead of sys.exit.

then, added check-opcode target to Makefile, and it's done in make check.

check_js_opcode.py just calls get_opcodes function and logs the result.

now bytecode documentation is verified at every push :)
Attachment #8843555 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8843555 [details] [diff] [review]
Verify bytecode documentation in js/src/vm/Opcodes.h in make check.

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

::: js/src/vm/opcode.py
@@ +223,5 @@
> +            if m2:
> +                opcode.stack_uses = m2.group(1)
> +                opcode.stack_defs = m2.group(2)
> +
> +            merged = opcode

I do not understand this line.

From what I understand you want to iterate over all the lines of the comment, and fill the content of opcode.
Then you alias this opcode object with the merged name.
But later, you compare content of the opcode with the merged one.

So my question is how do you make sure you compare them properly one against the other, and not override the comment content with the macro content?

Wouldn't it be easier to record the comment_opcode and the macro_opcode?
 - if comment_opcode is not None at the entry of the comment, then throw because you detected 2 comments.
 - if macro_opcode is not None at the entry of the macro parsing, then throw because you saw 2 macros.
 - if macro_opcode is not None at the entry of the comment parsing, then throw because the comment is not before the macro.
 - if comment_opcode is None at the entry of the macro parsing, then throw because there is no comment before the macro.

If both the macro_opcode and comment_opcode are not None:
 - Compare the opcodes
 - Merge them and store them in the opcodes table.
 - reset macro_opcode and comment_opcode to None.

@@ +243,5 @@
> +                if flag != 'JOF_BYTE':
> +                    flags.append(flag.replace('JOF_', ''))
> +            opcode.flags = ', '.join(flags)
> +
> +            if merged == opcode:

How can this ever be false?
Sorry, I overlooked the bugmail.

I agree that the code is unnecessarily complicated.
I'll try simplifying it.

what I want to do is the following:
  1. gather information from comment
  2. associate the information in comment to the first opcode after the comment
  3. if there are more opcodes after the first opcode, without having
     comment between them, merge those into single group and associate
     the comment information to them,
     (so, |merged| is about merging 2 opcodes, not about opcode+comment)
     this happens in the following case:

>     /*
>      * Pops the top two values 'lval' and 'rval' from the stack, then pushes
>      * the result of applying the arithmetic operation to them.
>      *   Category: Operators
>      *   Type: Arithmetic Operators
>      *   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) \
>     macro(JSOP_MOD,       31, "mod",        "%",          1,  2,  1, JOF_BYTE|JOF_LEFTASSOC|JOF_ARITH) \



> From what I understand you want to iterate over all the lines of the
> comment, and fill the content of opcode.
> Then you alias this opcode object with the merged name.

yes


> But later, you compare content of the opcode with the merged one.

the comparison is between 2 opcodes that shares same comment, like JSOP_SUB and JSOP_MUL above.


> So my question is how do you make sure you compare them properly one against
> the other, and not override the comment content with the macro content?

I guess the equivalent thing of |merged = opcode| should be performed in more clear way.


> Wouldn't it be easier to record the comment_opcode and the macro_opcode?
>  - if comment_opcode is not None at the entry of the comment, then throw
> because you detected 2 comments.
>  - if macro_opcode is not None at the entry of the macro parsing, then throw
> because you saw 2 macros.
>  - if macro_opcode is not None at the entry of the comment parsing, then
> throw because the comment is not before the macro.
>  - if comment_opcode is None at the entry of the macro parsing, then throw
> because there is no comment before the macro.

there can be the situation that macro appears contiguously.
otherwise, storing those into 2 separate object sounds better :)


> If both the macro_opcode and comment_opcode are not None:
>  - Compare the opcodes
>  - Merge them and store them in the opcodes table.
>  - reset macro_opcode and comment_opcode to None.

and keep it as current group, and associate the contiguous opcodes into it.


> @@ +243,5 @@
> > +                if flag != 'JOF_BYTE':
> > +                    flags.append(flag.replace('JOF_', ''))
> > +            opcode.flags = ', '.join(flags)
> > +
> > +            if merged == opcode:
> 
> How can this ever be false?

when it saw the JSOP_SUB+JSOP_MUL case above.
Comment on attachment 8843555 [details] [diff] [review]
Verify bytecode documentation in js/src/vm/Opcodes.h in make check.

clearing r? now.
I'll ask again once I simplify the code.
Attachment #8843555 - Flags: review?(nicolas.b.pierron)
Separated CommentInfo and OpcodeInfo.
CommentInfo stores the information in comment, and OpcodeInfo stores everything, copying information from CommentInfo.
Attachment #8843555 - Attachment is obsolete: true
Attachment #8848849 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8848849 [details] [diff] [review]
Verify bytecode documentation in js/src/vm/Opcodes.h in make check.

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

::: js/src/vm/opcode.py
@@ +86,5 @@
> +        self.ndefs_override = ''
> +
> +# Holds the information stored in the macro with the following format:
> +#   macro({name}, {value}, {display_name}, {image}, {length}, {nuses}, {ndefs},
> +#         {flags})

Thanks, these comments and the fact that they match the property names definitely help understanding what is going on.

@@ +182,5 @@
> +                                 r"([^,]+),\s*"     # image
> +                                 r"([0-9\-]+),\s*"  # length
> +                                 r"([0-9\-]+),\s*"  # nuses
> +                                 r"([0-9\-]+),\s*"  # ndefs
> +                                 r"([^\)]+)"        # format

nit: Use capturing named group[1], such that  m.group(1)  does not forces one to go backward to read these comments:

  r"([0-9]+),\s*" # val
  m.group(3)

becomes

  r"(?P<value>[0-9]+),\s*"
  m.group('value')

[1] https://docs.python.org/2/howto/regex.html#non-capturing-and-named-groups

@@ +293,5 @@
> +                                    "{name}".format(name=opcode.name))
> +                add_to_index(index, opcode)
> +            else:
> +                if group_head.length != opcode.length:
> +                    raise Exception("length should be same for contiguous opcodes: "

nit: contiguous opcodes --> opcodes of the same group.
Attachment #8848849 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c829d4bc7c0275a5df379da7855b1a9199991226
Bug 1343417 - Verify bytecode documentation in js/src/vm/Opcodes.h in make check. r=nbp
https://hg.mozilla.org/mozilla-central/rev/c829d4bc7c02
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.