Opened 15 years ago

Closed 15 years ago

#2071 closed defect

SKY: Bogus code

Reported by: fingolfin Owned by: SF/olki
Priority: normal Component: Engine: Sky
Keywords: Cc:
Game: Beneath a Steel Sky

Description

GCC 4 produces the following warning:

sky/logic.cpp: In member function ‘uint16 Sky::Logic::script(uint16, uint16)’: sky/logic.cpp:1340: warning: operation on ‘scriptData’ may be undefined

Indeed, that code is not well-defined: scriptData += READ_LE_UINT16(scriptData++)/2 - 1;

In particular, depending on the compiler, scriptData might end up off by one in either direction. The change was introduced by olki here:

<http://cvs.sourceforge.net/viewcvs.py/scummvm/scummvm/sky/ logic.cpp?r1=1.84&r2=1.85>

Looking at the old code, I believe this line should really be:

scriptData += READ_LE_UINT16(scriptData)/2;

I might be mistaken, though.

Ticket imported from: #1224968. Ticket imported from: bugs/2071.

Change History (8)

comment:1 by SF/olki, 15 years ago

Indeed, I just checked with the assembly code, and your fix is correct. I can't test right now though, so I won't commit immediately.

comment:2 by SF/olki, 15 years ago

Status: newpending

comment:3 by fingolfin, 15 years ago

Status: pendingnew

comment:4 by fingolfin, 15 years ago

OK, thanks.

I am not even sure whether the old code worked correctly, or not; I guess it depends on which compiler you use, on which arch, and with which optimizations settings; only checking disasm would probably tell us for sure what the old code does :-/

So, the old code may have worked correctly "by chance",dunno.

comment:5 by fingolfin, 15 years ago

Duh, silly me. Of course printfs will also tell us what the "bogus" old code did ;-)

comment:6 by SF/olki, 15 years ago

Well, the original code does the following: flodswl eax,esi add esi,eax sub esi,2 where flodswl is a macro:

flodswl macro dest,source ;load long register from word movzx dest,word ptr [source] add source,2 endm

So first 2 is added to esi (scriptData), then eax is added to it, and finally 2 is substracted again. That calculation is obviously completely unnecessary. So "scriptData += READ_LE_UINT16(scriptData)/2;" should be the correct fix.

comment:7 by SF/olki, 15 years ago

Status: newclosed

comment:8 by SF/olki, 15 years ago

scriptData += READ_LE_UINT16(scriptData++)/2 - 1; and scriptData += READ_LE_UINT16(scriptData)/2; give the same results on gcc version 4.0.0 (Apple Computer, Inc. build 5026) The game seems to work fine, so I committed.

Note: See TracTickets for help on using tickets.