Unsigned integer addition does not overflow about assemblyscript

[dcodeIO]

I've added a test case in the commit above that appears to succeed. Might be that the example is too basic to trigger the issue?

For reference: There is some deferred masking/sign-extending going on in the compiler, hence it's not unlikely that you found an issue there.

[torch2424]

Hmmm possibly, here is the exact code I am using:

  // INC (HL)
    // 1  12
    // Z 0 H -
    let registerHL: u16 = concatenateBytes(Cpu.registerH, Cpu.registerL);
    let valueAtHL: u8 = <u8>eightBitLoadFromGBMemory(registerHL);
    // Creating a varible for this to fix assemblyscript overflow bug
    // Requires explicit casting
    // https://github.com/AssemblyScript/assemblyscript/issues/26
    let incrementer: u8 = 1;
    checkAndSetEightBitHalfCarryFlag(<u8>valueAtHL, <i16>incrementer);
    valueAtHL = <u8>valueAtHL + <u8>incrementer;

    if (valueAtHL === 0) {
      setZeroFlag(1);
    } else {
      setZeroFlag(0);
    }
    setSubtractFlag(0);
    eightBitStoreIntoGBMemory(registerHL, <u8>valueAtHL);
    numberOfCycles = 12;

However, it did not work as:

// INC (HL)
    // 1  12
    // Z 0 H -
    let registerHL: u16 = concatenateBytes(Cpu.registerH, Cpu.registerL);
    let valueAtHL: u8 = eightBitLoadFromGBMemory(registerHL);
    checkAndSetEightBitHalfCarryFlag(<u8>valueAtHL, 1);
    valueAtHL += 1;
    if (valueAtHL === 0) {
      setZeroFlag(1);
    } else {
      setZeroFlag(0);
    }
    setSubtractFlag(0);
    eightBitStoreIntoGBMemory(registerHL, <u8>valueAtHL);
    numberOfCycles = 12;

The eightBitLoadFromMemory() simply returns a u8, using the store() built in :)

Let me know if this helps, otherwise I can try some more examples

[dcodeIO]

Hmm, haven't yet managed to reproduce. This is the test case, maybe you have an idea?

[torch2424]

Hmmm, later tonight I'll try building an official test case, and then pass it over :) That, or maybe the issue runs deeper than just unsigned addition...Maybe it is just on my end?

And if it does mean anything, there have been a handful of times where I declare a variable as u8, but the compiler will give me errors claiming that it is a u16

[dcodeIO]

And if it does mean anything, there have been a handful of times where I declare a variable as u8, but the compiler will give me errors claiming that it is a u16

Hmm, one thing that comes to my mind in this regard is that binary expressions always resort to the left-hand-side type. This might result in diagnostics like this when comparing an u8 to an u16, while it's silent when comparing an u16 to an u8.

[torch2424]

Oh that would make sense!

I still owe you a test haha! I'll see if I can whip one up today, I have a break between classes. I got super invested in the emulator again so my bad I haven't made one yet. Which by the way, can play a good amount of games now (with a bunch of graphical errors haha!)

[dcodeIO]

Just tried out your emulator (running Tetris ^^) and I think I found the issue leading to invalid variable types that you reported. Essentially, when a scoped let declaration reused a local, the local wasn't updated with the new type, leading to random conversion warnings and possibly other strange things like vars not properly overflowing. That should be fixed now :)

Regression test showing the issue: https://github.com/AssemblyScript/assemblyscript/blob/master/tests/compiler/scoped.ts#L17

Btw. with master, opcodes.ts:695, possible fix: registerHL -= 1; (due to recent changes to literal compilation)

[torch2424]

Oh wow! Thanks for checking it out! :)

So just re-install the latest assemblyscript master?

And this doesn't solve the original unsigned addition correct?

[dcodeIO]

So just re-install the latest assemblyscript master?

Yeah, you can also point npm at it, like so, and then update occasionally: npm install --save-dev AssemblyScript/assemblyscript

And this doesn't solve the original unsigned addition correct?

It might solve that as well (didn't double-check), as I see a lot of lets there, but not sure.

[torch2424]

Oh nice thank you! I'm going to update my wasm-playground repo, that way I have a clean slate, and do this tommorow! Totally remembered today is valentines day haha!

[torch2424]

So, got around to it tonight actually! So in my wasm-playground repo, I was able to reproduce this bug!

Here is the commit from a branch I made to test bugs, with just this bug (ignore the static class thing): torch2424/wasm-playground@6e89629

And yes, I installed the latest assemblyscript from github 😄

Essentially it is the following:

In your Js file with the Wasm Instance:

console.log(`unsignedIntegerOverflowBug(), 0x${instance.exports.unsignedIntegerOverflowBug().toString(16)}`);

In your wasm file

export function unsignedIntegerOverflowBug(): u8 {
  let badByte: u8 = 0xFF;
  badByte++;
  return badByte;
}

Screenshot

[dcodeIO]

Thanks, confirming. Going to take a look :)

[dcodeIO]

The issue was that there are two cases for compiling unary increment and decrement. When context is void anyway, it skips generating a temp. local, but missed wrapping small ints, while it properly did that in the temp. local case.

Should be fixed now :)

[torch2424]

Oh rad! I'll test this, and the other cases I mentioned tonight 😄 .

Also, This is out of the scope of this issue but, on my Wasmboy project, I noticed some weird import/export error I was getting, I don't know if it was a circular dependency or what. I'm having trouble reproducing it. Do you mind if I make another issue with that, with a branch of my repo, even though I'm not entirely sure if it's a bug on my end or yours?

And thanks again for looking through the project and stuff! :)

[dcodeIO]

Sure, the more issues, the more becomes fixed :)

Theoretically, the compiler shouldn't be affected by circular dependencies as parsing and compilation/final checking are sequential steps, but, as always, it's totally possible that I messed something up.

[torch2424]

Awesome! I'll go ahead and do that tonight once I confirm more cases on this 😄

[torch2424]

Confirmed all cases, closing this! Thanks for fixing this! 😄