Skip to content
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.

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

[SR-4858] IndexPath.hashValue broken on Linux #3861

Closed
tbkka opened this issue May 10, 2017 · 9 comments
Closed

[SR-4858] IndexPath.hashValue broken on Linux #3861

tbkka opened this issue May 10, 2017 · 9 comments

Comments

@tbkka
Copy link
Contributor

tbkka commented May 10, 2017

Previous ID SR-4858
Radar None
Original Reporter @tbkka
Type Bug
Status Resolved
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug, Linux, StarterBug
Assignee None
Priority Medium

md5: 3cdcfaf07c165283d1906f11248eb5f9

Issue Description:

In trying to use IndexPaths as dictionary keys, we found that the hashValue implementation is entirely broken on Linux.

More details are at:

apple/swift-protobuf#513

@phausler
Copy link
Member

The most recent Darwin version of IndexPath has a better hashValue implementation

@tbkka
Copy link
Contributor Author

tbkka commented May 10, 2017

Does it fix the overflow problem mentioned in that thread?

@phausler
Copy link
Member

the listed bug was that it was calling object conversion not overflows, any case of numeric calculation there was ripped directly from the C implementation (which is considerably faster than bridging an entire object)

@phausler
Copy link
Member

print(IndexPath.init(indexes: [1, 2, 3]).hashValue)
print(IndexPath.init(indexes: [Int.max, 2, Int.max]).hashValue)

works just fine: it does not crash from any overflow cases:

206158430467
-68719476989

@tbkka
Copy link
Contributor Author

tbkka commented May 10, 2017

A later comment in that thread:

> And that new impl of hashValue doesn't use the overflow operators, so it will choke on large values

Integer addition in C does not trap on overflow as it does in Swift, so you have to be very careful copying code. In this case, I think you get a crash for [Int.max >> 8, Int.max >> 36]:

```
func hashIndexes(first: Int, last: Int, count: Int) -> Int {
let totalBits = MemoryLayout<Int>.size * 8
let lengthBits = 8
let firstIndexBits = (totalBits - lengthBits) / 2
let a = count
let b = first << lengthBits
let c = last << (lengthBits + firstIndexBits)
return a + b + c
}

print(hashIndexes(first: Int.max >> 8, last: Int.max >> 36, count: 2)) // EXC_BAD_INSTRUCTION
```

@phausler
Copy link
Member

hmm that it does. That is a separate bug and can easily be fixed, it would be nice if swift was actually consistent about the story on numeric values (float/double follow C rules whereas Int seems to not)

@phausler
Copy link
Member

it is worth noting that if you have IndexPaths with values like Int.max>> 8 something very fishy is going on to begin with (it is not a common use case)

@phausler
Copy link
Member

Here is a pr for the Darwin version (including a test to verify)

apple/swift#9476

@swift-ci
Copy link
Contributor

swift-ci commented Aug 1, 2017

Comment by Matt Rajca (JIRA)

This was fixed in June: #1067

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants