for loop with large int values returns negative numbers

[dcuny]

Godot version

Godot v4.1.1.stable

System information

Godot v4.1.1.stable - Windows 10.0.19045 - Vulkan (Forward+) - dedicated GeForce GT 640 () - AMD FX(tm)-8320 Eight-Core Processor (8 Threads)

Issue description

for loops with large int values return negative values instead of the correct values.

Steps to reproduce

Example code:

const BLICKS:int = 705600000
var start_loop:int = BLICKS * 40
var end_loop:int = BLICKS * 42
var step_loop: int = BLICKS / 32
for i in range( start_loop, end_loop, step_loop ):
		print(i)

Example output:

-1840771072
-1818721072
-1796671072
-1774621072
-1752571072
-1730521072
-1708471072
...

In contrast, a while loop returns the correct values:

const BLICKS:int = 705600000
var start_loop:int = BLICKS * 40
var end_loop:int = BLICKS * 42
var step_loop: int = BLICKS / 32
while start_loop < end_loop:
	print(start_loop)
	start_loop += step_loop

Correct output:

28224000000
28246050000
28268100000
28290150000
28312200000
28334250000
28356300000
28378350000
...

Minimal reproduction project

N/A

[AThousandShips]

I think this is because internally the range argument uses a Vector3i which is 32 bits signed, CC @dalexeev

[AThousandShips]

Correction, it seems to be due to the fact that range internally counts with int, the vector based approach was before:

godot/modules/gdscript/gdscript_utility_functions.cpp

Lines 167 to 220 in a574c02

	case 3: {
	VALIDATE_ARG_NUM(0);
	VALIDATE_ARG_NUM(1);
	VALIDATE_ARG_NUM(2);
	int from = *p_args[0];
	int to = *p_args[1];
	int incr = *p_args[2];
	if (incr == 0) {
	*r_ret = RTR("Step argument is zero!");
	r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
	return;
	}
	Array arr;
	if (from >= to && incr > 0) {
	*r_ret = arr;
	return;
	}
	if (from <= to && incr < 0) {
	*r_ret = arr;
	return;
	}
	// Calculate how many.
	int count = 0;
	if (incr > 0) {
	count = ((to - from - 1) / incr) + 1;
	} else {
	count = ((from - to - 1) / -incr) + 1;
	}
	Error err = arr.resize(count);
	if (err != OK) {
	r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
	*r_ret = Variant();
	return;
	}
	if (incr > 0) {
	int idx = 0;
	for (int i = from; i < to; i += incr) {
	arr[idx++] = i;
	}
	} else {
	int idx = 0;
	for (int i = from; i > to; i += incr) {
	arr[idx++] = i;
	}
	}
	*r_ret = arr;
	} break;

Your start value is far far larger than what fits in 32 signed bits, and in fact when truncated into 32 bits it yields -1840771071

[AThousandShips]

Can take a look at fixing this tomorrow

[dcuny]

Thanks for the prompt response!

[AThousandShips]

Got a basic solution working, but will have to look at some details, will open a PR tomorrow

[dalexeev]

I think this is because internally the range argument uses a Vector3i which is 32 bits signed

Yes, the for loop it can iterate Vector{2,2i,3,3i}, the analyzer implicitly replaces the constant range() with an int, float or vector so as not to allocate an array unlike range() utility. There is also a PR #71564 for non-constant range(). I'm not sure there's an easy fix for this other than fixing/adding opcode to the GDScript VM.

[AThousandShips]

Replacing int with int64_t solves it at least in this specific case, will do some more testing tomorrow

[kleonc]

See #67025, in there I'm changing range to use int64_t but it also mentions the issue with the optimized version. Not sure how relevant/outdated that PR is though, it's kinda in limbo. If anyone wants to take it over / extract that part - feel free to do so.

[AThousandShips]

That is a more comprehensive fix indeed, will take a look at that too

This was my initial approach:

--- a/modules/gdscript/gdscript_utility_functions.cpp
+++ b/modules/gdscript/gdscript_utility_functions.cpp
@@ -122,11 +122,15 @@ struct GDScriptUtilityFunctionsDefinitions {
 			} break;
 			case 1: {
 				VALIDATE_ARG_NUM(0);
-				int count = *p_args[0];
+				int64_t count = *p_args[0];
 				Array arr;
 				if (count <= 0) {
 					*r_ret = arr;
 					return;
+				} else if (count >= INT32_MAX) {
+					*r_ret = RTR("Count is too big!");
+					r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
+					return;
 				}
 				Error err = arr.resize(count);
 				if (err != OK) {
@@ -135,7 +139,7 @@ struct GDScriptUtilityFunctionsDefinitions {
 					return;
 				}
 
-				for (int i = 0; i < count; i++) {
+				for (int64_t i = 0; i < count; i++) {
 					arr[i] = i;
 				}
 
@@ -145,21 +149,27 @@ struct GDScriptUtilityFunctionsDefinitions {
 				VALIDATE_ARG_NUM(0);
 				VALIDATE_ARG_NUM(1);
 
-				int from = *p_args[0];
-				int to = *p_args[1];
+				int64_t from = *p_args[0];
+				int64_t to = *p_args[1];
 
 				Array arr;
 				if (from >= to) {
 					*r_ret = arr;
 					return;
 				}
-				Error err = arr.resize(to - from);
+				int64_t count = to - from;
+				if (count >= INT32_MAX) {
+					*r_ret = RTR("Count is too big!");
+					r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
+					return;
+				}
+				Error err = arr.resize(count);
 				if (err != OK) {
 					r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
 					*r_ret = Variant();
 					return;
 				}
-				for (int i = from; i < to; i++) {
+				for (int64_t i = from; i < to; i++) {
 					arr[i - from] = i;
 				}
 				*r_ret = arr;
@@ -169,9 +179,9 @@ struct GDScriptUtilityFunctionsDefinitions {
 				VALIDATE_ARG_NUM(1);
 				VALIDATE_ARG_NUM(2);
 
-				int from = *p_args[0];
-				int to = *p_args[1];
-				int incr = *p_args[2];
+				int64_t from = *p_args[0];
+				int64_t to = *p_args[1];
+				int64_t incr = *p_args[2];
 				if (incr == 0) {
 					*r_ret = RTR("Step argument is zero!");
 					r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
@@ -196,6 +206,12 @@ struct GDScriptUtilityFunctionsDefinitions {
 					count = ((from - to - 1) / -incr) + 1;
 				}
 
+				if (count >= INT32_MAX) {
+					*r_ret = RTR("Count is too big!");
+					r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
+					return;
+				}
+
 				Error err = arr.resize(count);
 
 				if (err != OK) {
@@ -206,12 +222,12 @@ struct GDScriptUtilityFunctionsDefinitions {
 
 				if (incr > 0) {
 					int idx = 0;
-					for (int i = from; i < to; i += incr) {
+					for (int64_t i = from; i < to; i += incr) {
 						arr[idx++] = i;
 					}
 				} else {
 					int idx = 0;
-					for (int i = from; i > to; i += incr) {
+					for (int64_t i = from; i > to; i += incr) {
 						arr[idx++] = i;
 					}
 				}

[AThousandShips]

Might try work on this when I have more time, otherwise my above code and #67025 can be used as starting points for someone wanting to solve it