Correcting false assertion about shield queue length and enforcing exact check by better accounting. Fixes job004026 <https://www.ravenbrook.com/project/mps/issue/?action=job&job=job004026>. Mitigating GitHub issue #205 <#205> with extra shield type checks.

Author: rptb1

code/mpmst.h

@@ -669,13 +669,14 @@ typedef struct ShieldStruct {
   Sig sig;           /* design.mps.sig.field */
   BOOLFIELD(inside); /* <design/shield#.def.inside> */
   BOOLFIELD(suspended); /* mutator suspended? */
-  BOOLFIELD(queuePending); /* queue insertion pending? */
   Seg *queue;        /* queue of unsynced segs */
   Count length;      /* number of elements in shield queue */
   Index next;        /* next free element in shield queue */
   Index limit;       /* high water mark for cache usage */
   Count depth;       /* sum of depths of all segs */
   Count unsynced;    /* number of unsynced segments */
+  Count unqueued;    /* number of unsynced unqueued segments */
+  Count synced;      /* number of synced queued segments */
   Count holds;       /* number of holds */
   SortStruct sortStruct; /* workspace for queue sort */
 } ShieldStruct;

code/shield.c

@@ -19,13 +19,14 @@ void ShieldInit(Shield shield)
 {
   shield->inside = FALSE;
   shield->suspended = FALSE;
-  shield->queuePending = FALSE;
   shield->queue = NULL;
   shield->length = 0;
   shield->next = 0;
   shield->limit = 0;
   shield->depth = 0;
   shield->unsynced = 0;
+  shield->unqueued = 0;
+  shield->synced = 0;
   shield->holds = 0;
   shield->sig = ShieldSig;
 }
@@ -55,6 +56,8 @@ void ShieldFinish(Shield shield)
 
   AVER(shield->depth == 0);
   AVER(shield->unsynced == 0);
+  AVER(shield->unqueued == 0);
+  AVER(shield->synced == 0);
   AVER(shield->holds == 0);
   shield->sig = SigInvalid;
 }
@@ -69,6 +72,8 @@ Bool ShieldCheck(Shield shield)
   CHECKL(shield->queue == NULL || shield->length > 0);
   CHECKL(shield->limit <= shield->length);
   CHECKL(shield->next <= shield->limit);
+  CHECKL(shield->unqueued <= shield->unsynced);
+  CHECKL(shield->synced <= shield->limit);
 
   /* The mutator is not suspended while outside the shield
      <design/shield#.inv.outside.running>. */
@@ -85,11 +90,11 @@ Bool ShieldCheck(Shield shield)
   /* There are no unsynced segments when we're outside the shield. */
   CHECKL(shield->inside || shield->unsynced == 0);
 
-  /* Every unsynced segment should be on the queue, because we have to
-     remember to sync it before we return to the mutator. */
-  CHECKL(shield->limit + shield->queuePending >= shield->unsynced);
+  /* The queue contains exactly all the unsynced segments that aren't
+     unqueued, and the queued synced segments. */
+  CHECKL(shield->limit == shield->unsynced - shield->unqueued + shield->synced);
 
-  /* The mutator is suspeneded if there are any holds. */
+  /* The mutator is suspended if there are any holds. */
   CHECKL(shield->holds == 0 || shield->suspended);
 
   /* This is too expensive to check all the time since we have an
@@ -105,7 +110,8 @@ Bool ShieldCheck(Shield shield)
       if (!SegIsSynced(seg))
         ++unsynced;
     }
-    CHECKL(unsynced + shield->queuePending == shield->unsynced);
+    CHECKL(unsynced == shield->limit - shield->synced);
+    CHECKL(unsynced + shield->unqueued == shield->unsynced);
   }
 #endif
 
@@ -125,6 +131,8 @@ Res ShieldDescribe(Shield shield, mps_lib_FILE *stream, Count depth)
                "  next      $U\n", (WriteFU)shield->next,
                "  length    $U\n", (WriteFU)shield->length,
                "  unsynced  $U\n", (WriteFU)shield->unsynced,
+               "  unqueued  $U\n", (WriteFU)shield->unqueued,
+               "  synced    $U\n", (WriteFU)shield->synced,
                "  holds     $U\n", (WriteFU)shield->holds,
                "} Shield $P\n",    (WriteFP)shield,
                NULL);
@@ -168,11 +176,22 @@ static void shieldSetSM(Shield shield, Seg seg, AccessSet mode)
     if (SegIsSynced(seg)) {
       SegSetSM(seg, mode);
       ++shield->unsynced;
+      if (seg->queued) {
+        AVER(shield->synced > 0);
+        --shield->synced;
+      } else
+        ++shield->unqueued;
     } else {
       SegSetSM(seg, mode);
       if (SegIsSynced(seg)) {
         AVER(shield->unsynced > 0);
         --shield->unsynced;
+        if (seg->queued)
+          ++shield->synced;
+        else {
+          AVER(shield->unqueued > 0);
+          --shield->unqueued;
+        }
       }
     }
   }
@@ -187,11 +206,22 @@ static void shieldSetPM(Shield shield, Seg seg, AccessSet mode)
     if (SegIsSynced(seg)) {
       SegSetPM(seg, mode);
       ++shield->unsynced;
+      if (seg->queued) {
+        AVER(shield->synced > 0);
+        --shield->synced;
+      } else
+        ++shield->unqueued;
     } else {
       SegSetPM(seg, mode);
       if (SegIsSynced(seg)) {
         AVER(shield->unsynced > 0);
         --shield->unsynced;
+        if (seg->queued)
+          ++shield->synced;
+        else {
+          AVER(shield->unqueued > 0);
+          --shield->unqueued;
+        }
       }
     }
   }
@@ -220,9 +250,11 @@ static void shieldSync(Shield shield, Seg seg)
   SHIELD_AVERT_CRITICAL(Seg, seg);
 
   if (!SegIsSynced(seg)) {
+    /* TODO: Could assert something about the unsync count going down. */
     shieldSetPM(shield, seg, SegSM(seg));
     ProtSet(SegBase(seg), SegLimit(seg), SegPM(seg));
   }
+  AVER_CRITICAL(SegIsSynced(seg));
 }
 
 
@@ -239,6 +271,7 @@ static void shieldSuspend(Arena arena)
 
   AVERT(Arena, arena);
   shield = ArenaShield(arena);
+  AVERT(Shield, shield);
   AVER(shield->inside);
 
   if (!shield->suspended) {
@@ -275,6 +308,7 @@ void (ShieldRelease)(Arena arena)
 
   AVERT(Arena, arena);
   shield = ArenaShield(arena);
+  AVERT(Shield, shield);
   AVER(shield->inside);
   AVER(shield->suspended);
 
@@ -318,6 +352,11 @@ static Seg shieldDequeue(Shield shield, Index i)
   AVER(seg->queued);
   shield->queue[i] = NULL; /* to ensure it can't get re-used */
   seg->queued = FALSE;
+  if (SegIsSynced(seg)) {
+    AVER(shield->synced > 0);
+    --shield->synced;
+  } else
+    ++shield->unqueued;
   return seg;
 }
 
@@ -444,11 +483,15 @@ static void shieldQueue(Arena arena, Seg seg)
   /* <design/trace#.fix.noaver> */
   AVERT_CRITICAL(Arena, arena);
   shield = ArenaShield(arena);
+  AVERT_CRITICAL(Shield, shield);
   SHIELD_AVERT_CRITICAL(Seg, seg);
 
   if (SegIsSynced(seg) || seg->queued)
     return;
 
+  /* This segment is unsynced and not queued. */
+  AVER(shield->unqueued > 0);
+
   if (SegIsExposed(seg)) {
     /* This can occur if the mutator isn't suspended, we expose a
        segment, then raise the shield on it.  In this case, the
@@ -519,6 +562,7 @@ static void shieldQueue(Arena arena, Seg seg)
   shield->queue[shield->next] = seg;
   ++shield->next;
   seg->queued = TRUE;
+  --shield->unqueued;
 
   if (shield->next >= shield->limit)
     shield->limit = shield->next;
@@ -539,16 +583,14 @@ void (ShieldRaise)(Arena arena, Seg seg, AccessSet mode)
   SHIELD_AVERT(Seg, seg);
   AVERT(AccessSet, mode);
   shield = ArenaShield(arena);
-  AVER(!shield->queuePending);
-  shield->queuePending = TRUE;
+  AVERT(Shield, shield);
 
   /* <design/shield#.inv.prot.shield> preserved */
   shieldSetSM(ArenaShield(arena), seg, BS_UNION(SegSM(seg), mode));
 
   /* Ensure <design/shield#.inv.unsynced.suspended> and
      <design/shield#.inv.unsynced.depth> */
   shieldQueue(arena, seg);
-  shield->queuePending = FALSE;
 
   /* Check queue and segment consistency. */
   AVERT(Arena, arena);
@@ -591,6 +633,7 @@ void (ShieldEnter)(Arena arena)
 
   AVERT(Arena, arena);
   shield = ArenaShield(arena);
+  AVERT(Shield, shield);
   AVER(!shield->inside);
   AVER(shield->depth == 0);
   AVER(!shield->suspended);
@@ -620,28 +663,40 @@ static void shieldDebugCheck(Arena arena)
   Seg seg;
   Count queued = 0;
   Count depth = 0;
+  Count unqueued = 0;
+  Count unsynced = 0;
+  Count synced = 0;
 
   AVERT(Arena, arena);
   shield = ArenaShield(arena);
+  AVERT(Shield, shield);
   AVER(shield->inside || shield->limit == 0);
 
   if (SegFirst(&seg, arena))
     do {
       depth += SegDepth(seg);
       if (shield->limit == 0) {
         AVER(!seg->queued);
-        AVER(SegIsSynced(seg));
+        AVER(shield->unsynced > 0 || SegIsSynced(seg));
         /* You can directly set protections here to see if it makes a
            difference. */
         /* ProtSet(SegBase(seg), SegLimit(seg), SegPM(seg)); */
-      } else {
-        if (seg->queued)
-          ++queued;
       }
+      if (seg->queued)
+        ++queued;
+      if (!SegIsSynced(seg))
+        ++unsynced;
+      if (seg->queued && SegIsSynced(seg))
+        ++synced;
+      if (!seg->queued && !SegIsSynced(seg))
+        ++unqueued;
     } while(SegNext(&seg, arena, seg));
 
   AVER(depth == shield->depth);
   AVER(queued == shield->limit);
+  AVER(unsynced == shield->unsynced);
+  AVER(unqueued == shield->unqueued);
+  AVER(synced == shield->synced);
 }
 #endif
 
@@ -668,6 +723,7 @@ void (ShieldFlush)(Arena arena)
 
   AVERT(Arena, arena);
   shield = ArenaShield(arena);
+  AVERT(Shield, shield);
 #ifdef SHIELD_DEBUG
   shieldDebugCheck(arena);
 #endif
@@ -687,6 +743,7 @@ void (ShieldLeave)(Arena arena)
 
   AVERT(Arena, arena);
   shield = ArenaShield(arena);
+  AVERT(Shield, shield);
   AVER(shield->inside);
   AVER(shield->depth == 0); /* no pending covers */
   AVER(shield->holds == 0);
@@ -721,6 +778,7 @@ void (ShieldExpose)(Arena arena, Seg seg)
   /* <design/trace#.fix.noaver> */
   AVERT_CRITICAL(Arena, arena);
   shield = ArenaShield(arena);
+  AVERT(Shield, shield);
   AVER_CRITICAL(shield->inside);
 
   SegSetDepth(seg, SegDepth(seg) + 1);
@@ -747,6 +805,7 @@ void (ShieldCover)(Arena arena, Seg seg)
   /* <design/trace#.fix.noaver> */
   AVERT_CRITICAL(Arena, arena);
   shield = ArenaShield(arena);
+  AVERT_CRITICAL(Shield, shield);
   AVERT_CRITICAL(Seg, seg);
   AVER_CRITICAL(SegPM(seg) == AccessSetEMPTY);