Skip to content

Raise alignment limit from 2^15 to 2^31 - 1 #43097

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

Merged
merged 4 commits into from
Jul 9, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 14 additions & 15 deletions src/librustc/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,11 +285,13 @@ impl Size {
}

/// Alignment of a type in bytes, both ABI-mandated and preferred.
/// Since alignments are always powers of 2, we can pack both in one byte,
/// giving each a nibble (4 bits) for a maximum alignment of 2<sup>15</sup> = 32768.
/// Each field is a power of two, giving the alignment a maximum
/// value of 2^(2^8 - 1), which is limited by LLVM to a i32, with
/// a maximum capacity of 2^31 - 1 or 2147483647.
#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
pub struct Align {
raw: u8
abi: u8,
pref: u8,
}

impl Align {
Expand All @@ -298,7 +300,7 @@ impl Align {
}

pub fn from_bytes(abi: u64, pref: u64) -> Result<Align, String> {
let pack = |align: u64| {
let log2 = |align: u64| {
// Treat an alignment of 0 bytes like 1-byte alignment.
if align == 0 {
return Ok(0);
Expand All @@ -312,39 +314,36 @@ impl Align {
}
if bytes != 1 {
Err(format!("`{}` is not a power of 2", align))
} else if pow > 0x0f {
Err(format!("`{}` is too large", align))
Copy link
Member

@eddyb eddyb Jul 8, 2017

Choose a reason for hiding this comment

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

You should keep the check here though (and update it for 31 bits).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this struct care about the 31 bit limit? I've done the checking for that closer to the parsing. The previous limit here was due to the limitations of using just 1 nibble, but now it has no such limit.

Copy link
Member

Choose a reason for hiding this comment

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

The parsing is for the user-facing value. Here we must protect any internal guarantees.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. Will fix.

} else {
Ok(pow)
}
};

Ok(Align {
raw: pack(abi)? | (pack(pref)? << 4)
abi: log2(abi)?,
pref: log2(pref)?,
})
}

pub fn abi(self) -> u64 {
1 << (self.raw & 0xf)
1 << self.abi
}

pub fn pref(self) -> u64 {
1 << (self.raw >> 4)
1 << self.pref
}

pub fn min(self, other: Align) -> Align {
let abi = cmp::min(self.raw & 0x0f, other.raw & 0x0f);
let pref = cmp::min(self.raw & 0xf0, other.raw & 0xf0);
Align {
raw: abi | pref
abi: cmp::min(self.abi, other.abi),
pref: cmp::min(self.pref, other.pref),
}
}

pub fn max(self, other: Align) -> Align {
let abi = cmp::max(self.raw & 0x0f, other.raw & 0x0f);
let pref = cmp::max(self.raw & 0xf0, other.raw & 0xf0);
Align {
raw: abi | pref
abi: cmp::max(self.abi, other.abi),
pref: cmp::max(self.pref, other.pref),
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1398,7 +1398,7 @@ impl_stable_hash_for!(struct ReprFlags {
#[derive(Copy, Clone, Eq, PartialEq, RustcEncodable, RustcDecodable, Default)]
pub struct ReprOptions {
pub int: Option<attr::IntType>,
pub align: u16,
pub align: u32,
pub flags: ReprFlags,
}

Expand Down
10 changes: 5 additions & 5 deletions src/libsyntax/attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -974,11 +974,11 @@ pub fn find_repr_attrs(diagnostic: &Handler, attr: &Attribute) -> Vec<ReprAttr>
let mut align_error = None;
if let ast::LitKind::Int(align, ast::LitIntType::Unsuffixed) = value.node {
if align.is_power_of_two() {
// rustc::ty::layout::Align restricts align to <= 32768
if align <= 32768 {
acc.push(ReprAlign(align as u16));
// rustc::ty::layout::Align restricts align to <= 2147483647
if align <= 2147483647 {
acc.push(ReprAlign(align as u32));
} else {
align_error = Some("larger than 32768");
align_error = Some("larger than 2147483647");
}
} else {
align_error = Some("not a power of two");
Expand Down Expand Up @@ -1027,7 +1027,7 @@ pub enum ReprAttr {
ReprExtern,
ReprPacked,
ReprSimd,
ReprAlign(u16),
ReprAlign(u32),
}

#[derive(Eq, Hash, PartialEq, Debug, RustcEncodable, RustcDecodable, Copy, Clone)]
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/repr-align.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ struct A(i32);
#[repr(align(15))] //~ ERROR: invalid `repr(align)` attribute: not a power of two
struct B(i32);

#[repr(align(65536))] //~ ERROR: invalid `repr(align)` attribute: larger than 32768
#[repr(align(4294967296))] //~ ERROR: invalid `repr(align)` attribute: larger than 2147483647
struct C(i32);

fn main() {}
17 changes: 17 additions & 0 deletions src/test/run-pass/align-struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ struct AlignContainsPacked {
b: Packed,
}

// The align limit was originally smaller (2^15).
// Check that it works with big numbers.
#[repr(align(0x10000))]
struct AlignLarge {
stuff: [u8; 0x10000],
}

impl Align16 {
// return aligned type
pub fn new(i: i32) -> Align16 {
Expand Down Expand Up @@ -193,4 +200,14 @@ pub fn main() {
assert_eq!(mem::align_of_val(&a.b), 1);
assert_eq!(mem::size_of_val(&a), 16);
assert!(is_aligned_to(&a, 16));

let mut arr = [0; 0x10000];
arr[0] = 132;
let large = AlignLarge {
stuff: arr,
};
Copy link
Member

Choose a reason for hiding this comment

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

Try using box (not Box::new!) - maybe you're blowing the stack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I thought an upstream change might result in that but I didn't think to avoid it with box.

assert_eq!(large.stuff[0], 132);
assert_eq!(mem::align_of::<AlignLarge>(), 0x10000);
assert_eq!(mem::align_of_val(&large), 0x10000);
assert!(is_aligned_to(&large, 0x10000));
}