Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug and Release Builds Evaluate freed_object == null Differently #41179

Closed
Gastronok opened this issue Aug 10, 2020 · 11 comments
Closed

Debug and Release Builds Evaluate freed_object == null Differently #41179

Gastronok opened this issue Aug 10, 2020 · 11 comments
Milestone

Comments

@Gastronok
Copy link
Contributor

Godot version:
3.2.2 Stable

OS/device including version:
Windows 10 x64

Issue description:
In debug builds, checking freed_object == null will return true. In release builds, it will return false.

This is especially pernicious as most people aren't used to is_instance_valid() and so they'll check to make sure their variables aren't null before operating on them and everything will work fine in the editor or their debug builds, but then they'll get segfaults with no debug information in release builds.

Steps to reproduce:

extends Node

func _ready() -> void:
	var child : Node = $Child
	child.free()
	if not is_instance_valid(child): # Debug and Release agree here.
		printt("Child is invalid.")
	if child == null: # In Debug builds this will be true.
		printt("Child is null.")
	else: # In Release builds it will be false.
		print("Child is not null.")

Minimal reproduction project:
Here's a project with a compiled Windows 10 x64 verbose stdout file-logging-enabled release executable.
Crashers.zip

If you run the release build you'll get "Child is invalid. / Child is not null." in the log.txt. If you run it via the editor you'll see "Child is invalid. / Child is null." in the console.

Doing child.[anything] in the if child != null: branch will cause the release build to segfault (also preventing any printed-but-not-flushed output from showing up in log.txt) while edit/debug builds will never execute that code.

@Calinou
Copy link
Member

Calinou commented Aug 10, 2020

Related to #28922.

@Zireael07
Copy link
Contributor

Note that is_instance_valid() has a spate of its own issues, including pointing to some other memory...

@akien-mga
Copy link
Member

CC @RandomShaper

@RandomShaper RandomShaper added this to the 3.2 milestone Sep 8, 2020
@RandomShaper
Copy link
Member

The dangling Variant fix went to far in that it was considering variables pointing to freed objects to be null. That was solid, but too divergent from what happens on release. PR coming soon...

@jkb0o
Copy link
Contributor

jkb0o commented Sep 8, 2020

Why not move that Dangling Variant Fix from debug to release? From the point of view a developer of a big project in production, stability and consistent between debug/release builds is more important than performance lost (not much at all).

@RandomShaper
Copy link
Member

That would be probably a good idea, but it would take some discussion and benchmarking on various platforms. So for the time being let's just fix this (anyway in Godot 3.2 decaying to null is not wanted).

@akien-mga
Copy link
Member

Fixed by #41866.

@oeleo1
Copy link

oeleo1 commented Oct 1, 2020

Hey, is this The Fix (tm) for the crashing Release builds while Debug builds were fine? If so, @RandomShaper just won at least 2 pints of beer from me (or any other human wish x2)! Sadly there's no Oktoberfest this year, but we'll catch up.

@RandomShaper
Copy link
Member

@oeleo1, sorry but we're not there yet. This is just to make comparisons against null to behave in debug mode as they do on release.

I'll try to pursue that as soon as I have some free time. Anyway, I already took note of the beers, to be paid when this is finally done. :)

@oeleo1
Copy link

oeleo1 commented Oct 1, 2020

@RandomShaper Doh! Okay. And is there a way I can buy you some free time in the meantime? :-)

@RandomShaper
Copy link
Member

Well, I at least made the proposal: godotengine/godot-proposals#1589

There were some ideas from @reduz about safe/unsafe builds that I may have included in the proposal, but I think this is already fine for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants