search

icculus / mojozork

A simple Z-Machine implementation in a single C file. Now with online multiplayer! :)
https://www.patreon.com/posts/54997062
zlib License
121 stars 10 forks source link

MojoZork Does Not Pass "CZECH - Comprehensive Z-machine Emulation CHecker" #26

Open sduensin opened 7 months ago

sduensin commented 7 months ago

I've been playing a lot with MojoZork, using it to learn Z-Machine internals, and generally mucking about. I'm curious/confused how MojoZork can fail a lot of stack-based memory operations and yet still run Zork. Building CZECH for v3 and feeding it to MojoZork results in the following (big paste alert):

CZECH: the Comprehensive Z-machine Emulation CHecker, version 0.8
Test numbers appear in [brackets].

print works or you wouldn't be seeing this.

Jumps [2]: jump.je..........jg.......jl.......jz...offsets..
Variables [32]: push/pull..pop.store.load.dec....

ERROR [40] (dec sp) Expected 9; got -1

ERROR [41] (dec sp) Expected 1; got 10

.inc....

ERROR [47] (inc sp) Expected 11; got 1

ERROR [48] (inc sp) Expected 1; got 10

.
    dec_chk........
bad [58]
inc_chk.........
Arithmetic ops [69]: add.......sub.......
    mul........div...........mod...........
Logical ops [113]: not....and.....or.....
Memory [127]: loadw.loadb..storeb..storew...
Subroutines [135]: call....ret.
    rtrue.rfalse.ret_popped.
    Computed call...
Objects [146]: get_parent....get_sibling.......get_child......jin.......
    test_attr......set_attr....clear_attr....set/clear/test_attr..
    get_next_prop......get_prop_len/get_prop_addr....
    get_prop..........put_prop ..........
    remove..insert.......
Indirect Opcodes [225]: load.

ERROR [225] (load sp -> result) Expected 45; got 44

ERROR [226] (load sp -> result) Expected 44; got 43

.

ERROR [228] (load [spointer] -> result) Expected 45; got 44

ERROR [229] (load [spointer] -> result) Expected 44; got 43

....

ERROR [234] (load [sp=spointer] -> result) Expected 45; got 44

ERROR [235] (load [sp=spointer] -> result) Expected 44; got 43

.

ERROR [237] (load sp -> sp) Expected 45; got 44

...

ERROR [241] (load [sp=spointer] -> sp) Expected 45; got 44

store.

ERROR [243] (store sp 83) Expected 44; got 45

.

ERROR [245] (store [spointer] 83) Expected 44; got 45

.

ERROR [247] (store [sp=spointer] 83) Expected 44; got 45

..........

ERROR [258] (store sp sp) Expected 43; got 44

.

ERROR [260] (store [sp=spointer] sp) Expected 43; got 44

......
    pull..........

ERROR [277] (pull sp) Expected 43; got 44

.

ERROR [279] (pull [sp=spointer]) Expected 43; got 44

.

ERROR [281] (pull [spointer]) Expected 43; got 44

inc.........

ERROR [291] (inc sp) Expected 46; got 1

ERROR [292] (inc sp) Expected 44; got 45

ERROR [293] (inc [spointer]) Expected 46; got 23840

ERROR [294] (inc [spointer]) Expected 44; got 45

ERROR [295] (inc [sp=spointer]) Expected 46; got 1

ERROR [296] (inc [sp=spointer]) Expected 44; got 45

dec.........

ERROR [306] (dec sp) Expected 44; got -1

ERROR [307] (dec sp) Expected 44; got 45

ERROR [308] (dec [spointer]) Expected 44; got 23838

ERROR [309] (dec [spointer]) Expected 44; got 45

ERROR [310] (dec [sp=spointer]) Expected 44; got -1

ERROR [311] (dec [sp=spointer]) Expected 44; got 45

    inc_chk.........

ERROR [321] (inc_chk sp) Expected 46; got 1

ERROR [322] (inc_chk sp) Expected 44; got 45

ERROR [323] (inc_chk [spointer]) Expected 123; got 71

ERROR [324] (inc_chk [sp=spointer]) Expected 46; got 1

ERROR [325] (inc_chk [sp=spointer]) Expected 44; got 45

dec_chk.........

ERROR [335] (dec_chk sp) Expected 123; got 71

ERROR [336] (dec_chk [spointer]) Expected 123; got 71

ERROR [337] (dec_chk [sp=spointer]) Expected 123; got 71

Misc [339]: test...random.verify

ERROR [342]  Expected 0; got 1

Header (No tests)
    interpreter 0  ()
    Flags on: NO status, 
    Flags off: time game, story file split, screen-splitting, variable-pitch-default, transcripting on, fixed-pitch on, 

Print opcodes [344]: Tests should look like... '[Test] opcode (stuff): stuff'
print_num (0, 1, -1, 32767,-32768, -1): 0, 1, -1, 32767, -32768, -1
[350] print_char (abcd): abcd
[354] new_line:

There should be an empty line above this line.
print_ret (should have newline after this)
.
print_addr (Hello.): Hello.

print_paddr (A long string that Inform will put in high memory):
A long string that Inform will put in high memory
Abbreviations (I love 'xyzzy' [two times]): I love 'xyzzy'  I love 'xyzzy'

[361] print_obj (Test Object #1Test Object #2): Test Object #1Test Object #2

Performed 362 tests.
Passed: 301, Failed: 42, Print tests: 19
Didn't crash: hooray!
Last test: quit!

I ran the same CZECH through jZip and it passes all tests. I tried to fix it but apparently don't know enough about how the stack operations are supposed to work yet.

Ideas?

(You can find CZECH here: https://github.com/jeffnyman/zifmia/tree/master/testers/czech )

sduensin commented 7 months ago

Ah HA! Seems we both missed this little gem:

6.3.4

In the seven opcodes that take indirect variable references 
(inc, dec, inc_chk, dec_chk, load, store, pull), 
an indirect reference to the stack pointer does not push or 
pull the top item of the stack - it is read or written in place.
icculus commented 7 months ago

Ah HA! Seems we both missed this little gem:

I took a quick shot at this, and it resolved some of those errors: down from 42 fails to 29.

I need to look at this more closely to see what's still going wrong, though.

EDIT: updated diff to fix silly bug, down to 24.

diff --git a/mojozork.c b/mojozork.c
index 432ce16..aa4ffe5 100644
--- a/mojozork.c
+++ b/mojozork.c
@@ -155,11 +155,13 @@ static uint8 *unpackAddress(const uint32 addr)
     return NULL;
 } // unpackAddress

-static uint8 *varAddress(const uint8 var, const int writing)
+static uint8 *varAddress(const uint8 var, const int writing, const int indirect)
 {
     if (var == 0) // top of stack
     {
-        if (writing)
+        if (indirect)
+            return (uint8 *) (writing ? GState->sp : GState->sp - 1);  // "6.3.4: In the seven opcodes that take indirect variable references (inc, dec, inc_chk, dec_chk, load, store, pull), an indirect reference to the stack pointer does not push or pull the top item of the stack - it is read or written in place."
+        else if (writing)
         {
             if ((GState->sp-GState->stack) >= (sizeof (GState->stack) / sizeof (GState->stack[0])))
                 GState->die("Stack overflow");
@@ -200,7 +202,7 @@ static void opcode_call(void)
     // no idea if args==0 should be the same as calling addr 0...
     if ((args == 0) || (operands[0] == 0))  // legal no-op; store 0 to return value and bounce.
     {
-        uint8 *store = varAddress(storeid, 1);
+        uint8 *store = varAddress(storeid, 1, 0);
         WRITEUI16(store, 0);
     } // if
     else
@@ -274,7 +276,7 @@ static void doReturn(const uint16 val)
     const uint8 storeid = (uint8) *(--GState->sp);  // pop the result storage location.

     dbg("returning: new pc=%X, bp=%u, sp=%u\n", (unsigned int) (GState->pc-GState->story), (unsigned int) GState->bp, (unsigned int) (GState->sp-GState->stack));
-    uint8 *store = varAddress(storeid, 1);  // and store the routine result.
+    uint8 *store = varAddress(storeid, 1, 0);  // and store the routine result.
     WRITEUI16(store, val);
 } // doReturn

@@ -295,28 +297,28 @@ static void opcode_rfalse(void)

 static void opcode_ret_popped(void)
 {
-    uint8 *ptr = varAddress(0, 0);   // top of stack.
+    uint8 *ptr = varAddress(0, 0, 0);   // top of stack.
     const uint16 result = READUI16(ptr);
     doReturn(result);
 } // opcode_ret_popped

 static void opcode_push(void)
 {
-    uint8 *store = varAddress(0, 1);   // top of stack.
+    uint8 *store = varAddress(0, 1, 0);   // top of stack.
     WRITEUI16(store, GState->operands[0]);
 } // opcode_push

 static void opcode_pull(void)
 {
-    const uint8 *ptr = varAddress(0, 0);   // top of stack.
+    const uint8 *ptr = varAddress(0, 0, 0);   // top of stack.
     const uint16 val = READUI16(ptr);
-    uint8 *store = varAddress((uint8) GState->operands[0], 1);
+    uint8 *store = varAddress((uint8) GState->operands[0], 1, 1);
     WRITEUI16(store, val);
 } // opcode_pull

 static void opcode_pop(void)
 {
-    varAddress(0, 0);   // this causes a pop.
+    varAddress(0, 0, 0);   // this causes a pop.
 } // opcode_pop

 static void updateStatusBar(void);
@@ -328,14 +330,14 @@ static void opcode_show_status(void)

 static void opcode_add(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const sint16 result = ((sint16) GState->operands[0]) + ((sint16) GState->operands[1]);
     WRITEUI16(store, result);
 } // opcode_add

 static void opcode_sub(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const sint16 result = ((sint16) GState->operands[0]) - ((sint16) GState->operands[1]);
     WRITEUI16(store, result);
 } // opcode_sub
@@ -412,7 +414,7 @@ static void opcode_jump(void)

 static void opcode_div(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     if (GState->operands[1] == 0)
         GState->die("Division by zero");
     const uint16 result = (uint16) (((sint16) GState->operands[0]) / ((sint16) GState->operands[1]));
@@ -421,7 +423,7 @@ static void opcode_div(void)

 static void opcode_mod(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     if (GState->operands[1] == 0)
         GState->die("Division by zero");
     const uint16 result = (uint16) (((sint16) GState->operands[0]) % ((sint16) GState->operands[1]));
@@ -430,35 +432,35 @@ static void opcode_mod(void)

 static void opcode_mul(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 result = (uint16) (((sint16) GState->operands[0]) * ((sint16) GState->operands[1]));
     WRITEUI16(store, result);
 } // opcode_mul

 static void opcode_or(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 result = (GState->operands[0] | GState->operands[1]);
     WRITEUI16(store, result);
 } // opcode_or

 static void opcode_and(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 result = (GState->operands[0] & GState->operands[1]);
     WRITEUI16(store, result);
 } // opcode_and

 static void opcode_not(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 result = ~GState->operands[0];
     WRITEUI16(store, result);
 } // opcode_not

 static void opcode_inc_chk(void)
 {
-    uint8 *store = varAddress((uint8) GState->operands[0], 1);
+    uint8 *store = varAddress((uint8) GState->operands[0], 1, 1);
     sint16 val = READUI16(store);
     store -= sizeof (uint16);
     val++;
@@ -468,7 +470,7 @@ static void opcode_inc_chk(void)

 static void opcode_inc(void)
 {
-    uint8 *store = varAddress((uint8) GState->operands[0], 1);
+    uint8 *store = varAddress((uint8) GState->operands[0], 1, 1);
     sint16 val = (sint16) READUI16(store);
     store -= sizeof (uint16);
     val++;
@@ -477,7 +479,7 @@ static void opcode_inc(void)

 static void opcode_dec_chk(void)
 {
-    uint8 *store = varAddress((uint8) GState->operands[0], 1);
+    uint8 *store = varAddress((uint8) GState->operands[0], 1, 1);
     sint16 val = (sint16) READUI16(store);
     store -= sizeof (uint16);
     val--;
@@ -487,7 +489,7 @@ static void opcode_dec_chk(void)

 static void opcode_dec(void)
 {
-    uint8 *store = varAddress((uint8) GState->operands[0], 1);
+    uint8 *store = varAddress((uint8) GState->operands[0], 1, 1);
     sint16 val = (sint16) READUI16(store);
     store -= sizeof (uint16);
     val--;
@@ -496,15 +498,15 @@ static void opcode_dec(void)

 static void opcode_load(void)
 {
-    const uint8 *valptr = varAddress((uint8) (GState->operands[0] & 0xFF), 0);
+    const uint8 *valptr = varAddress((uint8) (GState->operands[0] & 0xFF), 0, 1);
     const uint16 val = READUI16(valptr);
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     WRITEUI16(store, val);
 } // opcode_load

 static void opcode_loadw(void)
 {
-    uint16 *store = (uint16 *) varAddress(*(GState->pc++), 1);
+    uint16 *store = (uint16 *) varAddress(*(GState->pc++), 1, 0);
     FIXME("can only read from dynamic or static memory (not highmem).");
     FIXME("how does overflow work here? Do these wrap around?");
     const uint16 offset = (GState->operands[0] + (GState->operands[1] * 2));
@@ -514,7 +516,7 @@ static void opcode_loadw(void)

 static void opcode_loadb(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     FIXME("can only read from dynamic or static memory (not highmem).");
     FIXME("how does overflow work here? Do these wrap around?");
     const uint16 offset = (GState->operands[0] + GState->operands[1]);
@@ -545,7 +547,7 @@ static void opcode_storeb(void)

 static void opcode_store(void)
 {
-    uint8 *store = varAddress((uint8) (GState->operands[0] & 0xFF), 1);
+    uint8 *store = varAddress((uint8) (GState->operands[0] & 0xFF), 1, 1);
     const uint16 src = GState->operands[1];
     WRITEUI16(store, src);
 } // opcode_store
@@ -784,7 +786,7 @@ static uint16 getDefaultObjectProperty(const uint16 propid)

 static void opcode_get_prop(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 objid = GState->operands[0];
     const uint16 propid = GState->operands[1];
     uint16 result = 0;
@@ -805,7 +807,7 @@ static void opcode_get_prop(void)

 static void opcode_get_prop_addr(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 objid = GState->operands[0];
     const uint16 propid = GState->operands[1];
     uint8 *ptr = getObjectProperty(objid, propid, NULL);
@@ -815,7 +817,7 @@ static void opcode_get_prop_addr(void)

 static void opcode_get_prop_len(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     uint16 result;

     if (GState->operands[0] == 0)
@@ -837,7 +839,7 @@ static void opcode_get_prop_len(void)

 static void opcode_get_next_prop(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 objid = GState->operands[0];
     const int firstProp = (GState->operands[1] == 0);
     uint16 result = 0;
@@ -883,14 +885,14 @@ static uint16 getObjectRelationship(const uint16 objid, const uint8 relationship

 static void opcode_get_parent(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 result = getObjectRelationship(GState->operands[0], 4);
     WRITEUI16(store, result);
 } // opcode_get_parent

 static void opcode_get_sibling(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 result = getObjectRelationship(GState->operands[0], 5);
     WRITEUI16(store, result);
     doBranch((result != 0) ? 1: 0);
@@ -898,7 +900,7 @@ static void opcode_get_sibling(void)

 static void opcode_get_child(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 result = getObjectRelationship(GState->operands[0], 6);
     WRITEUI16(store, result);
     doBranch((result != 0) ? 1: 0);
@@ -1162,7 +1164,7 @@ static uint16 doRandom(const sint16 range)

 static void opcode_random(void)
 {
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const sint16 range = (sint16) GState->operands[0];
     const uint16 result = doRandom(range);
     WRITEUI16(store, result);
@@ -1555,7 +1557,7 @@ static int parseOperand(const uint8 optype, uint16 *operand)
         case 0: *operand = (uint16) READUI16(GState->pc); return 1;  // large constant (uint16)
         case 1: *operand = *(GState->pc++); return 1;  // small constant (uint8)
         case 2: { // variable
-            const uint8 *addr = varAddress(*(GState->pc++), 0);
+            const uint8 *addr = varAddress(*(GState->pc++), 0, 0);
             *operand = READUI16(addr);
             return 1;
         }
@@ -1586,7 +1588,7 @@ static void calculateStatusBar(char *buf, size_t buflen)
 {
     // if not a score game, then it's a time game.
     const int score_game = (GState->header.version < 3) || ((GState->header.flags1 & (1<<1)) == 0);
-    const uint8 *addr = varAddress(0x10, 0);
+    const uint8 *addr = varAddress(0x10, 0, 0);
     const uint16 objid = READUI16(addr);
     const uint16 scoreval = READUI16(addr);
     const uint16 movesval = READUI16(addr);
diff --git a/multizorkd.c b/multizorkd.c
index 0981fc4..878c5cb 100644
--- a/multizorkd.c
+++ b/multizorkd.c
@@ -1131,7 +1131,7 @@ static void writestr_multizork(const char *str, const uintptr slen)
 static void opcode_get_prop_addr_multizork(void)
 {
     Instance *inst = (Instance *) GState;  // this works because zmachine_state is the first field in Instance.
-    uint8 *store = varAddress(*(GState->pc++), 1);
+    uint8 *store = varAddress(*(GState->pc++), 1, 0);
     const uint16 objid = remap_objectid(GState->operands[0]);
     const uint16 propid = GState->operands[1];
     const uint16 external_mem_objects_base = ZORK1_EXTERN_MEM_OBJS_BASE;  // ZORK 1 SPECIFIC MAGIC
icculus commented 7 months ago

(Actually, I can see it's different test numbers failing now, so I probably broke something new when fixing this, haha)

sduensin commented 7 months ago

I've basically used your code as the basis of the ZIP I'm writing. Rather than copy/paste and change a few things, I've re-keyed the entire thing and replaced all the pointers into VM memory and the stack with just indices. I've also made a few other changes and plan to keep going with V4, V5, etc. Feel free to compare with and steal from: https://skunkworks.kangaroopunch.com/skunkworks/another-z-machine