Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The shield is not checked in cool builds #205

Open
rptb1 opened this issue Mar 29, 2023 · 2 comments · May be fixed by #209
Open

The shield is not checked in cool builds #205

rptb1 opened this issue Mar 29, 2023 · 2 comments · May be fixed by #209
Assignees
Labels
essential Will cause failure to meet customer requirements. Assign resources.

Comments

@rptb1
Copy link
Member

rptb1 commented Mar 29, 2023

Change d82da0e 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:

  1. CHECKD does not recurse in a cool (debugging) build, because by default that build uses CheckLevelSHALLOW. This means that ArenaCheck does not invoke ShieldCheck.
  2. Important shield functions such as ShieldEnter take an arena instead of a shield, then check that. But that doesn't invoke ShieldCheck (see above).
  3. We do not regularly run tests with CheckLevelDEEP which would cause CHECKD to recurse.

With several possible solutions:

  1. CHECKD might be the wrong thing for a single-instance inlined structure such as ShieldStruct within ArenaStruct.
  2. Shield functions should take a shield and check it.
  3. 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 e47bc4c04 with @rptb1 and @thejayps .

A similar problem may apply to HistoryStruct introduced by 811d535 and possibly others.

@rptb1
Copy link
Member Author

rptb1 commented Mar 29, 2023

This might have been found sooner if we had better coverage checks. See #139 .

@rptb1
Copy link
Member Author

rptb1 commented Mar 29, 2023

2. Shield functions should take a shield and check it.

Functions like ShieldLower start like this

mps/code/shield.c

Lines 561 to 566 in 179341b

void (ShieldLower)(Arena arena, Seg seg, AccessSet mode)
{
Shield shield;
AVERT(Arena, arena);
shield = ArenaShield(arena);

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.

mps/code/seg.c

Line 197 in 179341b

ShieldLower(arena, seg, seg->sm);

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.

rptb1 added a commit that referenced this issue Mar 30, 2023
…act 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.
@rptb1 rptb1 self-assigned this Mar 31, 2023
rptb1 added a commit that referenced this issue Mar 31, 2023
… the shield gets checked. Also makes the shield code smaller and more natural. Fixes GitHub issue #205 <#205>.
@rptb1 rptb1 linked a pull request Mar 31, 2023 that will close this issue
@rptb1 rptb1 linked a pull request Mar 31, 2023 that will close this issue
@thejayps thejayps added the essential Will cause failure to meet customer requirements. Assign resources. label Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
essential Will cause failure to meet customer requirements. Assign resources.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants