[Freeciv-Dev] (PR#9862) Nasty bug in look_for_charge()

["Per I. Mathisen" <per@xxxxxxxxxxx>]

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

Using bitshift operators on numbers that could be negative is a really bad
idea. This is done in look_for_charge(), which sometimes gives totally
nonsensical targets for AI defense, and sends AI defensive units runnings
around for no apparent reason.

Attached patch fixes this problem. This problem also exists in S1_14, and
the patch should probably be backported.

We (I) should perhaps audit all use of bitshift operators in the AI and
check if there are more of this kind of error.

  - Per

Index: ai/advmilitary.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/advmilitary.c,v
retrieving revision 1.170
diff -u -r1.170 advmilitary.c
--- ai/advmilitary.c    14 Aug 2004 21:29:42 -0000      1.170
+++ ai/advmilitary.c    29 Aug 2004 18:45:05 -0000
@@ -187,8 +187,8 @@
   } unit_list_iterate_end;
 
   if (defense > 1<<12) {
-    freelog(LOG_VERBOSE, "Very large defense in assess_defense_quadratic: %d 
in %s",
-            defense, pcity->name);
+    CITY_LOG(LOG_VERBOSE, pcity, "Overflow danger in assess_defense_quadratic:"
+             " %d", defense);
     if (defense > 1<<15) {
       defense = 1<<15; /* more defense than we know what to do with! */
     }
Index: ai/aiunit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
retrieving revision 1.328
diff -u -r1.328 aiunit.c
--- ai/aiunit.c 25 Aug 2004 18:24:18 -0000      1.328
+++ ai/aiunit.c 29 Aug 2004 18:45:05 -0000
@@ -936,7 +936,11 @@
       continue; 
     }
     dist = unit_move_turns(punit, mycity->x, mycity->y);
-    def = (mycity->ai.danger - assess_defense_quadratic(mycity)) >> dist;
+    def = (mycity->ai.danger - assess_defense_quadratic(mycity));
+    if (def <= 0) {
+      continue;
+    }
+    def = def >> dist;
     if (def > best && ai_fuzzy(pplayer, TRUE)) { 
       *acity = mycity; 
       best = def; 
@@ -944,8 +948,11 @@
    } city_list_iterate_end;
   }
 
-  UNIT_LOG(LOGLEVEL_BODYGUARD, punit, "was looking for charge, best want %d",
-           best * 100 / toughness);
+  UNIT_LOG(LOGLEVEL_BODYGUARD, punit, "look_for_charge, best=%d, "
+           "type=%s(%d,%d)", best * 100 / toughness, *acity ? (*acity)->name
+           : (*aunit ? unit_name((*aunit)->type) : ""), 
+           *acity ? (*acity)->x : (*aunit ? (*aunit)->x : 0),
+           *acity ? (*acity)->y : (*aunit ? (*aunit)->y : 0));
   
   return ((best * 100) / toughness);
 }