Make cast_to a static member of Object. #10581
Conversation
|
Man I think you messed up git somehow, what are all those commits doing there? |
|
@Zireael07 yeah, I accidentally rebased on my fork rather than master, sorry :) fixed now |
|
I've not checked the diff thoroughly, but your idea is nice. But then I wonder if commits adding null checks just because of the UB issue should be reverted, as no longer meaningful. |
|
This commit already removes the redundant null checks, e.g. 0f4d1ad#diff-6435cbec06d56101c9bcc0061478fae7L276 |
|
Terrific. :) |
|
Just a note, I thought dereferencing a null pointer was fine as long as |
hpvb
force-pushed
the
fix-gcc6+
branch
2 times, most recently
from
83126c7 to
40421e8
Compare
|
@Zylann I think we're hitting the problem explained in this blogpost: https://www.viva64.com/en/b/0226/ |
|
@Zylann This post copy/pastes some part of the relevant standard: https://www.viva64.com/en/w/V704/ |
hpvb
force-pushed
the
fix-gcc6+
branch
2 times, most recently
from
94ced6b to
389182b
Compare
hpvb
force-pushed
the
fix-gcc6+
branch
3 times, most recently
from
e9443c2 to
7e4f5da
Compare
hpvb
changed the title
This is to prepare to replace all instances of the member version of cast_to().
Currently we rely on some undefined behavior when Object->cast_to() gets called with a Null pointer. This used to work fine with GCC < 6 but newer versions of GCC remove all codepaths in which the this pointer is Null. However, the non-static cast_to() was supposed to be null safe. This patch makes cast_to() Null safe and removes the now redundant Null checks where they existed. It is explained in this article: https://www.viva64.com/en/b/0226/
|
I can't wait not to notice the 50 potential bugs this will have magically fixed :P |
These Null checks were removed in godotengine#10581 but actually changed the logic of the functions in this case. This fixes godotengine#10654
Currently we rely on some undefined behavior when Object->cast_to() gets
called with a Null pointer. This used to work fine with GCC < 6 but
newer versions of GCC remove all codepaths in which the this pointer is
Null. However, the non-static cast_to() was supposed to be null safe.
This patch makes cast_to() Null safe and removes the now redundant Null
checks where they existed.
The following demos have been imported and run without issues on Fedora 26/GCC 7.1.1:
This validates that it at least addresses #10517 and #10515 also. These were already fixed by this commit removes those null checks again and the crashes are still gone. This suggests to me that this will likely fix other random crashes on GCC6+ also.