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

Fix alignment of stores to scalar pair #56300

Merged
merged 1 commit into from
Nov 29, 2018
Merged

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Nov 27, 2018

The alignment for the second element of a scalar pair is not the same as for the first element, make sure it is calculated correctly. This fixes #56267.

r? @eddyb

The alignment for the second element of a scalar pair is not the
same as for the first element. Make sure it is computed correctly
based on the element size.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 27, 2018
@eddyb
Copy link
Member

eddyb commented Nov 27, 2018

@nikomatsakis @Mark-Simulacrum Any chance we could still get this into the beta?

@eddyb eddyb added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 27, 2018
@eddyb
Copy link
Member

eddyb commented Nov 27, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Nov 27, 2018

📌 Commit d8190af has been approved by eddyb

@bors bors 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 Nov 27, 2018
@Mark-Simulacrum
Copy link
Member

Cc @rust-lang/compiler for beta approval. This fixes a clear beta regression but I'm not sure what the impact of that regression is.

Overall I'm fairly neutral on backporting - the patch looks relatively small.

@oli-obk oli-obk added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 28, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Nov 28, 2018

Accepting for beta, small patch for fixing an unsoundness regression.

@pietroalbini
Copy link
Member

@bors p=10 (beta-accepted)

@eddyb
Copy link
Member

eddyb commented Nov 28, 2018

I think there's more, similar, issues, I tried to do a search and found this:

let load = self.load(llptr, place.align);

That's the load counterpart to this PR, so I think it should also be fixed..

EDIT: I'll be opening a PR soon.
EDIT2: Here it is: #56329.

// The store writing to bar.1 should have alignment 4. Not checking
// other stores here, as the alignment will be platform-dependent.

// CHECK: store i32 [[TMP1:%.+]], i32* [[TMP2:%.+]], align 4
Copy link
Member

Choose a reason for hiding this comment

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

This could be using %{{.+}}, since TMP1 and TMP2 aren't used. (but we should merge this PR first)

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 28, 2018
Fix alignment of stores to scalar pair

The alignment for the second element of a scalar pair is not the same as for the first element, make sure it is calculated correctly. This fixes rust-lang#56267.

r? @eddyb
@bors
Copy link
Contributor

bors commented Nov 29, 2018

⌛ Testing commit d8190af with merge 5f387a6...

bors added a commit that referenced this pull request Nov 29, 2018
Fix alignment of stores to scalar pair

The alignment for the second element of a scalar pair is not the same as for the first element, make sure it is calculated correctly. This fixes #56267.

r? @eddyb
@bors
Copy link
Contributor

bors commented Nov 29, 2018

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

bors added a commit that referenced this pull request Nov 29, 2018
rustc_codegen_llvm: don't overalign loads of pair operands.

Counterpart to #56300, but for loads instead of stores.
@bors bors merged commit d8190af into rust-lang:master Nov 29, 2018
@nikomatsakis nikomatsakis removed the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 29, 2018
@nikomatsakis
Copy link
Contributor

Removing beta-accepted so we can discuss in today's @rust-lang/compiler meeting.

@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 29, 2018
@nikomatsakis
Copy link
Contributor

Relevant: affects num-cpus, a pretty fundamental crate

@pnkfelix
Copy link
Member

discussed in T-compiler meeting. beta-accepting.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 29, 2018
@alexcrichton
Copy link
Member

@nikic can you help out with the backport to beta? While this patch applies cleanly it generates:

error[E0277]: the trait bound `&builder::Builder<'_, 'll, 'tcx>: rustc_target::abi::HasDataLayout` is not satisfied
   --> librustc_codegen_llvm/mir/operand.rs:301:47
    |
301 |                 let b_offset = a_scalar.value.size(bx).align_to(b_scalar.value.align(bx).abi);
    |                                               ^^^^ the trait `rustc_target::abi::HasDataLayout` is not implemented for `&builder::Builder<'_, 'll, 'tcx>`

error[E0599]: no method named `align_to` found for type `rustc_target::abi::Size` in the current scope
   --> librustc_codegen_llvm/mir/operand.rs:301:56
    |
301 |                 let b_offset = a_scalar.value.size(bx).align_to(b_scalar.value.align(bx).abi);
    |                                                        ^^^^^^^^

error[E0277]: the trait bound `&builder::Builder<'_, 'll, 'tcx>: rustc_target::abi::HasDataLayout` is not satisfied
   --> librustc_codegen_llvm/mir/operand.rs:301:80
    |
301 |                 let b_offset = a_scalar.value.size(bx).align_to(b_scalar.value.align(bx).abi);
    |                                                                                ^^^^^ the trait `rustc_target::abi::HasDataLayout` is not implemented for `&builder::Builder<'_, 'll, 'tcx>`

error[E0615]: attempted to take value of method `abi` on type `rustc_target::abi::Align`
   --> librustc_codegen_llvm/mir/operand.rs:301:90
    |
301 |                 let b_offset = a_scalar.value.size(bx).align_to(b_scalar.value.align(bx).abi);
    |                                                                                          ^^^
    |
    = help: maybe a `()` to call it is missing?

error: aborting due to 4 previous errors

Some errors occurred: E0277, E0599, E0615.
For more information about an error, try `rustc --explain E0277`.

and I'm not sure how to easily fix those off the top of my head!

@nikic
Copy link
Contributor Author

nikic commented Nov 29, 2018

@alexcrichton The bx arguments probably need to be bx.cx on beta.

@nikic
Copy link
Contributor Author

nikic commented Nov 29, 2018

The whole expression should be a_scalar.value.size(bx.cx).abi_align(b_scalar.value.align(bx.cx)).

@alexcrichton
Copy link
Member

Thanks! I think that solves the first and third errors, but the second/fourth still persist :(

error[E0599]: no method named `align_to` found for type `rustc_target::abi::Size` in the current scope
   --> librustc_codegen_llvm/mir/operand.rs:301:59
    |
301 |                 let b_offset = a_scalar.value.size(bx.cx).align_to(b_scalar.value.align(bx.cx).abi);
    |                                                           ^^^^^^^^

error[E0615]: attempted to take value of method `abi` on type `rustc_target::abi::Align`
   --> librustc_codegen_llvm/mir/operand.rs:301:96
    |
301 |                 let b_offset = a_scalar.value.size(bx.cx).align_to(b_scalar.value.align(bx.cx).abi);
    |                                                                                                ^^^
    |
    = help: maybe a `()` to call it is missing?

error: aborting due to 2 previous errors

Some errors occurred: E0599, E0615.
For more information about an error, try `rustc --explain E0599`.

@nikic
Copy link
Contributor Author

nikic commented Nov 29, 2018

@alexcrichton Could you please try the a_scalar.value.size(bx.cx).abi_align(b_scalar.value.align(bx.cx)) variant from my last comment? That's what similar code on beta uses. If that doesn't do it I'll try locally.

@alexcrichton
Copy link
Member

Appears to work! Or at least it compiles, running some tests real quick. Thanks!

@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 29, 2018
bors added a commit that referenced this pull request Nov 30, 2018
Rollup beta backports

* #56264
* #56300
* #56322
* #56329

Neither #56300 nor #56329 applied cleanly, but I think I've gotten it working with @nikic's [help](#56300 (comment)) (thanks!)

Closes #56311
Closes #56263
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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

llvm lint: Undefined behavior: Memory reference address is misaligned
10 participants