mpz_get_si: possible undefined behaviour? [Was: Micro-GMP]

[Marco Bodrato]

Ciao,

Il 2018-12-03 13:43 Vincent Lefevre ha scritto:
> On 2018-12-03 13:04:11 +0100, Torbjorn Granlund wrote:
>> 2. For signed source types, the + 1 - 1 trickery might help for 
>> MIN_INT

> with the type of x. In such a case, the + 1 - 1 trickery is harmless,

It is wonderful to count how many messages we wrote about a single line 
in the code, defining a harmless macro :-D


Thanks to the attention focused (by Paul's ideas) on the _si functions 
and to Vincent's observation, I looked again into our implementation of 
the function mpz_get_si (z), an I suspect a possible issue.

Both in the main library and in mini- we have more or less the following 
lines:

  if (size < 0)
    /* This expression is necessary to properly handle LONG_MIN */
    return -1 - (long) ((zp[0] - 1) & LONG_MAX);

Assume z represents -LONG_MAX-1, then the size is negative and
zp[0] == LONG_MAX + 1

i.e.

- (long) ((zp[0] - 1) & LONG_MAX) == -LONG_MAX

The next step is: subtract 1.

If LONG_MIN == -LONG_MAX-1, this step is correct.
But, what about the LONG_MIN == -LONG_MAX case?
The result is undefined, isn't it?

Which value we should actually return when the given mpz does not fit in 
the target type is not so well defined nor important, but triggering an 
undefined behaviour seems a bad idea anyway.


I'd suggest to substitute "-1" with "(LONG_MIN + LONG_MAX)", I mean 
something like:

return (LONG_MIN + LONG_MAX) -
        (long) ((zp[0] - (-LONG_MAX - LONG_MIN)) & LONG_MAX);


Comments about this analysis or better solutions?

Regards,
m