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

Basic USVM TS type system with type coercion #215

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

zishkaz
Copy link
Collaborator

@zishkaz zishkaz commented Sep 30, 2024

No description provided.

@zishkaz zishkaz self-assigned this Sep 30, 2024
@zishkaz zishkaz marked this pull request as draft September 30, 2024 11:40
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@zishkaz
Copy link
Collaborator Author

zishkaz commented Oct 3, 2024

This PR indirectly introduces a bug

@zishkaz zishkaz marked this pull request as ready for review October 3, 2024 08:24
.github/workflows/build-and-run-tests.yml Show resolved Hide resolved
usvm-core/src/main/kotlin/org/usvm/StateForker.kt Outdated Show resolved Hide resolved
usvm-core/src/main/kotlin/org/usvm/StateForker.kt Outdated Show resolved Hide resolved
usvm-ts/src/main/kotlin/org/usvm/TSBinaryOperator.kt Outdated Show resolved Hide resolved

class TSTestResolver {
class TSTestResolver(
private val state: TSState
Copy link
Member

Choose a reason for hiding this comment

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

Why? Resolver should be able to resolve many states independently

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TSTestResolver was instantiated for each returned state, and I needed access to state.typeStorage.
So I decided to add state as private constructor param.

usvm-ts/src/test/kotlin/org/usvm/util/TSTestResolver.kt Outdated Show resolved Hide resolved
private fun approximateParam(expr: UConcreteHeapRef, idx: Int, model: UModelBase<EtsType>): TSObject =
with(expr.ctx as TSContext) {
val suggestedType = state.getSuggestedType(idx)
return suggestedType?.let { newType ->
Copy link
Member

Choose a reason for hiding this comment

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

Something odd is happening here. You can always access all required information using symbolic values and symbolic values only, otherwise you will encounter path divergence. This map cannot be built from concrete values to type info, only from symbolic ones

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe different types of ref entities (reg readings, etc.) have unique ways to generate a key. For example, reg readings can use its' idx field as a key, and this key leads to correct list of types from any model.

Copy link
Member

Choose a reason for hiding this comment

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

But you might store it with another key -- if I resolve something from registerReading(1), it doesn't mean that I always accessed this value using this key. Probably, I could do it with registerReading(2).getByIndex(3), i could have accessed it with RR(3) (because of aliasing) and many many other ways

Comment on lines +344 to +345
// TODO: draft
override fun internEquals(other: Any): Boolean = structurallyEqual(other)
Copy link
Member

Choose a reason for hiding this comment

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

Why is it a draft? Seems like we can implement it

private fun generateAdditionalExprs(rawExprs: List<UExpr<out USort>>): List<UExpr<out USort>> = with(ctx) {
if (!rawExprs.all { it.sort == boolSort }) return emptyList()
val newExpr = when (baseExpr.sort) {
addressSort -> addedExprCache.putOrNull(mkEq(fpToBoolSort(asFp64()), asBool()))
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we need to add a connection between baseSort expr and them? If it already exists for baseSort <-> fp and bool, it exists for bool and fp by transitivity as well

private fun approximateParam(expr: UConcreteHeapRef, idx: Int, model: UModelBase<EtsType>): TSObject =
with(expr.ctx as TSContext) {
val suggestedType = state.getSuggestedType(idx)
return suggestedType?.let { newType ->
Copy link
Member

Choose a reason for hiding this comment

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

But you might store it with another key -- if I resolve something from registerReading(1), it doesn't mean that I always accessed this value using this key. Probably, I could do it with registerReading(2).getByIndex(3), i could have accessed it with RR(3) (because of aliasing) and many many other ways

// (primitives can't be cast to ref in TypeScript type coercion)
addressSort -> {
val fpToBoolLink = mkEq(fpToBoolSort(asFp64()), asBool())
val boolToRefLink = mkEq(asBool(), (baseExpr as UExpr<UAddressSort>).neq(mkNullRef()))

Check warning

Code scanning / detekt

Reports multiple space usages Warning

Unnecessary long whitespace
@@ -6,7 +6,9 @@
import org.usvm.PathNode
import org.usvm.TSContext
import org.usvm.TSTarget
import org.usvm.UAddressSort

Check warning

Code scanning / detekt

Detects unused imports Warning

Unused import
import org.usvm.UCallStack
import org.usvm.UExpr

Check warning

Code scanning / detekt

Detects unused imports Warning

Unused import

private fun approximateParam(expr: UConcreteHeapRef, idx: Int, model: UModelBase<EtsType>): TSObject =
when (val tr = model.typeStreamOf(expr).take(1)) {
is TypesResult.SuccessfulTypesResult -> with (expr.tctx) {

Check warning

Code scanning / detekt

Reports spaces around parentheses Warning test

Unexpected spacing before "("
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.

2 participants