[Freeciv-Dev] (PR#9694) valgrind warning in comtemplate_terrain_improvements

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9694 >

In PR#9637 Marcelo reported this valgrind warning:

==22018==
==22018== Conditional jump or move depends on uninitialised value(s)
==22018== at 0x811805B: get_activity_text (unit.c:605)
==22018== by 0x80A9F6A: contemplate_terrain_improvements
(settlers.c:1524)
==22018== by 0x8126DF3: ai_manage_cities (aicity.c:591)
==22018== by 0x812B59F: ai_do_last_activities (aihand.c:384)

I haven't reproduced this (it would take a very long time for valgrind 
to run that far on the autogame he provided) but it seems obvious enough 
why it is happening.  If evaluate_improvements() returns 0 then best_act 
is not set.  The caller is supposed to check the return value but in 
this case we don't, and so we get a bogus activity check.  (Of course 
this is only in a log; it may not even be called unless you have city 
debugging enabled.  I dunno.)

This patch fixes this by having evaluate_improvements set the activity 
to ACTIVITY_IDLE if the goodness is 0.  This seems easier than having 
the callers set it (which one of them did already) and better than 
having a conditional in contemplate_terrain_improvements _just_ for the log.

I also added an assertion to get_activity_text().  If passed a bad 
activity it should fail the assert.  This should help us track down any 
future bugs here (which are likely to be serious, even though this one 
wasn't).

jason

? stamp-h1.in
? stamp-h2.in
Index: common/unit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.c,v
retrieving revision 1.214
diff -u -r1.214 unit.c
--- common/unit.c       2 Aug 2004 16:59:14 -0000       1.214
+++ common/unit.c       13 Aug 2004 17:29:16 -0000
@@ -641,6 +641,7 @@
     break;
   }
 
+  assert(0);
   return _("Unknown");
 }
 
Index: server/settlers.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/settlers.c,v
retrieving revision 1.195
diff -u -r1.195 settlers.c
--- server/settlers.c   13 Aug 2004 15:59:13 -0000      1.195
+++ server/settlers.c   13 Aug 2004 17:29:16 -0000
@@ -907,7 +907,8 @@
 /**************************************************************************
 Finds tiles to improve, using punit.
 How nice a tile it finds is returned. If it returns >0 gx,gy indicates the
-tile it has chosen, and bestact indicates the activity it wants to do.
+tile it has chosen, and bestact indicates the activity it wants to do.  If
+0 is returned then there are no activities available.
 **************************************************************************/
 static int evaluate_improvements(struct unit *punit,
                                 enum unit_activity *best_act, 
@@ -1051,6 +1052,11 @@
            "Settler %d@(%d,%d) wants to %s at (%d,%d) with desire %d",
            punit->id, punit->x, punit->y, get_activity_text(*best_act),
            *gx, *gy, best_newv);
+  } else {
+    /* Fill in dummy values.  The callers should check if the return value
+     * is > 0 but this will avoid confusing them. */
+    *best_act = ACTIVITY_IDLE;
+    *gx = *gy = -1;
   }
 
   return best_newv;
@@ -1064,7 +1070,7 @@
 {
   struct cityresult result;
   int best_impr = 0;            /* best terrain improvement we can do */
-  enum unit_activity best_act = ACTIVITY_IDLE; /* compat. kludge */
+  enum unit_activity best_act;
   int gx = -1, gy = -1;
   struct ai_data *ai = ai_data_get(pplayer);