Fix overflow when reading negative s8

[cherue]

When doing bitwise operations in JS the values are always converted to a ~~signed~~ (edit: not always signed, see here) 32-bit integer and the result is also always a signed 32-bit integer.

The calculation that converts two u4s to one s8 assumes only positive inputs, this change makes sure of that.

I discovered this when writing a test for JS 53-bit integer overflows, but that test and the error on overflow aren't part of this.

[GreyCat]

@cherue, can you clarify which test shows the problem in JS right now?

[cherue]

Right now no test shows the problem.

I am writing a test called js_overflow that will test that the JS runtime throws an error when reading numbers smaller than Number.MIN_SAFE_INTEGER or bigger than Number.MAX_SAFE_INTEGER (this is not implemented yet).

The ksy for this test currently looks like this:

meta:
  id: js_overflow
seq:
  - id: signed_negative_be
    type: s8be
  - id: signed_negative_le
    type: s8le
  - id: signed_positive_be
    type: s8be
  - id: signed_positive_le
    type: s8le
  - id: unsigned_be
    type: u8be
  - id: unsigned_le
    type: u8le
instances:
  overflow_signed_negative_be:
    pos: 48
    type: s8be
  overflow_signed_negative_le:
    pos: 56
    type: s8le
  overflow_signed_positive_be:
    pos: 64
    type: s8be
  overflow_signed_positive_le:
    pos: 72
    type: s8le
  overflow_unsigned_be:
    pos: 80
    type: u8be
  overflow_unsigned_le:
    pos: 88
    type: u8le

While writing this test I noticed that signed_negative_be and signed_negative_le were wrong. See js_overflow.bin.zip for the binary file.

Also I misnamed my branch, it is the low bytes that overflow. The high bytes can't ever overflow because the sign bit is always set. I'll fix that and then write up a better explanation of the problem.

[cherue]

Let's look at Number.MIN_SAFE_INTEGER or -9007199254740991 or 0x ff e0 00 00 00 00 00 01 (big-endian) to demonstrate the problem.

First, the s8 is read as two u4s:

var high = this.readU4be(); // 0x ff e0 00 00
var low  = this.readU4be(); // 0x 00 00 00 01

Then high and low are XORed with 0x ff ff ff ff:

high = high ^ 0xffffffff; // 0x 00 1f ff ff
low  = low  ^ 0xffffffff; // 0x ff ff ff fe

And finally they are combined to create the s8:

return -(0x100000000 * high + low) - 1; 

The expected result is -9007199254740991 but currently the result is -9007194959773695 (WebIDE has an outdated runtime so it's still treated as a positive number).

This happens because the result of XOR is defined to be a signed 32-bit integer. Which means the final step, which should be:

return -(0x100000000 * 2097151 + 4294967294) - 1;
// -9007199254740991

currently is:

return -(0x100000000 * 2097151 + -2) - 1;
// -9007194959773695

[cherue]

@GreyCat do you want a test for this first?

How about an integer_random test? It would be a binary file filled with random data and then instances with all integer types and pos: 0 and repeat: eos. That should catch most edge cases. The KST and the tests would be huge though.