mps
mps copied to clipboard
The shield is not checked in cool builds
Change d82da0e1401d5541a9fbba78ca2bd5b03cb0d6c1 refactored the shield into its own structure, ShieldStruct, which makes sense in terms of abstractions. Various checks that were part of GlobalCheck were moved to the new ShieldCheck and shield checking was then done in the usual way by a CHECKD . The trouble with this change is that ShieldCheck is never reached in normal builds.
There are several problems:
- CHECKD does not recurse in a cool (debugging) build, because by default that build uses CheckLevelSHALLOW. This means that ArenaCheck does not invoke ShieldCheck.
- Important shield functions such as ShieldEnter take an arena instead of a shield, then check that. But that doesn't invoke ShieldCheck (see above).
- We do not regularly run tests with CheckLevelDEEP which would cause CHECKD to recurse.
With several possible solutions:
- CHECKD might be the wrong thing for a single-instance inlined structure such as ShieldStruct within ArenaStruct.
- Shield functions should take a shield and check it.
- We should build and run a deep checking build on at least one platform.
On this last point, a build with make -f lii6ll.gmk VARIETY=cool CFLAGSDEBUG='-O0 -g3 -DCHECKLEVEL=CheckLevelDEEP' testci
currently fails in several tests.
This issue was discovered during a walkthrough of https://github.com/Ravenbrook/mps/commit/e47bc4c04 with @rptb1 and @thejayps .
A similar problem may apply to HistoryStruct introduced by 811d535a8ba89aeff044632a52eff33ae1de11f0 and possibly others.
This might have been found sooner if we had better coverage checks. See #139 .
2. Shield functions should take a shield and check it.
Functions like ShieldLower start like this https://github.com/Ravenbrook/mps/blob/179341b6cc2d9ab623ea53d10914d66c3668d47d/code/shield.c#L561-L566
but could be changed to start like this:
void (ShieldLower)(Shield shield, Seg seg, AccessSet mode)
{
Arena arena;
AVERT(Shield, shield);
arena = ShieldArena(shield);
which introduces a very slight inconvenience in, e.g. https://github.com/Ravenbrook/mps/blob/179341b6cc2d9ab623ea53d10914d66c3668d47d/code/seg.c#L197
which will need to say:
ShieldLower(ArenaShield(arena), seg, seg->sm);
etc.
Shield functions taking an arena is a minor inconsistency that has bitten us.