DavidKinder / Windows-Frotz

Z-code interpreter for Windows, based on Stefan Jokisch's Frotz interpreter core.
http://www.davidkinder.co.uk/frotz.html
GNU General Public License v2.0
58 stars 12 forks source link

Bug in the unsorted dictionary code #22

Closed sijnstra closed 2 years ago

sijnstra commented 2 years ago

Hi, There seems to be a bug in the custom dictionary code when I test it with ziptest.z6. While testing functionality against my own interpreter, M4ZVM, I got very different results while I was comparing the read/input functionality. In particular, WinFrotz doesn't seem to scan an unordered dictionary correctly. My results are on the left with WinFrotz 1.23 on the right (exactly the same behaviour in 1.22). Based on the responses I suspect WinFrotz may need a tweak, but happy to be corrected.

p.s. it seems to already know the words red and blue

image

sijnstra commented 2 years ago

I'm logging this one on Frotz as I believe it nay be a core Frotz issue. The same behaviour occurs in dfrotz.

DavidKinder commented 2 years ago

Link to the referenced "ziptest.z6" example Z-code file: https://eblong.com/infocom/gamefiles/ziptest-r13-s890619.z6

sijnstra commented 2 years ago

The part of the standard which might not be handled properly is around supplying a negative dictionary length: tokenise VAR:251 1B 5 tokenise text parse dictionary flag

This performs lexical analysis (see read above).

The dictionary and flag operands are optional.

If a non-zero dictionary is supplied, it is used (if not, the ordinary game dictionary is). If the flag is set, unrecognised words are not written into the parse buffer and their slots are left unchanged: this is presumably so that if several tokenise instructions are performed in a row, each fills in more slots without wiping those filled by the others.

Parsing a user dictionary is slightly different. A user dictionary should look just like the main one but need not be alphabetically sorted. If the number of entries is given as -n, then the interpreter reads this as "n entries unsorted". This is very convenient if the table is being altered in play: if, for instance, the player is naming things.

DavidKinder commented 2 years ago

I had a brief look into this. Frotz does handle negative dictionary lengths correctly as far as I can see. This is not that surprising given that Infocom's Spellbreaker and Graham Nelson's Balances both use this sort of trick for writing on objects, and they both work correctly.

What seems to be happening is that, while the unsorted dictionary supplied by the game is being recognized as the correct length, words in it are not being matched. Either the encoding of the entered words is wrong, or the dictionary checking logic is wrong. Again, though, since Spellbreaker and Balances work, it is not immediately obvious what is wrong. It may be necessary to disassemble ziptest to see what it is actually doing.

DavidKinder commented 2 years ago

Okay, I've found the problem. It's not in the unsorted dictionary handling, but it is in the tokenization logic. The problem shows up because the custom dictionary used for ziptest to store the entered words starts with zero separator characters. This is valid, but unusual. In tokenise_line() in text.c there is this code that is intended to step over the separator characters:

  do {
    LOW_BYTE (sep_addr, separator)
    sep_addr++;
  } while (c != separator && --sep_count != 0);

This works fine for sep_count > 0, but not if sep_count == 0: sep_count will decrement and wrap round, as it is an unsigned value. Wrapping the above in a check for sep_count > 0 seems to fix the problem:

  if (sep_count > 0) {
    do {
      LOW_BYTE (sep_addr, separator)
      sep_addr++;
    } while (c != separator && --sep_count != 0);
  }

I will do some more testing before checking this in.

sijnstra commented 2 years ago

Awesome - glad you found it. I had forgotten about that edge case completely. It has a much more catastrophic effect on my 8-bit terp. I can see now how they would be connected.

Oddly I get exactly the same issue with both dfrotz and frotz. version 2.44. Maybe the downloadable packages are too old and I need to set up something to compile the more recent versions.

DavidKinder commented 2 years ago

It makes sense that you'd see the same issue in the command line version of Frotz: looking at old source code this bug has been there since at least Frotz 2.32, and probably before that. Windows Frotz started from Frotz 2.40, though I've made plenty of changes since then. It's also definitely present in the latest command line Frotz version in DG's gitlab repository.

DavidKinder commented 2 years ago

Fixed in commit b5d9e522fb79e76499878cac10075b4388c1313e.