Decompiler mysteries

[troflip]

Thought there was already a topic for this, but couldn't find one.

In Quest For Glory 1 (Hero's Quest), in Feature.sc, Act:canBeHere, there is this clause in an if statement:

Code: [Select]

(and
(== illegalBits 0)
(& signal --UNKNOWN-PROP-NAME--)  ; SCI Companion couldn't figure out the property name.
)

You can SV.exe has problems with it too:

Code: [Select]

code_0825: pTos illegalBits
ldi $0
eq?
bnt code_0833
pTos signal
pToa property_26044   // It doesn't know what property it is either... property_26044!
and
code_0833: bt code_0843

The byte code sequence is $62 $cb78, which is pToa with a value of $cb78 (52088 decimal... SV.exe shows half this number, I don't know why... anyway). Selector numbers should be less than $1000 generally (look at the selector vocab resource). $cb78 is crazy.

If you look in other games' implementations of canBeHere, you can see that the signal is supposed to be compared to $2000, which is the ignoreHorizon flag (or possibly $4000, ignAct - different games do different things). That is, it's supposed to look like this:

Code: [Select]

(and
(== illegalBits 0)
(& signal ignoreHorizon)  ; what it's supposed to be
)

(That would be a ldi operation, which would be the byte code sequence $34 $2000).

So I  guess the mystery is, what was the typo/bug in the original source code that cause that kind of byte code to be emitted?

Maybe the name of whatever flag they were using somehow collided with the name of a property selector? (But then why the crazy property selector number?)

[lskovlun]

My guess would be a compiler bug... similarly with the broken objects and duplicate export tables that have been seen in some games by the ScummVM team. Someone picked up a development compiler, tried it with the game while in development, and it ended up in the final game. Actually, since this is a system script, the guilty party in this case could be an interp programmer.

[troflip]

There have been a a couple of game bugs exposed by the decompiler that were, upon closer inspection, the result of obvious typos in the original source code.

My guess would be that instead of typing

Code: [Select]

(& signal ignrHrz) ; where (define ignrHrz $2000)

the programmer typed

Code: [Select]

(& signal ignoreHorizon) ; where ignoreHorizon is a method selector

The pToa instruction would then make sense - the compiler mistakenly thought it was property selector. The selector number doesn't make sense (ignoreHorizon is selector $d6, but the compiled code used $cb78), but maybe the Sierra compiler treated method selectors differently (although it's difficult to tell what's a method and what's a property. Or maybe the compiler was aware enough to know that ignoreHorizon is not a property on the self object, and produced garbage output instead of flagging an error. Or maybe, since ignoreHorizon *is* a method on self object (Actor), it used some value that represented that method...

[OmerMor]

I looked this up as well.
This exact problem can be found in many SCI0 games.

I checked the original source code for LSL3 and QfG2, and they have exactly the same code for this function, with the problematic line highlighted:

Code: [Select]

(method (canBeHere)
   ;; We have chosen a new point for the actor.  Check its validity.
   
   ;Set the base rectangle which we check for collisions.
   (if baseSetter
      (baseSetter doit: self)       ;user-supplied code
   else
      (BaseSetter self)             ;use the default kernel call
   )
   
   ;We can be blocked by:
   (return
      (and
         (or
            (and
               (== illegalBits 0)
               (& signal ignoreActors)  ; <=== PROBLEM HERE ===
            )
            ;;Illegal controls and baseRect intersections.
            (CanBeHere self (cast elements?))
         )
         
         ;;Being above the horizon.
         (or
            (& signal ignrHrz)
            (not (IsObject curRoom))
            (>= y (curRoom horizon?))
         )
         
         ;Being in conflict with a Block or Cage.
         (or   
            (== blocks NULL)
            (blocks allTrue: #doit: self)
         )
         
      )
   )
)
However, in QfG2 it compiles to:

Code: [Select]

pTos signal
ldi $4000
Instead of:

Code: [Select]

pTos  signal
pToa  property_633
Both games have the ignoreActors as selector for the method View::ignoreActors (Actor is a View).
Both also have this define:

Code: [Select]

(define ignrAct $4000) ;can ignore other actors
I guess in QfG2, someone defined ignoreActors as an alias to ignrAct and the compiler picked it.

As to why the selector for pToa got messed up - I guess it's a compiler bug.

[troflip]

Ah, so I was right! haha, it's really interesting to see all these bugs...

Another one I've come across a bunch are send calls with zero parameters. The decompiler ends up spitting out something like

Code: [Select]

(if (someThing) ... )

Because it thinks "something" is actually a send target, and needs to be enclosed in parens. But this is a compiler error in SCI Companion, and needs to be:

Code: [Select]

(if someThing ... )


It turns out the original source code actually had extra parens there too (and the compiler compiled it successfully, with the exception that it followed it up with a send opcode that does nothing).

[OmerMor]

Turns out ScummVM knows about this bug (the first one):

sci::validate_property()

Code: [Select]

if (index < 0 || (uint)index >= obj->getVarCount()) {
// This is same way sierra does it and there are some games, that contain such scripts like
//  iceman script 998 (fred::canBeHere, executed right at the start)
debugC(kDebugLevelVM, "[VM] Invalid property #%d (out of [0..%d]) requested from object %04x:%04x (%s)",
index, obj->getVarCount(), PRINT_REG(obj->getPos()), s->_segMan->getObjectName(obj->getPos()));
return dummyReg;
}
I agree that it's cool finding all these bugs. 
Might be interesting to peek at the original compiler and find the bug...

[troflip]

Looks like their compiler had a bug when a case statement was empty too - if it was the last case statement in a switch, it would branch off to some random position. This is the source of a lot of bad branch statements that confuse the decompiler.