[Freeciv-Dev] Re: (PR#13385) server crash in get_invention

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=13385 >

Jason Short wrote:
> <URL: http://bugs.freeciv.org/Ticket/Display.html?id=13385 >
> 
> This is with current CVS plus the unknown-goto patch (PR#6977).  However
> the latter is unlikely to be responsible.
> 
> Not reproducable as it happened while I was playing a single-player
> game.  It happened about 20 turns into the game.
> 
> #2  0xb7e512df in __assert_fail () from /lib/tls/libc.so.6
> #3  0x080fa928 in get_invention (pplayer=0x0, tech=199) at tech.c:59
> #4  0x080fb110 in base_total_bulbs_required (pplayer=0x81fef44, tech=199)
>     at tech.c:400
> #5  0x080fb0d9 in total_bulbs_required (pplayer=0x81fef44) at tech.c:366
> #6  0x0805dc11 in update_tech (plr=0x81fef44, bulbs=2) at techtools.c:468
> #7  0x0806fb96 in update_city_activity (pplayer=0x81fef44, pcity=0x84406d0)
>     at cityturn.c:1489
> #8  0x0806d196 in update_city_activities (pplayer=0x81fef44) at
> cityturn.c:371
> #9  0x080508be in end_phase () at srv_main.c:640
> #10 0x08052715 in main_loop () at srv_main.c:1701
> #11 0x08052e71 in srv_loop () at srv_main.c:1962
> #12 0x0805292d in srv_main () at srv_main.c:1779
> #13 0x0804ae55 in main (argc=8, argv=0xbffff8b4) at civserver.c:242

The pplayer==NULL thing is just a red herring.  The bug is that
get_invention is being called on A_UNSET.

This fixed assertion means all callers of total_bulbs_required (which is
a lot of them) must be audited.  total_bulbs_required cannot be called
on A_UNSET.  This is the correct thing to do since the return value is
garbage (1 bulb) anyway which leads to ugly displays (in the report,
science dialog, etc.) and probably AI bugs.  The attached patch fixes
just a few of these (it's not an audit by any means) and fixes some
longstanding display buglets in the process.

Another choice is to fix total_bulbs_required to special-case A_UNSET.

-jason

Index: client/climisc.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/climisc.c,v
retrieving revision 1.167
diff -u -r1.167 climisc.c
--- client/climisc.c    21 Jun 2005 16:21:00 -0000      1.167
+++ client/climisc.c    30 Jun 2005 16:43:34 -0000
@@ -308,9 +308,13 @@
 struct sprite *client_research_sprite(void)
 {
   if (can_client_change_view() && game.player_ptr) {
-    int index = (NUM_TILES_PROGRESS
-                * get_player_research(game.player_ptr)->bulbs_researched)
-      / (total_bulbs_required(game.player_ptr) + 1);
+    int index = 0;
+
+    if (get_player_research(game.player_ptr)->researching != A_UNSET) {
+      index = (NUM_TILES_PROGRESS
+              * get_player_research(game.player_ptr)->bulbs_researched)
+       / (total_bulbs_required(game.player_ptr) + 1);
+    }
 
     /* This clipping can be necessary since we can end up with excess
      * research */
Index: client/text.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/text.c,v
retrieving revision 1.43
diff -u -r1.43 text.c
--- client/text.c       21 Jun 2005 16:21:00 -0000      1.43
+++ client/text.c       30 Jun 2005 16:43:35 -0000
@@ -467,6 +467,15 @@
     RETURN;
   }
   assert(ours >= 0 && theirs >= 0);
+  if (get_player_research(plr)->researching == A_UNSET) {
+    if (theirs == 0) {
+      add(_("Progress: no research target (%d pts/turn)"), ours);
+    } else {
+      add(_("Progress: no research target "
+           "(%d pts/turn, %d pts/turn from team)"), ours, theirs);
+    }
+    RETURN;
+  }
   turns_to_advance = (total_bulbs_required(plr) + ours + theirs - 1)
                      / (ours + theirs);
   if (theirs == 0) {
@@ -611,12 +620,17 @@
 {
   INIT;
 
-  add(_("Shows your progress in researching "
-       "the current technology.\n%s: %d/%d."),
-      get_tech_name(game.player_ptr,
-                   get_player_research(game.player_ptr)->researching),
-      get_player_research(game.player_ptr)->bulbs_researched,
-      total_bulbs_required(game.player_ptr));
+  add_line(_("Shows your progress in researching the current technology."));
+  if (get_player_research(game.player_ptr)->researching == A_UNSET) {
+    add_line(_("no research target."));
+  } else {
+    /* TRANS: <tech>: <amount>/<total bulbs> */
+    add_line(_("%s: %d/%d."),
+            get_tech_name(game.player_ptr,
+                          get_player_research(game.player_ptr)->researching),
+            get_player_research(game.player_ptr)->bulbs_researched,
+            total_bulbs_required(game.player_ptr));
+  }
   RETURN;
 }
 
Index: client/gui-gtk-2.0/repodlgs.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk-2.0/repodlgs.c,v
retrieving revision 1.90
diff -u -r1.90 repodlgs.c
--- client/gui-gtk-2.0/repodlgs.c       21 Jun 2005 16:21:00 -0000      1.90
+++ client/gui-gtk-2.0/repodlgs.c       30 Jun 2005 16:43:37 -0000
@@ -332,12 +332,38 @@
   gtk_widget_grab_focus(science_change_menu_button);
 }
 
+/****************************************************************************
+  Called to set several texts in the science dialog.
+****************************************************************************/
+static void update_science_text(void)
+{
+  char text[512];
+  gdouble pct;
+  struct player_research *research = get_player_research(game.player_ptr);
+
+  if (research->researching == A_UNSET) {
+    my_snprintf(text, sizeof(text), "%d/-", research->bulbs_researched);
+    pct = 0.0;
+  } else {
+    my_snprintf(text, sizeof(text), "%d/%d",
+               research->bulbs_researched,
+               total_bulbs_required(game.player_ptr));
+    pct = CLAMP((gdouble) research->bulbs_researched
+               / total_bulbs_required(game.player_ptr), 0.0, 1.0);
+  }
+
+  gtk_progress_bar_set_text(GTK_PROGRESS_BAR(science_current_label), text);
+  gtk_progress_bar_set_fraction(GTK_PROGRESS_BAR(science_current_label), pct);
+
+  /* work around GTK+ refresh bug. */
+  gtk_widget_queue_resize(science_current_label);
+}
+
 /****************************************************************
 ...
 *****************************************************************/
 void science_change_callback(GtkWidget *widget, gpointer data)
 {
-  char text[512];
   size_t to = (size_t) data;
 
   if (GTK_TOGGLE_BUTTON(science_help_toggle)->active) {
@@ -346,24 +372,11 @@
      * there may be a better way to do this?  --dwp */
     science_dialog_update();
   } else {
-    gdouble pct;
-    struct player_research *research = get_player_research(game.player_ptr);
 
     gtk_widget_set_sensitive(science_change_menu_button,
                             can_client_issue_orders());
-    my_snprintf(text, sizeof(text), "%d/%d",
-               research->bulbs_researched,
-               total_bulbs_required(game.player_ptr));
-    pct = CLAMP((gdouble) research->bulbs_researched
-               / total_bulbs_required(game.player_ptr), 0.0, 1.0);
-
-    gtk_progress_bar_set_text(GTK_PROGRESS_BAR(science_current_label), text);
-    gtk_progress_bar_set_fraction(GTK_PROGRESS_BAR(science_current_label),
-       pct);
+    update_science_text();
     
-    /* work around GTK+ refresh bug. */
-    gtk_widget_queue_resize(science_current_label);
-
     dsend_packet_player_research(&aconnection, to);
   }
 }
@@ -428,7 +441,6 @@
   char text[512];
   GtkWidget *item;
   GList *sorting_list = NULL, *it;
-  gdouble pct;
   GtkSizeGroup *group1, *group2;
   struct player_research *research = get_player_research(game.player_ptr);
 
@@ -459,16 +471,8 @@
   gtk_widget_set_sensitive(science_change_menu_button,
                           can_client_issue_orders());
 
-  my_snprintf(text, sizeof(text), "%d/%d",
-             research->bulbs_researched,
-             total_bulbs_required(game.player_ptr));
+  update_science_text();
 
-  pct = CLAMP((gdouble) research->bulbs_researched
-             / total_bulbs_required(game.player_ptr), 0.0, 1.0);
-
-  gtk_progress_bar_set_text(GTK_PROGRESS_BAR(science_current_label), text);
-  gtk_progress_bar_set_fraction(GTK_PROGRESS_BAR(science_current_label), pct);
- 
   /* work around GTK+ refresh bug. */
   gtk_widget_queue_resize(science_current_label);
  
Index: server/report.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/report.c,v
retrieving revision 1.66
diff -u -r1.66 report.c
--- server/report.c     21 Jun 2005 16:21:02 -0000      1.66
+++ server/report.c     30 Jun 2005 16:43:38 -0000
@@ -93,6 +93,7 @@
 static const char *percent_to_text(int value);
 static const char *production_to_text(int value);
 static const char *economics_to_text(int value);
+static const char *science_to_text(int value);
 static const char *mil_service_to_text(int value);
 static const char *pollution_to_text(int value);
 
@@ -111,7 +112,7 @@
   {'N', N_("Population"),       get_population,  population_to_text,  TRUE },
   {'A', N_("Land Area"),        get_landarea,    area_to_text,        TRUE },
   {'S', N_("Settled Area"),     get_settledarea, area_to_text,        TRUE },
-  {'R', N_("Research Speed"),   get_research,    percent_to_text,     TRUE },
+  {'R', N_("Research Speed"),   get_research,    science_to_text,     TRUE },
   {'L', N_("Literacy"),         get_literacy,    percent_to_text,     TRUE },
   {'P', N_("Production"),       get_production,  production_to_text,  TRUE },
   {'E', N_("Economics"),        get_economics,   economics_to_text,   TRUE },
@@ -353,7 +354,7 @@
 
 static int get_research(struct player *pplayer)
 {
-  return (pplayer->score.techout * 100) / total_bulbs_required(pplayer);
+  return pplayer->score.techout;
 }
 
 static int get_literacy(struct player *pplayer)
@@ -560,6 +561,11 @@
   return value_units(value, _(" M goods"));
 }
 
+static const char *science_to_text(int value)
+{
+  return value_units(value, _(" bulbs"));
+}
+
 static const char *mil_service_to_text(int value)
 {
   return value_units(value, PL_(" month", " months", value));
Index: server/techtools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/techtools.c,v
retrieving revision 1.12
diff -u -r1.12 techtools.c
--- server/techtools.c  30 Jun 2005 08:07:10 -0000      1.12
+++ server/techtools.c  30 Jun 2005 16:43:38 -0000
@@ -460,6 +460,13 @@
   int excessive_bulbs;
   struct player_research *research = get_player_research(plr);
 
+  /* At the end of the turn an unset tech target will lead to one being
+   * picked randomly.  You can't save bulbs over between turns. */
+  if (research->researching == A_UNSET) {
+    choose_random_tech(plr);
+  }
+  assert(research->researching != A_UNSET);
+
   /* count our research contribution this turn */
   plr->bulbs_last_turn += bulbs;
 
@@ -468,7 +475,7 @@
   excessive_bulbs =
       (research->bulbs_researched - total_bulbs_required(plr));
 
-  if (excessive_bulbs >= 0 && research->researching != A_UNSET) {
+  if (excessive_bulbs >= 0) {
     tech_researched(plr);
     if (research->researching != A_UNSET) {
       update_tech(plr, 0);