[Merged] Fix UB related to engine enum layout #326
Conversation
lilizoey
added
bug
c: ffi
|
oh i accidentally included #323, i'll remove that in a bit |
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-326 |
lilizoey
changed the title
|
bors try |
- Move `AsArg` into `GodotFfi` - Refactor `GodotFuncMarshal`
tryBuild succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
| // Some places Godot will pass along 64-bit numbers that are intended to be interpreted as unsigned | ||
| // integers despite Godot integers being signed by default. | ||
| impl_godot_marshalling!(u64 as i64; lossy); |
There was a problem hiding this comment.
Regarding this, I will open an issue, so we don't forget to remove it from being passed via #[func].
326: Fix UB related to engine enum layout r=Bromeon a=lilizoey `EngineEnum`s should be represented as 32-bit values, but also should be passed as i64 to/from godot in function parameters/return values. This fixes that by having engine enums not implement `GodotFfi` but instead only implements `GodotFuncMarshal`. To support this change, `GodotFuncMarshal` has been refactored: - `Via` now represents only the intermediate type, and must implement `GodotFfi` - Add error-types for when conversion fails - Add explicit conversion methods to/from `Via`, we can't just use `TryFrom` here since not all types we wanna use have the proper `TryFrom` conversions. like f32 and f64 With this change, a minor change was made to the class generator, such that all arguments are first converted to their `Via` type, before a pointer to them is made. And return pointers are now dereferenced to the `Via` type before they're converted to the proper type. This means that `GodotFuncMarshal` is now the type that represents whether something can be passed as an argument/return value to a ptrcall. `AsArg` was also removed and its method moved into `GodotFfi` as that simplifies codegen. (we can just always call `GodotFfi::as_arg_ptr` to create the pointer, rather than treating classes and other types differently). In the future we can try splitting up `GodotFfi` more, but i do think that `AsArg` belonged more in `godot_ffi` anyway. Co-authored-by: Lili Zoey <lili.andersen@nrk.no>
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
|
What is wrong with merging lately? Merge conflicts after merging? Maybe time to look into #255 soon... |
Bromeon
changed the title
EngineEnums should be represented as 32-bit values, but also should be passed as i64 to/from godot in function parameters/return values.This fixes that by having engine enums not implement
GodotFfibut instead only implementsGodotFuncMarshal.To support this change,
GodotFuncMarshalhas been refactored:Vianow represents only the intermediate type, and must implementGodotFfiVia, we can't just useTryFromhere since not all types we wanna use have the properTryFromconversions. like f32 and f64With this change, a minor change was made to the class generator, such that all arguments are first converted to their
Viatype, before a pointer to them is made. And return pointers are now dereferenced to theViatype before they're converted to the proper type.This means that
GodotFuncMarshalis now the type that represents whether something can be passed as an argument/return value to a ptrcall.AsArgwas also removed and its method moved intoGodotFfias that simplifies codegen. (we can just always callGodotFfi::as_arg_ptrto create the pointer, rather than treating classes and other types differently). In the future we can try splitting upGodotFfimore, but i do think thatAsArgbelonged more ingodot_ffianyway.