Godot: Editor crashes when clicking on empty area in 2D scene (GCC 6)

[zaps166]

Hi!

Operating system or device - Godot version:
Arch Linux, GCC 6.1.1, x86-64 build, Godot 2.0.3

Issue description (what happened, and what was expected):
Editor crashes when clicking on empty area in 2D scene.

Steps to reproduce:

• compile Godot "target=release_debug" using GCC 6 (any optimization level other than -O0),
• run the editor,
• switch to 2D scene,
• add root Node,
• click on empty area.

Current behavior: crash
Expected behavior: doesn't crash

I've noticed that Godot calls a method cast_to() when the pointer points to 0x0. This method doesn't use member variables, but anyway it crashes on GCC 6. I think calling a method when pointer is 0x0 especially for casting from this is not a good idea, because the behavior is undefined - in most cases it works properly, but - as You can see - not always (e.g. GCC 6 or probably other special cases).

Godot crashes somewhere inside C++ library __cxxabiv1::__dynamic_cast (the rdi register - which is first function argument treated as a pointer - is 0x0, so it must crash):

        50 [1]  in /build/gcc-multilib/src/gcc/libstdc++-v3/libsupc++/dyncast.cc
0x7f42c08e89d0  <+0x0020>        48 8b 07                    mov    rax,QWORD PTR [rdi]

When compiling with -O0 optimization the it works fine - it doesn't call the __cxxabiv1::__dynamic_cast function and it returns NULL immediately. The __cxxabiv1::__dynamic_cast function mustn't be called with NULL argument.
It is difficult to debug it on -Og level due to optimizations (can't debug on -O0, because it works properly), but when I reading the assembly code, it looks for me like GCC 6 does an optimization: it doesn't check the this argument before calling the dynamic_cast, because it expects that this will never be NULL.

This patch fixes the issue:

diff --git a/tools/editor/plugins/canvas_item_editor_plugin.cpp b/tools/editor/plugins/canvas_item_editor_plugin.cpp
index 0213dbd..7f045e9 100644
--- a/tools/editor/plugins/canvas_item_editor_plugin.cpp
+++ b/tools/editor/plugins/canvas_item_editor_plugin.cpp
@@ -1492,7 +1492,7 @@ void CanvasItemEditor::_viewport_input_event(const InputEvent& p_event) {
        while ((n && n != scene && n->get_owner() != scene) || (n && !n->is_type("CanvasItem"))) {
            n = n->get_parent();
        };
-       c = n->cast_to<CanvasItem>();
+       c = n ? n->cast_to<CanvasItem>() : 0;
 #if 0
        if ( b.pressed ) box_selection_start( click );
 #endif

The alternative is to check whether this isn't NULL before calling the dynamic_cast inside Object::cast_to<>() method.

Maybe some other crashes caused by GCC 6 are also related to this?

[vnen]

Thanks a lot for this info. Dyncast is consistently making crashes when compiled with target=release_debug which add -O2 optimization level. If this can fix the issue plus the info on #4623 it may make Godot be compatible with GCC 6.

[zaps166]

4623 is caused by data alignment (data is not aligned for SSE2) - I've got the same.

[zaps166]

Is it better to add the condition to canvas_item_editor_plugin.cpp and all other places which does the same thing (calls cast_to on NULL pointer - if any) or just add additional if to check this inside Object::cast_to<>() method? Additional if also fixes the problem, but I don't know whether other compilers doesn't generate unnecessary second check for this (one added manually, the second from the compiler).

//Edit: Or maybe fix everywhere and add an ASSERT?

[zaps166]

There is more places which calls cast_to on NULL pointer. GCC 6 also ignores manual checks whether this is NULL or not. Working patch with volatile:

diff --git a/core/object.h b/core/object.h
index c344401..7cdfc73 100644
--- a/core/object.h
+++ b/core/object.h
@@ -492,11 +492,12 @@ public:
        template<class T>
        T *cast_to() {

+               // Don't allow some compilers to optimize which causes missing null-pointer check before casting.
+               if (!(volatile void *)this)
+                       return NULL;
 #ifndef NO_SAFE_CAST
                return SAFE_CAST<T*>(this);
 #else
-               if (!this)
-                       return NULL;
                if (is_type_ptr(T::get_type_ptr_static()))
                        return static_cast<T*>(this);
                else
@@ -507,11 +508,12 @@ public:
        template<class T>
        const T *cast_to() const {

+               // Don't allow some compilers to optimize which causes missing null-pointer check before casting.
+               if (!(volatile void *)this)
+                       return NULL;
 #ifndef NO_SAFE_CAST
                return SAFE_CAST<const T*>(this);
 #else
-               if (!this)
-                       return NULL;
                if (is_type_ptr(T::get_type_ptr_static()))
                        return static_cast<const T*>(this);
                else

I noticed that optimized code on other compilers shouldn't generate two checks, but it is hard to say when looking at optimized assembly code.

If this is correct I can do a PR :)

[jedStevens]

I am also getting this, fixing this issue is a bit out of my range of knowledge but if someone could point to a log file or something, I should be able to provide some more info on what is going on.

[jedStevens]

After reading this over and giving some compiling a try, it seems that the version coming from git and compiled as stated in the docs works fine. It seems the issue I have is with the version coming from the AUR. Not sure how this helps but after I compiled Godot from source I stopped getting this error.

[zaps166]

I compiled Godot from source I stopped getting this error.

Did you compile Godot with target=debug (-O0 optimization)? If yes (what is your executable name?) - the editor won't crash, but you'll have much slower engine.

Sadly no error messages are coming up when ran through a terminal.

Probably because Godot starts new process when opening a project, e.g. try: bin/godot.x11.opt.tools.64 -path demos/2d/platformer -e and you'll see a segfault.

GCC 6 on any optimization level other than -O0 in Godot code doesn't check for NULL pointer in class method (Godot calls a class method on NULL pointer which uses address from this), so it crashes on dynamic_cast when it gets NULL. The fix is to force it by adding volatile or add conditions before every cast_to which can be NULL (currently I found two places, so safer is check it by volatile).

[zaps166]

currently I found two places, so safer is check it by volatile

Together with #4614 there are at least three places which causes crash due to cast_to on NULL this pointer: scene/gui/check_box.cpp:62

[vnen]

@reduz can you also check this, please (especially the proposed patch in https://github.com/godotengine/godot/issues/4673#issuecomment-219794048).

[jedStevens]

I compiled with no target flag to get it working; scons platform=x11. I recompiled using target=debug for this and still it works fine. The binary created is here ~godot/bin/godot.x11.tools.64. I also used the -e flag on the version of godot installed via the AUR, my broken version, and no error messages were thrown.

[vnen]

I compiled with no target flag to get it working; scons platform=x11.

The default is target=debug, that's why there's no crash. The one from AUR is certainly compiled with target=release_debug, which causes this crash.

[reduz]

No... it should never be the situation where we avoid a crash in that function. If this==NULL , it's okay that a crash happens, we catch plenty of bugs thanks to that.

[akien-mga]

It's still a crash, so reopening until we have a fix.

[reduz]

oh, sorry, I thought i closed the PR

[jedStevens]

Confirmed that compiling with target=release_debug gives the same error as mentioned.

[zaps166]

(...) we catch plenty of bugs thanks to that.

So in this case that is better to avoid an_object->cast_to<Something>() and use a static method like: Object::cast_to<Something>(an_object). This leads to change every cast_to in entire engine, but the current code is not fully correct for C++ (GCC 6 doesn't like this) :) The current non-static cast_to can be also patched (https://github.com/godotengine/godot/issues/4673#issuecomment-219794048) and deprecated to not break the API for other projects.

This won't crash without additional volatile null-pointer check and this could also avoid other possible problems like returning address with offset due to vtable when this == nullptr (0x0+offset != 0x0, so another possible crash).

[zaps166]

@jedStevens you can do a workaround: target=release_debug use_llvm=yes to get optimized and stable code (sudo pacman -S clang).

[vnen]

For reference, #4636 and #4614 both are caused by the same issue as this. It should be checked if the null pointer is on the same place or if there are more than one bug.

[shackra]

I use scons platform=x11 tools=yes target=release_debug use_llvm=yes bits=32 but the bug is still present. My bad, I just copied the wrong executable after compiling. BTW, I'm the maintainer of Godot in AUR (godot and godot-pulse packages, actually)

[vnen]

I'm having a look at this. It seems it's best to check for null in each individual crash case, as it seems we're calling cast_to in NULL objects. For instance, this one which causes #4636.

What I find odd is why this is not a problem in debug builds nor in builds with other compilers.

[zaps166]

What I find odd is why this is not a problem in debug builds nor in builds with other compilers.

IMHO it is GCC 6 additional optimization which assumes that this is never NULL, because in C++ methods with NULL pointer shouldn't be called if it uses this (Godot's case) or member variables.

The best solution is to change cast_to to static method (https://github.com/godotengine/godot/issues/4673#issuecomment-220338623). The change can be done automatically by regexp or something like else (many lines contains cast_to, so manually it will take long time :smile:).

Workaround with volatile which disables the optimization: https://github.com/godotengine/godot/issues/4673#issuecomment-219794048 - this fixes the crash, but it is ugly :smile:

It seems it's best to check for null in each individual crash case (...)

Probably nobody knows where NULL can happen. Checking for NULL before cast_to will make the code messy, because it isn't necessary (dynamic_cast can handle NULL properly even in GCC 6, but not in methods with NULL this).

[Razzlegames]

How is this == NULL while executing code within the object instance? Is someone doing something weird (setting itself to NULL etc), or is this a concurrency issue?

[vnen]

I'm really don't have the knowledge to understand what's going on here. For example, in the case of #4614 the crash happens here:

Control *Control::get_root_parent_control() const {

    const CanvasItem *ci=this;
    const Control *root=this;

    while(ci) {

        const Control *c = ci->cast_to<Control>(); // Crash
        if (c) {
            root=c;

            if (c->data.RI || c->data.MI || c->is_toplevel_control())
                break;
        }

        ci=ci->get_parent_item();
    }

    return const_cast<Control*>(root);
}

How come ci can be NULL right after a while(ci)? Seems GCC is overoptimizing something.

Changing cast_to to static sounds like a good solution.

[zaps166]

Does changing the line:

const Control *c = ci->cast_to<Control>(); // Crash

to

const Control *c = ci ? ci->cast_to<Control>() : 0;

fix the crash in this place?

Maybe

Control::get_root_parent_control() 

is called with NULL this, so GCC 6 doesn't check for it at first iteration... (because ci==this at first iteration)?

Try to add:

fprintf(stderr, "%p\n", ci);

before cast_to.

[Razzlegames]

This really seems like a concurrency problem. The cast makes sense to make static though, for sure.

Why would this ever be null unless someone is deleting or modifying the object while another thread is executing get_root_parent_control? (Well actually in that code example it could be an ancestor that is null, but I guess you're saying it invokes cast_to and crashes at the static_cast ?)

[Razzlegames]

BTW, I guess static_cast<type>(NULL) is known to cause crashes: http://stackoverflow.com/questions/1872571/null-pointer-compatibility-with-static-cast

You could try dynamic_cast(obj) and if obj is NULL, it will return NULL instead of crashing:
http://en.cppreference.com/w/cpp/language/dynamic_cast

[zaps166]

cast_to already calls dynamic_cast

[zaps166]

I don't think that this is a concurrency problem. Otherwise It'll be crashing on every compiler quite often.

I've debugged it a few days ago - the only thing is that GCC 6 doesn't check if this is NULL before casting. Also manually added if (this) weren't work (worked properly with volatile which disables optimizations).

[Razzlegames]

If dynamic_cast(NULL) causes a crash, this is a compiler bug.

[Razzlegames]

C++ standard: §5.2.7/4:

If the value of v is a null pointer value in the pointer case, the result is the null pointer value of type R

.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1905.pdf

[Razzlegames]

IMO, and if I understand your analysis correctly: this is not a Godot bug, and no further work needs to be done here. Just state the compiler version to avoid in the docs due to non-compliance with standards and site this closed ticket.

[zaps166]

If dynamic_cast(NULL) causes a crash, this is a compiler bug.

Yes, but not if it is called when this is NULL. Calling a method on NULL pointer is not a good idea:

Something *c = NULL;
c->method(); //"this" is NULL

because compiler may optimize it and it doesn't check the this value.

http://stackoverflow.com/questions/669742/accessing-class-members-on-a-null-pointer

(...) calling any function — even a non-virtual one — on a null pointer is undefined behavior.

I didn't read everything, but I can agree with this.

this is not a Godot bug

This is a Godot bug, because it calls a method on NULL pointer.

[Razzlegames]

I agree that calling a method on a NULL pointer is not a good idea. I'm still not sure how it made it to that point. However, it doesn't change the fact that the actual crashing was from NULL being passed to dynamic_cast. I suppose since it's undefined behavior (NULL->method()) anything goes.

[reduz]

if it only happens with this compiler, the problem must be somewhere else

On Sat, May 28, 2016 at 7:12 PM, Razzlegames [email protected]
wrote:

I agree that calling a method on a NULL pointer is not a good idea. I'm
still not sure how it made it to that point. However, it doesn't change the
fact that the actual crashing was from NULL being passed to dynamic_cast. I
suppose since it's undefined behavior (NULL->method()) anything goes.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/4673#issuecomment-222332038,
or mute the thread
https://github.com/notifications/unsubscribe/AF-Z27pCRRWCeB0qTdv7LtBQoLLsteDsks5qGL3DgaJpZM4IfdiH
.

[zaps166]

When dynamic_cast is called then compiler inserts an implicit condition for calling the dyncast function is standard C++ library. If a pointer is NULL then it returns NULL.
If GCC 6 ignores a if (this) {} then it crashes.

Example:

#include <stdio.h>

class A
{
    virtual void some_virtual_method()
    {}
public:
    template<typename ReturnType>
    ReturnType *cast_to()
    {
        return dynamic_cast<ReturnType *>(this);
    }

    template<typename ReturnType, typename CurrentType>
    static ReturnType *cast_to(CurrentType *ptr)
    {
        return dynamic_cast<ReturnType *>(ptr);
    }
};

class C
{};

class B : public A, public C
{};

int main()
{
    B *b = NULL;
    C *c;

    c = dynamic_cast<C *>(b); //OK
    printf("1: %p %p\n", b, c);

    c = A::cast_to<C>(b); //OK
    printf("2: %p %p\n", b, c);

    c = b->cast_to<C>(); //Crash
    printf("3: %p %p\n", b, c);

    return 0;
}

Run this on GCC 6:

$ g++ a.cpp -O0 && ./a.out
1: (nil) (nil)
2: (nil) (nil)
3: (nil) (nil)

$ g++ a.cpp -O2 && ./a.out
1: (nil) (nil)
2: (nil) (nil)
Segmentation fault

Conclusion: never call a method from a NULL pointer even if it works somewhere.

http://stackoverflow.com/questions/2474018/when-does-invoking-a-member-function-on-a-null-instance-result-in-undefined-beha

It's always undefined behavior to call a member function through a null pointer.

[Razzlegames]

I think it's perfectly acceptable to change this to a static function that uses dynamic_cast.

I don't think it should ever reach that point however since the pointer is always checked for NULL. Do you have any supporting evidence noted in the standard that would substantiate your hypothesis of this == NULL always being false?

If not, that's still a compiler bug (from optimization).

[Razzlegames]

But one thing to check is the caller. Is it really null when calling the reparent function? If so we definitely should be checking that, and that would be a Godot bug.

[Razzlegames]

Throw up a stack trace in gdb and check. Mind posting it here too? However if the caller is checking that object for NULL before calling the reparent, then things are fishy here.

Maybe you can get a core dump and upload it here? That would really help

[zaps166]

There are many cases where Godot crashes on GCC 6 because of cast_to method. Probably it crashes here: https://github.com/gcc-mirror/gcc/blob/gcc-6_1_0-release/libstdc%2B%2B-v3/libsupc%2B%2B/dyncast.cc#L50, because GCC 6 doesn't check this pointer before calling it (see https://github.com/godotengine/godot/issues/4673#issuecomment-222333565 when it doesn't do a check).

@Razzlegames do you mean this case https://github.com/godotengine/godot/issues/4673#issuecomment-222322343 ?

Again the example how the GCC 6 works (it is weird, but maybe correct due to optimizations :smile:)

#include <stdio.h>

class A
{
    virtual void some_virtual_method()
    {}
public:
    template<typename ReturnType>
    ReturnType *cast_to()
    {
        if (this)
        {
            printf("this: %p\n", this);
            return dynamic_cast<ReturnType *>(this);
        }
        printf("Compiler detects this as NULL\n");
        return NULL;
    }
};

class C
{};

class B : public A, public C
{};

int main()
{
    B *b = NULL;
    C *c = b->cast_to<C>(); //Crash
    printf("%p %p\n", b, c);

    return 0;
}

The output:

$ g++ a.cpp -O0 && ./a.out
Compiler detects this as NULL
(nil) (nil)

$ g++ a.cpp -O2 && ./a.out
this: (nil)
Segmentation fault

I've added a guard if (this) {...} and it prints (nil) value after a check, so the compiler assumes that this is never NULL (IMHO correct assumption). The same may happen with while here https://github.com/godotengine/godot/issues/4673#issuecomment-222322343 (ci is this, so if this is NULL than compiler can ignore the first iteration).

[zaps166]

Sanitizer output for the https://github.com/godotengine/godot/issues/4673#issuecomment-222356195

Clang 3.8.0 and GCC 6.1.1 x86-64

$ clang++ -O2 a.cpp -fsanitize=undefined && ./a.out 
a.cpp:11:7: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true
      [-Wundefined-bool-conversion]
                if (this)
                ~~  ^~~~
a.cpp:11:7: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true
      [-Wundefined-bool-conversion]
                if (this)
                ~~  ^~~~
a.cpp:30:12: note: in instantiation of function template specialization 'A::cast_to<C>' requested here
        C *c = b->cast_to<C>(); //Crash
                  ^
2 warnings generated.
a.cpp:30:9: runtime error: upcast of null pointer of type 'B'
a.cpp:30:9: runtime error: member call on null pointer of type 'A'
Compiler properly detects this as NULL
(nil) (nil)



$ g++ -O2 a.cpp -fsanitize=undefined && ./a.out 
a.cpp:30:22: runtime error: member call on null pointer of type 'struct A'
Segmentation fault

Now everything is clear - Godot bug. Even Clang complains about it, this must not be NULL.

[shackra]

Man, I'm glad you guys figure it out!

El domingo 29 de mayo del 2016 a las 0707 horas, Błażej Szczygieł escribió:

Sanitizer output for the https://github.com/godotengine/godot/issues/4673#issuecomment-222356195

Clang 3.8.0 and GCC 6.1.1 x86-64

$ clang++ -O2 a.cpp -fsanitize=undefined && ./a.out 
a.cpp:11:7: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true
      [-Wundefined-bool-conversion]
                if (this)
                ~~  ^~~~
a.cpp:11:7: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true
      [-Wundefined-bool-conversion]
                if (this)
                ~~  ^~~~
a.cpp:30:12: note: in instantiation of function template specialization 'A::cast_to<C>' requested here
        C *c = b->cast_to<C>(); //Crash
                  ^
2 warnings generated.
a.cpp:30:9: runtime error: upcast of null pointer of type 'B'
a.cpp:30:9: runtime error: member call on null pointer of type 'A'
Compiler properly detects this as NULL
(nil) (nil)



$ g++ -O2 a.cpp -fsanitize=undefined && ./a.out 
a.cpp:30:22: runtime error: member call on null pointer of type 'struct A'
Segmentation fault

Now everything is clear - Godot bug. Even Clang complains about it, this must not be NULL.

---

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/godotengine/godot/issues/4673#issuecomment-222359708

👋 Pax et bonum.
Jorge Araya Navarro
https://es.gravatar.com/shackra

[reduz]

@zaps166 So you mean that with other compilers, the dynamic_cast is receiving NULL but it's working when it shouldn't?

[zaps166]

@reduz dynamic_cast works properly with NULL pointer on every compiler. The only thing is that it receives NULL in class method which is called when this == NULL. This is incorrect (if you don't believe - see the sanitizer errors). The behaviour is undefined - it doesn't work on GCC 6, but it works on most compilers. This is not a dynamic_cast bug, see printf output here: https://github.com/godotengine/godot/issues/4673#issuecomment-222356195 - the output is this: (nil) after if (this) - the condition is ignored, because as sanitizer said:

'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true

If compiler assumes that this is always valid (because in C++ this pointer must not be NULL) then it can crash somewhere in libstdc++.

[zaps166]

Now tested on GCC 5.3.0: the example code (https://github.com/godotengine/godot/issues/4673#issuecomment-222356195) works properly there (doesn't crash), but when sanitizer is enabled it also crashes on GCC 5 and also shows the error.

$ g++-5 a.cpp -O2 && ./a.out 
Compiler detects this as NULL
(nil) (nil)


$ g++-5 a.cpp -O2 -fsanitize=undefined && ./a.out 
a.cpp:30:22: runtime error: member call on null pointer of type 'struct A'
Segmentation fault


$ g++-5 a.cpp -O0 -fsanitize=undefined && ./a.out 
a.cpp:30:22: runtime error: member call on null pointer of type 'struct A'
a.cpp:30:22: runtime error: member access within null pointer of type 'struct A'
Segmentation fault

So it is not a GCC 6 bug.

The best solution for me is to use a static method.

[reduz]

I think the solution is to fix whatever is calling the function with null
as a base pointer, it should be ok if it crashes

On Sun, May 29, 2016 at 11:46 AM, Błażej Szczygieł <[email protected]

wrote:

Now tested on GCC 5.3.0: the example code (#4673 (comment)
https://github.com/godotengine/godot/issues/4673#issuecomment-222356195)
works properly there (doesn't crash), but when sanitizer is enabled it also
crashes on GCC 5 and also shows the error.

$ g++-5 a.cpp -O2 && ./a.out
Compiler detects this as NULL
(nil) (nil)

$ g++-5 a.cpp -O2 -fsanitize=undefined && ./a.out
a.cpp:30:22: runtime error: member call on null pointer of type 'struct A'
Segmentation fault

$ g++-5 a.cpp -O0 -fsanitize=undefined && ./a.out
a.cpp:30:22: runtime error: member call on null pointer of type 'struct A'
a.cpp:30:22: runtime error: member access within null pointer of type 'struct A'
Segmentation fault

So it _is not_ a GCC 6 bug.

The best solution for me is to use a static method.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/4673#issuecomment-222364444,
or mute the thread
https://github.com/notifications/unsubscribe/AF-Z2wdDVOdLiAdxxsQUNRdOrd8uBRk7ks5qGabXgaJpZM4IfdiH
.

[Razzlegames]

Yes definitely. All the if(this) and if(!this) checks should be removed too. They should never happen and are superfluous.

This is a great time to consider cleaning up those warnings! These bugs are likely being pointed to in the warnings but they are being ignored.

I'll look and see where null checks are not being done and make a patch.

[Razzlegames]

Here's some added checks and a change that should fix: #4623

https://github.com/godotengine/godot/pull/4883

[Razzlegames]

@reduz
If we want "fast to fail", perhaps adding: -fsanitize=undefined is appropriate? Right now, crashes will be much harder to debug.

And this is a much easier patch :)

[Razzlegames]

But I still think this is appropriate since it's the editor:

diff --git a/tools/editor/plugins/canvas_item_editor_plugin.cpp b/tools/editor/plugins/canvas_item_editor_plugin.cpp
index 1b6d94b..13d6d08 100644
--- a/tools/editor/plugins/canvas_item_editor_plugin.cpp
+++ b/tools/editor/plugins/canvas_item_editor_plugin.cpp
@@ -1492,6 +1492,10 @@ void CanvasItemEditor::_viewport_input_event(const InputEvent& p_event) {
                while ((n && n != scene && n->get_owner() != scene) || (n && !n->is_type("CanvasItem"))) {
                        n = n->get_parent();
                };
+
+               if(n == NULL){
+                       return;
+               }
                c = n->cast_to<CanvasItem>();
 #if 0
                if ( b.pressed ) box_selection_start( click );

[zaps166]

I've made a static patch: https://github.com/zaps166/godot/commits/WIP_static__cast_to (commits should be squashed after review). Of course it fixes the crash on GCC 6 and also it makes minor code changes (e.g. removes some redundant checks). Anyone wants to test it :smile: ?

Is it better to use Object::cast_to or just move it out of class and use cast_to ?

[Razzlegames]

Is it better to use Object::cast_to or just move it out of class and use cast_to

It seems to be applicable to all types even if they are not Object (e.g. Ref), so outside would seem logical. Maybe in a utility header.

[zaps166]

It seems to be applicable to all types even if they are not Object (e.g. Ref)

Ref cannot use cast_to, because it is not a pointer, so I used operator * which returns a raw pointer containing by Ref (e.g. https://github.com/zaps166/godot/commit/50fe616945372f752e49d3383f64c67fa695eccb#diff-f0fa48118e647a12803e8f0d2ba92074R228).

Is this code really necessary: https://github.com/godotengine/godot/blob/master/core/object.h#L503 ?

Btw: ref_ptr.h and res_ptr.h files are unused, hmm...

[zaps166]

This compiles (and works) with my patch: https://github.com/zaps166/godot/blob/8bd86b1d6a7e95eb2a36ee523ad2639f71384de1/core/object.h#L501 :)

[zaps166]

Is it better to use Object::cast_to or just move it out of class and use cast_to

In this case it should stay in Object, because is_type_ptr is method of Object.

[zaps166]

@reduz @Razzlegames @vnen @akien-mga I think that the huge patch is now finished: https://github.com/zaps166/godot/commit/a184eb0bb4193ab0047ce07265ef64acc77cace0
I can do a PR with this commit (or fix it if necessary before). Otherwise I'll remove it...

Currently it breaks API compatibility, but if it is really necessary I can also add non-static cast_to method which will use the static method.

[vnen]

@zaps166 if it's on you fork already then it's easier to just open the PR. Then we can review the changes and comment on them.

[zaps166]

@vnen PR: https://github.com/godotengine/godot/pull/4936 (new SHA-1 due to typo fix in commit message)

[reduz]

will fix these issues in 3.0, when we change all the memory allocation code

[akien-mga]

Might have been improved by recent refactoring of core types in the master branch.

[bojidar-bg]

Can confirm that this issue is still valid, unfortunately.

[reduz]

I would really like to see traces of the crashes, since I think this is mostly godots fault

[Razzlegames]

Can you remove those if(this) conditionals etc? The behavior is a undefined. Once those things are sanitized the bug should go away. (Well at least the bug will be easier to spot I think)

[bojidar-bg]

Here is a gdb backtrace:

(gdb) bt
#0  __cxxabiv1::__dynamic_cast (src_ptr=0x0, src_type=0x102b9e200 <typeinfo for Object>, dst_type=0x102b89df0 <typeinfo for CanvasItem>, src2dst=0)
    at /build/gcc-multilib/src/gcc/libstdc++-v3/libsupc++/dyncast.cc:50
#1  0x0000000100ed1d20 in CanvasItemEditor::_viewport_gui_input(Ref<InputEvent> const&) ()
#2  0x0000000100963352 in MethodBind1<Ref<InputEvent> const&>::call(Object*, Variant const**, int, Variant::CallError&) ()
#3  0x0000000101ad52ab in Object::call(StringName const&, Variant const**, int, Variant::CallError&) ()
#4  0x0000000101ad94c9 in Object::emit_signal(StringName const&, Variant const**, int) ()
#5  0x0000000101ada2b5 in Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) ()
#6  0x00000001010858c2 in Viewport::_gui_call_input(Control*, Ref<InputEvent> const&) ()
#7  0x000000010108c6b8 in Viewport::_gui_input_event(Ref<InputEvent>) ()
#8  0x000000010108d727 in Viewport::input(Ref<InputEvent> const&) ()
#9  0x000000010108d887 in Viewport::_vp_input(Ref<InputEvent> const&) ()
#10 0x0000000100963352 in MethodBind1<Ref<InputEvent> const&>::call(Object*, Variant const**, int, Variant::CallError&) ()
#11 0x0000000101ad52ab in Object::call(StringName const&, Variant const**, int, Variant::CallError&) ()
#12 0x0000000101acf46e in Object::call(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) ()
#13 0x00000001010b991b in SceneTree::call_group_flags(unsigned int, StringName const&, StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) ()
#14 0x00000001010bb9d4 in SceneTree::input_event(Ref<InputEvent> const&) ()
#15 0x000000010064bd3b in InputDefault::parse_input_event(Ref<InputEvent> const&) ()
#16 0x0000000100641e06 in OS_X11::process_xevents() ()
#17 0x00000001006433c0 in OS_X11::run() ()
#18 0x000000010063c8ac in main ()

I couldn't get a crash in 3D viewport, only 2D, and only if there is a root node (as the OP mentioned).

[reduz]

did not meant to close it, please try again, as the commit above might fix it

[bojidar-bg]

Can no longer confirm on master, good job 😃
Potentially, other issues with GCC6 release mode exist, but we can fix them when they pop up. Thus, closing.