Jeff Walden [:Waldo] Assignee
	Description • 11 years ago

Bug 948583 part one crashed and burned, on Windows PGO only, with a compile error complaining about JSOP_MUTATEPROTO_LENGTH not being defined or somesuch.  This suggests some sort of build dependency bug in the jsautooplen.h stuff that's the cause of this.

But rather than figure out what the issue there actually is, let's just convert this to a higher-order macro and do away with all the host-binary generation gunk.  There's nothing here requiring code generation if we're willing to use C++ MOZ_CONSTEXPR_VAR variables (or heck, even plain old consts).

This patch is only *somewhat* messy, in places.  ;-)

Prompt review appreciated, as any change at all to opcodes will bitrot this.

	Jason Orendorff [:jorendorff]
	Comment 2 • 11 years ago

Comment on attachment 8364859 [details] [diff] [review]
Oops, forgot to qref

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

r=me, assuming the data in the table itself is unchanged (I didn't check).

It sure would be nice to remove all the opcode numbers from that table. They are nothing but a pain in the butt. The non-opcode-numbers can stay in FOR_EACH_TRAILING_UNUSED_OPCODE (unless you know a trick for that as well).

::: js/src/jsopcode.h
@@ +21,5 @@
>  /*
>   * JS operation bytecodes.
>   */
>  typedef enum JSOp {
> +#define ENUMERATE_OPCODE(op, val, ...) op = val,

This works on all the compilers we care about? Nice.

::: js/src/vm/Interpreter.cpp
@@ +1348,4 @@
>      ((v) == EnableInterruptsPseudoOpcode                                      \
>       ? LABEL(EnableInterruptsPseudoOpcode)                                    \
>       : LABEL(default)),
> +        FOR_EACH_TRAILING_UNUSEED_OPCODE(TRAILING_LABEL)

Typo here: "UNUSEED".

::: js/src/vm/Opcodes.h
@@ +525,5 @@
> +// the [0, 256) range.  Avert your eyes!  You don't want to know how the
> +// sausage gets made.
> +
> +#define VALUE_AND_VALUE_PLUS_ONE(op, val, ...) \
> +    val) && (val + 1 ==

This isn't so bad.

I didn't see anything in here that needs to be in js::detail.

@@ +541,5 @@
> +} // namespace detail
> +
> +// Define JSOP_*_LENGTH constants for all ops.
> +#define DEFINE_LENGTH_CONSTANT(op, val, name, image, len, ...) \
> +    MOZ_CONSTEXPR_VAR size_t op##_LENGTH = len;

Yay.

	Jeff Walden [:Waldo] Assignee
	Comment 4 • 11 years ago

Might be possible to remove the opcode numbers, yeah.  I tried not to scope-creep myself on it here, as I remembered some sort of complexity to it concerning numbering and/or ordering being important somehow.

Variadic macro stuff works with all our compilers, yes.  MSVC is a little buggy in how it expands __VA_ARGS__ -- see the sad comments in mfbt/Assertions.h about tiptoeing around MSVC bugs -- but that doesn't affect truly simple cases like this one.

(Incidentally, I like how jsopcode.tbl's comment at the top had a wrong example of how to define OPDEF -- too many arguments.  Fortunately everyone cargo-cults, so nobody was bitten, so the comment lingered, wrong.)

I actually got rid of the unused opcodes, originally, and just added UNUSED227-UNUSED255 as explicit opcodes.  The problem is there's some code that assumes jsbytecode(-1) isn't a valid opcode.  Figuring out a hackaround for that was more trouble than it was worth, so I just reverted to having two different macros for this.

Right now I am sadly on Windows, hence I didn't see the typo.  :-(

js::detail was for a slightly older iteration that had a variable for the boundary between biggest-opcode and unused-trailing.  Removed because this iteration no longer uses that, happily.

Landed with the wrong bug number, backed out, then relanded with the right one:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe06fb5e10a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb900e8085fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5aad0b45a84

Thanks for the super-fast review here, I was really not looking forward to having to rebase this at all.  :-)

	Phil Ringnalda (:philor)
	Comment 5 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/b5aad0b45a84