View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002159 | FSSCP | tables | public | 2010-03-22 17:49 | 2010-09-15 00:54 |
Reporter | Genghis | Assigned To | karajorma | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.6.12 RC1 | ||||
Fixed in Version | 3.6.13 | ||||
Summary | 0002159: Extra error-checking is needed for certain table values. | ||||
Description | Certain 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. | ||||
Tags | No tags attached. | ||||
|
Getting the same issue as with 0002151 but with subsystems overridden by .tbm. |
|
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). |
|
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. |
|
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. |
|
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. |
|
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. |
|
Option 3 is actually pretty good, it's something I could do right away. It would be a good stop-gap at least. |
|
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. |
|
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. |
|
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. |
|
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. |
|
Actually, currently release builds do ignore asserts and such. Do we want to have them be more verbose than they currently are you mean? |
|
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. |
|
That's all though. They skip over asserts entirely I believe. |
|
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; |
|
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. |
|
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. |
|
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? |
|
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. |
|
How about a warning then? |
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 |