Test PUSH/POP instructions with different operand size

[barotto]

From VOGONS thread https://www.vogons.org/viewtopic.php?f=9&t=60095

Another code bug revealed with the POP ES instruction: it wasn't using the operand size to perform the 16-bit POP. So it was always increasing/decreasing (E)SP with 2 instead of the proper 4.

Test for this.

[superfury]

You might as well make it a full push/pop test with different operand sizes and src/destinations? So all basic segment register push/pop, opcode 8F pop, opcode ff push, 80186+ immediate pushes(push imm8/16/32), off the top of my head.

[barotto]

Added:
50+rw PUSH r16
50+rd PUSH r32
58+rw POP r16
58+rd POP r32

[superfury]

There's also:
06 PUSH ES
07 POP ES
0E PUSH CS
16 PUSH SS
17 POP SS
1E PUSH DS
1F POP DS
60 PUSHA(D)
61 POPA(D)
68 PUSH imm16/32
6A PUSH imm8
8F /0 POP r/m16/32
9C PUSHF(D)
9D POPF(D)
FF /6 PUSH r/m16/32
0F A0 PUSH FS
0F A1 POP FS
0F A8 PUSH GS
0F A9 POP GS

Don't know how many of these are already tested against operand size, address size and/or stack address size(the B bit within the stack segment descriptor)? Although the stack address size might be outside of the current checks on this testsuite atm(since not much of protected mode is verified)?

[barotto]

I know, some of those instructions maybe used but none of them are explicity tested, yet.
If an instruction is "officially" tested, it is marked as such in in the opcodes list (intel-opcodes.ods). That list also discriminates for operand size, address size, and stack size.

[superfury]

So that means that the untested stack instructions still are:
PUSHF(D)/POPF(D)
PUSH imm(except one case of imm8)
PUSH/POP Segreg(all segment registers, except POP CS)
PUSH/POP mem16/32
PUSHA/POPA

Those are still untested against operand/address/stack size, but pretty important for proper execution flow?

Even most basic pretty important instructions are left unchecked?
So:
Most math instructions(sub,sbb,add,or,and,xor,cmp) in the low opcode range(00-3Fh).
Most MOV instructions.
LEAVE
32-bit relative jumps(0F 8*)
Most INC/DEC instructions(flags not affecting Carry flag, r/m16/32).
Various CMP instructions

Why isn't INC/DEC(r/m16/32)/CMP/TEST tested with test 0xEE?

[barotto]

PUSHA/POPA are heavily used for calls, that doesn't mean they are bug free though. They have to be properly tested for corner cases.
This is the count of all the instructions currently used: icnt.txt

Even most basic pretty important instructions are left unchecked?

Yep.

Why isn't INC/DEC(r/m16/32)/CMP/TEST tested with test 0xEE?

Because there are only 24 hours in a day, usually :)

[barotto]

Added:
0E PUSH CS
1E PUSH DS
1F POP DS
16 PUSH SS
17 POP SS
06 PUSH ES
07 POP ES
0FA0 PUSH FS
0FA1 POP FS
0FA8 PUSH GS
0FA9 POP GS

[superfury]

So, that still leaves:

60 PUSHA(D)
61 POPA(D)
68 PUSH imm16/32
6A PUSH imm8
8F /0 POP r/m16/32
9C PUSHF(D)
9D POPF(D)
FF /6 PUSH r/m16/32

As well as those INC/DEC(16&32 bits variants)/CMP/TEST on test EE(where, CMP=SUB and TEST=AND on a logical flag level(unaffected registers other than flags). INC/DEC is about the same as ADD/SUB with 1, but not affecting the carry flag).

Edit: It seems the INC/DEC/CMP/TEST instructions aren't tested during the 0xEE test at all?

Tests are still passing atm(with your latest commits).

Although, thinking about it, those PUSH imm8/16/32 might need some self-modifying code in order to be tested extensively(modifying the value that's pushed).

[barotto]

Can you test an older version of your emu to see if that POP ES bug you talked about at vogons can now be detected with test386?

self-modifying code

I think that would be a bit overkill... I'll probably test with some pattern.

[superfury]

I've just tested the current testsuite against a bit older commit of UniPCemu, before the POP ES fix. It does properly error out during the POST 0A tests.

[barotto]

It does properly error out during the POST 0A tests.

Nice.

Added:
FF /6 PUSH r/m16
FF /6 PUSH r/m32
8F /6 POP r/m16
8F /6 POP r/m32

[superfury]

Don't you mean 8F /0? 8F /1-7 are #UDs.

That just leaves:
60 PUSHA(D)
61 POPA(D)
68 PUSH imm16/32
6A PUSH imm8
9C PUSHF(D)
9D POPF(D)

Edit: So far no errors have occurred yet:D

[barotto]

Yes you're right, 8F/0, cut'n'paste error...

[barotto]

Added:
60 PUSHA(D)
61 POPA(D)

[barotto]

Added:
68 PUSH imm16/32
6A PUSH imm8

[superfury]

They all still check out. That just leaves PUSHF(D) and POPF(D) instructions. All other PUSH/POP operations are tested now.

[barotto]

They all still check out.

Is this a good or a bad thing? Do you expect any test to fail?

[superfury]

Well, I know some software not running properly, most of them not even running in protected mode(yet). Like California Games hanging on the stage select screen(any CPU emulation in UniPCemu, 8086+ problem), Windows 95 crashing(jumping after various REP block moves) into cleared RAM(containing 0000h opcodes), Day of The Tentacle(tentacle.exe) not seeing the command line and giving me a unexpected 'by 0\n' on the command line, before showing help on parameters then crashing with a NULL pointer load error(no matter what's on the command line). Windows 3.0 simply hangs with recursive protection faults in protected mode after IRET from the kernel.

That smells a whole lot like a big logic/(conditional) jump error to me. But the odd thing is that the conditional jumps themselves seem to run fine, as far as I can see from breakpoints.

Even the simple testsuite in the remaining instruction thread shows no visible errors(current commit), so only a few possible culprits are left atm: flag storage(pushf/popf during real and protected mode) and a handful of remaining instructions(haven't tested 32-bit stack with my "testsuite"'s ret immediate instruction yet, due to running in real mode).

[superfury]

Odd that with the latest improvements, some of those mentioned software(Windows 95 setup, Jazz Jackrabbit) tries to execute Protected-mode only instructions in real mode now(ARPL for Windows 95 setup, LLDT for Jazz Jackrabbit)?

[barotto]

ARPL and LLDT are protected mode only and #UD would be thrown if a program would try to execute them in real mode. Sounds like some jump not taken somewhere way before those instructions.

[superfury]

Well, the strange thing is that during my checking of the 8086, no errors were observed in basic execution. Jumps are made when flags are set and not made when not set. Unconditional jumps should also work. Then why is it ending up there?

The 0F 386+ conditional jumps do exactly the same as the 8086+ jumps.

[barotto]

Well, maybe the JMPs themselves are good but the code that decided to take or not those jumps is wrongly executed. The problem could actually be a thousand cycles before...
Do you use a prefetch queue? I once fixed a bug that made the prefetcher read the wrong code memory area.

[superfury]

Just checked all x86 jumps and conditional jumps. Each and every one should be working properly as far as I can see?

https://bitbucket.org/superfury/unipcemu/src/a32836f0af33adbbc690098ed72a52dcad23f18c/UniPCemu/cpu/?at=master

Look at opcodes*.c for all opcode implementations.

Look for CPU_jmprel for relative jumps and calls, CPU_jmpabs for absolute ones. "segmentWritten(CPU_SEGMENT_CS"(and destEIP load before that for it's EIP address to load) for the far jumps and calls.

I can't see any errors in those instructions processing.

Perhaps an error in the flags itself could also be a cause, but errors even happen on the 8086, which just pushes and pops them without modification of said data(raw access).

[superfury]

Just tried running Windows 3.0 in it's default mode(no parameters). I see it page faulting twice(no other faults), then returning to the MS-DOS 5.0 prompt with the Windows logo screen behind it.

Edit: Standard mode still triple faults for some odd reason. With only himem.sys and various drivers loaded, running win.exe without parameters returns it to the MS-DOS prompt after two paging faults on two different addresses(the last one being at EIP=0x8000XXXX, so within the kernel?).

[superfury]

I've verified(during execution with breakpoints on the flags in Visual C++) that most conditional jump and loop instructions work as documented. Both used breakpoints on condition match and non match after it being matched. Jumps are taken when required and not taken when required not to.

The only cases left untested are JO(overflow flag not set), JNO and JP(parity flag not set). All other jcc instructions work correctly.

[superfury]

Yes, a PIQ is emulated, but it should be functioning normally afaik. Since it's using the same segmented/paging logic to check virtual and physical addresses and access them, as the EU would have(except not calling the BIU pipeline, but instead buffering into a simple fifo buffer, while it's called as a non-active BIU T3 or T1 cycle itself(depending on whether it's a 80(1)86 or 80286+, which is the same as BIU memory access cycles(which simply have higher priority using the basic if-else logic)). So it's the same as normal memory accesses(only instead of being called by the EU, it's executing directly on the BIU).

The only way that could go wrong is when the basic segmentation and/or paging unit translation fails, which applies to the entire CPU(both prefetching, instruction fetching(EU<-PIQ) and normal instructions uses it this way).

Since the BIU just dumbly fetches bytes/word/dwords(depending on alignment) using it's own EIP duplicate(which is loaded when a PIQ flush occurs, so during jumps and CPU resetting), nothing much should be able to go wrong there?

The EIP address is always added to the base address of the CS segment descriptor, which uses the format as documented by the 80386+ CPU documentation. So the resulting address to fetch data from cannot be wrong, unless EIP(both BIU and register values) or the CS descriptor is loaded incorrectly, which would be a general problem having nothing to do with the BIU anyways(incorrect program execution).

[superfury]

Also, the BIU must be working properly, otherwise it can't even boot past the BIOS, since any jump would fail right away because of a wrong base address/offset.

[superfury]

Just tried running the AT BIOS, which confirms proper execution of the JP and JO conditional jumps. The only one left untested is JNO, both taken and not taken.