Skip to content

Commit

Permalink
Auto merge of #52972 - RalfJung:from_raw_parts_align, r=alexcrichton
Browse files Browse the repository at this point in the history
debug_assert to ensure that from_raw_parts is only used properly aligned

This does not help nearly as much as I would hope because everybody uses the distributed libstd which is compiled without debug assertions. For this reason, I am not sure if this is even worth it. OTOH, this would have caught the misalignment fixed by #42789 *if* there had been any tests actually using ZSTs with alignment >1 (we have a CI runner which has debug assertions in libstd enabled), and it seems to currently [fail in the rg testsuite](https://ci.appveyor.com/project/rust-lang/rust/build/1.0.8403/job/v7dfdcgn8ay5j6sb). So maybe it is worth it, after all.

I have seen the attribute `#[rustc_inherit_overflow_checks]` in some places, does that make it so that the *caller's* debug status is relevant? Is there a similar attribute for `debug_assert!`? That could even subsume `rustc_inherit_overflow_checks`: Something like `rustc_inherit_debug_flag` could affect *all* places that change the generated code depending on whether we are in debug or release mode. In fact, given that we have to keep around the MIR for generic functions anyway, is there ever a reason *not* to handle the debug flag that way? I guess currently we apply debug flags like `cfg` so this is dropped early during the MIR pipeline?

EDIT: I learned from @eddyb that because of how `debug_assert!` works, this is not realistic. Well, we could still have it for the rustc CI runs and then maybe, eventually, when libstd gets compiled client-side and there is both a debug and a release build... then this will also benefit users.^^
  • Loading branch information
bors committed Aug 19, 2018
2 parents a9fe312 + 1001b2b commit 8928de7
Show file tree
Hide file tree
Showing 8 changed files with 28 additions and 3 deletions.
2 changes: 2 additions & 0 deletions .travis.yml
Expand Up @@ -46,6 +46,8 @@ matrix:
# slow to run.

# OSX builders running tests, these run the full test suite.
# NO_DEBUG_ASSERTIONS=1 to make them go faster, but also do have some
# runners that run `//ignore-debug` tests.
#
# Note that the compiler is compiled to target 10.8 here because the Xcode
# version that we're using, 8.2, cannot compile LLVM for OSX 10.7.
Expand Down
4 changes: 4 additions & 0 deletions src/libcore/slice/mod.rs
Expand Up @@ -1785,6 +1785,7 @@ impl<T> [T] {
(self, &[], &[])
} else {
let (left, rest) = self.split_at(offset);
// now `rest` is definitely aligned, so `from_raw_parts_mut` below is okay
let (us_len, ts_len) = rest.align_to_offsets::<U>();
(left,
from_raw_parts(rest.as_ptr() as *const U, us_len),
Expand Down Expand Up @@ -1837,6 +1838,7 @@ impl<T> [T] {
(self, &mut [], &mut [])
} else {
let (left, rest) = self.split_at_mut(offset);
// now `rest` is definitely aligned, so `from_raw_parts_mut` below is okay
let (us_len, ts_len) = rest.align_to_offsets::<U>();
let mut_ptr = rest.as_mut_ptr();
(left,
Expand Down Expand Up @@ -3878,6 +3880,7 @@ unsafe impl<'a, T> TrustedRandomAccess for ExactChunksMut<'a, T> {
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T] {
debug_assert!(data as usize % mem::align_of::<T>() == 0, "attempt to create unaligned slice");
Repr { raw: FatPtr { data, len } }.rust
}

Expand All @@ -3891,6 +3894,7 @@ pub unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T] {
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub unsafe fn from_raw_parts_mut<'a, T>(data: *mut T, len: usize) -> &'a mut [T] {
debug_assert!(data as usize % mem::align_of::<T>() == 0, "attempt to create unaligned slice");
Repr { raw: FatPtr { data, len} }.rust_mut
}

Expand Down
14 changes: 14 additions & 0 deletions src/libcore/tests/slice.rs
Expand Up @@ -986,3 +986,17 @@ fn test_align_to_non_trivial() {
assert_eq!(aligned.len(), 4);
assert_eq!(prefix.len() + suffix.len(), 2);
}

#[test]
fn test_align_to_empty_mid() {
use core::mem;

// Make sure that we do not create empty unaligned slices for the mid part, even when the
// overall slice is too short to contain an aligned address.
let bytes = [1, 2, 3, 4, 5, 6, 7];
type Chunk = u32;
for offset in 0..4 {
let (_, mid, _) = unsafe { bytes[offset..offset+1].align_to::<Chunk>() };
assert_eq!(mid.as_ptr() as usize % mem::align_of::<Chunk>(), 0);
}
}
1 change: 1 addition & 0 deletions src/test/codegen/vec-clear.rs
Expand Up @@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// ignore-debug: the debug assertions get in the way
// compile-flags: -O

#![crate_type = "lib"]
Expand Down
1 change: 1 addition & 0 deletions src/test/codegen/vec-iter-collect-len.rs
Expand Up @@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// ignore-debug: the debug assertions get in the way
// no-system-llvm
// compile-flags: -O
#![crate_type="lib"]
Expand Down
1 change: 1 addition & 0 deletions src/test/codegen/vec-optimizes-away.rs
Expand Up @@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.
//
// ignore-debug: the debug assertions get in the way
// no-system-llvm
// compile-flags: -O
#![crate_type="lib"]
Expand Down
2 changes: 1 addition & 1 deletion src/tools/cargotest/main.rs
Expand Up @@ -33,7 +33,7 @@ const TEST_REPOS: &'static [Test] = &[
Test {
name: "ripgrep",
repo: "https://github.com/BurntSushi/ripgrep",
sha: "b65bb37b14655e1a89c7cd19c8b011ef3e312791",
sha: "ad9befbc1d3b5c695e7f6b6734ee1b8e683edd41",
lock: None,
packages: &[],
},
Expand Down
6 changes: 4 additions & 2 deletions src/tools/compiletest/src/header.rs
Expand Up @@ -615,12 +615,14 @@ impl Config {
common::DebugInfoLldb => name == "lldb",
common::Pretty => name == "pretty",
_ => false,
} || (self.target != self.host && name == "cross-compile") ||
} ||
(self.target != self.host && name == "cross-compile") ||
match self.compare_mode {
Some(CompareMode::Nll) => name == "compare-mode-nll",
Some(CompareMode::Polonius) => name == "compare-mode-polonius",
None => false,
}
} ||
(cfg!(debug_assertions) && name == "debug")
} else {
false
}
Expand Down

0 comments on commit 8928de7

Please sign in to comment.