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