542932 - nanojit: make opcode range checks safer

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [inactive]]

Currently whether an opcode is CSEable is determine by an opcode range check -- opcodes in certain ranges are CSEable.  This is awful.  

I propose to add a field to LIRopcode.tbl that indicates if an instruction is CSEable.  As well as being safer, this will allow opcodes to be numbered much more sensibly, eg. you can put LIR_ldc next to LIR_ld!

Comment 1

[Edwin Smith]

+1

other predicates that check opcode ranges could be candidates too (conditions?).

Comment 2

[Nicholas Nethercote [inactive]]

For conditions we currently have this:

       bool isCmp() const {
            LOpcode op = opcode();
            return (op >= LIR_eq  && op <= LIR_uge) ||
                   (op >= LIR_qeq && op <= LIR_quge) ||
                   (op >= LIR_feq && op <= LIR_fge);
        }  

which I think isn't too bad -- it's very sensible to keep each group of comparisons together, although we could add some static assertions to ensure this.  But it would be good to factor the above code out into three methods: isCmpI(), isCmpQ(), isCmpF() because lots of places have things like this:

       if (condop >= LIR_eq && condop <= LIR_ge) {

maybe isCmpU() too.

There's also this thing where op^1 gives the inverted comparison (eg. lt -> gt) which would be better hidden within a function cmpInvert().

And we have this which could have a function of its own:

    NanoAssert(op >= LIR_fadd && op <= LIR_fdiv);

And jstracer.cpp has this at one point:

    if ((op >= LIR_sub && op <= LIR_ush) ||   // sub, mul, (callh), or, xor, (no
t,) lsh, rsh, ush
        (op >= LIR_fsub && op <= LIR_fmod)) { // fsub, fmul, fdiv, fmod

which is worrying because the comments don't match reality and I'm guessing it works out but more by luck than design.

TR doesn't seem to have any range checks that need fixing.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: NJ patch

In this patch I:

- Added static assertions for all cases where opcodes have to have
  particular values relative to each other.  Some existing weaker static 
  assertions were replaced in the process.

- Added an 'isCse' field to all opcodes in LIRopcode.tbl and changed
  isCseOpcode() to use it.

- Inlined isCse() in the one place it was used and removed its definition.

- Added various is*CmpOpcode() functions to encapsulate the range checks.

- Added various invert*Opcode() functions to encapsulate the use of (op ^ 1).

- Used these new functions everywhere suitable, replacing all opcode range
  checks that occurred in the wild.

- Introduced the OP_UN() macro for unused opcodes, which is more compact
  than the code it replaced.

- Removed a redundant assertion in the the ARM and MIPS backends (in both 
  bases there is an assertion later in the same function achieving the same
  effect).

I checked that the generated code for TM on SunSpider is unchanged.

In theory, you can now rearrange the opcode order arbitrarily and it'll (a)
fail to compile due to static assertion failures, or (b) compile and run
correctly.  I haven't tried this yet but I will before landing the patch.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: TM patch

This patch is straightforward except for the bit in TraceRecorder::binary().
The opcode range checking there was kind of bogus, given the inputs to
binary() it was equivalent to "if (op != LIR_fadd)".  But because of the
structure of record_JSOP_ADD() those JSVAL_IS_STRING() cases could never
succeed if op==LIR_fadd.  So I removed the if and replaced it with
op!=LIR_fadd assertions in the IS_STRING cases.

Comment 5

[Andreas Gal :gal]

Comment on attachment 427721 [details] [diff] [review]
TM patch

Here is the code in question from jstracer.cpp. As njn pints out we never hit those paths for fadd.

    if (JSVAL_IS_STRING(l) || JSVAL_IS_STRING(r)) {
	LIns* args[] = { stringify(r), stringify(l), cx_ins };
        LIns* concat = lir->insCall(&js_ConcatStrings_ci, args);
        guard(false, lir->ins_peq0(concat), OOM_EXIT);
	set(&l, concat);
        return ARECORD_CONTINUE;
    }

    return InjectStatus(binary(LIR_fadd));

Comment 6

[Steven Johnson]

Comment on attachment 427720 [details] [diff] [review]
NJ patch

"isCses" is declared as "int" -- we should really use C99 types for new code. (For that matter, since the only legal values are -1/0/1, seems like int8_t would be more than adequate.)

Comment 7

[Nicholas Nethercote [inactive]]

I changed isCses to use int8_t.

http://hg.mozilla.org/projects/nanojit-central/rev/a5291bc7b0ce

http://hg.mozilla.org/tracemonkey/rev/c15ecee288b6
http://hg.mozilla.org/tracemonkey/rev/9a50f32c2963

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> 
> In theory, you can now rearrange the opcode order arbitrarily and it'll (a)
> fail to compile due to static assertion failures, or (b) compile and run
> correctly.  I haven't tried this yet but I will before landing the patch.

BTW, I tried this by sorting the opcodes into alphabetical order, then rearranging them to satisfy the static assertions.  After doing that TM trace-tests passed, so that seems like a pretty thorough test.

Comment 9

[Robert Sayre]

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