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

Handle validation tasks inside CPAchecker #1078

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

marian-lingsch
Copy link
Contributor

@marian-lingsch marian-lingsch commented Sep 16, 2024

The goal of this MR is to handle validation tasks following the proposed format in SV-Benchmarks. A short description of the new option is given in the SV-Benchmarks README

The goal is to leave the previous behavior of CPAchecker untouched i.e. it still being able to validate witnesses given explicitly through extra parameters in a benchmark definition file. If both ways to pass witnesses, through the task-definition and through the benchmark file, are used then CPAchecker fails with an error, as should be expected.

@marian-lingsch marian-lingsch self-assigned this Sep 16, 2024
@PhilippWendler PhilippWendler marked this pull request as draft September 17, 2024 04:44
Copy link
Member

@PhilippWendler PhilippWendler left a comment

Choose a reason for hiding this comment

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

I marked the PR as draft as long as the witness option is still a proposal and not community agreed.

The PR should link to the documentation that has the definition of that option.

benchexec/tools/cpachecker.py Outdated Show resolved Hide resolved
benchexec/tools/cpachecker.py Outdated Show resolved Hide resolved
benchexec/tools/cpachecker.py Outdated Show resolved Hide resolved
@marian-lingsch
Copy link
Contributor Author

Thanks a lot for your comments!

They have now been addressed.

@PhilippWendler
Copy link
Member

There are several tool-info modules that build on top of the tool-info module of CPAchecker. Most of them do not override cmdline, so should be safe, but I think we should go through each of grep cmdline $(grep -Rl cpachecker benchexec/tools/) and check whether they need adjustments. In particular cpa-witness2test.py even calls _get_additional_options().

Marian Lingsch-Rosenfeld added 2 commits September 18, 2024 11:05
@marian-lingsch
Copy link
Contributor Author

There are several tool-info modules that build on top of the tool-info module of CPAchecker. Most of them do not override cmdline, so should be safe, but I think we should go through each of grep cmdline $(grep -Rl cpachecker benchexec/tools/) and check whether they need adjustments. In particular cpa-witness2test.py even calls _get_additional_options().

Thanks a lot for pointing this out!

Just went through all the different tools which use CPAchecker's tool info module. Only CPA-witness2test had to be adjusted, the others should not be affected. MetaVal also needs to be adapated, but that will be a problem for another PR.

@PhilippWendler
Copy link
Member

There are several tool-info modules that build on top of the tool-info module of CPAchecker. Most of them do not override cmdline, so should be safe, but I think we should go through each of grep cmdline $(grep -Rl cpachecker benchexec/tools/) and check whether they need adjustments. In particular cpa-witness2test.py even calls _get_additional_options().

Thanks a lot for pointing this out!

Just went through all the different tools which use CPAchecker's tool info module. Only CPA-witness2test had to be adjusted, the others should not be affected.

Hm, but now cpa-witness2test.py does not fail anymore if there is more than one (non-witness) input file. And is -witness the correct way to pass witnesses to cpa-witness2test? Did you test this tool-info module with your new tasks as well?

MetaVal also needs to be adapated, but that will be a problem for another PR.

Hm, what does that mean? How will the tool-info module behave in the meantime? It should still continue work for existing users.

@marian-lingsch
Copy link
Contributor Author

There are several tool-info modules that build on top of the tool-info module of CPAchecker. Most of them do not override cmdline, so should be safe, but I think we should go through each of grep cmdline $(grep -Rl cpachecker benchexec/tools/) and check whether they need adjustments. In particular cpa-witness2test.py even calls _get_additional_options().

Thanks a lot for pointing this out!
Just went through all the different tools which use CPAchecker's tool info module. Only CPA-witness2test had to be adjusted, the others should not be affected.

Hm, but now cpa-witness2test.py does not fail anymore if there is more than one (non-witness) input file. And is -witness the correct way to pass witnesses to cpa-witness2test? Did you test this tool-info module with your new tasks as well?

With the latest commit it fails again if there is more than one non-witness file.
-witness is also the correct way to pass the parameter as seen here. It also works as expected when running it on the benchmark definition files.

MetaVal also needs to be adapated, but that will be a problem for another PR.

Hm, what does that mean? How will the tool-info module behave in the meantime? It should still continue work for existing users.

Currently it will simply not work for the proposed validation task files. It will continue working as intended with the current way of validating witnesses.

@PhilippWendler
Copy link
Member

MetaVal also needs to be adapated, but that will be a problem for another PR.

Hm, what does that mean? How will the tool-info module behave in the meantime? It should still continue work for existing users.

Currently it will simply not work for the proposed validation task files. It will continue working as intended with the current way of validating witnesses.

Ok, this is good to hear. What happens if someone passes a new task-definition file with the witness option? Will it just be ignored (like all other tool-info modules would do), or is the behavior worse than ignoring the additional option, i.e., would it fail or do something meaningless if a user passes a new task-definition file?

I think it would be good if we keep it such that every tool-info modules either fully supports some option, or ignores its existence completely. That is the least confusing for users. Otherwise we would have three different behaviors of the Metaval tool-info module for validation tasks: in old versions of BenchExec it just ignores the witness option, in future versions it will eventually support it, and in the meantime yet something else.

@marian-lingsch
Copy link
Contributor Author

Ok, this is good to hear. What happens if someone passes a new task-definition file with the witness option? Will it just be ignored (like all other tool-info modules would do), or is the behavior worse than ignoring the additional option, i.e., would it fail or do something meaningless if a user passes a new task-definition file?

Currently MetaVal will just not work for the validation task definition format, since it ignores the witness option. Since it will be missing the expected witness option which is passed to it using --metavalWitness and will therefore just print its --help section when run.

I think it would be good if we keep it such that every tool-info modules either fully supports some option, or ignores its existence completely. That is the least confusing for users. Otherwise we would have three different behaviors of the Metaval tool-info module for validation tasks: in old versions of BenchExec it just ignores the witness option, in future versions it will eventually support it, and in the meantime yet something else.

I agree that keeping the behavior of validators consistent is important, but IMO this issue is orthogonal to this PR, since all validators will need to be adjusted in order to work with the validation format and handle the new option correctly.

If I understood your proposal correctly, for the next release of BenchExec all validators should have adapted their tool-info module such that there is a specific version of BenchExec from which the validation task definition files start working and previously the witness option will just be ignored.

@PhilippWendler
Copy link
Member

Ok, this is good to hear. What happens if someone passes a new task-definition file with the witness option? Will it just be ignored (like all other tool-info modules would do), or is the behavior worse than ignoring the additional option, i.e., would it fail or do something meaningless if a user passes a new task-definition file?

Currently MetaVal will just not work for the validation task definition format, since it ignores the witness option. Since it will be missing the expected witness option which is passed to it using --metavalWitness and will therefore just print its --help section when run.

But not if I use an appropriate benchmark definition that includes the necessary options for witnesses (just as in the previous years), wouldn't it?

I think it would be good if we keep it such that every tool-info modules either fully supports some option, or ignores its existence completely. That is the least confusing for users. Otherwise we would have three different behaviors of the Metaval tool-info module for validation tasks: in old versions of BenchExec it just ignores the witness option, in future versions it will eventually support it, and in the meantime yet something else.

I agree that keeping the behavior of validators consistent is important, but IMO this issue is orthogonal to this PR, since all validators will need to be adjusted in order to work with the validation format and handle the new option correctly.

If I understood your proposal correctly, for the next release of BenchExec all validators should have adapted their tool-info module such that there is a specific version of BenchExec from which the validation task definition files start working and previously the witness option will just be ignored.

No, I never said this. I said that every tool-info modules should either support the option or ignore it completely. But I don't want that tool-info module X ignores option Y in versions A, B, C, does something that is broken in versions D and E, and fully supports option Y in versions F and beyond.

Of course it would be nice for users if "there is a specific version of BenchExec from which the validation task definition files start working and previously the witness option will just be ignored", but I would not require that.

@marian-lingsch
Copy link
Contributor Author

Ok, this is good to hear. What happens if someone passes a new task-definition file with the witness option? Will it just be ignored (like all other tool-info modules would do), or is the behavior worse than ignoring the additional option, i.e., would it fail or do something meaningless if a user passes a new task-definition file?

Currently MetaVal will just not work for the validation task definition format, since it ignores the witness option. Since it will be missing the expected witness option which is passed to it using --metavalWitness and will therefore just print its --help section when run.

But not if I use an appropriate benchmark definition that includes the necessary options for witnesses (just as in the previous years), wouldn't it?

Exactly

I think it would be good if we keep it such that every tool-info modules either fully supports some option, or ignores its existence completely. That is the least confusing for users. Otherwise we would have three different behaviors of the Metaval tool-info module for validation tasks: in old versions of BenchExec it just ignores the witness option, in future versions it will eventually support it, and in the meantime yet something else.

I agree that keeping the behavior of validators consistent is important, but IMO this issue is orthogonal to this PR, since all validators will need to be adjusted in order to work with the validation format and handle the new option correctly.
If I understood your proposal correctly, for the next release of BenchExec all validators should have adapted their tool-info module such that there is a specific version of BenchExec from which the validation task definition files start working and previously the witness option will just be ignored.

No, I never said this. I said that every tool-info modules should either support the option or ignore it completely. But I don't want that tool-info module X ignores option Y in versions A, B, C, does something that is broken in versions D and E, and fully supports option Y in versions F and beyond.

Of course it would be nice for users if "there is a specific version of BenchExec from which the validation task definition files start working and previously the witness option will just be ignored", but I would not require that.

I see, then I somewhat misunderstood your comment. But I completely agree with you on this.

@PhilippWendler
Copy link
Member

Ok, this is good to hear. What happens if someone passes a new task-definition file with the witness option? Will it just be ignored (like all other tool-info modules would do), or is the behavior worse than ignoring the additional option, i.e., would it fail or do something meaningless if a user passes a new task-definition file?

Currently MetaVal will just not work for the validation task definition format, since it ignores the witness option. Since it will be missing the expected witness option which is passed to it using --metavalWitness and will therefore just print its --help section when run.

But not if I use an appropriate benchmark definition that includes the necessary options for witnesses (just as in the previous years), wouldn't it?

Exactly

Then we should ensure that this PR does not cause a regression for this situation (new task definition with appropriate benchmark definition with manually defined options for Metaval).

@marian-lingsch
Copy link
Contributor Author

Ok, this is good to hear. What happens if someone passes a new task-definition file with the witness option? Will it just be ignored (like all other tool-info modules would do), or is the behavior worse than ignoring the additional option, i.e., would it fail or do something meaningless if a user passes a new task-definition file?

Currently MetaVal will just not work for the validation task definition format, since it ignores the witness option. Since it will be missing the expected witness option which is passed to it using --metavalWitness and will therefore just print its --help section when run.

But not if I use an appropriate benchmark definition that includes the necessary options for witnesses (just as in the previous years), wouldn't it?

Exactly

Then we should ensure that this PR does not cause a regression for this situation (new task definition with appropriate benchmark definition with manually defined options for Metaval).

Looking at the tool info module of MetaVal a regression should not occur due to thi PR.

Copy link
Member

@PhilippWendler PhilippWendler left a comment

Choose a reason for hiding this comment

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

Can be merged once the community has agreed on and finalized the specification of the new option in the task-definition files. Would be nice to add a link to the spec to the code once this happens.

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

Successfully merging this pull request may close these issues.

2 participants