471513 - invalid C++ in jsopcode.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paolo Bonzini]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_6; en-us) AppleWebKit/525.27.1 (KHTML, like Gecko) Version/3.2.1 Safari/525.27.1
Build Identifier: present in hg tree

C++ enums have slightly different rules than in C, and assigning of #define-d identifiers to an enum as done in jsopcode.cpp is suspect:

930 /*
931  * These pseudo-ops help js_DecompileValueGenerator decompile JSOP_SETNAME,
932  * JSOP_SETPROP, and JSOP_SETELEM, respectively.  They are never stored in
933  * bytecode, so they don't preempt valid opcodes.
934  */
935 #define JSOP_GETPROP2   256
936 #define JSOP_GETELEM2   257

where the enum only needs 8 bits.  Indeed this caused a crash with GCC trunk (that has been fixed since).  See comment #8 in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=16693 (not the rest of the bug, which was another case of a GCC bug exposed by this C++ feature).

Reproducible: Always

Comment 1

[Brian Crowder]

What is the reason for these opcodes not residing in the opcode table with the rest?  Is that the right patch for this?

Comment 2

[Brendan Eich [:brendan]]

Please read the comments in the code (including the one cited in this bug)!

One fix is to use int for op's type where needed.

Another fix is to get rid of these encodings by doing something else to achieve the same end: decompile a set opcode as if it were its get counterpart.

/be

Comment 3

[Brian Crowder]

Sorry, I looked at this comment, and at the other comments regarding these opcodes in the code near where they're used, and none of it makes clear to me why we aren't better off simply putting these "dummy" opcodes into the opcode table.

Also, I don't understand your "decompile a set opcode as if it were its get counterpart" remark, can you elaborate?

Comment 4

[Brendan Eich [:brendan]]

We don't add fake opcode table entries just for the decompiler -- those obligate the interpreter and other consumers to (at least) ignore these fake ops. This is a local issue to jsopcode.cpp.

The arity (stack balance) of set ops differs from get. Study ndefs and nuses for JSOP_SETNAME or JSOP_SETPROP vs. JSOP_GETPROP2.

Perhaps a simple bool flag would do the trick better. These fake ops go back 12+ years, so don't take them as the last word :-P.

/be

Comment 5

[Brian Crowder]

That clarifies perfectly, thanks a lot.

Comment 6

[Brendan Eich [:brendan]]

I should have pointed out JSOP_BACKPATCH{,_POP}, which are fake ops that *do* tie up jsopcode.tbl and other space. These are used only by the emitter. They are not good as precedent, so we shouldn't add more -- but you could do worse than to

#define JSOP_GETPROP2 JSOP_BACKPATCH
#define JSOP_GETELEM2 JSOP_BACKPATCH_POP

and declare victory in this bug.

Separate bug would move these off the end of JSOP_LIMIT, yet still (asserted to be) <= 255, so the emitter could store them temporarily in bytecode. Or, find another way to do things in the emitter that isn't ugly or expensive.

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

Should we do the same for JSOP_{GROUP,CONDSWITCH,TRY,FINALLY,STARTXML,STARTXMLEXPR} as well?  IIRC we have those because the cost of the opcodes was less than the cost of the alternatives, such as extending source-note range, but I may not recall well!

Comment 8

[Brendan Eich [:brendan]]

Some of those can go too. Indeed if you can find a free source note with the right arity, even if it is used on a different op or set of ops, you can alias its srcnode opcode with a new name and use it on the new op without collision. But you need matching arity (and offsetBias, grr -- that too could be eliminated).

IIRC, JSOP_STARTXML* were added before mrbkap and I fully appreciated the room to add srcnote opcodes that have a certain arity but apply to disjoint sets of JSOP_* codes.

We probably shouldn't over-invest in any such cleanup, given higher return opportunities elsewhere. Fixing this bug quickly via the BACKPATCH freebies seems best to me.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #4)
> The arity (stack balance) of set ops differs from get. Study ndefs and nuses
> for JSOP_SETNAME or JSOP_SETPROP vs. JSOP_GETPROP2.

Er, vs. JSOP_GETPROP, of course. The only point of JSOP_GETPROP2 is to have a distinct code value in the "op" local to switch on, to find the case that does the extra pop (to throw away the RHS of the "set" op, turning it into its "get" counterpart as far as decompilation goes).

/be

Comment 10

[Brian Crowder]

(In reply to comment #6)
> #define JSOP_GETPROP2 JSOP_BACKPATCH
> #define JSOP_GETELEM2 JSOP_BACKPATCH_POP
> 
> and declare victory in this bug.

This by itself won't suffice, since other code relies on these being >= JSOP_LIMIT (another good argument against putting them in the opcode table, perhaps), but that code could be fixed to just check against the values.

I think perhaps making them be JSOP_LIMIT + 1 and JSOP_LIMIT + 2 respectively and static-asserting <= 255 is the easiest.  I'll try a patch shortly.

Comment 11

[Brendan Eich [:brendan]]

Could use JSOP_LIMIT and JSOP_LIMIT+1 -- "limit" means fencepost, or one past last valid value (which is "max") in SpiderMonkey naming conventions.

Either is fine, decompiler doesn't have to be super-fast and testing two opcode values is about as fast as testing >= the limit (the opcode values are adjacent so the (uintN(op - JSOP_BACKPATCH) <= 1U) trick works).

/be

Comment 12

[Paolo Bonzini]

comment #10/#11 seems the best solution to me, as an external observer :-)

Comment 13

[Brian Crowder]

Attachment: v1, use JSOP_LIMIT and JSOP_LIMIT + 1

This is the patch that was just discussed.  It passes js/tests.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 354877 [details] [diff] [review]
v1, use JSOP_LIMIT and JSOP_LIMIT + 1

Great minimal patch, presuming the C++ standard does not allow a compiler to somehow restrict enum-typed variables to hold exactly only the enumerated values, not any value that fits in the nearest representation type.

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 354877 [details] [diff] [review]
v1, use JSOP_LIMIT and JSOP_LIMIT + 1

Has this landed in tm? It should go into m-c too, ASAP. Then 1.9.1.

/be

Comment 16

[Brian Crowder]

http://hg.mozilla.org/mozilla-central/rev/f521464b817a

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354877 [details] [diff] [review]
v1, use JSOP_LIMIT and JSOP_LIMIT + 1

a191=beltzner

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/eee5e9b80421