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

[crucible] Add deadman timeout for online solver actions #617

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kquick
Copy link
Member

@kquick kquick commented Jan 17, 2021

Fixes #439 by implementing
a parallel timeout operation (at 1 second longer than the timeout
requested of the solver itself) whose expiration causes the solver
process to be killed.

This helps with issue 439 because Yices encounters some internal
divergence issue that is not stopped by the specified timeout
value (demonstrable by running the crux-llvm
test-data/golden/strlen_test2 with the Yices solver option).

This also helps provide timeout management for solvers that do not
provide a built-in timeout capability (like Boolector).

Fixes #439 by implementing
a parallel timeout operation (at 1 second longer than the timeout
requested of the solver itself) whose expiration causes the solver
process to be killed.

This helps with issue 439 because Yices encounters some internal
divergence issue that is not stopped by the specified timeout
value (demonstrable by running the crux-llvm
test-data/golden/strlen_test2 with the Yices solver option).

This also helps provide timeout management for solvers that do not
provide a built-in timeout capability (like Boolector).
@kquick
Copy link
Member Author

kquick commented Jan 17, 2021

Note that this requires GaloisInc/what4#83

then doAction
else let deadmanTimeoutPeriod = (fromInteger $
getGoalTimeoutInMilliSeconds
(solverGoalTimeout p) + 1000) * 1000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This multiplication by 1000 is suspicious... is threadDelay in microseconds? If so, let's comment about that.

Copy link
Contributor

@robdockins robdockins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of this, although I'm always a little suspicious of any changes to the process handling, as it's easy to have unintended effects. Have you tested that everything works as it should? Solvers get killed when they need to and not when they don't? Do keyboard interrupts still work correctly, etc?

Copy link
Contributor

@atomb atomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really happy to have this functionality, but would also second @robdockins concern. We should make sure we test this as thoroughly as possible.

@travitch
Copy link
Contributor

A note we discussed on a bug triage call: last time I was working on timeouts for yices, I think I saw that it has different handling of signals if you are using the MCSAT backend vs the standard backend. I think that it could swallow signals sometimes, which might be something we need to report upstream.

@kquick kquick marked this pull request as draft March 18, 2021 23:25
@kquick
Copy link
Member Author

kquick commented Mar 18, 2021

Just a note, the timeout should be handled at the what4 level, although there may be associated handling here. There is a what4 change in progress, but not yet submitted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

per-goal timeout option doesn't seem to be working for yices
4 participants