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

[2.1, 3.0, 3.1, 3.2, 3.2.1] Dangling variant issue #41682

Closed
xsellier opened this issue Sep 1, 2020 · 8 comments
Closed

[2.1, 3.0, 3.1, 3.2, 3.2.1] Dangling variant issue #41682

xsellier opened this issue Sep 1, 2020 · 8 comments
Assignees
Milestone

Comments

@xsellier
Copy link
Contributor

xsellier commented Sep 1, 2020

Related PR:
#38119

Godot version:
2.1.*, 3.0, 3.1, 3.2, 3.2.1

Godot Version where is the bug is fixed:
3.2.2 (haven't tested the master branch)

OS/device including version:
N/A

Issue description:
Dangling pointer 101
I was talking with @RandomShaper about that issue.
Backporting the linked PR to those versions would fix the issue in debug version, but it would still happen in release version.
I backported it, and removed the debug flags from the PR, after having ran several benchmarks, I noticed that the performances are somewhat identical

Steps to reproduce:
Start a new project, add the following code to the _init function:

    var a = {
        control = Control.new()
    }

    print(a)
    # Print: Control[876]
    a.control.free()
    Control.new()
    print(a)
    # Print: Control[877] instead of [Deleted Object]

Minimal reproduction project:
N/A

@xsellier xsellier changed the title [2.1, 3.0, 3.1, 3.2, 3.2.1] Variant danling issue [2.1, 3.0, 3.1, 3.2, 3.2.1] Dangling variant issue Sep 1, 2020
@RandomShaper RandomShaper self-assigned this Sep 1, 2020
@xsellier
Copy link
Contributor Author

xsellier commented Sep 2, 2020

I made some more debugging, and I noticed the following behavior:

    var a = {
        control = Control.new()
    }

    print(a)
    # Print: Control[876]
    a.control.free()
    Node.new()
    print(a)
    # Print: [Deleted Object]

It seems to happens when you create the same type of object.

@akien-mga akien-mga added this to the 4.0 milestone Sep 2, 2020
@jkb0o
Copy link
Contributor

jkb0o commented Sep 2, 2020

Backporting the linked PR to those versions would fix the issue in debug version, but it would still happen in release version.
I backported it, and removed the debug flags from the PR, after having ran several benchmarks, I noticed that the performances are somewhat identical

@RandomShaper are there any plans to make #38119 make work in release build as well as debug? I'd like to move to 3.2.3 and discard my own fix of this bug, but with a debug-only fix, it seems difficult.

@akien-mga
Copy link
Member

Assigning 4.0 milestone as this still happens in 3.2.2+ and master (*) in release builds, so we need a thorough fix for release mode that can also be backported to 3.2.x, possibly 2.1.x, and if there's demand for it 3.0.x or 3.1.x (though I consider 3.0.x nearly EOL).

(*) I couldn't confirm for master yet as GDScript seems broken in target=release builds, I can't get it to print() :)

@xsellier
Copy link
Contributor Author

xsellier commented Sep 2, 2020

I already backported the PR on the v2.1 branch, but it changes a lot of files, plus it does not compile for Windows debug since the __atomic functions are not builtin in the MSVC compiler (only in GCC). There a 2 solutions, make a wrapper for MSVC (but it would be tricky), or use <atomic>, but the compilation fails in this case saying that the atomic functions have been deleted.

Or even better, find a workaround that does not require the __atomic functions

@girng
Copy link

girng commented Sep 2, 2020

On 2.1.6, it prints (control:[Deleted Object])

@xsellier
Copy link
Contributor Author

xsellier commented Sep 3, 2020

On 2.1.6, it prints (control:[Deleted Object])

@girng I don't have the same stuff here (this is v2.1.6 official):
image

    var a = {
        control = Control.new()
    }

    print(a)
    # Print: Control[559]
    a.control.free()
    Control.new()
    print(a)
    # Print: Control[560] instead of [Deleted Object]

image

@RandomShaper
Copy link
Member

The main bug report here is indeed what is expected to happen.

@xsellier, also found that in some cases the bug doesn't show up even in release builds. After thinking about this, I can say that in a release build either can happen: a new object is put where a freed one was or the new one gets a different memory address. What happens depends on how the allocator works, the memory size of the object types, etc.

Since @xsellier also did some benchmarking with positive results and so many people is asking for dangling Variants to be prevented on release builds as well, I think it's time to really consider it. That would also affect 4.0. The approach there is different so it should require some separate benchmarking and some further discussions.

@xsellier xsellier mentioned this issue Oct 2, 2020
3 tasks
@akien-mga
Copy link
Member

This is fixed in 3.4 actually.

@akien-mga akien-mga modified the milestones: 4.0, 3.4 Dec 14, 2021
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

6 participants