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

'Array count too large' when declaring large inline arrays in structs #4658

Open
SaculRennorb opened this issue Jan 5, 2025 · 4 comments
Open

Comments

@SaculRennorb
Copy link

SaculRennorb commented Jan 5, 2025

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

  Odin:    dev-2024-12-nightly:cf53404
  OS:      Windows 10 Enterprise N (version: 20H2), build 19042.1466   
  CPU:     Intel(R) Core(TM) i5-8400 CPU @ 2.80GHz
  RAM:     65470 MiB
  Backend: LLVM 18.1.8

Expected Behavior

This

package main

A :: struct {
	a : [8]u8, // good 
	b : [1 * 1024 * 1024 * 1024]u8, // not good 
}

main :: proc() {  }

should compile without issue. While instantiating such a struct would most likely blow the stack, just declaring it should not be a problem.

Current Behavior

The build errors with Array count too large, 1073741824

Failure Information (for bugs)

Steps to Reproduce

Please provide detailed steps for reproducing the issue.

  1. copy the provided snippet into a new file
  2. run odin build on the file
  3. error

there is some ongoing discussion on the discord about the exact cause of this;
https://discord.com/channels/568138951836172421/568871298428698645/1325284384969986099
https://discord.com/channels/568138951836172421/1325287603284738130
but im going to open a bug report for now

@SaculRennorb SaculRennorb changed the title 'Array count too large' for large inline arrays 'Array count too large' when declaring large inline arrays in structs Jan 5, 2025
@alektron
Copy link

alektron commented Jan 5, 2025

I believe I may have some insight into this already.
Forgive me if I am completely off, this is my first time poking into the compiler.

This is the code in check_type.cpp that emits the error:

switch (count.used) {
case 0: return 0;
case 1: return big_int_to_u64(&count);
}
gbAllocator a = heap_allocator();
String str = big_int_to_string(a, &count);
error(e, "Array count too large, %.*s", LIT(str));
gb_free(a, str.text);

If I understand correctly count.used is checking for the number of "storage elements" of the BigInt that are in use.
The data type to use for storage can be set via the MP_16BIT and MP_64BIT defines (with 32 bit being the default).

I debugged that function and the data type used seems to be U32.
However the code assumes it to be U64 and only checks used for 1 when it must check it for <=2 (or the MP_64BIT define must be set).

I am not entirely sure why the BigInt implementation requires 2 elements to store 1024 * 1024 * 1024 == 1073741824 but it seems to be the case here.

@novicearts
Copy link

Turns out this problem is more general than just inline arrays in structs.
If I try to create a large global array in general, I get the same error as well.

GLOBAL_BACKING_BUFFER_0 := [255*cast(u64)mem.Megabyte]byte{}
255 MB is the upper limit of what I can define in the array.

Go one above that and you get the following error:
image

@laytan
Copy link
Collaborator

laytan commented Jan 8, 2025

I can increase the limit to max(i64) without too many changes, not sure if people would be satisfied with that?

diff --git a/src/check_expr.cpp b/src/check_expr.cpp
index 231ece2f4..7a4c13d59 100644
--- a/src/check_expr.cpp
+++ b/src/check_expr.cpp
@@ -4841,9 +4841,18 @@ gb_internal bool check_index_value(CheckerContext *c, Type *main_type, bool open
 
 			} else { // NOTE(bill): Do array bound checking
 				i64 v = -1;
-				if (i.used <= 1) {
-					v = big_int_to_i64(&i);
+				switch (i.used) {
+				case 0: v = 0; break;
+				case 1: v = big_int_to_i64(&i); break;
+				default:
+					static BigInt max = big_int_make_i64(I64_MAX);
+					if (big_int_cmp(&max, &i) > 0) {
+						v = big_int_to_i64(&i);
+						GB_ASSERT(v >= 0);
+					}
+					break;
 				}
+
 				if (value) *value = v;
 				bool out_of_bounds = false;
 				if (v < 0) {
diff --git a/src/check_type.cpp b/src/check_type.cpp
index 44108ccbe..775586ba5 100644
--- a/src/check_type.cpp
+++ b/src/check_type.cpp
@@ -2626,13 +2626,26 @@ gb_internal i64 check_array_count(CheckerContext *ctx, Operand *o, Ast *e) {
 				gb_free(a, str.text);
 				return 0;
 			}
+
 			switch (count.used) {
 			case 0: return 0;
-			case 1: return big_int_to_u64(&count);
+			case 1: return big_int_to_i64(&count);
+			default:
+				static BigInt max = big_int_make_i64(I64_MAX);
+				if (big_int_cmp(&max, &count) >= 0) {
+					i64 res = big_int_to_i64(&count);
+					GB_ASSERT(res >= 0);
+					return res;
+				}
+				break;
 			}
+
+			ERROR_BLOCK();
+
 			gbAllocator a = heap_allocator();
 			String str = big_int_to_string(a, &count);
 			error(e, "Array count too large, %.*s", LIT(str));
+			error_line("\tNote: The maximum array count is %td\n", I64_MAX);
 			gb_free(a, str.text);
 			return 0;
 		}

LLVM accepts up to max(u64), but that would require some types to change in the compiler frontend.

You could probably support "unlimited" by just making it multiple arrays in the backend too, but that would be an undertaking.

@SaculRennorb
Copy link
Author

SaculRennorb commented Jan 8, 2025

Id say the change to max(i64) would already be good enough. i32 is just too little to work with on modern computers (if you use if for a byte array buffer).

"unlimited" size is pointless imo, since thats way more than any computer has memory ever. For the same reason i64 should easily be enough for any task, but i personally would prefer it being u64 since that makes more sense in my mind. (im aware it doesn't matter since the array length is const and just a untyped number afaict, it just feels wired talking about a length in terms of a signed integer)

u8 arrays are the extreme case, since its the smallest type and therefore needs the largest lengths. Al other types should therefore also be covered with i64.

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

No branches or pull requests

4 participants