Added windows cross compile builds & fixed build issues

[ashbob999]

• Added cross compiling builds (x64, x86, ARM64) for windows (MSVC), and fixed build issues.
• Added support for using MSVC ARM intrinsics.
• Added tests for sz_u32_clz which will help catch if the tests are run without BMI support.
• Fixed build issues when building MacOS Universal2 binaries.
• Disabled AVX for 32-bit binaries.
• Fixed lots of issues when building a Windows 32-bit binary.
• Updated release.yml to build x86/x64/arm64 windows versions, and included the .lib file in the archive.
• Added checks for making sure stringzillite is built without any dependencies.
• Fixed Windows stringzillite might be broken #185

[ashbob999]

@ashvardanian
For the x86 (32-bit) builds, there are warnings about conversion loss of data, due to converison between sz_size_t and sz_u64_t.

sz_sorted_idx_t is defined as a sz_u64_t.

Functions where this happens: sz_partition, sz_merge, sz_sort_introsort_recursion, sz_sort_partial.

Plus there are other places where the sz_u64_t are used directly, but only to be converted to sz_size_t.

StringZilla/include/stringzilla/stringzilla.h

Lines 3478 to 3487 in a34d836

	SZ_PUBLIC void sz_sort_insertion(sz_sequence_t *sequence, sz_sequence_comparator_t less) {
	sz_u64_t *keys = sequence->order;
	sz_size_t keys_count = sequence->count;
	for (sz_size_t i = 1; i < keys_count; i++) {
	sz_u64_t i_key = keys[i];
	sz_size_t j = i;
	for (; j > 0 && less(sequence, i_key, keys[j - 1]); --j) keys[j] = keys[j - 1];
	keys[j] = i_key;
	}
	}

Other functions it occurs in: _sz_sift_down, sz_sort_introsort_recursion, sz_string_unpack.

So should all of these be changed to use sz_size_t?

Also should we be running the NumPy tests when building the python binaries?

[ashvardanian]

Hey @ashbob999! Those places require the unsigned integer to be 64 bit, so it shouldn't be changed.

[ashbob999]

Hey @ashbob999! Those places require the unsigned integer to be 64 bit, so it shouldn't be changed.

Although, if I have understood the sz_sort_insertion function correctly and the sz_sequence_t struct.

That the order member is a pointer to an array of indexes, which specify the sorted order, and the count is the number of indexes in the array. Then don't we already have a mismatch, because we have a uint32 for the count and a uint64 for the index of each string?

And because the order pointer array is populated with the data ptr from a std::vector, which the vector max size is 32-bit.

[ashbob999]

@ashvardanian what should I do about the warnings for the 32-builds (for the sorting functions)?

[ashvardanian]

@ashbob999, what's the cleanest way to reproduce some of those warnings/errors with GCC/Clang? It would be easier to choose a path forward if I can better understand their nature.

[ashbob999]

@ashvardanian

Just building it as 32-bit (m32 and adding -Wconversion although this adds other warnings for clang) should be enough. Although I cannot seem to reproduce some of the warnings that MSVC emits.

For example the warning about converting the result of sz_blend_u64 doesn't get emitted even though it is truncating from u64 to u32.

StringZilla/include/stringzilla/stringzilla.h

Line 3162 in dd2b949

*space = sz_u64_blend(SZ_STRING_INTERNAL_SPACE, string->external.space, is_big_mask);

But only a very few actually get emitted (GCC).

[1/2] Building CXX object CMakeFiles/stringzilla_test_cpp14.dir/scripts/test.cpp.o
In file included from /usr/include/c++/11/cassert:44,
                 from ../include/stringzilla/stringzilla.hpp:57,
                 from ../scripts/test.cpp:21:
../scripts/test.cpp: In function ‘void test_sequence_algorithms()’:
../scripts/test.cpp:1347:90: warning: conversion from ‘__gnu_cxx::__alloc_traits<std::allocator<long long unsigned int>, long long unsigned int>::value_type’ {aka ‘long long unsigned int’} to ‘std::vector<std::__cxx11::basic_string<char> >::size_type’ {aka ‘unsigned int’} may change value [-Wconversion]
 1347 |             for (std::size_t i = 1; i != dataset_size; ++i) { assert(dataset[order[i - 1]] <= dataset[order[i]]); }
      |                                                                                          ^
../scripts/test.cpp:1347:111: warning: conversion from ‘__gnu_cxx::__alloc_traits<std::allocator<long long unsigned int>, long long unsigned int>::value_type’ {aka ‘long long unsigned int’} to ‘std::vector<std::__cxx11::basic_string<char> >::size_type’ {aka ‘unsigned int’} may change value [-Wconversion]
 1347 |             for (std::size_t i = 1; i != dataset_size; ++i) { assert(dataset[order[i - 1]] <= dataset[order[i]]); }
      |                                                                                                               ^
[2/2] Linking CXX executable stringzilla_test_cpp14

But for whatever reason (maybe my building of the 32-bit with GCC/Clang is broken) but if I build it as 64-bit but with sz_size_t and sz_ssize_t changed to be uint32_t and int32_t respectively, then I get the warnings that MSVC emitted.

[ashvardanian]

Can you think of any better way of forcing the 4B limit on the user?

I would stick to size_t for sizes and focus on designing a better sorting algorithm. Long-term sustainable solutions are better than short-term gains 😉

[ashvardanian]

Hey @ashbob999! I'll have more time to look into this next week.

I want to thank you for following the git message style - it's very pleasing to see! Unlike most PRs, where I end up squashing the patches and merging under a new name, I would love to preserve your entire commit history. I'd just ask you to reformat/squash the last three commits that deviate from the style. Thanks again!

[ashbob999]

@ashvardanian no problem, I purposely did not make them that way jsut because they were fixing issues that I noticed from previous commits in the merge.

I can squash them into a single one, labelled as minor fixes (or equivalent), do you also want me to rebate to fix one in the middle a35cc50?

[recurseml]

⚠️ Only 5 files will be analyzed due to processing limits.

[recurseml]

⚠️ Only 5 files will be analyzed due to processing limits.

[recurseml]

Incorrect function declaration. _allmul is a compiler intrinsic that must return ULONGLONG (64-bit unsigned integer), but it's declared as void. This could cause issues with the compiler's handling of the return value and potentially lead to runtime errors. The function should be declared as: 'ULONGLONG __declspec(naked) _allmul()'.

📚 Relevant Docs

---

React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)

[recurseml]

Incorrect function declaration. _aullrem is a compiler intrinsic that must return ULONGLONG (64-bit unsigned integer), but it's declared as void. This mismatched return type could cause improper handling of the remainder value in arithmetic operations. The function should be declared as: 'ULONGLONG __declspec(naked) _aullrem()'.

📚 Relevant Docs

---

React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)

[recurseml]

Potential integer overflow and incorrect size handling in sz_fill_serial(). The multiplication of value by 0x0101010101010101ull assumes a 64-bit size_t. On 32-bit platforms, this will result in undefined behavior due to integer overflow, as sz_size_t would be 32-bit. This affects memory filling operations.

📚 Relevant Docs

---

React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)

[recurseml]

Incorrect comment indicates misaligned memory access prevention, but the code actually doesn't handle alignment properly for both 32-bit and 64-bit architectures, which could lead to undefined behavior on certain platforms. The alignment check should use sizeof(sz_size_t) consistently rather than hardcoded values.

---

React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)

[recurseml]

😱 Found 4 issues. Time to roll up your sleeves! 😱