80x86 Co Pro: Issues with IDIV

[hoglet67]

From Dominic:

Just looking over warnings for various code checkers . Does anyone know an x86 checker like dormann ? I think cpu80186.c line 1124 has a bug. it looks as though it is written for a 16 bit divide where as it should be an 8 bit divide. line 1143 I think needs to be reinstated bit with (s2 < 0x80)? s2 :....

PiTubeDirect/src/cpu80186/cpu80186.c

Line 1143 in 6e10b31

//s2 = (s2 < 0x8000) ? s2 : ((~s2 + 1) & 0xffff); //always true

This would be a long-standing fake86 bug.

[hoglet67]

There is also a bug in this line:

PiTubeDirect/src/cpu80186/cpu80186.c

Line 1141 in 6e10b31

sign = (((s1 ^ s2) & 0x8000) != 0);

I wonder if the best solution is to just sign-extend the divisor to 16 bits.

i.e. instead of

  s2 = divisor

have

  s2 = divisor;
  if (s2 & 0x80) {
    s2 |= 0xff00;
  }

Then both s1 and s2 are 16 bits, and the code should be the same as the 16-bit version.

[dp111]

looks sensible : https://www.felixcloutier.com/x86/idiv

…

On Mon, 6 Sept 2021 at 15:32, David Banks ***@***.***> wrote: There is also a bug in this line: https://github.com/hoglet67/PiTubeDirect/blob/6e10b31991d06468f07a98120dcbdba19f73dc6b/src/cpu80186/cpu80186.c#L1141 I wonder if the best solution is to just sign-extend the divisor to 16 bits. i.e. instead of s2 = divisor have s2 = divisor; if (s2 & 0x80) { s2 |= 0xff00; } Then both s1 and s2 are 16 bits, and the code should be the same as the 16-bit version. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#127 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEVVFIQ5WYDXKUUHFQCXSE3UATGHHANCNFSM5DQRNO6Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[hoglet67]

There may be a second issue with idiv.

The documentation states:

Non-integral results are truncated (chopped) towards 0.

I believe the implication of this is that the sign of the remainder should depend only on the sign of the dividend:

 14 / 3 : quotient =  4 remainder =   2    (checking:  3* 4+2= 14)  
 14 /-3 : quotient = -4 remainder =   2    (checking: -3*-4+2= 14)
-14 / 3 : quotient = -4 remainder =  -2    (checking: -3* 4-2=-14)
-14 /-3 : quotient =  4 remainder =  -2    (checking:  3*-4-2=-14)

I don't believe the current code is correct in this respect...

  if (sign)
  {
    d1 = (~d1 + 1) & 0xff;
    d2 = (~d2 + 1) & 0xff;
  }

It looks like the current code always makes the sign of the quotient (d1) and remainder (d2) the same.

I really need to try write some test cases, and maybe get someone to run them on a real 80186 Co Pro.

[hoglet67]

Here's a small test case for IDIV8:

Here's the (correct) result from the Matchbox Co Pro:

Here's the (incorrect) result from PiTubeDirect Alpha4:

And here's the result after the fix:

[hoglet67]

Here's a small test case for IDIV16:

Here's the (correct) result from the Matchbox Co Pro:

Here's the (incorrect) result from PiTubeDirect Alpha4:

And here's the result after the fix:

[hoglet67]

Dominic, if you get the chance, could you run the updated code through cppcheck?