[freeciv-ai] (PR#2634) ai_military_attack() bug#2

["Gregory Berkolaiko via RT" <rt@xxxxxxxxxxxxxx>]

On Mon, 23 Dec 2002, Per I. Mathisen via RT wrote:

> On Mon, 23 Dec 2002, Gregory Berkolaiko via RT wrote:
> > > It is possible to attack and die, and still do another loop in
> > > ai_military_attack, leading to a core dump. If we add the first check, we
> >
> > This looks quite critical to me. Why haven't I seen any coredumps yet?
> 
> Because bug #1 ensured this code was almost never called.

right.  Probably never...

> > > can remove the latter. This should be fixed for S1_14 as well as head, but
> > > for S1_14 we can keep the latter check as well to be absolutely sure.
> >
> > So beta4 ? ;)
> 
> No, by keeping both checks we do it failsafe. So a new beta won't be
> needed.

I think we might should let it be.  Maybe add another check, but that's 
it.  AI was functioning before, so it's ok...

On IRC people are not terribly happy about beta3...

For CVS, I started making your changes and got carried away.  Attached is 
the result.  It makes sure ai_unit_attack returns a bool value, 
ai_unit_rampage also returns a bool rather than nobody knows why punit.
Also reodered things for uniformity.

Withouth the 
-          || could_unit_move_to_tile(punit, dest_x, dest_y) == 0) {
+          || !(could_unit_move_to_tile(punit, dest_x, dest_y) == 0)) {
change, the savegames are identical (it took me an hour to verify it, 
damn, forgot to get upto-date savegames and also forgot to recompile at 
the right moment).

With this change it compiles and runs.  AI seems to be normal erratic and 
slow self.

G.

? pitfight.sh
? pitfight_g.sh
? saves
Index: ai/aitools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aitools.c,v
retrieving revision 1.68
diff -u -r1.68 aitools.c
--- ai/aitools.c        2002/12/21 11:44:00     1.68
+++ ai/aitools.c        2002/12/23 19:07:23
@@ -270,10 +270,11 @@
 /**************************************************************************
   Move and attack with an ai unit. We do not wait for server reply.
 **************************************************************************/
-void ai_unit_attack(struct unit *punit, int x, int y)
+bool ai_unit_attack(struct unit *punit, int x, int y)
 {
   struct packet_move_unit pmove;
   int sanity = punit->id;
+  bool alive;
 
   assert(punit != NULL);
   assert(unit_owner(punit)->ai.control);
@@ -285,12 +286,14 @@
   pmove.unid = punit->id;
   handle_unit_activity_request(punit, ACTIVITY_IDLE);
   handle_move_unit(unit_owner(punit), &pmove);
+  alive = (find_unit_by_id(sanity) != NULL);
 
-  if (find_unit_by_id(sanity) && same_pos(x, y, punit->x, punit->y)) {
-    if (has_bodyguard(punit)) {
-      ai_unit_bodyguard_move(punit->ai.bodyguard, x, y);
-    }
+  if (alive && same_pos(x, y, punit->x, punit->y)
+      && has_bodyguard(punit)) {
+    ai_unit_bodyguard_move(punit->ai.bodyguard, x, y);
   }
+
+  return alive;
 }
 
 /**************************************************************************
Index: ai/aitools.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aitools.h,v
retrieving revision 1.31
diff -u -r1.31 aitools.h
--- ai/aitools.h        2002/12/18 19:05:21     1.31
+++ ai/aitools.h        2002/12/23 19:07:23
@@ -37,7 +37,7 @@
 void ai_unit_new_role(struct unit *punit, enum ai_unit_task task, int x, int 
y);
 
 bool ai_unit_make_homecity(struct unit *punit, struct city *pcity);
-void ai_unit_attack(struct unit *punit, int x, int y);
+bool ai_unit_attack(struct unit *punit, int x, int y);
 bool ai_unit_move(struct unit *punit, int x, int y);
 
 struct city *dist_nearest_city(struct player *pplayer, int x, int y,
Index: ai/aiunit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
retrieving revision 1.241
diff -u -r1.241 aiunit.c
--- ai/aiunit.c 2002/12/21 09:39:23     1.241
+++ ai/aiunit.c 2002/12/23 19:07:24
@@ -65,7 +65,7 @@
 static void ai_military_findjob(struct player *pplayer,struct unit *punit);
 static void ai_military_gohome(struct player *pplayer,struct unit *punit);
 static void ai_military_attack(struct player *pplayer,struct unit *punit);
-static struct unit *ai_military_rampage(struct unit *punit, int threshold);
+static bool ai_military_rampage(struct unit *punit, int threshold);
 
 static int unit_move_turns(struct unit *punit, int x, int y);
 static bool unit_role_defender(Unit_Type_id type);
@@ -1054,19 +1054,22 @@
   Find and kill anything adjacent to us that we don't like with a 
   given threshold until we have run out of juicy targets or movement. 
   Wraps ai_military_findvictim().
+  Returns TRUE if survived the rampage session.
 **************************************************************************/
-static struct unit *ai_military_rampage(struct unit *punit, int threshold)
+static bool ai_military_rampage(struct unit *punit, int threshold)
 {
-  int x, y, id = punit->id;
+  int x, y;
   int count = punit->moves_left + 1; /* break any infinite loops */
 
-  while (punit && punit->moves_left > 0 && count-- > 0
+  while (punit->moves_left > 0 && count-- > 0
          && ai_military_findvictim(punit, &x, &y) >= threshold) {
-    ai_unit_attack(punit, x, y);
-    punit = find_unit_by_id(id);
+    if (!ai_unit_attack(punit, x, y)) {
+      /* Died */
+      return FALSE;
+    }
   }
   assert(count >= 0);
-  return punit;
+  return TRUE;
 }
 
 /*************************************************************************
@@ -1109,7 +1112,7 @@
     }
   } else {
     /* I had these guys set to just fortify, which is so dumb. -- Syela */
-    punit = ai_military_rampage(punit, 40 * SHIELD_WEIGHTING);
+    (void) ai_military_rampage(punit, 40 * SHIELD_WEIGHTING);
   }
 
   /* is this insanity supposed to be a sanity check? -- per */
@@ -1568,7 +1571,7 @@
       freelog(LOG_DEBUG, "INHOUSE. GOTO AI_NONE(%d)", punit->id);
       ai_unit_new_role(punit, AIUNIT_NONE, -1, -1);
       /* aggro defense goes here -- Syela */
-      punit = ai_military_rampage(punit, 2); /* 2 is better than pillage */
+      (void) ai_military_rampage(punit, 2); /* 2 is better than pillage */
     } else {
       UNIT_LOG(LOG_DEBUG, punit, "GOHOME");
       (void) ai_unit_goto(punit, pcity->x, pcity->y);
@@ -1961,7 +1964,7 @@
   /* Main attack loop */
   do {
     /* First find easy adjacent enemies; 2 is better than pillage */
-    if (!(punit = ai_military_rampage(punit, 2))) {
+    if (!ai_military_rampage(punit, 2)) {
       return; /* we died */
     }
 
@@ -1975,28 +1978,32 @@
     /* Then find enemies the hard way */
     find_something_to_kill(pplayer, punit, &dest_x, &dest_y);
     if (!same_pos(punit->x, punit->y, dest_x, dest_y)) {
-     int repeat = 0;
-     while (repeat < 2) {
-      repeat++;
+     int repeat;
+
+     for(repeat = 0; repeat < 2; repeat++) {
+
       if (!is_tiles_adjacent(punit->x, punit->y, dest_x, dest_y)
           || !can_unit_attack_tile(punit, dest_x, dest_y)
-          || could_unit_move_to_tile(punit, dest_x, dest_y) == 0) {
+          || !(could_unit_move_to_tile(punit, dest_x, dest_y) == 0)) {
         /* Can't attack or move usually means we are adjacent but
          * on a ferry. This fixes the problem (usually). */
-        int i = ai_military_gothere(pplayer, punit, dest_x, dest_y);
-        if (i <= 0) {
-          return; /* dead or stuck */
-        } else {
-          UNIT_LOG(LOG_DEBUG, punit, "mil att gothere -> %d, %d", 
-                   dest_x, dest_y);
+        UNIT_LOG(LOG_DEBUG, punit, "mil att gothere -> %d, %d", 
+                 dest_x, dest_y);
+        if (ai_military_gothere(pplayer, punit, dest_x, dest_y) <= 0) {
+          /* Died or got stuck */
+          return;
         }
       } else {
         /* Close combat. fstk sometimes want us to attack an adjacent
          * enemy that rampage wouldn't */
         UNIT_LOG(LOG_DEBUG, punit, "mil att bash -> %d, %d", dest_x, dest_y);
-        ai_unit_attack(punit, dest_x, dest_y);
+        if (!ai_unit_attack(punit, dest_x, dest_y)) {
+          /* Died */
+          return;
+        }
       }
-     } /* while */
+
+     } /* for-loop */
     } else {
       /* FIXME: This happens a bit too often! */
       UNIT_LOG(LOG_DEBUG, punit, "fstk didn't find us a worthy target!");
@@ -2004,9 +2011,6 @@
       ct = 0;
     }
 
-    if (!(punit = find_unit_by_id(id))) {
-      return; /* we died */
-    }
     ct--; /* infinite loops from railroads must be stopped */
   } while (punit->moves_left > 0 && ct > 0);
 
@@ -2272,7 +2276,7 @@
   if (pcity) {
     /* rest in city until the hitpoints are recovered, but attempt
        to protect city from attack */
-    if ((punit = ai_military_rampage(punit, 2))) {
+    if (ai_military_rampage(punit, 2)) {
       freelog(LOG_DEBUG, "%s's %s(%d) at (%d, %d) recovering hit points.",
               pplayer->name, unit_type(punit)->name, punit->id, punit->x,
               punit->y);
@@ -2282,7 +2286,7 @@
   } else {
     /* goto to nearest city to recover hit points */
     /* just before, check to see if we can occupy at enemy city undefended */
-    if (!(punit = ai_military_rampage(punit, 99999))) { 
+    if (!ai_military_rampage(punit, 99999)) { 
       return; /* oops, we died */
     }