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. Thefuel_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
fromhandle_before_return
instead, alongside some of thewmemcheck
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 tohandle_before_return
while I was looking at it.
jameysharp requested fitzgen for a review on PR #8837.
jameysharp requested wasmtime-compiler-reviewers for a review on PR #8837.
jameysharp requested wasmtime-core-reviewers for a review on PR #8837.
jameysharp requested alexcrichton for a review on PR #8837.
jameysharp commented on PR #8837:
I think Nick's out for a few days so I'm handing this off to you instead, Alex.
alexcrichton submitted PR review:
Good catch! Mind adding a test or two here as well?
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 ofbefore_translate_operator
for any ofunreachable
,return
, or various kinds of calls. So doing it again inhandle_before_return
is unnecessary—and callingfuel_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 byafter_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 areturn
instruction after the end of every function…
Last updated: Dec 23 2024 at 13:07 UTC