Tests assert with SHIELD_DEBUG set

[rptb1]

Running tests with SHIELD_DEBUG enabled with make -f lii6ll.gmk VARIETY=cool CFLAGS='-DSHIELD_DEBUG' clean testci at current master (179341b) produces multiple assertion failures in various tests, all of the form:

shield.c:644: MPS ASSERTION FAILED: queued == shield->limit

It is likely that this assertion is incorrect, but this needs thorough investigation. Errors in shield invariants are very costly.

Found while checking the validity of e47bc4c04 .

We should also run a build with SHIELD_DEBUG set as part of CI, to prevent similar problems early.

[rptb1]

Some analysis.

Running gdb on mpsicv yields this backtrace:

#8  0x0000555555605a07 in mps_lib_assert_fail (file=0x55555562dbaf "shield.c", 
    line=644, condition=0x55555562df9c "queued == shield->limit") at mpsliban.c:87
#9  0x00005555555dd33e in shieldDebugCheck (arena=0x7ffff7ffa000) at shield.c:644
#10 0x00005555555dcfef in ShieldFlush (arena=0x7ffff7ffa000) at shield.c:672
#11 0x00005555555d1d5f in segAbsFinish (inst=0x7ffff6cad008) at seg.c:205
#12 0x00005555555d2b4c in gcSegFinish (inst=0x7ffff6cad008) at seg.c:1508

segAbsFinish removes the segment from its pool's segment ring before calling ShieldFlush

mps/code/seg.c

Lines 188 to 206 in 179341b

	RingRemove(SegPoolRing(seg));
	arena = PoolArena(SegPool(seg));
	/* TODO: It would be good to avoid deprotecting segments eagerly
	when we free them, especially if they're going to be
	unmapped. This would require tracking of protection independent
	of the existence of a SegStruct. */
	if (seg->sm != AccessSetEMPTY) {
	ShieldLower(arena, seg, seg->sm);
	}
	seg->rankSet = RankSetEMPTY;
	/* See <code/shield.c#shield.flush> */
	AVER(seg->depth == 0);
	if (seg->queued)
	ShieldFlush(PoolArena(SegPool(seg)));
	AVER(!seg->queued);

shieldDebugCheck uses SegFirst and SegNext to iterate over all segments, during which it counts which ones claim to be in the shield queue

mps/code/shield.c

Lines 628 to 641 in 179341b

	if (SegFirst(&seg, arena))
	do {
	depth += SegDepth(seg);
	if (shield->limit == 0) {
	AVER(!seg->queued);
	AVER(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;
	}
	} while(SegNext(&seg, arena, seg));

Some time ago, SegFirst/SegNext were changed to use the pool rings rather than the tract table to find segments.

This means that shieldDebugCheck does not see the segment that is being finished, and so its count is off by one.

Moving the RingRemove after ShieldFlush in segAbsFinish causes tests to stop asserting (and take much much longer to run!)

Note: We will eventually eliminate the tract table and have a tree of all segments, at which point SegFirst/SegNext can become independent of the pool ring.