aleaxit / gmpy

General Multi-Precision arithmetic for Python 2.6+/3+ (GMP, MPIR, MPFR, MPC)
https://gmpy2.readthedocs.io/en/latest/
GNU Lesser General Public License v3.0
516 stars 87 forks source link

Use int instead of char for flag variables #490

Closed jamesjer closed 2 months ago

jamesjer commented 2 months ago

I attempted to build version 2.2.0 for Fedora Rawhide. All of the non-x86 architectures (aarch, ppc64le, and s390x) failed a test:

=================================== FAILURES ===================================
_____________________________ test_mpz_from_bytes ______________________________

    @settings(max_examples=1000)
>   @given(integers(), integers(min_value=0, max_value=10000),
           sampled_from(['big', 'little']), booleans())

test/test_mpz.py:108: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/lib/python3.13/site-packages/hypothesis/core.py:1300: in _raise_to_user
    raise the_error_hypothesis_found
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

x = -2, length = 5, byteorder = 'little', signed = True

    @settings(max_examples=1000)
    @given(integers(), integers(min_value=0, max_value=10000),
           sampled_from(['big', 'little']), booleans())
    @example(0, 0, 'big', False)
    @example(0, 0, 'little', False)
    @example(0, 1, 'big', False)
    @example(128, 1, 'big', True)
    @example(128, 1, 'little', True)
    @example(-129, 1, 'big', True)
    @example(-129, 1, 'little', True)
    @example(-1, 0, 'big', True)
    @example(-1, 1, 'big', True)
    @example(-1, 1, 'little', True)
    @example(-2, 0, 'big', True)
    @example(-2, 0, 'little', True)
    @example(-1, 3, 'big', True)
    @example(-2, 3, 'big', True)
    @example(-2, 5, 'little', True)
    def test_mpz_from_bytes(x, length, byteorder, signed):
        try:
            bytes = x.to_bytes(length, byteorder, signed=signed)
        except OverflowError:
            assume(False)
        else:
            rx = int.from_bytes(bytes, byteorder, signed=signed)
>           assert rx == mpz.from_bytes(bytes, byteorder, signed=signed)
E           AssertionError: assert -2 == mpz(-4294967297)
E            +  where mpz(-4294967297) = <built-in method from_bytes of type object at 0xffffa57105b0>(b'\xfe\xff\xff\xff\xff', 'little', signed=True)
E            +    where <built-in method from_bytes of type object at 0xffffa57105b0> = mpz.from_bytes
E           Falsifying explicit example: test_mpz_from_bytes(
E               x=-2,
E               length=5,
E               byteorder='little',
E               signed=True,
E           )

test/test_mpz.py:132: AssertionError

The problem is the use of the char type for flag variables. The C standard says that char can be equivalent to either signed char or unsigned char. The code in GMPy_MPZ_Method_From_Bytes assigns -1 to such a variable, endian. On platforms where char == unsigned char, the value stored is actually 255, a positive value.

This PR converts flag variables declared with type char to type int instead, for the following reasons:

  1. Using char doesn't save any space. These are local variables, so they are either in a CPU register (which is 32 or 64 bits, even if we only use 8 bits), or on the stack where, for alignment reasons, 32 bits will be reserved even though we only use 8 bits of it.
  2. Using char instead of int generates less efficient code. Modern 64-bit processors are optimized for processing 64-bit and 32-bit quantities. They can operate on 8-bit quantities, but take more clock cycles to do so.
  3. The char type is only useful for unsigned 7-bit values; i.e., 0 to 127. That is the intersection of the signed char and unsigned char types.

With this change, the build succeeds on all architectures.

codecov-commenter commented 2 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.50%. Comparing base (224745b) to head (4b3aee6). Report is 30 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #490 +/- ## ========================================== - Coverage 85.52% 85.50% -0.03% ========================================== Files 50 50 Lines 11656 11646 -10 Branches 2202 2201 -1 ========================================== - Hits 9969 9958 -11 - Misses 1687 1688 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

skirpichev commented 2 months ago

Windows fix: #491

jamesjer commented 2 months ago

I think the PR comment is unnecessarily verbose. How does this look?

skirpichev commented 2 months ago

Yep, fine.

casevh commented 2 months ago

Thanks for the fix.

skirpichev commented 2 months ago

Maybe this should go to the next bugfix release?

casevh commented 2 months ago

I agree. I can release 2.2.1 this weekend.

On Thu, Jul 11, 2024, 11:46 PM Sergey B Kirpichev @.***> wrote:

Maybe this should go to the next bugfix release?

— Reply to this email directly, view it on GitHub https://github.com/aleaxit/gmpy/pull/490#issuecomment-2224927276, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMR233MLC63XWDGHKDBIM3ZL53TZAVCNFSM6AAAAABKTGOBYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRUHEZDOMRXGY . You are receiving this because you modified the open/close state.Message ID: @.***>