80286: BBC Basic trig is wrong

[BigEd]

I'm guessing this is a 286 core emulation bug, although it could possibly be a bug in BBC Basic.

PRINT SIN(2)
1.00140532
PRINT COS(0.7)
1.00984219

(We know these values are wrong because they are greater than one.)

Found by running

CD BBCBASIC
BBCBASIC
CHAIN"SINE"

[jgharston]

From testing it seems to be an emulation bug. I've just tested 80x86 BBC BASIC here on PCEm on RISC OS, and from the DOS prompt on Windows: C:\>bbcbasic PC BBC BASIC Version 4.82 by Richard Russell, Jeff...etc. (C) Copyright Richard Russell 2000 >PRINT SIN(2) 0.909297427 >PRINT COS(0.7) 0.764842187 >

(I can't seem to get code blocks working without line breaks)

[BigEd]

Just in case it matters, the version I'm running on the 286 is 4.61 with a date of 1987.

[BigEd]

I've compared with the Matchbox - that works fine, so this must be an emulation bug on the Pi.

[BigEd]

The problem turns up just as we pass PI/4: P.SIN(PI/4) P.SIN(PI/4+1/2^34) It wouldn't be too surprising if the code for SIN does a range reduction, so tests if the argument is over PI/4 and then does something. (Edit: perhaps computes COS instead, so COS is the broken code.)

[BigEd]

Thanks - copied that link and erased the comment!

[hoglet67]

FYI, I just confirmed this bug is not present in B-Em 2.2 / BBC Basic 4.6.1

[hoglet67]

As I can easily re-build B-Em, I think I will try to get an instruction trace from there.

According to Ed, 1000:22d8 is the start of =SIN

; SIN token b5 handler
1000:22d8 e810fa  call 1cebh
1000:22db e8cb09  call loc_00002ca9

[dp111]

Might be worth trying a few more tests to see if it is a SIN error or a printing error.

e.g. PRINT SIN(2)*SIN(2) PRINT 1.00140532

On 22 February 2017 at 15:21, David Banks notifications@github.com wrote:

As I can easily re-build B-Em, I think I will try to get an instruction trace from there.

According to Ed, 1000:22d8 is the start of =SIN

; SIN token b5 handler 1000:22d8 e810fa call 1cebh 1000:22db e8cb09 call loc_00002ca9

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/hoglet67/PiTubeDirect/issues/39#issuecomment-281699989, or mute the thread https://github.com/notifications/unsubscribe-auth/AStSosdmpR7j1vkkTYOHPslOrwL4j9w3ks5rfFKSgaJpZM4MGdB6 .

[BigEd]

Yes, checked, the result is indeed the wrong number.

[dp111]

pushf is another instruction I wasn't sure if it was correct

On 22 February 2017 at 15:52, BigEd notifications@github.com wrote:

Yes, checked, the result is indeed the wrong number.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hoglet67/PiTubeDirect/issues/39#issuecomment-281709695, or mute the thread https://github.com/notifications/unsubscribe-auth/AStSorXAUjIIMhMJDnRaUnI6g4vdZE6Jks5rfFmxgaJpZM4MGdB6 .

[dp111]

as it appears to set the overflow flag

On 22 February 2017 at 15:58, Dominic Plunkett dominic.plunkett@gmail.com wrote:

pushf is another instruction I wasn't sure if it was correct

On 22 February 2017 at 15:52, BigEd notifications@github.com wrote:

Yes, checked, the result is indeed the wrong number.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hoglet67/PiTubeDirect/issues/39#issuecomment-281709695, or mute the thread https://github.com/notifications/unsubscribe-auth/AStSorXAUjIIMhMJDnRaUnI6g4vdZE6Jks5rfFmxgaJpZM4MGdB6 .

[BigEd]

I've opened ticket #42 for suspicious findings in the code

[hoglet67]

Here's a pair of traces (B-Em and PiTubeDirect) of 10000 instruction addresses, triggered by PC=0x22D8 which is the start of SIN, executing: PRINT SIN(PI/4+0.001)

bem_addrs.txt ptd_addrs.txt

The one line difference early on is a false positive, down to differences in how prefixes are implemented.

The two big block differences appear to be interrupts.

First significant divergence is at line 4500, which is a branch differently taken at 1689:2770

[hoglet67]

I wonder if I should re-do the traces withA=SIN(PI/4+0.001) to make sure we are not just seeing different numbers being printed?

[dp111]

coudl you have a quick test with the following for pushf :

case 0x9C: / 9C PUSHF / push(makeflagsword() | 0x0000); break;

On 22 February 2017 at 16:47, David Banks notifications@github.com wrote:

I wonder if I should re-do the traces withA=SIN(PI/4+0.001) to make sure we are not just seeing different numbers being printed?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hoglet67/PiTubeDirect/issues/39#issuecomment-281727529, or mute the thread https://github.com/notifications/unsubscribe-auth/AStSotdYAK_-G5fz00oSWgPUD8UZz-Zzks5rfGaFgaJpZM4MGdB6 .

[hoglet67]

B-Em:

AX=5DB9 BX=8000 CX=8080 DX=2566 DI=0000 SI=020E BP=0000 SP=FFDC
PC=276D CS=0071 DS=0563 ES=0563 SS=0563 FLAGS=0200
0071:276B 00000000 00000000 00000000

120689991 276D 276D

0071:276D 1B D3 5B ; SBB DX, BX

AX=5DB9 BX=8000 CX=8080 DX=A566 DI=0000 SI=020E BP=0000 SP=FFDC
PC=276F CS=0071 DS=0563 ES=0563 SS=0563 FLAGS=0A85
0071:276D 00000000 00000000 0000000

[BigEd]

So that's 2566 - 8000 - 0 which is A566 and a borrow, so carry is set, that's the bottom bit of the Flags. It looks right, as it should!

[hoglet67]

Here's some register dumps around the SBB instruction at 276D: bem_276d.txt ptd_276d.txt

Divergence of the code traces happens after the 4th execution of 276D.

It seems the carry flag on exit is different.

[BigEd]

But the flags are also very different on entry?! What's going on...

[BigEd]

OK, this makes sense. The code we have adds the carry to the operand. In this case the operand is FFFF, which is -1 in a 16 bit context, but the addition is done in a 32 bit context. Bit 16 gets set and is going to trigger the carry.

Might be a fix to do that initial addition in a cast-to-16-bit-integers way. https://github.com/hoglet67/PiTubeDirect/blob/boa-dev/src/cpu80186/cpu80186.c#L203 The same trick is done for 8-bit SBB, presumably with the same result.

[hoglet67]

Ed, have you tested this fix? Does it have any effect?

It doesn't even compile for me!

Off for an hour, back at 9pm....

[BigEd]

I haven't! I haven't even built it...

[hoglet67]

Hmm, it looks wrong to me. Off for an hour, back at 9pm....

[BigEd]

You're right, it's bogus, doesn't even compile. The operands are already 16 bit. So how has this gone wrong? Carry can only be set if dst gets some high bits set.

[hoglet67]

The problem is with: v2 += v3;

When v2=0xFFFF and v3=1 then v2=0 whereas v2 should really be 0x10000.

v2 needs more precision, or it needs re-coding.

[BigEd]

It's odd because it looks like he's made a point of adding the carry before doing the subtraction.

But you're right, a quick test shows that removing v2 += v3; and doing dst = (uint32_t) v1 - (uint32_t) v2 - v3; instead fixes the SIN problem.

(Whatever we do for 16 bit we should surely do similarly for 8 bit.)

[BigEd]

I've updated my pull request accordingly.

[hoglet67]

I just made the v2 (in the function prototype) a uint32_t which I guess amounts to the same thing.

A graph of SIN and COS looks good now with no discontinuities.

It also fixes #40 which I wasn't expecting.

[BigEd]

Somewhere today I've seen code of the form x = v1 - (v2 + v3) which is I think the simplest way to express this, with whatever casting might be absolutely necessary. Misleading casting is misleading.

Although, who knows, those parenthesis might affect the way the calculation is done, in a bad way.

Can I suggest we don't change the function prototype? That sounds obscure to me. The code will be harder to understand.

[hoglet67]

I agree your fix is cleaner.

I'm just holding off a tad longer though; I'm suspicious of the calculation of AF

[BigEd]

For ref, B-em does this:

static void x86setsbc16(uint16_t a, uint16_t b)
{
        uint32_t c=(uint32_t)a-(((uint32_t)b)+tempc);
        flags&=~0x8D5;
        flags|=(znptable16[c&0xFFFF]&~4);
        flags|=(znptable8[c&0xFF]&4);
        if (c&0x10000) flags|=C_FLAG;
        if ((a^b)&(a^c)&0x8000) flags|=V_FLAG;
        if (((a&0xF)-(b&0xF))&0x10)      flags|=A_FLAG;
}

[hoglet67]

There is evidence in the earier register trace of AF (flags bit 4) being calculated differently on SBB:

BeebEm, AF ends up as 1

AX=AE94 BX=C90F CX=7F7F DX=0041 DI=DAA2 SI=020E BP=DE80 SP=FFE4
PC=276D CS=0071 DS=0563 ES=0563 SS=0563 FLAGS=0281
1689:276D 1B D3 5B
AX=AE94 BX=C90F CX=7F7F DX=3731 DI=DAA2 SI=020E BP=DE80 SP=FFE4
PC=276F CS=0071 DS=0563 ES=0563 SS=0563 FLAGS=0211

PiTubeDirect, AF ends up as 0:

AX=AE94 BX=C90F CX=7F7F DX=0041 DI=DAA2 SI=020E BP=DE80 SP=FFE4
PC=276D CS=1689 DS=C90F ES=AE94 SS=0041 FLAGS=0283
1689:276D - 1B D3                         : SBB DX, BX
AX=AE94 BX=C90F CX=7F7F DX=3731 DI=DAA2 SI=020E BP=DE80 SP=FFE4
PC=276F CS=1689 DS=C90F ES=AE94 SS=3731 FLAGS=0203

Honestly, I'm not sure which is correct.

[BigEd]

Am I right in thinking the half-carry will only be used in decimal adjusted arithmetic? (I see there's also an ascii adjust AAA which uses it.)

It's not good that we don't have any test.

Could you instrument B-em and run all the BBC demo and benchmark programs and see if an adjust instruction ever occurs?

[hoglet67]

Adjust flag. Set if an arithmetic operation generates a carry or a borrow out of bit 3 of the result; cleared otherwise. This flag is used in binarycoded decimal (BCD) arithmetic.

Based on this, it should take account of carry in, which B-Em's implementation does not.

I haven't 100% grokked the thinking behind Fake86's: af = (((v1 ^ v2 ^ dst) & 0x10) == 0x10) ? 1 : 0; but I think it's actually correct.

[hoglet67]

If we want to feed this back "upstream" this is where Mike Chambers hangs out: http://www.vcfed.org/forum/showthread.php?27822-How-Emulators-Work/page5

[hoglet67]

Closing as fixed. I've also checked in the improved tracing code.