Author Topic: preFAST output (Read 10055 times)

portej05 · **on:** May 08, 2009, 07:03:51 am

I've attached the preFAST (/analyze) output for 5199.
It's purely for interest. At least some of the warnings are for scenarios that could never occur.

(More info on preFAST)

[attachment deleted by ninja]

karajorma · **Reply #1 on:** May 08, 2009, 08:17:34 am

code - 0 error(s), 1254 warning(s)

Sounds about right

portej05 · **Reply #2 on:** May 08, 2009, 09:14:52 am

But keep in mind that those are analysis warnings.... They may or may not be warnings, and they may or may not be right. So, is a warning a warning if it may not be a warning?

Flipside · **Reply #3 on:** May 08, 2009, 09:24:23 am

...

Come back BBC Basic! All is forgiven!

karajorma · **Reply #4 on:** May 08, 2009, 11:09:38 am

Quote from: portej05 on May 08, 2009, 09:14:52 am

But keep in mind that those are analysis warnings.... They may or may not be warnings, and they may or may not be right. So, is a warning a warning if it may not be a warning?

I know that a bunch of them are unreachable but it's still a rather high number I suspect.

portej05 · **Reply #5 on:** May 08, 2009, 11:13:32 am

The one that I've had a look at is:

Code: [Select]

code\autopilot\autopilot.cpp(1061) : warning C6200: Index '399' is out of valid index range '0' to '74' for non-stack buffer 'struct wing * Wings'

Code: [Select]

if (Ships[i].wingnum != -1)
			{
				for (j = 0; j < MAX_AI_GOALS; j++)
				{
					ai_goal *aigp = &Wings[i].ai_goals[j];

					if ( ((aigp->ship_name != NULL) && !stricmp(aigp->ship_name, goal_name))
							&& (aigp->ai_mode == goal) )

If I'm reading this right, if a ship in a slot above 75 is allocated to a wing, the game'll crash - but my understanding of the ship structure is not good atm

EDIT: To clarify: i goes from 0-399, Wings only has 75 elements

karajorma · **Reply #6 on:** May 08, 2009, 11:25:33 am

Unless I'm mistaken the code is actually assuming that the Wing index and the ship index are the same number. And they almost certainly aren't.

I haven't looked at the code but I suspect that was meant to read

Code: [Select]

ai_goal *aigp = &Wings[Ships[i].wingnum].ai_goals[j];

EDIT : Look at this if you get the chance too. You may have found the cause.

portej05 · **Reply #7 on:** May 08, 2009, 11:55:16 am

yeah, was looking at that earlier

Is there anywhere where I can submit random small patches?

e.g.

Code: [Select]

bool IsVisited(int nav)
{
	if (nav > MAX_NAVPOINTS && nav < 0)
		return 0;

	if (Navs[nav].flags & NP_VISITED)
		return 1;
	return 0;
}

should be

Code: [Select]

bool IsVisited(int nav)
{
	if (nav => MAX_NAVPOINTS && nav < 0)
		return 0;

	if (Navs[nav].flags & NP_VISITED)
		return 1;
	return 0;
}

taylor · **Reply #8 on:** May 08, 2009, 12:11:07 pm

Quote from: karajorma on May 08, 2009, 11:25:33 am

Unless I'm mistaken the code is actually assuming that the Wing index and the ship index are the same number. And they almost certainly aren't.

I haven't looked at the code but I suspect that was meant to read

Code: [Select]
ai_goal *aigp = &Wings[Ships[i].wingnum].ai_goals[j];

EDIT : Look at this if you get the chance too. You may have found the cause.

Wow, and that was one of my changes too.

I believe that code was actually a copy&paste from somewhere else though, so it might be a good idea to do a quick search for the original in case it was wrong as well. It was probably just a copy&paste error however and the original source is likely fine.

karajorma · **Reply #9 on:** May 08, 2009, 12:19:08 pm

Yep, that's exactly what appears to have happened. If you search for ai_goal *aigp = &Wings you find another example only a few lines down where the limit for i is MAX_WINGS.

chief1983 · **Reply #10 on:** May 08, 2009, 02:36:12 pm

So the source is correct but the copy paste is wrong in the different context, right?

taylor · **Reply #11 on:** May 08, 2009, 03:06:02 pm

Yeah, the code just needed an edit after the paste, but I missed it. And unfortunately the test mission for this only included a case which worked perfectly with this broken code.

The code itself was part of some changes to fix AI related Mantis bugs with regards to autopilot, so it should all be good once the change that karajorma pointed out has been made.

chief1983 · **Reply #12 on:** May 08, 2009, 03:42:32 pm

Well I'll have Woolie taking a look at it then, we'll see he thinks. Or ARSPR.

portej05 · **Reply #13 on:** May 09, 2009, 01:43:04 am

I'm finding a lot of cases like this:

Code: [Select]

popup.cpp(682)
if ( (Game_mode & GM_NORMAL) || ((Game_mode && GM_MULTIPLAYER) && !(Game_mode & GM_IN_MISSION)) ){
		game_stop_time();
	}

I'm going to change it to

Code: [Select]

popup.cpp(682)
if ( (Game_mode & GM_NORMAL) || ((Game_mode & GM_MULTIPLAYER) && !(Game_mode & GM_IN_MISSION)) ){
		game_stop_time();
	}

but I'm not sure what effect this is going to have - does anyone know what this will do?

Flipside · **Reply #14 on:** May 09, 2009, 01:48:22 am

I don't code in C++, but that looks odd, in C++ does the '&' function return true for two 'false' as well as two 'true' results, with the '&&' function only returning a True for two positive results?

portej05 · **Reply #15 on:** May 09, 2009, 01:53:56 am

yep. Whoever was coding this stuff was pretty tired, and these are bug causing problems that are almost impossible to track down

There are also a fair few cases of this kinda thing (ambiguous ordering - what happens is compiler dependent):

Code: [Select]

if ( !Player->flags & PLAYER_FLAGS_MATCH_TARGET ) {

This one has got me baffled:

Code: [Select]

if ( (PF_ALLOW_DEAD_KEYS) && (Game_mode & GM_IN_MISSION) ) {

This one will always return true if sv->type is not zero:

Code: [Select]

if(sv->type && SEXP_VARIABLE_NUMBER)

Problem is, I don't know what side effects changing these will have.

karajorma · **Reply #16 on:** May 09, 2009, 01:59:24 am

Quote from: portej05 on May 09, 2009, 01:43:04 am

but I'm not sure what effect this is going to have - does anyone know what this will do?

It will fix it.

Go ahead with the change. It was obviously a mistake and as far as I can see fixing it won't cause any change in behaviour. The only two actual game modes are single and multiplayer. With that in mind look at what the code was doing

Code: [Select]

if ( (Game_mode & GM_NORMAL) || ((true) && !(Game_mode & GM_IN_MISSION)) )
So if the game is in SP mode (GM_NORMAL) or some other mode (GM_MULTIPLAYER is the only other mode) when not in mission, do something. i.e exactly what fixing it will do.

Flipside · **Reply #17 on:** May 09, 2009, 02:03:20 am

Must admit, that's why I always break statements like that down into bite-sized sections, it's so easy to sink into Boolean hell.

portej05 · **Reply #18 on:** May 09, 2009, 02:04:26 am

Kara: So one of GM_MULTIPLAYER or GM_NORMAL will always be defined?

portej05 · **Reply #19 on:** May 09, 2009, 02:12:24 am

In the multiplayer section:

Code: [Select]

if( 
	MULTI_CONNECTED(Net_players[idx]) && 
	(Net_player != &Net_players[idx]) && 
	(
		(Net_players[idx].state != NETPLAYER_STATE_DEBRIEF) || 
		(Net_players[idx].state != NETPLAYER_STATE_DEBRIEF_ACCEPT) || 
		(Net_players[idx].state != NETPLAYER_STATE_DEBRIEF_REPLAY)
	) 
  ){

The three != cases will always be true when combined with ||. Should they have been &&?

News:

Author Topic: preFAST output (Read 10055 times)

preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output

Re: preFAST output