View Issue Details

IDProjectCategoryView StatusLast Update
0002159FSSCPtablespublic2010-09-15 00:54
ReporterGenghis Assigned Tokarajorma  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.12 RC1 
Fixed 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.

Activities

FUBAR-BDHR

2010-04-02 08:37

developer   ~0011851

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

Genghis

2010-04-04 15:26

developer   ~0011852

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).

chief1983

2010-04-04 16:45

administrator   ~0011854

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.

FUBAR-BDHR

2010-04-04 20:26

developer   ~0011855

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.

Genghis

2010-04-06 02:15

developer   ~0011859

Last edited: 2010-04-06 02: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.

FUBAR-BDHR

2010-04-06 02:32

developer   ~0011860

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.

Genghis

2010-04-06 13:42

developer   ~0011861

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

chief1983

2010-04-06 15:00

administrator   ~0011862

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.

FUBAR-BDHR

2010-04-06 16:29

developer   ~0011863

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.

Genghis

2010-04-06 17:45

developer   ~0011864

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.

FUBAR-BDHR

2010-04-06 18:26

developer   ~0011865

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.

chief1983

2010-04-06 18:53

administrator   ~0011866

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

FUBAR-BDHR

2010-04-06 19:08

developer   ~0011867

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.

chief1983

2010-04-06 19:12

administrator   ~0011868

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

Genghis

2010-04-09 04:58

developer   ~0011871

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?

2010-04-09 04:58

 

extra_error_checking.diff (11,543 bytes)   
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;
extra_error_checking.diff (11,543 bytes)   

chief1983

2010-04-09 14:49

administrator   ~0011873

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.

karajorma

2010-05-02 05:43

administrator   ~0011922

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.

karajorma

2010-05-02 10:24

administrator   ~0011925

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?

Genghis

2010-05-02 13:23

developer   ~0011926

Last edited: 2010-05-02 13: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.

karajorma

2010-05-03 04:53

administrator   ~0011929

How about a warning then?

Issue History

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