-
Notifications
You must be signed in to change notification settings - Fork 73
Add vacuity detection in MIR and JVM backends. #2339
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
Conversation
* Extracted vacuity logic (checkAssumptionsForContradictions, etc.) into a new backend independent module: SAWCentral.Crucible.Common.Vacuity. * Hooked vacuity checks into: * mir_verify in SAWCentral.Crucible.MIR.Builtins * jvm_verify in SAWCentral.Crucible.JVM.Builtins * verifyMethodSpec and refineMethodSpec in SAWCentral.Crucible.LLVM.Builtins Fixes GaloisInc#2028
This PR addresses all prior review feedback:
I opted to open a new PR with a single additional commit rather than rewriting commit history to include the issue reference (#2028), to avoid disrupting references from the original PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking nice!
Do you know what is causing this test failure?
test2028_vacuity_detect: Expected 3 contradictions, found 0
test.sh: line 3: eval saw -j '/home/runner/work/saw-script/saw-script/intTests/jars/bcprov-jdk16-145.jar:/home/runner/work/saw-script/saw-script/intTests/jars/galois.jar:/home/runner/work/saw-script/saw-script/intTests/jars/org.sat4j.core.jar': No such file or directory
FAIL
intTests/IntegrationTest.hs:185:
expected: ExitSuccess
but got: ExitFailure 1
Use -p '/test2028_vacuity_detect/' to rerun this test only.
test.bc: test.c | ||
clang -emit-llvm -c -o $@ $< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that --detect-vacuity
is already tested in the LLVM backend as part of the test0064_detect_vacuity
test case. I'd favor removing the LLVM-specific parts from test2028_vacuity_detect
to avoid redundancy. (It might be worth referring to test0064_detect_vacuity
from test2028_vacuity_detect/README
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make the suggested changes to avoid redundancy.
As for the test failure, I haven't figured out exactly why the runner fails. I tested the case manually in a SAW Docker container built from my changes, and it worked as expected (see below). I suspect the test runner doesn't like how I invoke the SAW command in the script, maybe due to how $SAW is formatted or evaluated.
Unless you have a better suggestion, I'll split the test into two shell scripts (JVM and MIR) and rely on the existing LLVM test to cover that backend.
docker run -it --rm --entrypoint /bin/bash -v "$PWD":/workdir -v "$PWD/cryptol-specs":/cryptol-specs -v "$HOME/jdk8-classes":/jdk -w /workdir -e CRYPTOLPATH=/cryptol-specs -e CLASSPATH=/jdk:/workdir -e SAW=saw saw:vacuity
saw@194779f19baa:/workdir$ bash test.sh
[21:17:24.843] Loading file "test.saw"
[21:17:24.877] Verifying fun_c ...
[21:17:24.887] Contradiction detected! Computing minimal core of contradictory assumptions:
[21:17:24.902] ConditionMetadata {conditionLoc = ProgramLoc {plFunction = llvm_precond, plSourceLoc = test.saw:6:3}, conditionTags = fromList [], conditionType = "specification assertion", conditionContext = ""}: precondition
[21:17:24.902] Because of the contradiction, the following proofs may be vacuous.
[21:17:24.902] Simulating fun_c ...
[21:17:24.903] Checking proof obligations fun_c ...
[21:17:24.903] Proof succeeded! fun_c
[21:17:24.936] Verifying test/648686db::fun_rs[0] ...
[21:17:24.953] Contradiction detected! Computing minimal core of contradictory assumptions:
[21:17:24.967] ConditionMetadata {conditionLoc = ProgramLoc {plFunction = mir_precond, plSourceLoc = test.saw:14:3}, conditionTags = fromList [], conditionType = "specification assertion", conditionContext = ""}: precondition
[21:17:24.968] Because of the contradiction, the following proofs may be vacuous.
[21:17:24.968] Simulating test/648686db::fun_rs[0] ...
[21:17:24.968] Checking proof obligations test/648686db::fun_rs[0] ...
[21:17:24.968] Proof succeeded! test/648686db::fun_rs[0]
[21:17:25.050] Contradiction detected! Computing minimal core of contradictory assumptions:
[21:17:25.064] ConditionMetadata {conditionLoc = ProgramLoc {plFunction = jvm_precond, plSourceLoc = test.saw:22:3}, conditionTags = fromList [], conditionType = "specification assertion", conditionContext = ""}: precondition
[21:17:25.064] Because of the contradiction, the following proofs may be vacuous.
registering standard overrides
registering user-provided overrides
registering assumptions
simulating function
[21:17:25.073] Proof succeeded! fun_jvm
Found 3 expected contradictions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see what is going on now. You can reproduce the issue by running ./build.sh
followed by:
$ PATH=$(pwd)/bin:$PATH cabal run integration_tests -- -p test2028_vacuity_detect
Which is how the CI invokes the test suite.
The problem is this line of the test case:
n=$( "$SAW" --detect-vacuity test.saw 2>&1 | tee /dev/stderr | grep -c "Contradiction detected" ) |
This quotes the SAW
environment variable. When run through the integration_tests
test suite, SAW
is defined to be saw -j '/home/runner/work/saw-script/saw-script/intTests/jars/bcprov-jdk16-145.jar:/home/runner/work/saw-script/saw-script/intTests/jars/galois.jar:/home/runner/work/saw-script/saw-script/intTests/jars/org.sat4j.core.jar'
. This means that when you invoke "$SAW"
, you are actually invoking "saw -j ..."
, which causes the shell to treat everything between the double-quotes as one giant command name. This isn't what you want, since SAW
is intentionally including both the command and its arguments.
The simplest fix would be to remove the double quotes around "$SAW"
, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I fixed and tested it this morning. If all tests pass in github I will merge.
- Removed LLVM proof from test2028_vacuity_detect since vacuity detection for LLVM is already covered by test0064. - Fixed quoting of $SAW in test.sh to allow proper execution via test runner. Related to GaloisInc#2028.
@@ -0,0 +1,8 @@ | |||
This test checks for vacuous proofs in the MIR, and JVM backends. It skips | |||
LLVM as that is already tested in test0064. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLVM as that is already tested in test0064. | |
LLVM as that is already tested in test0064_detect_vacuity. |
Fixes #2028