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

Testing with deep checking reports false positives #208

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

Testing with deep checking reports false positives #208

rptb1 opened this issue Mar 30, 2023 · 3 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 30, 2023

#205 says:

... a build with make -f lii6ll.gmk VARIETY=cool CFLAGSDEBUG='-O0 -g3 -DCHECKLEVEL=CheckLevelDEEP' testci currently fails in several tests.

and also:

We should build and run a deep checking build on at least one platform.

Several steps are needed:

  1. Introduce an easier way to build and test with deep checking.
  2. Add testing with deep checking to CI so that we catch problems early.
  3. Analyse and fix current test failures with deep checking.

I have found false positives in deep checking errors have been introduced in various changes:

  1. branch/2016-04-16/trace-gens finishes the ring of generations but goes on to use the arena with an invalid trace.
  2. branch/2020-08-31/walk synthesizes a trace that fails to pass TraceCheck because it has no generations.
  3. Deep checking has an infinite recursion in ArenaCheck via ShieldCheck, which checks some segments, which check their addresses via ChunkOfAddr, which checks the arena with ArenaCheck. I haven't discovered when this was introduced yet.
  4. ArenaRootsWalk synthesizes an invalid trace in TraceStateINIT that is flipped. ArenaRootsWalk dates back to 1999 so it was introduced by some other change. I haven't discovered when yet.

There may be more. I have yet to run the entire test suite.

So far I have no evidence that any of these would cause failures in production, but deep checking is a valuable tool when, e.g. working on subtle parts of the MPS like the shield, and we ought to fix it up.

@rptb1
Copy link
Member Author

rptb1 commented Mar 30, 2023

We should check whether CHECKLEVEL_DYNAMIC (using a variable to control checking levels) has a reasonable overhead and perhaps use it by default for the cool variety. That would avoid proliferation of varieties and allow for easy switching to deep checking without rebuilding.

@rptb1
Copy link
Member Author

rptb1 commented Mar 30, 2023

We should probably include a deep checking run in our release procedure along with the other testing methods. See also #125

@rptb1
Copy link
Member Author

rptb1 commented Mar 30, 2023

I was investigating the process issues around this, and thought I'd note here the related issues I found.

  • job003302 mps_arena_create fails with deep consistency checks
  • job003692 MPS deep checking fails at bootstrap
  • job004026 Failed shield assertions with DEEP checking

@rptb1 rptb1 linked a pull request Mar 30, 2023 that will close this issue
@rptb1 rptb1 linked a pull request Mar 31, 2023 that will close this issue
@rptb1 rptb1 self-assigned this Mar 31, 2023
@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