Stream: git-wasmtime

Topic: wasmtime / issue #5316 Fix issues in `FunctionBuilder` API

Wasmtime GitHub notifications bot (Nov 22 2022 at 17:10):

Rodrigodd commented on issue #5316:

Pinging @jameysharp because it is the one that I talked about this.

Wasmtime GitHub notifications bot (Nov 22 2022 at 17:23):

bjorn3 commented on issue #5316:

The func fields needs to be mutable for cg_clif. module.declare_func_in_func() needs it as well as a hack I use to support variadic functions (https://github.com/bjorn3/rustc_codegen_cranelift/blob/a449d0d1431b789adf7c4c27e0db028ad2218a4a/src/abi/mod.rs#L542) Others may have other reasons to directly modify the function.

The distinction is that even if you call func_mut(), you can only change the contents of the Function.

*builder.func_mut() = Function::new() is allowed and replaces the entirety of the function.

Wasmtime GitHub notifications bot (Nov 22 2022 at 18:02):

jameysharp commented on issue #5316:

Oh, right, it doesn't let you change where the pointer points, but it does let you change what it points to in any way you want. I guess then that's only useful to highlight those call-sites which might break FunctionBuilders invariants. Which is still worth something, but maybe not enough to justify the change.

I don't understand your other comments though, @bjorn3. As far as I can tell, the callers of Module::declare_func_in_func and the line you pointed to in codegen_terminator_call are all well-supported by the func_mut() accessor.

Regarding the finalize() call in cg-clif's codegen_fn_body: do you mind changing that to let finalize consume the FunctionBuilder? It looks to me like passing bcx as an argument instead of keeping it in fx might not be too invasive of a change, for example.

Wasmtime GitHub notifications bot (Nov 22 2022 at 18:12):

bjorn3 commented on issue #5316:

I don't understand your other comments though, @bjorn3. As far as I can tell, the callers of Module::declare_func_in_func and the line you pointed to in codegen_terminator_call are all well-supported by the func_mut() accessor.

They are. I was just saying that func_mut() doesn't prevent replacing the Function unlike what you said.

Regarding the finalize() call in cg-clif's codegen_fn_body: do you mind changing that to let finalize consume the FunctionBuilder? It looks to me like passing bcx as an argument instead of keeping it in fx might not be too invasive of a change, for example.

Passing bcx as argument would require passing it everywhere. What would work though is to move the finalize() call to the caller of codegen_fn_body. I did have to move it out of the tcx.sess.time("codegen clif ir") call too which would slightly regress accuracy of performance measurements, but that isn't a big deal as I expect finalize() to be much cheaper than the rest of codegen_fn_body.

Wasmtime GitHub notifications bot (Nov 22 2022 at 18:44):

Rodrigodd commented on issue #5316:

fixed a warning

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:10):

jameysharp commented on issue #5316:

I chatted with @cfallin to double-check my reasoning. @Rodrigodd, please drop the second commit (getters for FunctionBuilder::func), and I'll merge the first one (making finalize consume self).

Limiting access to the func field has some value, even though it doesn't prevent any bugs since func_mut() allows effectively the same thing; at least uses of func_mut mark where such bugs could occur. However, we concluded that's not enough to justify a breaking API change that's somewhat tedious to fix.

Forcing FunctionBuilder to be a short-lived borrow of the FunctionBuilderContext, on the other hand, has minimal impact on callers and clarifies how these types are intended to be used, so that change is worth doing.

Wasmtime GitHub notifications bot (Nov 23 2022 at 23:13):

jameysharp commented on issue #5316:

Thank you!

Last updated: Oct 23 2024 at 20:03 UTC