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

Tracking issue for Integer Overflow (RFC 560) #22020

Closed
11 of 15 tasks
aturon opened this issue Feb 6, 2015 · 25 comments
Closed
11 of 15 tasks

Tracking issue for Integer Overflow (RFC 560) #22020

aturon opened this issue Feb 6, 2015 · 25 comments
Assignees
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. P-medium Medium priority T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Feb 6, 2015

RFC: rust-lang/rfcs#560
Final text: https://github.com/rust-lang/rfcs/blob/master/text/0560-integer-overflow.md

List of tasks to accomplish:


@aturon aturon added the B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. label Feb 6, 2015
@aturon
Copy link
Member Author

aturon commented Feb 6, 2015

cc @nikomatsakis

@aturon
Copy link
Member Author

aturon commented Feb 6, 2015

cc @Aatch

@glaebhoerl
Copy link
Contributor

(cc #20795)

@pnkfelix
Copy link
Member

(cc #22532)

@pnkfelix
Copy link
Member

pnkfelix commented Mar 3, 2015

@aturon does a -Z flag count for "Option to forcibly enable overflow checking" ?

@aturon
Copy link
Member Author

aturon commented Mar 5, 2015

@pnkfelix I'm not sure; I wasn't the one who updated this with a checklist. @nikomatsakis @alexcrichton?

@alexcrichton
Copy link
Member

I would personally consider a -Z flag to fit the bill

@nikomatsakis
Copy link
Contributor

Interesting. I consider a -Z flag adequate for this bug, perhaps,
but not a good overall solution. However, it seems like having a good
set of public flags around this falls under
#22492

On Wed, Mar 04, 2015 at 05:27:27PM -0800, Alex Crichton wrote:

I would personally consider a -Z flag to fit the bill


Reply to this email directly or view it on GitHub:
#22020 (comment)

@codyps
Copy link
Contributor

codyps commented Mar 9, 2015

May also want to add:

  • Implement wrapping_negate method from the WrappingOps trait

(or whatever it should be named)

To match with:

  • Optional error checking on unary -

@pnkfelix
Copy link
Member

for consistency with the Shl trait etc in core:ops, we probably should name the wrapping shift methods wrapping_shl, etc

(of course it won't be a 100% correspondence anyway; e.g. we implement Shl for every kind of integer RHS, but I suspect supporting that might be silly in the WrappingOps trait. Well, we'll see.)

@pnkfelix
Copy link
Member

@aturon to be 100% clear, the lint that is described here:

Lint for use of one of the above operations in an unsafe fn or fn containing unsafe blocks

is a reference to the lint section of the RFC, and therefore is talking about linting for uses of {+, -, *, <<, >>, as <integral-type>} in a function with unsafe blocks, and not about linting for uses of the newly added WrappingOps methods in such functions...

Right?

@aturon
Copy link
Member Author

aturon commented Mar 11, 2015

@pnkfelix I didn't make the detailed list here -- I would check with @nikomatsakis

@nikomatsakis
Copy link
Contributor

@pnkfelix correct

@pnkfelix
Copy link
Member

(regarding shift behavior, I thought the (old) discussion on #1877 was interesting. In particular there is a semi-open question of whether we should follow Java in our fallback behavior for the RHS of a shift, or if we should just pass it directly to LLVM, and live with underspecification in general.)

@glaebhoerl
Copy link
Contributor

If we weren't willing to live with underspecification around the result of arithmetic overflows (with checks off), then it seems we should be consistent and be the same way with respect to shifts also.

@pnkfelix
Copy link
Member

@glaebhoerl yes that is a reasonable conclusion to draw.

In any case, I have left a prominent spot in the code indicating what lines to uncomment to follow Java's approach; see:
https://github.com/rust-lang/rust/pull/23536/files#diff-88ce76a36e25f7f0d19bc4cdd58ee1b8R2584

Update: To be clear: I originally wrote and tested the code with those masks in place. I only commented them out after reading #1877. It should be trivial to uncomment them if that's what we want.

@pnkfelix
Copy link
Member

@glaebhoerl okay, due to #10183 and #23551, my mind is now completely turned around on this issue: As far as I can tell, we must ensure that the input-rhs is in the half-open range [0, sizeof(lhs_t)*8), because otherwise LLVM can produce nonsense.

@pnkfelix
Copy link
Member

I think all of the backwards compatibility issues have now been resolved.

@pnkfelix
Copy link
Member

Not sure how important that lint was. Nominating for discussion at next triage mtg.

@rust-highfive rust-highfive added P-medium Medium priority and removed P-backcompat-lang labels Apr 23, 2015
@pnkfelix
Copy link
Member

(oops I added a "triage:" comment and then deleted it, but of course rust-highfive cannot turn back time.)

@pnkfelix
Copy link
Member

My intent was to nominate, but also suggest a P-medium assignment for the remaining work here. I will put back the P-backcompat-lang label.

@pnkfelix pnkfelix added I-nominated and removed P-medium Medium priority labels Apr 23, 2015
@brson brson added the P-high High priority label Apr 29, 2015
@brson brson removed this from the 1.0 milestone Apr 29, 2015
@pnkfelix
Copy link
Member

triage: Remaining work is P-medium (at most).... lint may only qualify as a "nice to have" at this point, it is unclear how important it will be.

@pnkfelix pnkfelix added P-medium Medium priority and removed P-high High priority I-nominated labels Apr 30, 2015
@alexcrichton alexcrichton added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 11, 2015
@alexcrichton
Copy link
Member

I believe this has basically all been implemented, so closing.

@pnkfelix
Copy link
Member

(Well there is the lint that was listed in the description, but its questionable whether we actually need/want that...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. P-medium Medium priority T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants