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

Jump to bottom

[SR-11346] range.index(range.startIndex, offsetBy: 128) crashes for range = Int8.min ... Int8.max #53747

Open
mayoff opened this issue Aug 21, 2019 · 5 comments
Open

[SR-11346] range.index(range.startIndex, offsetBy: 128) crashes for range = Int8.min ... Int8.max #53747

mayoff opened this issue Aug 21, 2019 · 5 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella

Comments

@mayoff
Copy link

mayoff commented Aug 21, 2019

Previous ID SR-11346
Radar None
Original Reporter @mayoff
Type Bug
Additional Detail from JIRA
Votes 0
Component/s Standard Library
Labels Bug
Assignee None
Priority Medium

md5: 8036ff5efbf7e5a2fad781b94b0cac71

Issue Description:

Test script:

struct NonrandomNumberGenerator: RandomNumberGenerator {
    func next() -> UInt64 { return 128 }
}
var rng = NonrandomNumberGenerator()
_ = (Int8.min ... Int8.max).randomElement(using: &rng)

Output:

:; xcrun swift
Welcome to Apple Swift version 5.0.1 (swiftlang-1001.0.82.4 clang-1001.0.46.4).
Type :help for assistance.
  1>     struct NonrandomNumberGenerator: RandomNumberGenerator { 
  2.         func next() -> UInt64 { return 128 } 
  3.     } 
  4.     var rng = NonrandomNumberGenerator() 
  5.     _ = (Int8.min ... Int8.max).randomElement(using: &rng)
Fatal error: Not enough bits to represent the passed value
2019-08-21 17:20:58.082481-0500 repl_swift[35823:34610771] Fatal error: Not enough bits to represent the passed value
Process 35823 stopped
* thread #​1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
    frame #&#8203;0: 0x00007fff6c1d1483 libswiftCore.dylib`function signature specialization <Arg[0] = Exploded, Arg[1] = Exploded, Arg[2] = Dead, Arg[3] = Dead> of Swift._fatalErrorMessage(_: Swift.StaticString, _: Swift.StaticString, file: Swift.StaticString, line: Swift.UInt, flags: Swift.UInt32) -> Swift.Never + 307
@mayoff
Copy link
Author

mayoff commented Aug 21, 2019

Further investigation reveals that the problem is actually down in index(_:offsetBy: ):

:; xcrun swift
Welcome to Apple Swift version 5.0.1 (swiftlang-1001.0.82.4 clang-1001.0.46.4).
Type :help for assistance.
  1> let range = Int8.min ... Int8.max 
range: ClosedRange<Int8> = -128...127
  2> range.index(range.startIndex, offsetBy: 128) 
Fatal error: Not enough bits to represent the passed value
2019-08-21 17:30:36.599597-0500 repl_swift[36791:34640406] Fatal error: Not enough bits to represent the passed value

@belkadan
Copy link
Contributor

This is "correct" behavior in that Int8.min...Int8.max isn't representable. @lorentey, @milseman, there's already a dup for this, right? Or am I misunderstanding the problem?

@mayoff
Copy link
Author

mayoff commented Aug 22, 2019

Are you saying that range.index(range.startIndex, offsetBy: 128) crashing is “correct” behavior for range = Int8.min ... Int8.max? I understand that Int8.max - Int8.min is not representable as an Int8, but the distance argument to index(_:offsetBy: ) is an Int, not a Int8.

@mayoff
Copy link
Author

mayoff commented Aug 22, 2019

So, investigating further, I find that the problem is in BinaryInteger's implementation of advanced(by: ), specifically here:

    return self.magnitude < n.magnitude
      ? Self(Int(self) + n)
      : self + Self(n)

Since self = -128 and n = 128, they have the same magnitude, so it chooses self + Self(n ), and Self(n ) crashes when n = 128.

Is the solution as simple as changing the condition to self.magnitude <= n.magnitude? When I copy the function to xAdvanced(by: ) and make this change, it no longer crashes for n = 128 (or for any legal combination of self: Int8 and n).

@mayoff
Copy link
Author

mayoff commented Aug 22, 2019

This could also be tuned better in FixedWidthInteger, where we have access to static property Self.bitWidth, so the test could be optimized out:

    return Self.bitWidth < Int.bitWidth
      ? Self(Int(self) + n)
      : self + Self(n)

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@oscbyspro oscbyspro mentioned this issue Feb 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mayoff @belkadan