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

Metering is not correct in presence of divergent control flow #4256

Closed
georgemitenkov opened this issue Oct 14, 2023 · 0 comments · Fixed by #5108
Closed

Metering is not correct in presence of divergent control flow #4256

georgemitenkov opened this issue Oct 14, 2023 · 0 comments · Fixed by #5108
Labels
bug Something isn't working 📦 lib-middlewares About wasmer-middlewares ⏱ metering priority-medium Medium priority issue
Milestone

Comments

@georgemitenkov
Copy link

Describe the bug

Metering instrumentation does not work for if-else statements.
Because the instrumentation is injected naively when the change on control flow is detected, for if-else statements the cost of preceding instructions is only included for the if branch.

Example of a program which has incorrect costs.

(module
(func $bomb (param $condition i32) (result i32)
    ;; dummy instructions to ensure high cost
    (local.set $condition (local.get $condition))
    (local.set $condition (local.get $condition))
    (local.set $condition (local.get $condition))
    (local.set $condition (local.get $condition))
    (if (result i32) (local.get $condition)
        (then
            (block (result i32) (local.get $condition))
         )
        (else
            (block (result i32) (local.get $condition))
        )
))

The instrumented code produced by Wasmer:

LocalGet { local_index: 0 }
LocalSet { local_index: 0 }
LocalGet { local_index: 0 }
LocalSet { local_index: 0 }
LocalGet { local_index: 0 }
LocalSet { local_index: 0 }
LocalGet { local_index: 0 }
LocalSet { local_index: 0 }
LocalGet { local_index: 0 }
If { blockty: Type(I32) }
Block { blockty: Type(I32) }
LocalGet { local_index: 0 }
METER(13)
End
METER(1)
Else
Block { blockty: Type(I32) }
LocalGet { local_index: 0 }
METER(3)
End
METER(1)
End
METER(1)
End

We can see that the else branch has lower cost than it should.

Additional context

To fix the issue, the metering has to account for diverging control flow, or a different algorithm should be used.

@Michael-F-Bryan Michael-F-Bryan added bug Something isn't working ⏱ metering 📦 lib-middlewares About wasmer-middlewares labels Oct 17, 2023
@Michael-F-Bryan Michael-F-Bryan added this to the v4.4 milestone Oct 17, 2023
@Michael-F-Bryan Michael-F-Bryan added the priority-medium Medium priority issue label Oct 17, 2023
@syrusakbary syrusakbary modified the milestones: v4.5, backlog Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-middlewares About wasmer-middlewares ⏱ metering priority-medium Medium priority issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants