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

Hyper Gobra #802

Open
wants to merge 41 commits into
base: master
Choose a base branch
from
Open

Hyper Gobra #802

wants to merge 41 commits into from

Conversation

jcp19
Copy link
Contributor

@jcp19 jcp19 commented Nov 29, 2024

Felix's implementation of the technique for checking SIF based on this paper

TODO:

@@ -68,7 +68,8 @@ object ConfigDefaults {
val DefaultZ3APIMode: Boolean = false
val DefaultDisableNL: Boolean = false
val DefaultMCEMode: MCE.Mode = MCE.Enabled
val DefaultEnableLazyImports: Boolean = false
val DefaultHyperMode: Hyper.Mode = Hyper.Enabled
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: change

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if we could leave this turned on but ensure that if a program does not make use of any SIF specs that we do not perform the product construction.
If this is not easily possible, we have to think about the hyperMode options as "off" currently results in turning all SIF specs to true, which is potentially dangerous if you are not aware of this. Imho, the default mode should make sure that non-SIF programs are encoded as today (i.e., without product construction) and either handle SIF programs soundly or report an error. However, having a mode that allows you to turn SIF checks off for a SIF program could be useful for debugging but this requires a warning / error that this is potentially unsound

Comment on lines 1 to 3
package viper.gobra.util

import javax.sound.sampled.{AudioFormat, AudioSystem}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: drop

/** Set this to only transform methods that contain relational assertions somewhere in their spec or body.
* May lead to invalid programs when such a method calls another methods that does contain such specs.
*/
var onlyTransformMethodsWithRelationalSpecs: Boolean = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ArquintL I guess setting this to true should allow us to keep the default for SIF. I will change it once I start working on this pR

Copy link
Member

Choose a reason for hiding this comment

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

That's not sufficient. Instead of checking which methods have relational spec and which do not, a first attempt would be to decide this for the entire input program. To keep this on a per-method level, we would first need to address the problem mentioned in the comment above namely that a non-SIF method calling a SIF method (or vice-versa) fails as the number of parameters / arguments does not match.
A similar issue occurs if we would mix methods encoded with optimized SIF encoding (enforcing low control flow) and the more complete SIF encoding as the latter one uses additional parameters (namely the activation variables) and also uses a different ordering for the parameters belonging to the 2nd execution (which is just an implementation detail)

@jcp19
Copy link
Contributor Author

jcp19 commented Jan 23, 2025

The current version, together with the changes from #818 which have been merged in scion leads to an exception on this file:

package bugs

// Bug 5.
// Trying to verify this file yields the following error message:
// 
// 		Precondition of call test1(ub) might not hold.
// 		Permission to ub[0] might not suffice.
//
// The reason can be seen in the generated Viper file: The `_termination_proof`
// method for `test0` replaces the permission amount (`write`) with `wildcard`.
// Note: In order for this bug to occur, `test1` needs to return a struct
// (at least not some primitive type), and the functions must be `pure` (and
// thus also annotated with `decreases`).
//
// This problem first occurred while trying to verify `pkg\slayers\path\epic`.

type Struct struct {}

requires acc(&ub[0])
decreases
pure func test1(ub []byte) Struct {
    return Struct{}
}

requires acc(&ub[0])
decreases
pure func test0(ub []byte) bool {
    return Struct{} == test1(ub)
}

Reported by @henriman

@jcp19 jcp19 added the SIF label Feb 1, 2025
@ArquintL
Copy link
Member

Regression (just in case we are not aware of it yet): in contrast to 8facab5, termination measures are no longer transformed resulting in crashes of Silicon

@jcp19
Copy link
Contributor Author

jcp19 commented Feb 20, 2025

So, the bug related to termination measures has been fixed (gobra no longer immediately crashes with consistency errors for programs that perform termination checking). @henriman

There is currently a seemingly absurd bug where ADT members are not translated to the Viper program. I will debug this one next. After this one, I guess it is a matter of cleaning up the code (sorely needed), reviewing everything, and dropping all copied files from the SIF plugin for Viper

var errs: Seq[AbstractError] = Seq.empty
val transformedN = n.transform{
case n: SIFLowExp =>
errs = errs :+ ConsistencyError(s"Low expression found: ${n.exp}", n.pos)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is a good solution because every package that imports sync to use a Mutex will use the new spec for this function, which contains low annotations. Thus, this solution forces us to always enable SIF when verifying concurrent code

Copy link
Member

Choose a reason for hiding this comment

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

I've also realized this after looking at the failing testcases ^^
Maybe we should distinguish between imported and non-imported SIF expressions and ignore the former while cause errors for the latter (if SIF is disabled).
What I want to avoid is any confusion as in my program contains SIF expressions but I (e.g. by mistake) configured Gobra not to do a product construction and, thus, I'm thinking I've verified SIF but actually Gobra didn't

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

Successfully merging this pull request may close these issues.

2 participants