Skip to content

Fix recoverable [U]Int128 division-by-zero #77854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 7, 2024
Merged

Fix recoverable [U]Int128 division-by-zero #77854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a609335
Fix recoverable [U]Int128 division-by-zero
oscbyspro Nov 27, 2024
43d98e6
Add preconditions to trapping [U]Int128 division methods.
oscbyspro Nov 29, 2024
13fac3a
Make consistent use of `_slowPath(_:)`.
oscbyspro Nov 29, 2024
09d91ca
Copy existing UInt128 division comments to division operators.
oscbyspro Nov 29, 2024
cdd2006
Add a comment about signed remainder overflow semantics as requested.
oscbyspro Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions stdlib/public/core/Int128.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,12 @@ extension Int128 {
public func dividedReportingOverflow(
by other: Self
) -> (partialValue: Self, overflow: Bool) {
_precondition(other != .zero, "Division by zero")
if self == .min && other == -1 { return (.min, true) }
if _slowPath(other == .zero) {
Copy link
Contributor

@stephentyrone stephentyrone Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there an observed performance issue that these _slowPath annotations are intended to address, or were they added speculatively to match the implementations for other Integer types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are speculative but match the implementation of smaller binary integer types. I can remove them if you believe they are unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More of a matter of my own curiosity. I'm ok with leaving them in, though they shouldn't matter much.

return (self, true)
}
if _slowPath(self == .min && other == (-1 as Self)) {
return (.min, true)
}
return (Self(Builtin.sdiv_Int128(self._value, other._value)), false)
}

Expand All @@ -307,8 +311,15 @@ extension Int128 {
public func remainderReportingOverflow(
dividingBy other: Self
) -> (partialValue: Self, overflow: Bool) {
_precondition(other != .zero, "Division by zero in remainder operation")
if self == .min && other == -1 { return (0, true) }
if _slowPath(other == .zero) {
return (self, true)
}
// This case is interesting because the remainder does not overflow; the
// analogous division does. Counting it as overflowing is consistent with
// documented behavior.
if _slowPath(self == .min && other == (-1 as Self)) {
Copy link
Contributor

@stephentyrone stephentyrone Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is somewhat interesting because the remainder does not overflow; rather the analogous division does. It's entirely plausible to me that this was originally defined "wrong" on the other integer types, but we're probably stuck with the current behavior. In any case, I think it merits a comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I return the equivalent of (0, false) in my project. So, I considered asking about this specific interaction, but I figured Swift chose C++ semantics ± trapping on undefined behavior. Is there a particular comment you have in mind?

Copy link
Collaborator

@xwu xwu Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the specified behavior—I was the one who aligned the actual behavior with the specification in #14219—but for this reason the wording of the corresponding precondition failure (at least at the time) was that "division results in overflow in remainder operation."

The current corresponding documentation for this operation on the other integer types correctly documents the subtlety in this regard: "Returns the remainder after dividing this value by the given value, along with a Boolean value indicating whether overflow occurred during division."

[Edit: Ah, so, refreshing my memory—my involvement in this was to make the *ReportingOverflow methods return a result instead of (clearly erroneously) trapping at runtime. It's always been the case (since before Swift 3 at least) that, for example, Int.min % -1 traps at runtime and complains about overflow. Hence, since I had to make remainderReportingOverflow return something for overflow, the line of thinking must have been essentially what @oscbyspro describes, that the "obvious" result here was to report true since the corresponding trapping operator does, in fact, trap and complain about overflow even if the notional operation doesn't.]

return (0, true)
}
return (Self(Builtin.srem_Int128(self._value, other._value)), false)
}
}
Expand Down Expand Up @@ -366,7 +377,13 @@ extension Int128 {
@available(SwiftStdlib 6.0, *)
@_transparent
public static func /(a: Self, b: Self) -> Self {
a.dividedReportingOverflow(by: b).partialValue
if _slowPath(b == .zero) {
_preconditionFailure("Division by zero")
}
if _slowPath(a == .min && b == (-1 as Self)) {
_preconditionFailure("Division results in an overflow")
}
return Self(Builtin.sdiv_Int128(a._value, b._value))
}

@available(SwiftStdlib 6.0, *)
Expand All @@ -378,7 +395,16 @@ extension Int128 {
@available(SwiftStdlib 6.0, *)
@_transparent
public static func %(a: Self, b: Self) -> Self {
a.remainderReportingOverflow(dividingBy: b).partialValue
if _slowPath(b == .zero) {
_preconditionFailure("Division by zero in remainder operation")
}
// This case is interesting because the remainder does not overflow; the
// analogous division does. Counting it as overflowing is consistent with
// documented behavior.
if _slowPath(a == .min && b == (-1 as Self)) {
_preconditionFailure("Division results in an overflow in remainder operation")
}
return Self(Builtin.srem_Int128(a._value, b._value))
}

@available(SwiftStdlib 6.0, *)
Expand Down
20 changes: 16 additions & 4 deletions stdlib/public/core/UInt128.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,9 @@ extension UInt128 {
public func dividedReportingOverflow(
by other: Self
) -> (partialValue: Self, overflow: Bool) {
_precondition(other != .zero, "Division by zero")
if _slowPath(other == .zero) {
return (self, true)
}
// Unsigned divide never overflows.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems ironic now, doesn't it?

Copy link
Contributor Author

@oscbyspro oscbyspro Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a way, I suppose. I would say that the ironic part is the overflow return label, however, since it represents both proper overflow and another kind of failure (division by zero). Still, I can remove it if you find it unfitting. Generally, I don't mess with comments I did not write.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to leave it, I think.

return (Self(Builtin.udiv_Int128(self._value, other._value)), false)
}
Expand All @@ -290,7 +292,9 @@ extension UInt128 {
public func remainderReportingOverflow(
dividingBy other: Self
) -> (partialValue: Self, overflow: Bool) {
_precondition(other != .zero, "Division by zero in remainder operation")
if _slowPath(other == .zero) {
return (self, true)
}
// Unsigned divide never overflows.
return (Self(Builtin.urem_Int128(self._value, other._value)), false)
}
Expand Down Expand Up @@ -349,7 +353,11 @@ extension UInt128 {
@available(SwiftStdlib 6.0, *)
@_transparent
public static func /(a: Self, b: Self) -> Self {
a.dividedReportingOverflow(by: b).partialValue
if _slowPath(b == .zero) {
_preconditionFailure("Division by zero")
}
// Unsigned divide never overflows.
return Self(Builtin.udiv_Int128(a._value, b._value))
}

@available(SwiftStdlib 6.0, *)
Expand All @@ -361,7 +369,11 @@ extension UInt128 {
@available(SwiftStdlib 6.0, *)
@_transparent
public static func %(a: Self, b: Self) -> Self {
a.remainderReportingOverflow(dividingBy: b).partialValue
if _slowPath(b == .zero) {
_preconditionFailure("Division by zero in remainder operation")
}
// Unsigned divide never overflows.
return Self(Builtin.urem_Int128(a._value, b._value))
}

@available(SwiftStdlib 6.0, *)
Expand Down