2019-12-13 08:14 EST


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0002159FSSCPtablespublic2010-09-14 20:54
ReporterGenghis 
Assigned Tokarajorma 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
Product Version3.6.12 RC1 
Target VersionFixed in Version3.6.13 
Summary0002159: Extra error-checking is needed for certain table values.
DescriptionCertain table values are used as look-ups into other tables, or are otherwise important and need to be meaningful. Many of them have validation checks that trigger warnings or errors if they fail, but some do not and fail silently.

For example: When a ship subsystem has an unknown armor type name, or when the IFF names are unknown.

The attached patch extra_error_checking.diff includes additional warnings for all of these cases that I found.
TagsNo tags attached.
Attached Files
  • diff file icon extra_error_checking.diff (11,543 bytes) 2010-04-09 00:58 -
    Index: code/mission/missionparse.cpp
    ===================================================================
    --- code/mission/missionparse.cpp	(revision 6030)
    +++ code/mission/missionparse.cpp	(working copy)
    @@ -711,6 +711,8 @@
     
     		if (index >= 0)
     			The_mission.ai_profile = &Ai_profiles[index];
    +		else
    +			WarningEx(LOCATION, "Mission: %s\nUnknown AI profile %s!", pm->name, temp );
     	}
     
     	Assert( The_mission.ai_profile != NULL );
    @@ -806,6 +808,9 @@
     		if (optional_string("+Default_ship:")) {
     			stuff_string(str, F_NAME, NAME_LENGTH);
     			ptr->default_ship = ship_info_lookup(str);
    +			if (-1 == ptr->default_ship) {
    +				WarningEx(LOCATION, "Mission: %s\nUnknown default ship %s!  Defaulting to %s.", pm->name, str, Ship_info[ptr->ship_list[0]].name );
    +			}
     			// see if the player's default ship is an allowable ship (campaign only). If not, then what
     			// do we do?  choose the first allowable one?
     			if (Game_mode & GM_CAMPAIGN_MODE || ((Game_mode & GM_MULTIPLAYER) && !(Net_player->flags & NETINFO_FLAG_AM_MASTER))) {
    @@ -2509,9 +2514,8 @@
     
     		// try and find the alternate name
     		p_objp->alt_type_index = (char)mission_parse_lookup_alt(name);
    -		Assert(p_objp->alt_type_index >= 0);
     		if(p_objp->alt_type_index < 0)
    -			mprintf(("Error looking up alternate ship type name!\n"));
    +			WarningEx(LOCATION, "Mission %s\nError looking up alternate ship type name %s!\n", pm->name, name);
     		else
     			mprintf(("Using alternate ship type name: %s\n", name));
     	}
    @@ -2525,9 +2529,8 @@
     
     		// try and find the callsign
     		p_objp->callsign_index = (char)mission_parse_lookup_callsign(name);
    -		Assert(p_objp->callsign_index >= 0);
     		if(p_objp->callsign_index < 0)
    -			mprintf(("Error looking up callsign!\n"));
    +			WarningEx(LOCATION, "Mission %s\nError looking up callsign %s!\n", pm->name, name);
     		else
     			mprintf(("Using callsign: %s\n", name));
     	}
    @@ -3193,9 +3196,15 @@
     			char cargo_name[NAME_LENGTH];
     			stuff_string(cargo_name, F_NAME, NAME_LENGTH);
     			int index = string_lookup(cargo_name, Cargo_names, Num_cargo, "cargo", 0);
    -			if (index == -1 && (Num_cargo < MAX_CARGO)) {
    -				index = Num_cargo;
    -				strcpy(Cargo_names[Num_cargo++], cargo_name);
    +			if (index == -1) {
    +				if (Num_cargo < MAX_CARGO) {
    +					index = Num_cargo;
    +					strcpy(Cargo_names[Num_cargo++], cargo_name);
    +				}
    +				else {
    +					WarningEx(LOCATION, "Maximum number of cargo names (%d) exceeded, defaulting to Nothing!", MAX_CARGO);
    +					index = 0;
    +				}
     			}
     			Subsys_status[i].subsys_cargo_name = index;
     		}
    @@ -4887,6 +4896,9 @@
     				}
     			}
     
    +			if (z == NUM_NEBULAS)
    +				WarningEx(LOCATION, "Mission %s\nUnknown nebula %s!", pm->name, str);
    +
     			if (optional_string("+Color:")) {
     				stuff_string(str, F_NAME, MAX_FILENAME_LEN);
     				for (z=0; z<NUM_NEBULA_COLORS; z++){
    @@ -4897,6 +4909,9 @@
     				}
     			}
     
    +			if (z == NUM_NEBULA_COLORS)
    +				WarningEx(LOCATION, "Mission %s\nUnknown nebula color %s!", pm->name, str);
    +
     			if (optional_string("+Pitch:")){
     				stuff_int(&Nebula_pitch);
     			} else {
    Index: code/mission/missioncampaign.cpp
    ===================================================================
    --- code/mission/missioncampaign.cpp	(revision 6030)
    +++ code/mission/missioncampaign.cpp	(working copy)
    @@ -152,6 +152,11 @@
     			}
     		}
     
    +		if (name == NULL) {
    +			nprintf(("Warning", "Invalid campaign type \"%s\"\n", campaign_type));
    +			break;
    +		}
    +
     		if (desc) {
     			if (optional_string("+Description:")) {
     				*desc = stuff_and_malloc_string(F_MULTITEXT, NULL, MISSION_DESC_LENGTH);
    @@ -207,6 +212,8 @@
     			stuff_string(name, F_NAME, MAX_FILENAME_LEN);
     			if (num < max)
     				list[num++] = vm_strdup(name);
    +			else
    +				mprintf(("MISSIONCAMPAIGN: Maximum number of missions exceeded (%d)!", max));
     		}
     	}
     
    @@ -2282,7 +2289,11 @@
     			}
     
     			event_count++;
    -			Assert(event_count < MAX_MISSION_EVENTS);
    +			if (event_count > MAX_MISSION_EVENTS) {
    +				mprintf(("MISSIONCAMPAIGN: Maximum number of events exceeded (%d)!", MAX_MISSION_EVENTS));
    +				event_count = MAX_MISSION_EVENTS;
    +				break;
    +			}
     		}
     	}
     
    @@ -2305,7 +2316,11 @@
     			}
     
     			count++;
    -			Assert(count < MAX_GOALS);
    +			if (count > MAX_GOALS) {
    +				mprintf(("MISSIONCAMPAIGN: Maximum number of goals exceeded (%d)!", MAX_GOALS));
    +				count = MAX_GOALS;
    +				break;
    +			}
     		}
     	}
     
    Index: code/mission/missionmessage.cpp
    ===================================================================
    --- code/mission/missionmessage.cpp	(revision 6030)
    +++ code/mission/missionmessage.cpp	(working copy)
    @@ -264,12 +264,16 @@
     		}
     	}
     
    +	if ( i == MAX_PERSONA_TYPES )
    +		WarningEx(LOCATION, "Unknown persona type in messages.tbl -- %s\n", type );
    +
     	char cstrtemp[NAME_LENGTH];
     	if ( optional_string("+") )
     	{
    +		int j;
     		stuff_string(cstrtemp, F_NAME, NAME_LENGTH);
     
    -		for (int j = 0; j < (int)Species_info.size(); j++)
    +		for (j = 0; j < (int)Species_info.size(); j++)
     		{
     			if (!strcmp(cstrtemp, Species_info[j].species_name))
     			{
    @@ -277,12 +281,11 @@
     				break;
     			}
     		}
    +
    +		if ( j == (int)Species_info.size() )
    +			WarningEx(LOCATION, "Unknown species in messages.tbl -- %s\n", cstrtemp );
     	}
     
    -	if ( i == MAX_PERSONA_TYPES )
    -		Error(LOCATION, "Unknown persona type in messages.tbl -- %s\n", type );
    -
    -
     	Num_personas++;
     }
     
    @@ -367,6 +370,9 @@
     	if ( optional_string("+Persona:") ) {
     		stuff_string(persona_name, F_NAME, NAME_LENGTH);
     		msg.persona_index = message_persona_name_lookup( persona_name );
    +
    +		if ( -1 == msg.persona_index )
    +			WarningEx(LOCATION, "Unknown persona in message %s in messages.tbl -- %s\n", msg.name, persona_name );
     	}
     
     	if ( !Fred_running)
    Index: code/nebula/neb.cpp
    ===================================================================
    --- code/nebula/neb.cpp	(revision 6030)
    +++ code/nebula/neb.cpp	(working copy)
    @@ -279,6 +279,9 @@
     		if(Neb2_bitmap_count < MAX_NEB2_BITMAPS){
     			strcpy_s(Neb2_bitmap_filenames[Neb2_bitmap_count++], name);
     		}
    +		else {
    +			WarningEx(LOCATION, "nebula.tbl\nExceeded maximum number of nebulas (%d)!\nSkipping %s.", MAX_NEB2_BITMAPS, name);
    +		}
     	}
     
     	// poofs
    @@ -291,6 +294,9 @@
     		if(Neb2_poof_count < MAX_NEB2_POOFS){
     			strcpy_s(Neb2_poof_filenames[Neb2_poof_count++], name);
     		}
    +		else {
    +			WarningEx(LOCATION, "nebula.tbl\nExceeded maximum number of nebula poofs (%d)!\nSkipping %s.", MAX_NEB2_POOFS, name);
    +		}
     	}
     
     	//Distance
    Index: code/cmdline/cmdline.h
    ===================================================================
    --- code/cmdline/cmdline.h	(revision 6030)
    +++ code/cmdline/cmdline.h	(working copy)
    @@ -134,6 +134,7 @@
     extern int Cmdline_dis_weapons;
     extern int Cmdline_noparseerrors;
     extern int Cmdline_nowarn;
    +extern int Cmdline_extra_warn;
     extern int Cmdline_show_mem_usage;
     extern int Cmdline_show_pos;
     extern int Cmdline_show_stats;
    Index: code/cmdline/cmdline.cpp
    ===================================================================
    --- code/cmdline/cmdline.cpp	(revision 6030)
    +++ code/cmdline/cmdline.cpp	(working copy)
    @@ -385,6 +385,7 @@
     cmdline_parm dis_weapons("-dis_weapons", NULL);		// Cmdline_dis_weapons
     cmdline_parm noparseerrors_arg("-noparseerrors", NULL);	// Cmdline_noparseerrors  -- turns off parsing errors -C
     cmdline_parm nowarn_arg("-no_warn", NULL);			// Cmdline_nowarn
    +cmdline_parm extra_warn_arg("-extra_warn", NULL);	// Cmdline_extra_warn
     cmdline_parm fps_arg("-fps", NULL);					// Cmdline_show_fps
     cmdline_parm show_mem_usage_arg("-show_mem_usage", NULL);	// Cmdline_show_mem_usage
     cmdline_parm pos_arg("-pos", NULL);					// Cmdline_show_pos
    @@ -405,6 +406,7 @@
     int Cmdline_dis_weapons = 0;
     int Cmdline_noparseerrors = 0;
     int Cmdline_nowarn = 0; // turn warnings off in FRED
    +int Cmdline_extra_warn = 0;
     int Cmdline_show_mem_usage = 0;
     int Cmdline_show_pos = 0;
     int Cmdline_show_stats = 0;
    @@ -891,6 +893,11 @@
     		Cmdline_FRED2_htl = 1;
     	}*/
     
    +	if (extra_warn_arg.found())
    +	{
    +		Cmdline_extra_warn = 1;
    +	}
    +
     	if (timerbar_arg.found()) {
     		Cmdline_timerbar = 1;
     	}
    Index: code/hud/hudparse.cpp
    ===================================================================
    --- code/hud/hudparse.cpp	(revision 6030)
    +++ code/hud/hudparse.cpp	(working copy)
    @@ -391,6 +391,7 @@
     
     	if(ship_index == -1)
     	{
    +		WarningEx(LOCATION, "\"$Ship:\" name \"%s\" not found.", shipname);
     		return NULL;
     	}
     
    @@ -491,6 +492,9 @@
     		{
     			stuff_string(buffer, F_NAME, NAME_LENGTH);
     			cg->parent = hud_get_gauge(buffer);
    +			if (cg->parent == NULL) {
    +				WarningEx(LOCATION, "\"+Parent:\" HUD gauge \"%s\" not found!", buffer);
    +			}
     		}
     
     		Num_gauge_types++;
    Index: code/ship/ship.cpp
    ===================================================================
    --- code/ship/ship.cpp	(revision 6030)
    +++ code/ship/ship.cpp	(working copy)
    @@ -1170,6 +1170,8 @@
     
     		if (valid)
     			strcpy_s(sip->cockpit_pof_file, temp);
    +		else
    +			WarningEx(LOCATION, "Ship %s\nCockpit POF file \"%s\" invalid!", sip->name, temp);
     	}
     	if(optional_string( "+Cockpit offset:" ))
     	{
    @@ -1192,6 +1194,8 @@
     
     		if (valid)
     			strcpy_s(sip->pof_file, temp);
    +		else
    +			WarningEx(LOCATION, "Ship %s\nPOF file \"%s\" invalid!", sip->name, temp);
     	}
     
     	// ship class texture replacement - Goober5000 and taylor
    @@ -1253,6 +1257,8 @@
     
     		if (valid)
     			strcpy_s(sip->pof_file_hud, temp);
    +		else
    +			WarningEx(LOCATION, "Ship %s\POF target file \"%s\" invalid!", sip->name, temp);
     	}
     
     	// optional hud target LOD if not using special hud model
    @@ -2591,6 +2597,12 @@
     		iff_data[0] = iff_lookup(iff_1);
     		iff_data[1] = iff_lookup(iff_2);
     
    +		if (iff_data[0] == -1)
    +			WarningEx(LOCATION, "Ship %s\nIFF colour seen by \"%s\" invalid!", sip->name, iff_1);
    +
    +		if (iff_data[1] == -1)
    +			WarningEx(LOCATION, "Ship %s\nIFF colour when IFF is \"%s\" invalid!", sip->name, iff_2);
    +
     		// Set the color
     		required_string("+As Color:");
     		stuff_int_list(iff_color_data, 3, RAW_INTEGER_TYPE);
    @@ -2769,6 +2781,9 @@
     			if(optional_string("$Armor Type:")) {
     				stuff_string(buf, F_NAME, SHIP_MULTITEXT_LENGTH);
     				sp->armor_type_idx = armor_type_get_idx(buf);
    +
    +				if (sp->armor_type_idx == -1)
    +					WarningEx(LOCATION, "Ship %s, subsystem %s\nInvalid armor type %s!", sip->name, sp->subobj_name, buf);
     			}
     
     			//	Get default primary bank weapons
    Index: code/globalincs/windebug.cpp
    ===================================================================
    --- code/globalincs/windebug.cpp	(revision 6030)
    +++ code/globalincs/windebug.cpp	(working copy)
    @@ -1164,6 +1164,20 @@
     	Messagebox_active = false;
     }
     
    +void _cdecl WarningEx( char *filename, int line, const char *format, ... )
    +{
    +#ifndef NDEBUG
    +	if (Cmdline_extra_warn) {
    +		char msg[sizeof(AssertText1)];
    +		va_list args;
    +		va_start(args, format);
    +		vsprintf(msg, format, args);
    +		va_end(args);
    +		Warning(filename, line, msg);
    +	}
    +#endif
    +}
    +
     void _cdecl Warning( char *filename, int line, const char *format, ... )
     {
     	Global_warning_count++;
    Index: code/globalincs/pstypes.h
    ===================================================================
    --- code/globalincs/pstypes.h	(revision 6030)
    +++ code/globalincs/pstypes.h	(working copy)
    @@ -180,6 +180,7 @@
     extern void LuaError(struct lua_State *L, char *format=NULL, ...);
     extern void _cdecl Error( const char * filename, int line, const char * format, ... );
     extern void _cdecl Warning( char * filename, int line, const char * format, ... );
    +extern void _cdecl WarningEx( char *filename, int line, const char *format, ... );
     
     extern int Global_warning_count;
     extern int Global_error_count;
    
    diff file icon extra_error_checking.diff (11,543 bytes) 2010-04-09 00:58 +

-Relationships
+Relationships

-Notes

~0011851

FUBAR-BDHR (developer)

Getting the same issue as with 0002151 but with subsystems overridden by .tbm.

~0011852

Genghis (developer)

Is this an impediment to releasing it?

Any values which were being checked before this patch (e.g. "$Damage Lightning Type:") also exhibit this behaviour, raising the warning on the bad table entry even though a good override is coming later in a .tbm.

Are there mods out there which depend on this behaviour? Reason I ask is because it would be a bitch to fix since the parser is totally unaware when it encounters the bad table entry that there's going to be a good override coming later. I'd have to move -all- the checking to the end, or rearrange the parser somehow, and that's a BigProject(tm).

~0011854

chief1983 (administrator)

Currently the custom hud gauges table does virtually no error checking or reporting, but it's already being rewritten by Swifty and the new incarnation should have some level of that.

~0011855

FUBAR-BDHR (developer)

Well the biggest problem I see is that the usual way to make a patch for errors found with new builds is using a .tbm with +nocreate and just fix what's broken. When you do that with the new error checks you still get the same error messages you just fixed.

Also just because you are using the .tbl doesn't mean it's in your directory or your asset. You may be making a mod XYZ of mod ABC. ABC contains armor.tbl and ships.tbl with say armor types Armor1, Armor2, Armor3, etc that is assigned to ships in ship.tbl. You are using that ship.tbl from ABC via mod.ini but don't like the armor setup so you make your own armor.tbl and use newarmor-shp.tbm in ABC. Each ship in newarmor.shp.tbl has a +nocreate entry with armor type set to better_armor1, 2, 3, etc. These armor types are perfectly valid but when ships.tbl from ABC parses it sees the old ones and throws an error for every ship with an armor entry. Not a good thing.

Probably why no error checking was ever done for this stuff.

~0011859

Genghis (developer)

Last edited: 2010-04-05 22:16

I don't see a way around this without one of two options: 1) a fairly intrusive rewrite of the parser. 2) No errors.

I've actually had some ideas about how I'd like to do the parsing in a more data-driven way compared to the current method (very hard-coded). It would also make the error checking happen as a second pass after parsing is finished, and so this table/.tbm issue would disappear.

I'll prototype-up my thoughts and make something that replaces the loader for ships, and see where that takes me. Should be fun, will probably take me a couple of weeks.

~0011860

FUBAR-BDHR (developer)

Well there is an option 3. Make these checks only occur when a dev_tools flag is on. They are all good to know when doing development. This could also be a temp solution until a better solution is available.

Personally I like the second pass idea for standard detection of problems like this. Of course it would still be a good idea to allow a check all option like above. If you can clean up every single bad piece of data in a TC or mod you should and knowing they exist is the first step.

~0011861

Genghis (developer)

Option 3 is actually pretty good, it's something I could do right away. It would be a good stop-gap at least.

~0011862

chief1983 (administrator)

Any intrusive rewrite would definitely be a post-3.6.12 thing, but I'm definitely in support of it. Just need to keep tabs on the rest of the team to make sure it doesn't step on anyone's toes.

FUBAR, isn't that essentially what debug builds are? Dev_tools? Actually that's not entirely true of course, and I've been pushing for separating mod dev tools from actual code debugging. I believe that certain warnings should be available within Release builds via a flag, so as not to hurt runtime performance so strongly but still offer mods help catching most asset problems. Not really related to this, but that's how I'd like to see a flag like that used.

~0011863

FUBAR-BDHR (developer)

Well I haven't actually tried this patch in a release build but what worried me was the window that pops up that say something like this mod has xx errors right before pilot select. I was getting that with these errors in debug and I believe you get that in release as well.

Anyway this may be one case where you want the ability to disable the checking since it does give warnings that currently aren't actual errors. If you don't it will be the whole TBP null MOI issue again with no one wanting to check errors on mods because there are just too many error messages to bypass in the debugger.

~0011864

Genghis (developer)

My plan was to have a "verbosity" option with different levels:

level 0 = no warnings, asserts, Int3s, if there's a fatal problem it just crashes.
level 1 (release build default) = only warn about potentially fatal problems, i.e. Asserts, Int3s, etc.
level 2 (debug build default) = Warnings for non-fatal but still "show-stopping" problems.
level 3 = maximum chatter. all warnings.

It won't affect much, there'll be "default" versions of the Warning, Int3, etc functions that behave exactly as before, and then overloaded versions where you can specify a verbose level. I'll then go through my patch and drop all the new warnings to level 3.

~0011865

FUBAR-BDHR (developer)

Scrap level 0 and it's a good plan. Coupled with the current (and probably expanded) debug_filter.cfg style system it would be a real step forward in finding bugs and preventing crashes.

~0011866

chief1983 (administrator)

Actually, currently release builds do ignore asserts and such. Do we want to have them be more verbose than they currently are you mean?

~0011867

FUBAR-BDHR (developer)

I though current release builds had some error checking now and show a warning about number of errors but let you continue. I wouldn't want to remove that.

~0011868

chief1983 (administrator)

That's all though. They skip over asserts entirely I believe.

~0011871

Genghis (developer)

Alright that warning/logging/error system is a bit of a mess, so I did the minimum change necessary rather than risk a refactor with verbose levels.

I added a new cmdline option "-extra_warn", and a new function WarningEx. When extra_warn is on, WarningEx prints a regular warning (with callstack, etc). When it's off, the function does nothing, doesn't even increment the global warnings count. I then modified my patch to use WarningEx everywhere instead of Warning.

That oughta do it.

Incidentally, did you know there was a "-no_warn" option and a "-noparseerrors" one?

~0011873

chief1983 (administrator)

Yup. People have abused -no_warn as a means to bypass any warning using debug builds, which has just been silly because that's the whole point of using them.

~0011922

karajorma (administrator)

Actually most of the abuse was centred around bypassing warnings in the release builds back when they used to show warnings. Warnings were then changed to only show up in debug builds and the flag became rather pointless.

~0011925

karajorma (administrator)

I committed all of the changes apart from the ones to missioncampaign.cpp. I don't see why you've replaced a pair of asserts with mfprint()s. Wouldn't that result in exactly the kind of silent error this patch is designed to kill?

~0011926

Genghis (developer)

Last edited: 2010-05-02 09:24

My thinking was that if something is bad enough to trigger an assert it's fatal and the program cannot continue, whereas these situations didn't look fatal with appropriate handling.

~0011929

karajorma (administrator)

How about a warning then?
+Notes

-Issue History
Date Modified Username Field Change
2010-03-22 13:49 Genghis New Issue
2010-03-22 13:49 Genghis File Added: extra_error_checking.diff
2010-03-22 13:49 Genghis Status new => assigned
2010-03-22 13:49 Genghis Assigned To => Genghis
2010-03-22 13:49 Genghis Status assigned => confirmed
2010-03-22 13:50 Genghis Status confirmed => assigned
2010-04-02 04:37 FUBAR-BDHR Note Added: 0011851
2010-04-04 11:26 Genghis Note Added: 0011852
2010-04-04 12:45 chief1983 Note Added: 0011854
2010-04-04 16:26 FUBAR-BDHR Note Added: 0011855
2010-04-05 22:15 Genghis Note Added: 0011859
2010-04-05 22:16 Genghis Note Edited: 0011859
2010-04-05 22:32 FUBAR-BDHR Note Added: 0011860
2010-04-06 09:42 Genghis Note Added: 0011861
2010-04-06 11:00 chief1983 Note Added: 0011862
2010-04-06 12:29 FUBAR-BDHR Note Added: 0011863
2010-04-06 13:45 Genghis Note Added: 0011864
2010-04-06 14:26 FUBAR-BDHR Note Added: 0011865
2010-04-06 14:53 chief1983 Note Added: 0011866
2010-04-06 15:08 FUBAR-BDHR Note Added: 0011867
2010-04-06 15:12 chief1983 Note Added: 0011868
2010-04-09 00:58 Genghis Note Added: 0011871
2010-04-09 00:58 Genghis File Deleted: extra_error_checking.diff
2010-04-09 00:58 Genghis File Added: extra_error_checking.diff
2010-04-09 10:49 chief1983 Note Added: 0011873
2010-05-02 01:43 karajorma Note Added: 0011922
2010-05-02 06:24 karajorma Note Added: 0011925
2010-05-02 09:23 Genghis Note Added: 0011926
2010-05-02 09:24 Genghis Note Edited: 0011926
2010-05-03 00:53 karajorma Note Added: 0011929
2010-07-05 03:10 karajorma Assigned To Genghis => karajorma
2010-09-14 20:54 karajorma Status assigned => resolved
2010-09-14 20:54 karajorma Fixed in Version => 3.6.13
2010-09-14 20:54 karajorma Resolution open => fixed
+Issue History