swiftlang / swift-corelibs-foundation

The Foundation Project, providing core utilities, internationalization, and OS independence
swift.org
Apache License 2.0
5.29k stars 1.13k forks source link

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

Closed tbkka closed 7 years ago

tbkka commented 7 years ago
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:

https://github.com/apple/swift-protobuf/issues/513

phausler commented 7 years ago

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

tbkka commented 7 years ago

Does it fix the overflow problem mentioned in that thread?

phausler commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

https://github.com/apple/swift/pull/9476

swift-ci commented 7 years ago

Comment by Matt Rajca (JIRA)

This was fixed in June: https://github.com/apple/swift-corelibs-foundation/pull/1067