Opened 19 years ago
Closed 19 years ago
#2071 closed defect
SKY: Bogus code
| Reported by: | fingolfin | Owned by: | SF/olki |
|---|---|---|---|
| Priority: | normal | Component: | Engine: Sky |
| Version: | 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 , 19 years ago
comment:2 by , 19 years ago
| Status: | new → pending |
|---|
comment:3 by , 19 years ago
| Status: | pending → new |
|---|
comment:4 by , 19 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 , 19 years ago
Duh, silly me. Of course printfs will also tell us what the "bogus" old code did ;-)
comment:6 by , 19 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 , 19 years ago
| Status: | new → closed |
|---|
comment:8 by , 19 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.
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.