gh-141370: Fix undefined behavior when using Py_ABS()

[serhiy-storchaka]

• Issue: Undefined behavior in Py_ABS() macro with LLONG_MIN #141370

[skirpichev]

Can't we assume two's complement representation of integers and use simpler workaround? As this fix span several places, I would prefer if it could be refactored to a separate macro.

I.e. something like:

#defineMy_ABS(x, MAX) \
((x) < 0 ? ((x) >= -MAX ? -(x) : (U##MAX >> 1) + 1) : (x))

Nowadays two's complement is only case permitted by the C23 and is a de-facto standard. Do you have some system in mind on which we should care? I'm pretty sure all Tier 1-3 platforms fit to this picture.

[serhiy-storchaka]

All Tier 1-3 platforms perhaps fine with the current code. We change the code because we cannot be sure that it work on all exotic platforms and that future versions of the C compiler will not interpret an undefined behavior in interesting way.

The current gcc does not generate additions and substractions for this PR. It generates something more smart, although not so smart as for Py_ABS(). Although for your proposition it generates more complex code.

Details

#include<limits.h>#definePy_ABS(x) ((x) < 0 ? -(x) : (x))
#defineMy_ABS(x, MAX) \
((x) < 0 ? ((x) >= -MAX ? -(x) : (U##MAX >> 1) + 1) : (x))
unsigned intintabs0(intx) {
return (unsigned int)Py_ABS(x);
}
unsigned intintabs(intx) {
returnx<-INT_MAX
? (unsigned int)INT_MAX+ (unsigned int)-(x+INT_MAX)
: (unsigned int)Py_ABS(x);
}
unsigned intintabs2(intx) {
returnMy_ABS(x, INT_MAX);
}
unsigned longlongabs0(longx) {
return (unsigned long)Py_ABS(x);
}
unsigned longlongabs(longx) {
returnx<-LONG_MAX
? (unsigned long)LONG_MAX+ (unsigned long)-(x+LONG_MAX)
: (unsigned long)Py_ABS(x);
}
unsigned longlongabs2(longx) {
returnMy_ABS(x, LONG_MAX);
}

Anyway, the performance of this code is not critical (if there is any difference).

[skirpichev]

All Tier 1-3 platforms perhaps fine with the current code.

(Yes, and I suspect tests might be redundant in fact.)

Although for your proposition it generates more complex code.

A different version:

#define__GMP_CAST(type, expr) ((type) (expr))
#defineNEG_CAST(T,x) (- (__GMP_CAST (T, (x) + 1) - 1))
#defineABS_CAST(T,x) ((x) >= 0 ? __GMP_CAST (T, x) : NEG_CAST (T, x))

I found same approach in the GNU GMP, so just copied NIH code here.

Anyway, the performance of this code is not critical (if there is any difference).

Your solution looks ok for me. But in any case we should factor it to some macro.

[serhiy-storchaka]

For this version gcc generates exactly the same code as for the current code. But it is now free from undefined behavior.

[skirpichev]

LGTM

This version triggers a warning on M$ compiler, but this is probably ok.

[picnixz]

Suggested change

	#define_Py_ABS_CAST(T,x) ((x) >= 0 ? ((T) (x)) : (- (((T) ((x) + 1)) - 1)))
	#define_Py_ABS_CAST(T,x) ((x) >= 0 ? ((T) (x)) : (- (((T) ((x) + 1)) - 1)))

[picnixz]

For posterity:

_Py_ABS_CAST(uint8_t, (int8_t)-128) ==128

The (T)((x) + 1) - 1 is:

(
(
(uint8_t) (
(-128) +1// -127 (still int8_t)
) // 129 (2's complement on 8 bits)
) -1// 128 (as an uint8_t)
)

Since $-128\bmod{256} = 128$, we are good. For another number say -5 we have:

(
(
(uint8_t) (
(-5) +1// -4 (still int8_t)
) // 252 (2's complement on 8 bits)
) -1// 251 (as an uint8_t)
)

And now $-251 \bmod{256} = 5$ and we're good.

[miss-islington-app]

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

[miss-islington-app]

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 706fdda8b360120a25b272898df40c8913381723 3.13

[bedevere-app]

GH-142301 is a backport of this pull request to the 3.14 branch.

[bedevere-app]

GH-142304 is a backport of this pull request to the 3.13 branch.