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.

Sign up for GitHub

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

Lint casting signed integers smaller than isize to raw pointers #43641

Closed
wants to merge 1 commit into from
Closed

Lint casting signed integers smaller than isize to raw pointers #43641

wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Aug 4, 2017

cc #43291 (keeping open as tracking issue)

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb
Copy link
Member

eddyb commented Aug 4, 2017

Whoa whoa what is all this new code? I expected a trivial check to be used only for a cargobomb run.

@eddyb
Copy link
Member

eddyb commented Aug 4, 2017

cc @rust-lang/compiler

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 4, 2017

This code exists so we don't produce a suggestion like 5 + 6 as usize as *const T if we had (5 + 6) as *const T when starting out. It is 1:1 copied from clippy.

@eddyb
Copy link
Member

eddyb commented Aug 4, 2017

It is 1:1 copied from clippy.

Ah, great! Would've been less surprising to see that in the PR description. However, I'd still start small, with an error-by-default lint with no interesting suggestions, for cargobomb.

@oli-obk oli-obk force-pushed the truncating_for_fun_and___wait_what branch from 2bea2ab to 84bed45 Compare August 4, 2017 11:56
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 4, 2017

done

eddyb
eddyb reviewed Aug 4, 2017
View reviewed changes
src/test/run-pass/mir_misc_casts.rs Outdated
@@ -120,7 +120,7 @@ fn from_1i8()
let c10 = 1_i8 as u64;
let c11 = 1_i8 as f32;
let c12 = 1_i8 as f64;
let c13 = 1_i8 as *const libc::FILE;
let c13 = 1_i8 as isize as *const libc::FILE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct cast btw is through usize. Although... I'd leave the tests unchanged and just allow the lint in them.

ollie27
ollie27 reviewed Aug 4, 2017
View reviewed changes
Copy link
Member

@ollie27 ollie27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some overlap between this and #43339 btw.

src/librustc_lint/builtin.rs Outdated
ty::TyInt(ast::IntTy::Is) => return,
ty::TyInt(ast::IntTy::I128) |
ty::TyInt(ast::IntTy::I64) => {
if cx.tcx.data_layout.pointer_size.bytes() >= 8 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right. I think this should be <= and the if needs to be part of the match pattern to allow this to fall through:

ty::TyInt(ast::IntTy::I128) |
ty::TyInt(ast::IntTy::I64) if cx.tcx.data_layout.pointer_size.bytes() <= 8 => {
    return;
},
ty::TyInt(ast::IntTy::I32) if cx.tcx.data_layout.pointer_size.bytes() <= 4 => {
    return;
},
ty::TyInt(ast::IntTy::I16) if cx.tcx.data_layout.pointer_size.bytes() <= 2 => {
    return;
},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! you're right of course, I got the logic reversed.

ollie27
ollie27 reviewed Aug 4, 2017
View reviewed changes
src/librustc_lint/builtin.rs Outdated
// these we report
ty::TyInt(_) => {},
// only int casts are relevant
_ => {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume there's supposed to be a return here.

@shepmaster shepmaster added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Aug 4, 2017
@shepmaster
Copy link
Member

@eddyb I assume you will be starting the cargobomb?

frewsxcv added a commit to rust-lang/book that referenced this pull request Aug 4, 2017
@frewsxcv
Casting from a pointer should have type 'isize', not 'i32'.
79b6ba1 
Found via rust-lang/rust#43641.
@frewsxcv frewsxcv mentioned this pull request Aug 4, 2017
Merged
@frewsxcv
Copy link
Member

frewsxcv commented Aug 4, 2017

Looks like there's an error in the book. I opened a PR upstream: rust-lang/book#866.

@frewsxcv frewsxcv mentioned this pull request Aug 5, 2017
Closed
@frewsxcv
Copy link
Member

frewsxcv commented Aug 5, 2017

Upstream PR has merged. Updating the in-tree book submodule now: #43688.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 5, 2017

Thanks for taking care of that!

@oli-obk oli-obk changed the title Lint casting integers other than isize/usize to raw pointers Lint casting signed integers smaller than isize to raw pointers Aug 5, 2017
carols10cents pushed a commit to frewsxcv/rust that referenced this pull request Aug 5, 2017
@frewsxcv @carols10cents
Bump 'src/doc/book' git submodule.
abc76ae 
Primarily to pick up this change:

rust-lang/book#866

...to move this PR forward:

rust-lang#43641
bors added a commit that referenced this pull request Aug 5, 2017
@bors
Auto merge of #43688 - frewsxcv:frewsxcv-bump-book, r=carols10cents
dd1df35 
Bump 'src/doc/book' git submodule.

Primarily to pick up this change:

rust-lang/book#866

...to move this PR forward:

#43641
@frewsxcv
Copy link
Member

frewsxcv commented Aug 6, 2017

@oli-obk the book should be good now, you should rebase or merge in master

@eddyb
Copy link
Member

eddyb commented Aug 6, 2017

@frewsxcv Always rebase 😛!

@oli-obk oli-obk force-pushed the truncating_for_fun_and___wait_what branch from ca5681e to 72c77e9 Compare August 7, 2017 06:44
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 10, 2017

ping @eddyb

@eddyb
Copy link
Member

eddyb commented Aug 10, 2017

@bors try

@bors
Copy link
Contributor

bors commented Aug 10, 2017

⌛ Trying commit 72c77e9 with merge a1b0d1d...

bors added a commit that referenced this pull request Aug 10, 2017
@bors
Auto merge of #43641 - oli-obk:truncating_for_fun_and___wait_what, r=…
a1b0d1d 
…<try>

Lint casting signed integers smaller than `isize` to raw pointers

cc #43291 (keeping open as tracking issue)
@bors
Copy link
Contributor

bors commented Aug 10, 2017

☀️ Test successful - status-travis
State: approved= try=True

@eddyb
Copy link
Member

eddyb commented Aug 10, 2017

cc @rust-lang/infra Cargobomb crater requested.

@aidanhs
Copy link
Member

aidanhs commented Aug 10, 2017

I'll get this cargobomb run going.

@RalfJung RalfJung mentioned this pull request Aug 10, 2017
Closed
@aidanhs
Copy link
Member

aidanhs commented Aug 14, 2017

Cargobomb run failed, the disk filled up because I failed to do due diligence on disk space beforehand - sorry! I'll prepare a new run now.

@aidanhs
Copy link
Member

aidanhs commented Aug 18, 2017

Cargobomb complete: http://cargobomb-reports.s3.amazonaws.com/pr-43641/index.html

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 18, 2017 •
edited
Loading

I'll check the ones below once there's a PR against them

  • cargo-tarpaulin-0.3.13: ptrace(PTRACE_CONT, pid, ptr::null_mut(), (s as i32) as * mut c_void)
  • drm-rs-0.1.3:ffi::xf86drm_mode::drmModePageFlip(fd, crtc_id, fb_id, flags, user_data as *const _)
  • egl-0.2.7:ffi::eglGetConfigs(display, list.configs, config_size, list.count as *mut int32_t)
  • freertos_rs-0.1.0:pub fn freertos_rs_create_recursive_mutex() -> FreeRtosQueueHandle { 1 as _ }
  • nix-0.8.1, nix-0.9.0:ptrace(PTRACE_SETOPTIONS, pid, ptr::null_mut(), options as *mut c_void).map(drop)
  • rsjs-0.1.2:let ptr = js_int!("return RSJS.storeObject(RSJS.copyStringToHeap(RSJS.loadObject($0)));", obj) as *mut u16;
  • tracetree-0.1.5:let data = signal.map(|s| s as i32 as *mut c_void).unwrap_or(ptr::null_mut());
  • winit-0.7.5:ffi::CopyFromParent as *mut _ and self.x.screen_id as *mut libc::c_void
  • https://github.com/xd009642/tarpaulin#2c26a87cb45324289196280ac4b852793b8916de:ptrace(PTRACE_CONT, pid, ptr::null_mut(), (s as i32) as * mut c_void)

irrelevant failures

  • grnenv-rs-0.2.0: got /home/cargobomb/.groonga expected /home/grnenv/groonga
  • hashconsing-0.3.0: some runtime test failure
  • hyper-timeout-connector-0.1.0: can't access network during tests
  • librocksdb-sys-0.4.1: empty cargobomb output
  • milagro-crypto-0.1.14: unknown error during testing
  • netlib-src-0.7.0: empty cargobomb output
  • nss-sys-0.1.9: unknown error
  • owapi-0.1.0: runtime test failure
  • rocksdb-0.7.0: empty cargobomb output
  • simple-munin-plugin-0.1.0: runtime test failure
  • theban_interval_tree-0.7.1: runtime test failure
  • timekeeper-0.2.4: runtime test failure
  • tokio-proto-0.1.1: runtime test failure
  • vidar-0.1.0: runtime test failure
  • https://github.com/tinco/rust-static_any_map#909025e6146137938d43fd01b2f715a990358662: runtime test failure
  • https://github.com/vks/mpw-rs#d1ac69b31292245e9b48a3b267dd85720e46a53d: empty cargobomb output

@eddyb
Copy link
Member

eddyb commented Aug 18, 2017 •
edited
Loading

@oli-obk So the thing we were worried about was whether any of these would need the (buggy) zero-extension behavior, to know if we can just fix it right away.
Specifically, if there are negative integers, less wide than pointers, being cast to pointers.
EDIT: exempt from that are cases where the C side truncates back to the small integer width.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 18, 2017

I have checked everything but rsjs for which I was not able to find a repository. None of them actually depend on the buggy casting, since they only have positive values or 0.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 21, 2017

Closing in favour of simply making the backwards incompatible fix

@oli-obk oli-obk closed this Aug 21, 2017
@oli-obk oli-obk mentioned this pull request Aug 21, 2017
Closed
@oli-obk oli-obk deleted the truncating_for_fun_and___wait_what branch June 15, 2020 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ollie27 ollie27 ollie27 left review comments

@eddyb eddyb eddyb left review comments

Assignees

@eddyb eddyb

Labels
S-waiting-on-crater Status: Waiting on a crater run to be completed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@oli-obk @rust-highfive @eddyb @shepmaster @frewsxcv @bors @aidanhs @ollie27