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

trans: pad const structs to aligned size #37281

Merged
merged 1 commit into from
Oct 22, 2016
Merged

Conversation

TimNN
Copy link
Contributor

@TimNN TimNN commented Oct 19, 2016

Fixes #37222.

I'm not sure if that is the correct way to fix this, but it does work.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@jwilm
Copy link

jwilm commented Oct 20, 2016

Thanks for digging into that issue and putting a patch together @TimNN!

@pnkfelix pnkfelix added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 20, 2016
@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2016

📌 Commit 1c78b08 has been approved by pnkfelix

@@ -777,8 +777,10 @@ fn build_const_struct<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
offset += machine::llsize_of_alloc(ccx, val_ty(val));
}

if offset < st.min_size.bytes() {
cfields.push(padding(ccx, st.min_size.bytes() - offset));
let min_size = st.min_size.abi_align(st.align).bytes();
Copy link
Member

@eddyb eddyb Oct 20, 2016

Choose a reason for hiding this comment

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

st.stride() should work AFAICT (or it's "aligned_size", I forget).

@eddyb
Copy link
Member

eddyb commented Oct 20, 2016

@bors r-

Don't want to keep the long form in. cc @camlorn who changed that code recently.

@eddyb
Copy link
Member

eddyb commented Oct 20, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2016

📌 Commit 472515a has been approved by eddyb

@eddyb
Copy link
Member

eddyb commented Oct 20, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2016

📌 Commit f135697 has been approved by eddyb

@nikomatsakis
Copy link
Contributor

Discussed in @rust-lang/compiler meeting, marking as beta-accepted. But it's got to merge first I guess. =)

@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 20, 2016
@eddyb
Copy link
Member

eddyb commented Oct 20, 2016

Relevant commit: f16068e#diff-3dc76d318c3b3ca3bab97ea19a5dd685L1287 - st.size was aligned, but was replaced with the unaligned size, now min_size (I failed to see @camlorn's mistake during review).

However, while the bug affects beta, this PR can't be backported to beta directly, because the file there is just after that original change, with build_const_struct not even having access to the right alignment.

@bors
Copy link
Contributor

bors commented Oct 21, 2016

⌛ Testing commit f135697 with merge 101a132...

@bors
Copy link
Contributor

bors commented Oct 21, 2016

💔 Test failed - auto-win-gnu-64-opt

@TimNN
Copy link
Contributor Author

TimNN commented Oct 21, 2016

rustc: x86_64-pc-windows-gnu/stage2/test/stdtest-x86_64-pc-windows-gnu.exe
run: x86_64-pc-windows-gnu/stage2/test/stdtest-x86_64-pc-windows-gnu.exe

running 756 tests

command timed out: 1200 seconds without output, attempting to kill
program finished with exit code 1
elapsedTime=2063.338000

@arielb1
Copy link
Contributor

arielb1 commented Oct 21, 2016

@bors retry

@arielb1
Copy link
Contributor

arielb1 commented Oct 21, 2016

@bors retry

@alexcrichton
Copy link
Member

Looks like another instance of #33361. Unfortunately no idea what that is.

@bors
Copy link
Contributor

bors commented Oct 21, 2016

⌛ Testing commit f135697 with merge e64b41d...

bors added a commit that referenced this pull request Oct 21, 2016
trans: pad const structs to aligned size

Fixes #37222.

I'm not sure if that is the *correct* way to fix this, but it *does* work.
@TimNN
Copy link
Contributor Author

TimNN commented Oct 21, 2016

fatal: unable to access 'https://github.com/rust-lang/rust/': Could not resolve host: github.com 
program finished with exit code 128
elapsedTime=10.130828

😭

@arielb1
Copy link
Contributor

arielb1 commented Oct 21, 2016

@bors retry

@alexcrichton
Copy link
Member

@bors: retry force clean

@bors
Copy link
Contributor

bors commented Oct 22, 2016

⌛ Testing commit f135697 with merge 4642e08...

bors added a commit that referenced this pull request Oct 22, 2016
trans: pad const structs to aligned size

Fixes #37222.

I'm not sure if that is the *correct* way to fix this, but it *does* work.
@bors bors merged commit f135697 into rust-lang:master Oct 22, 2016
@TimNN TimNN deleted the pad-align branch October 22, 2016 10:40
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Beta segfault regression
9 participants