Opened 11 years ago

Closed 11 years ago

Last modified 17 months ago

#4011 closed defect (fixed)

SAGA: Compiler warnings; possibly an engine bug

Reported by: eriktorbjorn Owned by: bluegr
Priority: normal Component: Engine: SAGA
Keywords: Cc:
Game:

Description

Current ScummVM SVN

While compiling ScummVM with the -O2 option, I encountered the following warning:

./engines/saga/animation.h: In member function ‘int Saga::Anim::playCutaway(int, bool)’: ./engines/saga/animation.h:173: warning: array subscript is above array bounds ./engines/saga/animation.h:173: warning: array subscript is above array bounds

Part of the problem is the validateAnimationsId() function in animation.h, which does

if (animId >= MAX_ANIMATIONS) { ... animId is a cutaway animation id } if (_animations[animId] == NULL) { error( ... ); }

Changing that to "} else if (_animations[animId] == NULL) {" brings up the next set of warnings:

./engines/saga/animation.h: In member function ‘int Saga::Anim::playCutaway(int, bool)’: ./engines/saga/animation.h:188: warning: array subscript is above array bounds ./engines/saga/animation.h:188: warning: array subscript is above array bounds

This time, it's the getAnimation() function that's the culprit. It says:

if (animId > MAX_ANIMATIONS) return _cutawayAnimations[animId - MAX_ANIMATIONS]; return _animations[animId];

Shouldn't that be "if (animId >= MAX_ANIMATIONS)" instead?

Changing that still leaves me with a warning:

./engines/saga/animation.h: In member function ‘int Saga::Anim::playCutaway(int, bool)’: ./engines/saga/animation.h:172: warning: array subscript is above array bounds

And this time I'm at a complete loss. Because now the validateAnimationId() function reads:

if (animId >= MAX_ANIMATIONS) { ... } else if (_animations[animId] == NULL) { ... }

It's the "else if" it complains about, but I can't see why. It's the call to setFrameTime() in playCutaway() that seems to trigger it.

Ticket imported from: #2284298. Ticket imported from: bugs/4011.

Change History (6)

comment:1 by SF/h00ligan, 11 years ago

..... void validateAnimationId(uint16 animId) { if (animId >= MAX_ANIMATIONS) { if (animId >= MAX_ANIMATIONS + ARRAYSIZE(_cutawayAnimations)) error("validateAnimationId: animId out of range"); if (_cutawayAnimations[animId - MAX_ANIMATIONS] == NULL) { error("validateAnimationId: animId=%i unassigned", animId); } } else if (_animations[animId] == NULL) { // check only if animId < MAX_ANIMATIONS error("validateAnimationId: animId=%i unassigned.", animId); } } ..... AnimationData* getAnimation(uint16 animId) { validateAnimationId(animId); if (animId >= MAX_ANIMATIONS) // '>=' definitely better than '>'. otherwise we can't access _cutawayAnimations[0] element return _cutawayAnimations[animId - MAX_ANIMATIONS]; return _animations[animId]; } .....

i think these changes are right. i was unable to reproduce any warning on my vs c++ /w4 compiler with above changes.

comment:2 by bluegr, 11 years ago

I made some of the corrections mentioned, could you please check if you're still getting warnings?

comment:3 by eriktorbjorn, 11 years ago

I still get the last warning I described:

./engines/saga/animation.h: In member function ‘int Saga::Anim::playCutaway(int, bool)’: ./engines/saga/animation.h:175: warning: array subscript is above array bounds

Which goes away if I remove the call to setFrameTime(). (Not a solution, of course, but I wanted to find out which call triggered it.) But it makes absolutely no sense to me. Maybe it's a false positive.

comment:4 by bluegr, 11 years ago

Closing this, as the last warning seems to be a false positive...

comment:5 by bluegr, 11 years ago

Owner: set to bluegr
Resolution: fixed
Status: newclosed

comment:6 by digitall, 17 months ago

Component: Engine: SAGA
Note: See TracTickets for help on using tickets.