Skip to content

Conversation

@zhehangd
Copy link
Contributor

@zhehangd zhehangd commented Feb 16, 2023

Fixes #1020, fixes #1025 (only for Object* parameter crash, Ref<Object> crash reported recently is a different issue).

*reinterpret_cast<GDExtensionObjectPtr *> -> reinterpret_cast<GDExtensionObjectPtr> as the real type of p_ptr is Object* not Object**.

@zhehangd zhehangd requested a review from a team as a code owner February 16, 2023 06:02
@zhehangd zhehangd changed the title Fix PtrToArg<Object*> crash Fix crash using Object* as parameter Feb 16, 2023
@akien-mga akien-mga added bug This has been identified as a bug crash labels Feb 16, 2023
@akien-mga akien-mga requested a review from bruvzg March 29, 2023 05:37
struct PtrToArg<T *> {
_FORCE_INLINE_ static T *convert(const void *p_ptr) {
return reinterpret_cast<T *>(godot::internal::gde_interface->object_get_instance_binding(*reinterpret_cast<GDExtensionObjectPtr *>(const_cast<void *>(p_ptr)), godot::internal::token, &T::___binding_callbacks));
auto gde_obj_ptr = reinterpret_cast<GDExtensionObjectPtr>(const_cast<void *>(p_ptr));
Copy link
Member

Choose a reason for hiding this comment

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

Please state the type explicitly instead of using auto.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Looks fine, object_get_instance_binding takes GDExtensionObjectPtr which is Object *. auto is absolutely unnecessary, in this case it's always GDExtensionObjectPtr.

@zhehangd
Copy link
Contributor Author

Changed. Just wanted to avoid writing GDExtensionObjectPtr twice before and after reinterpret_cast.

@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

@zhehangd zhehangd force-pushed the fix_obj_ptr_crash branch from 41fedec to 093f067 Compare March 31, 2023 02:38
@zhehangd
Copy link
Contributor Author

Done

@akien-mga akien-mga merged commit e9942db into godotengine:master Apr 1, 2023
@akien-mga
Copy link
Member

Thanks!

@dsnopek
Copy link
Collaborator

dsnopek commented May 21, 2023

This PR appears to have caused a regression similar to #1045

When debugging an issue someone found while testing godotengine/godot#77010, I found that Godot appears to be passing Object ** to virtual methods, whereas this PR causes godot-cpp to treat them as Object *. So, again, there appears to be this difference in the calling convention between normal and virtual methods.

dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 23, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 25, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 25, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 25, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 25, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 25, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 25, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 26, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 26, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request May 26, 2023
dsnopek added a commit to dsnopek/godot-cpp that referenced this pull request Jun 7, 2023
…standardize on `Object **` encoding in ptrcall
akien-mga added a commit that referenced this pull request Jun 7, 2023
Revert the changes from PR #1044 and #1045 and standardize on `Object **` encoding in ptrcall
@zhehangd zhehangd deleted the fix_obj_ptr_crash branch October 7, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This has been identified as a bug crash

Projects

None yet

4 participants