BUG: Fix signed overflow issue in npy_gcd for INT_MIN on s390x#31360
Conversation
|
Since this is undefined behavior, shouldn't it be fixed for all the platforms, even if it manifests only on s390x? |
|
I'd agree, compilers presumably are smart enough to do whatever is best on the platform, so we can just keep the code identical. But if we bother to guarantee this, we should also add a test. (Ideally for all relevant combinations here.) |
…r types in test_gcd_overflow
|
Thank you for the quick review! |
|
Hi @seberg , |
|
Just ignore it, MKL had an update or so and now it's failing everywhere, we need to see what is actually wrong (but I am not sure anyone yet did). |
| # not relevant for lcm, where the result is unrepresentable anyway | ||
| # verify that we don't overflow when taking abs(x) for INT_MIN | ||
| # this was undefined behavior that manifested on s390x with GCC 11.5 | ||
| for dtype in (np.int8, np.int16, np.int32, np.int64): |
There was a problem hiding this comment.
that manifested on s390x with GCC 11.5
Ah, so we already have a test that but it only manifests with old GCC? Do we need a test for q == INT_MIN as well, since you are fixing it?
There was a problem hiding this comment.
Yes, the existing test test_gcd_overflow was already there but it only caught the bug on old GCC (11.5) on s390x. On other platforms like macOS ARM64 or x86 with newer GCC, the test passed even before the fix.
Regarding q == INT_MIN — I investigated and found even with the current fix, gcd(INT_MIN, INT_MIN) still returns a negative value because abs(INT_MIN) cannot be represented in the same signed type.
For example i have tested math.gcd(-128, -128) returns 128 but np.gcd returns -128 for int8. Should I open a separate issue to track this?
I have also added test cases where INT_MIN is passed as the second argument to verify commutativity — ensuring the fix works regardless of argument order.
There was a problem hiding this comment.
Yeah, I was just surprised since we have s390x CI. So it seems like the issue almost went away by itself as gcc 11.5 is pretty old (not that I mind the change, seems safe enough).
There was a problem hiding this comment.
That makes sense!
Since the fix is safe and correct for all platforms, it is better to have it. future compiler updates or optimization level changes could still trigger it. Happy to make any further adjustments if needed!
There was a problem hiding this comment.
If you do an objdump on numpy/_core/_multiarray_umath.cpython-314-s390x-linux-gnu.so you can see the bug triggered on this specific configuration with GCC 11.5 on s390x:
lpr %r2,%r4 # overflows INT_MIN so it stays negative
dsgfr %r0,%r2 # signed division with negative value which propagates further, aka our bug
When using -fwrapv:
lpr %r5,%r3 # Still overflow, INT_MIN negative
llgfr %r4,%r5 # zero-extend so the value is treated as unsigned
dlgr %r2,%r4 # unsigned division which produces the correct result
So with -fwrapv the issue is "resolved" in a way, but the original UB is still there.
The problem is in the lpr, which instead of computing the absolute value, it propagates a negative one.
Haven't disassembled with the patch from the PR, but I think that should resolve it.
|
Thanks @stratakis and @AnkitAhlawat7742, let's just get it in. Simple enough, and if we already have a test for it good to just avoid the UB probably. |
This PR fixes signed overflow issue in npy_gcd when one of the inputs is the minimum value of a signed integer type (INT_MIN) on s390x architecture. issue was the expression a < 0 ? -a : a in npy_math_internal.h.src causes undefined behavior when a = INT_MIN because negating INT_MIN overflows a signed integer on s390x. i see it is working fine with other arch so fix only provided to s390x
BUG: Fix signed overflow issue in npy_gcd for INT_MIN on s390x (#31360)
…#31360) This PR fixes signed overflow issue in npy_gcd when one of the inputs is the minimum value of a signed integer type (INT_MIN) on s390x architecture. issue was the expression a < 0 ? -a : a in npy_math_internal.h.src causes undefined behavior when a = INT_MIN because negating INT_MIN overflows a signed integer on s390x. i see it is working fine with other arch so fix only provided to s390x
FIX #31359
PR summary
This PR fixes signed overflow issue in npy_gcd when one of the inputs is the minimum value of a signed integer type (INT_MIN) on s390x architecture.
issue was the expression a < 0 ? -a : a in npy_math_internal.h.src causes undefined behavior when a = INT_MIN because negating INT_MIN overflows a signed integer on s390x. i see it is working fine with other arch so fix only provided to s390x
AI Disclosure
No AI tools were used in identifying, reproducing, or fixing this bug. The bug was identified by running below code after updating the code ..before the code update i see the mismatch in expected and actual result which i informed in issue #31359