Stream: git-wasmtime

Topic: wasmtime / PR #8837 wasmtime: Consume fuel on all return ...

Wasmtime GitHub notifications bot (Jun 19 2024 at 00:17):

jameysharp opened PR #8837 from jameysharp:consume-fuel-on-return to bytecodealliance:main:

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.

Wasmtime GitHub notifications bot (Jun 19 2024 at 00:17):

jameysharp requested fitzgen for a review on PR #8837.

Wasmtime GitHub notifications bot (Jun 19 2024 at 00:17):

jameysharp requested wasmtime-compiler-reviewers for a review on PR #8837.

Wasmtime GitHub notifications bot (Jun 19 2024 at 00:17):

jameysharp requested wasmtime-core-reviewers for a review on PR #8837.

Wasmtime GitHub notifications bot (Jun 19 2024 at 00:19):

jameysharp requested alexcrichton for a review on PR #8837.

Wasmtime GitHub notifications bot (Jun 19 2024 at 00:20):

jameysharp commented on PR #8837:

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

Wasmtime GitHub notifications bot (Jun 19 2024 at 00:24):

alexcrichton submitted PR review:

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

Wasmtime GitHub notifications bot (Jun 19 2024 at 05:07):

jameysharp commented on PR #8837:

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…

Last updated: Nov 22 2024 at 17:03 UTC