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

chris-eibl · 2025-03-23T09:33:04Z

I think this is a skip news?

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

chris-eibl · 2025-03-23T09:44:46Z

Objects/longobject.c

@@ -909,7 +909,9 @@ _PyLong_NumBits(PyObject *vv)
    assert(ndigits == 0 || v->long_value.ob_digit[ndigits - 1] != 0);
    if (ndigits > 0) {
        digit msd = v->long_value.ob_digit[ndigits - 1];
+#if SIZEOF_SIZE_T > 4


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 · 2025-04-17T06:50:23Z

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 · 2025-04-17T07:32:58Z

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 · 2025-04-17T16:29:45Z

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

picnixz · 2025-04-17T16:46:00Z

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) 
        
           # define UCS1_ASCII_CHAR_MASK 0x8080808080808080ULL 
        
           #elif (SIZEOF_SIZE_T == 4) 
        
           # define UCS1_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) 
        
           # define ASCII_CHAR_MASK 0x8080808080808080ULL 
        
           #elif (SIZEOF_SIZE_T == 4) 
        
           # define ASCII_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

    
           # ifdef Py_REF_DEBUG 
        
           static int 
        
           is_dead(PyObject *o) 
        
           { 
        
           #  if SIZEOF_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

    
           #ifdef MS_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 */ 
        
           #       define PLATFORM "win32" 
        
           #       define SIZEOF_VOID_P 8 
        
           #       define SIZEOF_TIME_T 8 
        
           #       define SIZEOF_OFF_T 4 
        
           #       define SIZEOF_FPOS_T 8 
        
           #       define SIZEOF_HKEY 8 
        
           #       define SIZEOF_SIZE_T 8 
        
           #       define ALIGNOF_SIZE_T 8 
        
           #       define ALIGNOF_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. */ 
        
           #       define HAVE_LARGEFILE_SUPPORT 
        
           #elif defined(MS_WIN32) 
        
           #       define PLATFORM "win32" 
        
           #       define HAVE_LARGEFILE_SUPPORT 
        
           #       define SIZEOF_VOID_P 4 
        
           #       define SIZEOF_OFF_T 4 
        
           #       define SIZEOF_FPOS_T 8 
        
           #       define SIZEOF_HKEY 4 
        
           #       define SIZEOF_SIZE_T 4 
        
           #       define ALIGNOF_SIZE_T 4 
        
                   /* MS VS2005 changes time_t to a 64-bit type on all platforms */ 
        
           #       if defined(_MSC_VER) && _MSC_VER >= 1400 
        
           #       define SIZEOF_TIME_T 8 
        
           #       else 
        
           #       define SIZEOF_TIME_T 4 
        
           #       endif 
        
           #       define ALIGNOF_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 · 2025-04-17T17:08:06Z

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 · 2025-04-18T08:45:50Z

Does this one need a backport?

chris-eibl · 2025-04-18T09:05:22Z

#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 · 2025-04-18T09:27:32Z

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 · 2025-04-18T09:38:23Z

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 · 2025-04-18T09:41:34Z

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.

fix clangcl warning for 32bit builds

248c6b5

bedevere-app bot added the awaiting review label Mar 23, 2025

bedevere-app bot mentioned this pull request Mar 23, 2025

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

Open

chris-eibl commented Mar 23, 2025

View reviewed changes

picnixz added the skip news label Mar 30, 2025

chris-eibl requested review from skirpichev and picnixz April 15, 2025 16:10

skirpichev approved these changes Apr 16, 2025

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Apr 16, 2025

use SIZEOF_SIZE_T == 8

dd92ba1

picnixz approved these changes Apr 17, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Apr 17, 2025

picnixz self-assigned this Apr 17, 2025

picnixz merged commit 80295a8 into python:main Apr 18, 2025
45 checks passed

bedevere-app bot removed the awaiting merge label Apr 18, 2025

chris-eibl deleted the fix_clangcl_longobject branch April 18, 2025 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Strings and codecs:

Free-threaded build:

Windows in general:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!