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

Detect unaligned fields via aggregate.align < field.align, instead of a packed flag. #46436

Merged
merged 7 commits into from
Dec 17, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Dec 1, 2017

Closes #46423. cc @oli-obk

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@kennytm kennytm added S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Dec 2, 2017
@kennytm
Copy link
Member

kennytm commented Dec 4, 2017

#46428 has been merged, removing S-blocked tag.

@arielb1
Copy link
Contributor

arielb1 commented Dec 4, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Dec 4, 2017

📌 Commit 9dbf495 has been approved by arielb1

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 4, 2017
@bors
Copy link
Contributor

bors commented Dec 5, 2017

⌛ Testing commit 9dbf4956e849ccd6f997ca12cf59d42493ca1f87 with merge 9e5f66604eb5713c2b809be6568a69e733f706ac...

@bors
Copy link
Contributor

bors commented Dec 5, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Dec 5, 2017

@bors retry

"error: Could not compile rustdoc." without any reasons?

@bors
Copy link
Contributor

bors commented Dec 5, 2017

⌛ Testing commit 9dbf4956e849ccd6f997ca12cf59d42493ca1f87 with merge 1d14c68b12283807fc669192af83d3d75f952a3f...

@bors
Copy link
Contributor

bors commented Dec 5, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Dec 5, 2017

@bors retry #33434

   Doc-tests core
error: test failed, to rerun pass '--doc'
command did not execute successfully: "C:\\projects\\rust\\build\\i686-pc-windows-gnu\\stage0/bin\\cargo.exe" "test" "--target" "i686-pc-windows-gnu" "-j" "2" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "C:\\projects\\rust\\src/libstd/Cargo.toml" "-p" "std:0.0.0" "-p" "unwind:0.0.0" "-p" "alloc_system:0.0.0" "-p" "std_unicode:0.0.0" "-p" "libc:0.0.0" "-p" "compiler_builtins:0.0.0" "-p" "panic_abort:0.0.0" "-p" "alloc:0.0.0" "-p" "core:0.0.0" "--"
expected success, got: exit code: 3221225477
failed to run: C:\projects\rust\build\bootstrap\debug\bootstrap test
Build completed unsuccessfully in 1:58:02
Command exited with code 1

@bors
Copy link
Contributor

bors commented Dec 5, 2017

⌛ Testing commit 9dbf4956e849ccd6f997ca12cf59d42493ca1f87 with merge cad9630522a85914c37d6526627c69d799073493...

@bors
Copy link
Contributor

bors commented Dec 5, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Dec 5, 2017

Could not compile rustdoc for i686-pc-windows-msvc again. This PR may have introduced a change that caused this to fail with some probability. Not going to retry.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 5, 2017
@eddyb
Copy link
Member Author

eddyb commented Dec 6, 2017

Is it running out of memory by any chance?

@kennytm
Copy link
Member

kennytm commented Dec 6, 2017

I don't know, the log doesn't track memory use 😞

@carols10cents
Copy link
Member

Soooo who might have some insight on why rustdoc can't build on appveyor? @rust-lang/infra ? @rust-lang/docs ?

@GuillaumeGomez
Copy link
Member

Without more logs, it's gonna be complicated...

@bors
Copy link
Contributor

bors commented Dec 16, 2017

☔ The latest upstream changes (presumably #46743) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member Author

eddyb commented Dec 16, 2017

r? @oli-obk on the last two three commits (also, I'll have to update miri itself, too, right?).

@@ -9,8 +9,7 @@ use rustc_const_math::ConstFloat;
#[derive(Copy, Clone, Debug)]
pub struct PtrAndAlign {
pub ptr: Pointer,
/// Remember whether this place is *supposed* to be aligned.
pub aligned: bool,
pub align: Align,
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! I wanted to do this all along

pub fn to_ptr(self) -> EvalResult<'tcx, MemoryPointer> {
let (ptr, extra) = self.to_ptr_extra_aligned();
// At this point, we forget about the alignment information -- the place has been turned into a reference,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove the comment?

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Threading through the alignments was the original idea. We scrapped it because we thought it would get too intrusive. With all the convenience methods on Align it seems good though

@oli-obk
Copy link
Contributor

oli-obk commented Dec 17, 2017

Just break miri. I want to refactor some of the ty arguments to layout arguments anyway, I'll just do that there

@eddyb
Copy link
Member Author

eddyb commented Dec 17, 2017

@bors r=arielb1,oli-obk

@bors
Copy link
Contributor

bors commented Dec 17, 2017

📌 Commit 799a83c has been approved by arielb1,oli-obk

@bors
Copy link
Contributor

bors commented Dec 17, 2017

⌛ Testing commit 799a83c with merge 3cc68ba...

bors added a commit that referenced this pull request Dec 17, 2017
Detect unaligned fields via `aggregate.align < field.align`, instead of a `packed` flag.

Closes #46423. cc @oli-obk
@eddyb eddyb added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed I-nominated S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 17, 2017
@bors
Copy link
Contributor

bors commented Dec 17, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1,oli-obk
Pushing 3cc68ba to master...

@bors bors merged commit 799a83c into rust-lang:master Dec 17, 2017
@eddyb eddyb deleted the unpacked branch December 17, 2017 18:54
dwrensha added a commit to dwrensha/seer that referenced this pull request Dec 18, 2017
bors added a commit that referenced this pull request Dec 18, 2017
rustc: ensure optimized enums have a properly aligned size.

Fixes #46769 by padding the optimized enums wrapping packed data as necessary.

Note that this is not the only way to solve this - on nightly, #46436 makes it easier to fix without adding new padding because of the replacement of `packed` flags with a non-redundant scheme.
But because it can't be backported, the optimal fix will be in a separate nightly-only PR (#46809).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants