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

wasmtime: Consume fuel on all return paths #8837

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jameysharp
Copy link
Contributor

As far as I can tell, when functions use a return instruction rather than falling off the end, fuel was not being tracked correctly. The fuel_function_exit method was only called from
after_translate_function, which is only called once all the instructions in the function have been translated, not at each return.

In this commit I switched to calling fuel_function_exit from handle_before_return instead, alongside some of the wmemcheck hooks. That should ensure that it happens on every function exit, regardless of what form that exit takes.

I think after_translate_function is fundamentally difficult to use correctly, and it wasn't used for anything else, so I've also removed it in this commit. And I did a minor cleanup at the site of one of the calls to handle_before_return while I was looking at it.

As far as I can tell, when functions use a `return` instruction rather
than falling off the end, fuel was not being tracked correctly. The
`fuel_function_exit` method was only called from
`after_translate_function`, which is only called once all the
instructions in the function have been translated, not at each return.

In this commit I switched to calling `fuel_function_exit` from
`handle_before_return` instead, alongside some of the `wmemcheck` hooks.
That should ensure that it happens on every function exit, regardless of
what form that exit takes.

I think `after_translate_function` is fundamentally difficult to use
correctly, and it wasn't used for anything else, so I've also removed it
in this commit. And I did a minor cleanup at the site of one of the
calls to `handle_before_return` while I was looking at it.
@jameysharp jameysharp requested review from a team as code owners June 19, 2024 00:17
@jameysharp jameysharp requested review from fitzgen and alexcrichton and removed request for a team and fitzgen June 19, 2024 00:17
@jameysharp
Copy link
Contributor Author

I think Nick's out for a few days so I'm handing this off to you instead, Alex.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Good catch! Mind adding a test or two here as well?

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm labels Jun 19, 2024
@jameysharp
Copy link
Contributor Author

Thanks for pointing me to those tests because I immediately got usefully confused about why the tests passed both before and after my changes.

It turns out that fuel_save_from_var is called by way of before_translate_operator for any of unreachable, return, or various kinds of calls. So doing it again in handle_before_return is unnecessary—and calling fuel_save_from_var multiple times is harmless but a waste of time—except when the return is an implicit fall-through off the end of the function. And that case is currently correctly handled by after_translate_function.

So the current implementation is not actually buggy!

But I still think the after_translate_function interface is sketchy. So I want to think more about what interface actually makes sense for instrumenting wasm translation like this, maybe drawing inspiration from whamm. Perhaps for instrumentation purposes we should behave as if there's a return instruction after the end of every function…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:wasm cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants