GH-131296: fix clang-cl warning on Windows in longobject.c for 32bit builds

[chris-eibl]

I think this is a skip news?

• Issue: clang-cl issues many warnings when building on Windows #131296

[chris-eibl]

Fix warning : result of comparison of constant 307445734561825860 with expression of type 'Py_ssize_t' (aka 'int') is always true [-Wtautological-constant-out-of-range-compare]

FTR: this is a general 32bit warning and not only related to 32bit clang-cl builds on Windows, see e.g. ARM Raspbian PR.

warning: comparison is always true due to limited range of data type [-Wtype-limits]

Interesting, that clang already emits this with no warning level given, but GCC only with -Wextra (which is used in Python builds anyway): https://godbolt.org/z/d3r4sP1q3

[skirpichev]

Maybe you could rather use SIZEOF_SIZE_T == 8 as test.

[chris-eibl]

Yeah, I think there exist no platforms with SIZEOF_SIZE_T being between 4 and 8. But there could be some with SIZEOF_SIZE_T being greater than 8 (hypothetically). So let's use SIZEOF_SIZE_T >= 8?

[skirpichev]

But there could be some with SIZEOF_SIZE_T being greater than 8 (hypothetically).

I doubt, maybe only for some (unknown) processor with 128-bit byte addressing.

[chris-eibl]

Changed to SIZEOF_SIZE_T == 8. In case such hypothetical platforms ever show up, we can anyways change again ...

[picnixz]

FTR, if we were to assume that SIZEOF_SIZE_T != 4 or 8, we would have some pretty bad surprises elsewhere:

Strings and codecs:

cpython/Objects/stringlib/find_max_char.h

Lines 10 to 16 in 5707837

	#if (SIZEOF_SIZE_T==8)
	# defineUCS1_ASCII_CHAR_MASK 0x8080808080808080ULL
	#elif (SIZEOF_SIZE_T==4)
	# defineUCS1_ASCII_CHAR_MASK 0x80808080U
	#else
	# error C 'size_t' size should be either 4 or 8!
	#endif

cpython/Objects/stringlib/codecs.h

Lines 11 to 17 in 5707837

	#if (SIZEOF_SIZE_T==8)
	# defineASCII_CHAR_MASK 0x8080808080808080ULL
	#elif (SIZEOF_SIZE_T==4)
	# defineASCII_CHAR_MASK 0x80808080U
	#else
	# error C 'size_t' size should be either 4 or 8!
	#endif

Free-threaded build:

cpython/Objects/object.c

Lines 355 to 365 in 5707837

	# ifdefPy_REF_DEBUG
	staticint
	is_dead(PyObject*o)
	{
	# ifSIZEOF_SIZE_T==8
	return (uintptr_t)o->ob_type==0xDDDDDDDDDDDDDDDD;
	# else
	return (uintptr_t)o->ob_type==0xDDDDDDDD;
	# endif
	}
	# endif

Windows in general:

cpython/PC/pyconfig.h.in

Lines 342 to 377 in 5707837

	#ifdefMS_WIN64
	/* maintain "win32" sys.platform for backward compatibility of Python code,
	the Win64 API should be close enough to the Win32 API to make this
	preferable */
	# definePLATFORM "win32"
	# defineSIZEOF_VOID_P 8
	# defineSIZEOF_TIME_T 8
	# defineSIZEOF_OFF_T 4
	# defineSIZEOF_FPOS_T 8
	# defineSIZEOF_HKEY 8
	# defineSIZEOF_SIZE_T 8
	# defineALIGNOF_SIZE_T 8
	# defineALIGNOF_MAX_ALIGN_T 8
	/* configure.ac defines HAVE_LARGEFILE_SUPPORT iff
	sizeof(off_t) > sizeof(long), and sizeof(long long) >= sizeof(off_t).
	On Win64 the second condition is not true, but if fpos_t replaces off_t
	then this is true. The uses of HAVE_LARGEFILE_SUPPORT imply that Win64
	should define this. */
	# defineHAVE_LARGEFILE_SUPPORT
	#elif defined(MS_WIN32)
	# definePLATFORM "win32"
	# defineHAVE_LARGEFILE_SUPPORT
	# defineSIZEOF_VOID_P 4
	# defineSIZEOF_OFF_T 4
	# defineSIZEOF_FPOS_T 8
	# defineSIZEOF_HKEY 4
	# defineSIZEOF_SIZE_T 4
	# defineALIGNOF_SIZE_T 4
	/* MS VS2005 changes time_t to a 64-bit type on all platforms */
	# if defined(_MSC_VER) &&_MSC_VER >= 1400
	# defineSIZEOF_TIME_T 8
	# else
	# defineSIZEOF_TIME_T 4
	# endif
	# defineALIGNOF_MAX_ALIGN_T 8
	#endif

There are other places where we check for sizeof(size_t) == 4 or 8, even in configure.ac. So we can safely assume that we're having 4 or 8.

[chris-eibl]

Yeah, I know. Didn't want to introduce more of them, but I am fine either way :)

And there are two #if SIZEOF_SIZE_T > 4 occurences in the current code base and one #if SIZEOF_SIZE_T < 8.

So many ways to skin that cat 🚀

[picnixz]

Does this one need a backport?

[chris-eibl]

#131296 (comment)

Let's not backport those because I suspect it will sometimes be hard (hence the "feature")

So far, non of the warnings I've fixed due to clang-cl have been backported. None of them was hard, though, but mostly, because many (all?) of them were clang-cl / Windows specific - and clang-cl is an unsupported compiler #131296 (comment):

I do consider uninvestigated warnings to be bugs. But when it isn't a https://peps.python.org/pep-0011/ tier platform arguing for it can be hard. I think clang-cl is close enough even if not on a tier yet as we obviously already care about clang warnings on other platforms that its value is demonstrable.

Maybe I've missed to mark some of them when the warning was not specific to only this combination, but here I noticed.

So indeed, this one could be packported, since it affects other platforms / compilers, too.

Whether that should be done I'm not experienced enough to answer.

My gut feeling: +0

[picnixz]

None of them was hard, though, but mostly, because many (all?) of them were clang-cl / Windows specific - and clang-cl is an unsupported compiler

I think I said "no backports" if those warnings were actually non-fatal, but here isn't it possible for the assertion to actually fail on some debug builds? if so it's better to backport it.

[chris-eibl]

The assert won't fire, the warning is actually about that it will never fire on 32bit debug builds:

Objects/longobject.c:912:24: warning: comparison is always true due to limited range of data type [-Wtype-limits]

It's really just to silence the warning, so maybe no backport then?

Unless we'd like to treat the warning as a bug, like Gregory said in #131296 (comment)

I do consider uninvestigated warnings to be bugs.

[picnixz]

Oh ok! so no backport then. We can only backport stuff to 3.13 now so I don't think it's worth it. Let's only backport stuff if there can be issues at runtime.