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

ERROR: Condition "!success" is true when returning an Array from gdextension #138

Closed
thebigG opened this issue Mar 5, 2023 · 11 comments
Closed
Labels
bug c: core Core components help wanted Extra attention is needed

Comments

@thebigG
Copy link
Contributor

thebigG commented Mar 5, 2023

[Edit bromeon: syntax highlighting]

Hi there,

I have the following function in my gdextension:

    #[func]
    fn get_sin_full_circle_2dvectors(
        &mut self
    ) -> Array {
        let mut points = varray![Vector2::new(0.0, 100.0).to_variant()];
        points
    }

My gdscript:

# Called when the node enters the scene tree for the first time.
func _ready():
	print("ready")
	var f = get_sin_full_circle_2dvectors();
#	print()
	pass # Replace with function body.

Output from godot:

ERROR: Condition "!success" is true.
   at: _ref (core/variant/array.cpp:63)

Here is the code from Godot:

	if (_fp == _p) {
		return; // whatever it is, nothing to do here move along
	}

	bool success = _fp->refcount.ref();

	ERR_FAIL_COND(!success); // should really not happen either

	_unref();

It seems like all this triggers ERR_FAIL_COND(!success); . Which if you follow the code means the following:

	_ALWAYS_INLINE_ bool ref() { // true on success
		return count.conditional_increment() != 0;
	}

Which says to me that return count.conditional_increment() != 0; is false and "conditional_increment()" returns 0. Does that mean that the current array implementation has a reference count of 0?

I know there are certain things in Godot that are ref-counted, so it would make sense if this fails given that the array has a ref count of zero. I'm not sure how gdextension objects map to ref-counter objects in the Godot side... So I was wondering if you guys had more insight into how to tackle this issue. Maybe I'm missing something obvious here,...

Thanks in advance

@thebigG
Copy link
Contributor Author

thebigG commented Mar 5, 2023

Some more details that might help:

Godot_v4.0-stable_linux.x86_64
PRETTY_NAME="Pop!_OS 22.04 LTS"
rustc 1.65.0 (897e37553 2022-11-02)
gdextension version:72870d19c9b364e5f7361f95171629b571e59be6

@Bromeon Bromeon added bug c: core Core components labels Mar 5, 2023
@Bromeon
Copy link
Member

Bromeon commented Mar 5, 2023

Thank you! We'll try to reproduce the behavior in tests. I would personally even consider "printing an error message" a bug, although it's sometimes hard to avoid with Godot, or only possible with performance penalties (checking the same operation twice).

Minor request, could you in the future use code tags with the corresponding language? The syntax is as follows (and code would be on a newline, so the 3 backticks are on separate lines, each.
```rs code ```
```cpp code ```
```gdscript code ```

@thebigG
Copy link
Contributor Author

thebigG commented Mar 5, 2023

Minor request, could you in the future use code tags with the corresponding language? The syntax is as follows (and code would be on a newline, so the 3 backticks are on separate lines, each.

Didn't even know this was something GitHub supported. Will do. Thanks for letting me know 👍

@Bromeon Bromeon added help wanted Extra attention is needed good first issue Good for newcomers and removed good first issue Good for newcomers labels Mar 5, 2023
@thebigG
Copy link
Contributor Author

thebigG commented Mar 5, 2023

Before 42f12f5 I had been using the old implementation for arrays and it was working fine. I'll look and see if I can spot something obvious in the diff.

@lilizoey
Copy link
Member

lilizoey commented Mar 7, 2023

So i can reproduce it, but only when i attach a script to the node that has this function, and call the function from the script attached to the node. if i call the function from outside that script it works fine.

This also works in the itests, but you need to specifically:

  • create a rust-class that inherits from node
  • write a function that returns an array
  • create a node that inherits from that rust class
  • attach a script to this node
  • call the rust-function from within that script, and store the result in a variable

@sadovsf
Copy link

sadovsf commented Mar 10, 2023

Hello, today i did run into probably similar issue with strings. They either get into gdscript as empty or crash over all.
System:

  • Godot: Godot_v4.0-stable_macos.universal
  • godot-rust: ac5f78c
  • OS: OSx Ventura 13.2.1

Minimal repro project:
repro.zip

From what i see there are 2 possible things that happen.

  1. Crash
  2. Script gets empty string

Only way how to make it work is part of repro as well, see test_node.rs. If you run godot project it should print
GET: 'native - Hello, world!' but crashes with this in log:

GET: ''
Godot(6903,0x7ff8528d2680) malloc: Incorrect checksum for freed object 0x1566af368: probably modified after being freed.
Corrupt value: 0xffffffff
Godot(6903,0x7ff8528d2680) malloc: *** set a breakpoint in malloc_error_break to debug

So my guess is that memory with string gets freed and then passed to Godot. Script sees it as empty and crashes on trying to free it again. Over all i would expect even super simple case like:

#[func]
fn get_text(&self) -> GodotString {
    GodotString::from("test")
}

or similar to "simply work"

@atrefonas
Copy link

The following code causes a crash for me:

#[func]
  pub fn get_buildings(&mut self) -> PackedStringArray {
    let mut arr = PackedStringArray::new();
    arr.push(GodotString::from("apt"));
    arr
  }

Godot:

extends Control

# Called when the node enters the scene tree for the first time.
func _ready():
    var buildings = Global.get_buildings()
    print(buildings)

@atrefonas
Copy link

I'm running into a similar issue with Strings.

This function does not work (it says it doesn't exist when I try to call it from Godot)

    #[func]
    pub fn set_building_mode(&mut self, building:GodotString) {
      godot_print!("worked! {:?}", building);
    }

This function, on the other hand, does work and I can call it from Godot:

    #[func]
    pub fn set_building_mode(&mut self, building:Variant) {
      let building_name = building.to_string();
      godot_print!("worked! {:?}", building);
    }

@lilizoey
Copy link
Member

lilizoey commented Mar 10, 2023

So the sanitizer build of godot is complaining about a use-after-free. And it appears that Drop is called on the array at some point when it shouldn't, as the free the sanitizer complains about happens in the drop-glue of Array.

thebigG added a commit to thebigG/gdextension that referenced this issue Mar 12, 2023
thebigG added a commit to thebigG/gdextension that referenced this issue Mar 12, 2023
-Add tests that would trigger godot-rust#138 before this fix.
thebigG added a commit to thebigG/gdextension that referenced this issue Mar 19, 2023
-Add integration tests.
@Bromeon Bromeon linked a pull request Mar 19, 2023 that will close this issue
thebigG added a commit to thebigG/gdextension that referenced this issue Mar 19, 2023
@lilizoey
Copy link
Member

The same issue seems to happen when returning refcounted objects from virtual methods. This isn't really possible to trigger yet because of #190, but attempting to do so will hit the same bug. It does seem like it has the same solution as what is being proposed in #165 though, just for virtual method calls as well.

thebigG added a commit to thebigG/gdextension that referenced this issue Mar 22, 2023
bors bot added a commit that referenced this issue Mar 22, 2023
165: Fix early destruction of objects returned from Rust (#138) r=Bromeon a=thebigG

Hi there,

I probably need to test this more thoroughly. Just opening a pull request for collaboration and such.

Basically it came down to for some reason(I'm still learning about this rust port of godot) ac02be1 triggers this behavior.  And this PR reverts that commit(except for the tests).


This patch fixes my use case, but I'll try to write unit tests and test in general to make sure we don't miss any other edge cases.


Hope this PR is clear enough.

Thanks
Lorenzo

Co-authored-by: thebigG <[email protected]>
@lilizoey
Copy link
Member

This was fixed in #204, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: core Core components help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants