GeezerVison

It's very tempting to convert all of these functions to use fixed-width integer types (uint16, uint32, uint64), but that seems like too invasive a change for a bugfix PR. It's something we could possibly do for 3.12 only.

It's very tempting to convert all of these functions to use fixed-width integer types (uint16, uint32, uint64), but that seems like too invasive a change for a bugfix PR. It's something we could possibly do for 3.12 only.

I agree, it would be better but best left for 3.12 only.

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit 1294440 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@kumaraditya303 I've updated this PR to be more efficient, while still avoiding undefined behaviour and implementation-defined behaviour (specifically, the conversion from an unsigned type to the corresponding signed type when the value being converted is not representable in the signed type; cf. C99 §6.3.1.3p3).

The sign-extension is now branch free (and should compile to a no-op in the case that the C type being used has exactly the correct width), and the conversion from unsigned to signed should always compile to a no-op on any semi-reasonable compiler.

Godbolt example for the case where sign extension is needed: https://godbolt.org/z/e8roKrn6r
Godbolt example for the case where no sign extension is needed: https://godbolt.org/z/bPhbsT9Kn

We are currently compiling all non-pydebug builds with -fwrapv (at least on gcc and clang). That means the shenanigans going on in _struct.c are actually perfectly defined behaviour.

I don't think we will turn off -fwrapv for 3.11. So there's no need for a stopgap solution.

I'd say: just do the Right Thing for 3.12 in main, and don't worry about 3.11. Especially if the Right Thing is very tempting to do anyway.

See also #96821 and #96678 (comment)

As a minimal change for 3.11, I would suggest enabling -fwrapv even when giving --with-pydebug.

That being said, I'm not opposed to this PR. It's a great start to getting everything safe for -fstrict-overflow.

I'd say: just do the Right Thing for 3.12 in main, and don't worry about 3.11. Especially if the Right Thing is very tempting to do anyway.

Agreed; I think I'll merge this for 3.12 only.

Some not very rigorous timings, on macOS/Intel, non-optimised non-debug build. (This PR is not primarily about performance, but it would be unfortunate if it caused a significant performance regression.)

On this branch:

mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('<hhh'); v=b'\x1b\xe0\xcfKSh'" "s.unpack(v)"
5000000 loops, best of 5: 92.6 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('>hhh'); v=b'\x1b\xe0\xcfKSh'" "s.unpack(v)"
5000000 loops, best of 5: 94.3 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('<iii'); v=b'\x1b\xe0\xcfKSh'*2" "s.unpack(v)"
5000000 loops, best of 5: 99.1 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('>iii'); v=b'\x1b\xe0\xcfKSh'*2" "s.unpack(v)"
5000000 loops, best of 5: 98.3 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('<qqq'); v=b'\x1b\xe0\xcfKSh'*4" "s.unpack(v)"
2000000 loops, best of 5: 99.1 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('>qqq'); v=b'\x1b\xe0\xcfKSh'*4" "s.unpack(v)"
2000000 loops, best of 5: 102 nsec per loop

On the main branch at commit 6281aff:

mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('<hhh'); v=b'\x1b\xe0\xcfKSh'" "s.unpack(v)"  
5000000 loops, best of 5: 94.9 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('>hhh'); v=b'\x1b\xe0\xcfKSh'" "s.unpack(v)"  
5000000 loops, best of 5: 98.2 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('<iii'); v=b'\x1b\xe0\xcfKSh'*2" "s.unpack(v)"
2000000 loops, best of 5: 99.1 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('>iii'); v=b'\x1b\xe0\xcfKSh'*2" "s.unpack(v)"
2000000 loops, best of 5: 104 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('<qqq'); v=b'\x1b\xe0\xcfKSh'*4" "s.unpack(v)"
2000000 loops, best of 5: 101 nsec per loop
mdickinson@lovelace cpython % ./python.exe -m timeit -s "import struct; s = struct.Struct('>qqq'); v=b'\x1b\xe0\xcfKSh'*4" "s.unpack(v)"
2000000 loops, best of 5: 112 nsec per loop