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

PlasmaPower · 2017-07-06T22:07:53Z

rust-highfive · 2017-07-06T22:07:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

PlasmaPower · 2017-07-07T14:01:58Z

@eddyb - I think you wrote the initial Align struct in #32939, does this PR look good?

I don't think I can change the reviewer after creating the PR.

eddyb · 2017-07-07T14:06:59Z

@PlasmaPower Did you mean 2^64 - 1? Or are you bounding it somewhere?

eddyb · 2017-07-07T14:08:08Z

src/librustc/ty/layout.rs

            } else {
                Ok(pow)
            }
        };

        Ok(Align {
-            raw: pack(abi)? | (pack(pref)? << 4)
+            abi: pack(abi)?,
+            pref: pack(pref)?,


I'd rename this to log2.

eddyb · 2017-07-07T14:08:49Z

src/libsyntax/attr.rs

-                                // rustc::ty::layout::Align restricts align to <= 32768
-                                if align <= 32768 {
-                                    acc.push(ReprAlign(align as u16));
+                                // rustc::ty::layout::Align restricts align to <= 2147483648


Ah but this is incorrect given that Align::from_bytes takes an u64.

PlasmaPower · 2017-07-07T14:09:00Z

@eddyb you're right, I did the math on the upper bound wrong. Will fix.

PlasmaPower · 2017-07-07T20:56:13Z

The resulting binary is segfaulting when I switch from u32s to u64s in some places, as necessary with a limit of 2^64 - 1. I'll investigate further.

eddyb · 2017-07-07T21:09:02Z

@PlasmaPower LLVM probably doesn't like alignments larger than 2^31 - 1.

PlasmaPower · 2017-07-07T21:11:51Z

@eddyb I'm assuming you mean larger than 2^32 - 1? I'm not sure why that would matter, and in addition I'm not testing alignments larger than 2^32 - 1. There might be another factor here. I might just stick with u32s though, as we shouldn't need anything larger than that.

eddyb · 2017-07-07T21:59:51Z

@PlasmaPower LLVM uses signed 32-bit integers for alignments IIRC.

PlasmaPower · 2017-07-08T15:29:44Z

I rebased on master and added the limit changes, but resulting binaries with big aligns still seem to be segfaulting. They were working before, but when I check out the previous commit it still seg faults. I might try removing my build directory and building the compiler fresh. Any ideas @eddyb ? When I run it with GDB, it says "no stack" after segfaulting.

eddyb · 2017-07-08T15:57:17Z

src/librustc/ty/layout.rs

@@ -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))


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

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.

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

Fair enough. Will fix.

eddyb · 2017-07-08T15:58:49Z

src/test/run-pass/align-struct.rs

+    arr[0] = 132;
+    let large = AlignLarge {
+        stuff: arr,
+    };


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

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

eddyb · 2017-07-08T16:26:58Z

cc @alexcrichton The PR in its current state might be failing in strange ways due to the new probestack logic. For the record, the updated test contains:

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

// Check the actual address is aligned
fn is_aligned_to<T>(p: &T, align: usize) -> bool {
    let addr = p as *const T as usize;
    (addr & (align - 1)) == 0
}

fn main() {
    let mut arr = [0; 0x10000];
    arr[0] = 132;
    let large = AlignLarge {
        stuff: arr,
    };
    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));
}

PlasmaPower · 2017-07-08T16:43:09Z

@eddyb That's exactly the change I thought of, but I don't know near enough about that low level stuff to know how to work around it or fix it.

PlasmaPower · 2017-07-08T17:01:06Z

Box syntax fixed it, but it turns out it wasn't the large alignment but instead the large array. This used to work: let x = [0; 0x10000] but now it needs to be let x = box [0; 0x10000].

eddyb · 2017-07-08T17:45:41Z

@bors r+

bors · 2017-07-08T17:45:42Z

📌 Commit b4973e9 has been approved by eddyb

bors · 2017-07-08T19:42:32Z

⌛ Testing commit b4973e9 with merge c531c46140a9d65da3e2d24396ebde3ee1c54335...

bors · 2017-07-08T19:50:10Z

💔 Test failed - status-travis

PlasmaPower · 2017-07-08T19:50:51Z

Failure seems unrelated to this PR.

eddyb · 2017-07-08T20:33:02Z

@bors retry

bors · 2017-07-08T22:09:10Z

⌛ Testing commit b4973e9 with merge 4b7f41a...

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

bors · 2017-07-09T00:21:12Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 4b7f41a to master...

rust-highfive assigned arielb1 Jul 6, 2017

PlasmaPower mentioned this pull request Jul 6, 2017

#[repr(align(x))] is limited to 32768 #42960

Closed

PlasmaPower changed the title ~~Raised alignment limit from 2^15 to 2^31~~ Raise alignment limit from 2^15 to 2^31 Jul 6, 2017

eddyb reviewed Jul 7, 2017

View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 7, 2017

PlasmaPower added 2 commits July 8, 2017 10:28

Raised alignment limit from 2^15 to 2^31

a10213f

Lower alignment limit down to 2^31 - 1 (from LLVM)

afed75a

PlasmaPower force-pushed the large-align branch from 60e1e63 to afed75a Compare July 8, 2017 15:27

eddyb reviewed Jul 8, 2017

View reviewed changes

PlasmaPower added 2 commits July 8, 2017 13:02

Box large array to avoid blowing the stack

da81a33

Internally limit alignment to 2^30

b4973e9

PlasmaPower changed the title ~~Raise alignment limit from 2^15 to 2^31~~ Raise alignment limit from 2^15 to 2^31 - 1 Jul 8, 2017

bors added a commit that referenced this pull request Jul 8, 2017

Auto merge of #43097 - PlasmaPower:large-align, r=eddyb

4b7f41a

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

bors merged commit b4973e9 into rust-lang:master Jul 9, 2017

pnkfelix mentioned this pull request Jul 13, 2017

Allocator traits and std::heap #32838

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PlasmaPower commented Jul 6, 2017

rust-highfive commented Jul 6, 2017

PlasmaPower commented Jul 7, 2017

eddyb commented Jul 7, 2017

eddyb Jul 7, 2017

eddyb Jul 7, 2017

PlasmaPower commented Jul 7, 2017

PlasmaPower commented Jul 7, 2017

eddyb commented Jul 7, 2017

PlasmaPower commented Jul 7, 2017

eddyb commented Jul 7, 2017 •

edited

Loading

PlasmaPower commented Jul 8, 2017

eddyb Jul 8, 2017 •

edited

Loading

PlasmaPower Jul 8, 2017

eddyb Jul 8, 2017

PlasmaPower Jul 8, 2017

eddyb Jul 8, 2017

PlasmaPower Jul 8, 2017

eddyb commented Jul 8, 2017

PlasmaPower commented Jul 8, 2017

PlasmaPower commented Jul 8, 2017

eddyb commented Jul 8, 2017

bors commented Jul 8, 2017

bors commented Jul 8, 2017

bors commented Jul 8, 2017

PlasmaPower commented Jul 8, 2017

eddyb commented Jul 8, 2017

bors commented Jul 8, 2017

bors commented Jul 9, 2017

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

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

Conversation

PlasmaPower commented Jul 6, 2017

rust-highfive commented Jul 6, 2017

PlasmaPower commented Jul 7, 2017

eddyb commented Jul 7, 2017

eddyb Jul 7, 2017

Choose a reason for hiding this comment

eddyb Jul 7, 2017

Choose a reason for hiding this comment

PlasmaPower commented Jul 7, 2017

PlasmaPower commented Jul 7, 2017

eddyb commented Jul 7, 2017

PlasmaPower commented Jul 7, 2017

eddyb commented Jul 7, 2017 • edited Loading

PlasmaPower commented Jul 8, 2017

eddyb Jul 8, 2017 • edited Loading

Choose a reason for hiding this comment

PlasmaPower Jul 8, 2017

Choose a reason for hiding this comment

eddyb Jul 8, 2017

Choose a reason for hiding this comment

PlasmaPower Jul 8, 2017

Choose a reason for hiding this comment

eddyb Jul 8, 2017

Choose a reason for hiding this comment

PlasmaPower Jul 8, 2017

Choose a reason for hiding this comment

eddyb commented Jul 8, 2017

PlasmaPower commented Jul 8, 2017

PlasmaPower commented Jul 8, 2017

eddyb commented Jul 8, 2017

bors commented Jul 8, 2017

bors commented Jul 8, 2017

bors commented Jul 8, 2017

PlasmaPower commented Jul 8, 2017

eddyb commented Jul 8, 2017

bors commented Jul 8, 2017

bors commented Jul 9, 2017

eddyb commented Jul 7, 2017 •

edited

Loading

eddyb Jul 8, 2017 •

edited

Loading