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

Add lint for checking exceeding bitshifts #17713 #18206

Merged
merged 1 commit into from
Nov 3, 2014

Conversation

hirschenberger
Copy link
Contributor

Add lint for checking exceeding bitshifts #17713

It also const-evaluates the shift width (RHS) to check more complex shifts like 1u8 << (4+5).
The lint-level is set to Warn but perhaps it must be Deny as in llvm exceeding bitshifts are undefined as @ben0x539 stated in #17713

@thestinger
Copy link
Contributor

It should be set to deny by default because it's always a logic error. The undefined behaviour issue is entirely separate from the concerns of a lint checking for it statically.

@hirschenberger
Copy link
Contributor Author

Should I change the level to Deny?

r?

@thestinger
Copy link
Contributor

@hirschenberger: I'll r+ it with the default set to Deny.

@hirschenberger
Copy link
Contributor Author

@thestinger done.

@hirschenberger
Copy link
Contributor Author

Perhaps we could spawn an extra lint message on negative shifts?

@thestinger
Copy link
Contributor

@hirschenberger: AFAIK all shifts are currently by uint so there aren't really any negative shifts.

@hirschenberger
Copy link
Contributor Author

@thestinger What do you think, should I add a check for negative shifts which also produce undefined behaviour in llvm

    let x = 1u8 << -2;

IL:

  %x = alloca i8
  store i8 undef, i8* %x

If yes, is it a completely new lint or the same with another message?

@thestinger
Copy link
Contributor

@hirschenberger: The handling of indexing and bit shifts is very buggy. The bugs relating to generic integers should be fixed. It doesn't need a lint.

@thestinger
Copy link
Contributor

this does not compile:

1u32 << -1i

this compiles, as if it inferred uint, but really uses int:

1u32 << -1

the same problem occurs in indexing

@hirschenberger
Copy link
Contributor Author

Ok, then the Problem will solve itself in the Future©

@hirschenberger
Copy link
Contributor Author

@thestinger hmm, what about the failing test? it relies on undefined behaviour, I think it is invalid.

@hirschenberger
Copy link
Contributor Author

Fixed failing test
r?

@hirschenberger
Copy link
Contributor Author

@thestinger Would you please r+ my fixes?

@hirschenberger
Copy link
Contributor Author

@thestinger Damn, renamed lint. I hope it's now ready to land

@hirschenberger
Copy link
Contributor Author

Next try to get this landed, it seems was a buildbot problem
r?

bors added a commit that referenced this pull request Nov 3, 2014
Add lint for checking exceeding bitshifts #17713

It also const-evaluates the shift width (RHS) to check more complex shifts like `1u8 << (4+5)`.
The lint-level is set to `Warn` but perhaps it must be `Deny` as in llvm exceeding bitshifts are undefined as @ben0x539 stated in #17713
@bors bors closed this Nov 3, 2014
@bors bors merged commit e5058a8 into rust-lang:master Nov 3, 2014
@ben0x539
Copy link
Contributor

ben0x539 commented Nov 3, 2014

Shouldn't 1u8 << 8 already trigger the lint, not just << 9? From http://llvm.org/docs/LangRef.html#shl-instruction: "If op2 is (statically or dynamically) negative or equal to or larger than the number of bits in op1, the result is undefined."

@hirschenberger
Copy link
Contributor Author

Oh, good hint. Look at the IR for

http://is.gd/OkCRea

  %n = alloca i8
  %n1 = alloca i8
  %n2 = alloca i8
  store i8 -128, i8* %n
  store i8 undef, i8* %n1
  store i8 undef, i8* %n2
  ret void

I'll change my code to catch the equal bits case.

2014-11-03 15:17 GMT+01:00 Benjamin Herr notifications@github.com:

Shouldn't 1u8 << 8 already trigger the lint, not just << 9? From
http://llvm.org/docs/LangRef.html#shl-instruction: "If op2 is (statically
or dynamically) negative or equal to or larger than the number of bits
in op1, the result is undefined."


Reply to this email directly or view it on GitHub
#18206 (comment).

lnicola pushed a commit to lnicola/rust that referenced this pull request Oct 8, 2024
Fix: Handle block exprs as modules when finding their parents

Fixes rust-lang#18187
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants