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

oscbyspro · 2024-11-27T14:24:43Z

This patch fixes the division-by-zero case in the following methods:

Int128/dividedReportingOverflow(by:)
Int128/remainderReportingOverflow(dividingBy:)
UInt128/dividedReportingOverflow(by:)
UInt128/remainderReportingOverflow(dividingBy:)

Documentation: /dividedReportingOverflow(by:)

Dividing by zero is not an error when using this method. For a value x, the result of x.dividedReportingOverflow(by: 0) is (x, true).

Documentation: /remainderReportingOverflow(dividingBy:)

Dividing by zero is not an error when using this method. For a value x, the result of x.remainderReportingOverflow(dividingBy: 0) is (x, true).

This patch fixes the division-by-zero case in the following methods: - `Int128/dividedReportingOverflow(by:)` - `Int128/remainderReportingOverflow(dividingBy:)` - `UInt128/dividedReportingOverflow(by:)` - `UInt128/remainderReportingOverflow(dividingBy:)`

oscbyspro · 2024-11-28T13:56:12Z

Looking closer at the implementation of Int128 and UInt128, I see that I'll have to add preconditions to /(a:b:) and %(a:b:) following this change. It also looks like Int128's division operator doesn't trap on ordinary overflow either. I'll mark this pull request as a draft until I have updated /(a:b:) and %(a:b:).

Jumping on the `_slowPath(_:)` bandwagon like all other integer types.

oscbyspro · 2024-12-04T08:28:20Z

Legend has it that the cordial ping rate is once a week. @stephentyrone – can you take a look at this pull request? Int128 and UInt128 do not perform division by zero like the smaller binary integers. It would be nice if we could fix that.

stephentyrone · 2024-12-04T17:26:47Z

stdlib/public/core/Int128.swift

@@ -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) {


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?

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

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

stephentyrone · 2024-12-04T17:37:09Z

stdlib/public/core/Int128.swift

+    if _slowPath(other == .zero) {
+      return (self, true)
+    }
+    if _slowPath(self == .min && other == (-1 as Self)) {


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.

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?

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.]

xwu · 2024-12-05T16:03:17Z

stdlib/public/core/UInt128.swift

-    _precondition(other != .zero, "Division by zero")
+    if _slowPath(other == .zero) {
+      return (self, true)
+    }
    // Unsigned divide never overflows.


This comment seems ironic now, doesn't it?

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.

It's fine to leave it, I think.

@stephentyrone

I have paraphrased @stephentyrone's and @xwu's review comments to the best of my ability.

stephentyrone · 2024-12-07T15:12:25Z

@swift-ci test

stephentyrone · 2024-12-07T15:13:04Z

@oscbyspro do you have bandwidth to make another PR for the 6.1 branch? If not, I can do so for you.

oscbyspro · 2024-12-07T15:51:23Z

I can do it with some patience on your part.

I'm currently reading the documentation for how Swift handles release branches:

stephentyrone · 2024-12-07T17:43:20Z

Why don't you give it a go, and if you get stuck I'll help.

The quick version, though, is check out release/6.1, create a new branch based on that, and cherry-pick your change to that branch. Then create a PR against release/6.1 using that branch. Write a PR message that links back to this PR against main and clearly indicates that it is a cherry-pick. Looking at an example may be helpful: #77699

oscbyspro · 2024-12-07T18:47:35Z

I don't usually git cherry-pick in my projects so I got a bit confused when the cherry-picked hashes were different. I suppose I thought git would combine the changes of each commit in isolation. In any case, I appreciate your streamlined instructions.

oscbyspro requested a review from a team as a code owner November 27, 2024 14:24

oscbyspro marked this pull request as draft November 28, 2024 13:56

oscbyspro added 3 commits November 29, 2024 11:23

Add preconditions to trapping [U]Int128 division methods.

43d98e6

Make consistent use of _slowPath(_:).

13fac3a

Jumping on the `_slowPath(_:)` bandwagon like all other integer types.

Copy existing UInt128 division comments to division operators.

09d91ca

oscbyspro marked this pull request as ready for review November 29, 2024 10:38

oscbyspro mentioned this pull request Nov 30, 2024

128-bit core binary integer support oscbyspro/Ultimathnum#143

Open

stephentyrone reviewed Dec 4, 2024

View reviewed changes

xwu reviewed Dec 5, 2024

View reviewed changes

Add a comment about signed remainder overflow semantics as requested.

cdd2006

I have paraphrased @stephentyrone's and @xwu's review comments to the best of my ability.

stephentyrone approved these changes Dec 7, 2024

View reviewed changes

oscbyspro mentioned this pull request Dec 7, 2024

[6.1][stdlib] Fix recoverable [U]Int128 division-by-zero #78045

Merged

stephentyrone merged commit 4d43fa9 into swiftlang:main Dec 7, 2024
5 checks passed

oscbyspro deleted the fixes/128-bit-division-by-zero branch December 8, 2024 08:13

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

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

Uh oh!

Conversation

oscbyspro commented Nov 27, 2024

Documentation: /dividedReportingOverflow(by:)

Documentation: /remainderReportingOverflow(dividingBy:)

Uh oh!

oscbyspro commented Nov 28, 2024

Uh oh!

oscbyspro commented Dec 4, 2024

Uh oh!

stephentyrone Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscbyspro Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

stephentyrone Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

stephentyrone Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscbyspro Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

xwu Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwu Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

oscbyspro Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentyrone Dec 7, 2024

Choose a reason for hiding this comment

Uh oh!

stephentyrone commented Dec 7, 2024

Uh oh!

stephentyrone commented Dec 7, 2024

Uh oh!

oscbyspro commented Dec 7, 2024

Uh oh!

stephentyrone commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oscbyspro commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentyrone Dec 4, 2024 •

edited

Loading

stephentyrone Dec 4, 2024 •

edited

Loading

xwu Dec 5, 2024 •

edited

Loading

oscbyspro Dec 5, 2024 •

edited

Loading

stephentyrone commented Dec 7, 2024 •

edited

Loading

oscbyspro commented Dec 7, 2024 •

edited

Loading