Add support for storing code model to LLVM module IR by kubo39 · Pull Request #74002 · rust-lang/rust

kubo39 · 2020-07-03T16:40:33Z

This patch avoids undefined behavior by linking different object files.
Also this would it could be propagated properly to LTO.

See https://reviews.llvm.org/D52322 and https://reviews.llvm.org/D52323.

This patch avoids undefined behavior by linking different object files. Also this would it could be propagated properly to LTO. See https://reviews.llvm.org/D52322 and https://reviews.llvm.org/D52323.

rust-highfive · 2020-07-03T16:40:36Z

r? @eddyb

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

nagisa

r? @nagisa

r=me after pub → crate

nagisa · 2020-07-04T02:50:17Z

 }

-fn to_llvm_code_model(code_model: Option<CodeModel>) -> llvm::CodeModel {
+pub fn to_llvm_code_model(code_model: Option<CodeModel>) -> llvm::CodeModel {


Consider pub(crate) or just crate.

nagisa · 2020-07-04T02:56:02Z

+// Linking object files with different code models is undefined behavior
+// because the compiler would have to generate additional code (to span
+// longer jumps) if a larger code model is used with a smaller one.
+// Therefore we will treat attempts to mix code models as an error.


This last sentence is super interesting, but it is not super clear where the "error" comes from. Does it happen at LTO time? Quick skim of the linked differentials does not answer it, you need to read the comments, so it would be great if more about this was said here.
I would also probably move it to this function’s call location as people are way more likely to read it in librustc_codegen_llvm than here.

This last sentence is super interesting, but it is not super clear where the "error" comes from. Does it happen at LTO time? Quick skim of the linked differentials does not answer it, you need to read the comments, so it would be great if more about this was said here.
Thank you for having a look, and sorry for not clear that.
I checked LLVM and IRMover works both link time and LTO time, so this happends either.However, this error won't happen at rustc comiple time, so I would just remove this line.

nagisa · 2020-07-04T14:03:32Z

@bors r+ Thanks!

bors · 2020-07-04T14:03:33Z

📌 Commit 67a5e2b has been approved by nagisa

Add support for storing code model to LLVM module IR This patch avoids undefined behavior by linking different object files. Also this would it could be propagated properly to LTO. See https://reviews.llvm.org/D52322 and https://reviews.llvm.org/D52323.

JohnTitor · 2020-07-05T13:31:17Z

Failed in #74054 (comment), @bors r-

kubo39 · 2020-07-05T15:22:30Z

CI shows:

failures:
---- [codegen] codegen/codemodels.rs#MODEL-KERNEL stdout ----
error in revision `MODEL-KERNEL`: compilation failed!
status: exit code: 1
command: "/checkout/obj/build/i686-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/codegen/codemodels.rs" "-Zthreads=1" "--cfg" "model_kernel" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/i686-unknown-linux-gnu/test/codegen/codemodels.MODEL-KERNEL" "-Crpath" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/i686-unknown-linux-gnu/native/rust-test-helpers" "--target=x86_64-unknown-linux-gnu" "-C" "code-model=kernel" "-L" "/checkout/obj/build/i686-unknown-linux-gnu/test/codegen/codemodels.MODEL-KERNEL/auxiliary" "--emit=llvm-ir"
stdout:
------------------------------------------
------------------------------------------
stderr:
------------------------------------------
error[E0463]: can't find crate for `std`
|
= note: the `x86_64-unknown-linux-gnu` target may not be installed
error: aborting due to previous error
For more information about this error, try `rustc --explain E0463`.
------------------------------------------
error in revision `MODEL-MEDIUM`: compilation failed!
status: exit code: 1
command: "/checkout/obj/build/i686-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/codegen/codemodels.rs" "-Zthreads=1" "--cfg" "model_medium" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/i686-unknown-linux-gnu/test/codegen/codemodels.MODEL-MEDIUM" "-Crpath" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/i686-unknown-linux-gnu/native/rust-test-helpers" "--target=x86_64-unknown-linux-gnu" "-C" "code-model=medium" "-L" "/checkout/obj/build/i686-unknown-linux-gnu/test/codegen/codemodels.MODEL-MEDIUM/auxiliary" "--emit=llvm-ir"
stdout:
------------------------------------------
------------------------------------------
stderr:
------------------------------------------
error[E0463]: can't find crate for `std`
|
= note: the `x86_64-unknown-linux-gnu` target may not be installed
error: aborting due to previous error
For more information about this error, try `rustc --explain E0463`.
------------------------------------------
failures:
[codegen] codegen/codemodels.rs#MODEL-KERNEL
[codegen] codegen/codemodels.rs#MODEL-MEDIUM
test result: FAILED. 165 passed; 2 failed; 42 ignored; 0 measured; 0 filtered out

kubo39 · 2020-07-09T16:33:53Z

What should I do next?

nagisa · 2020-07-09T23:16:22Z

@bors r+ rollup=never

@kubo39 Sorry, I’ve been busy with work stuff and couldn't dedicate any time to look at this again until now. Generally speaking pinging the reviewer is good enough after you fix the test failures. They will re-approve the PR and eventually bors should get to testing the PR.

bors · 2020-07-09T23:16:24Z

📌 Commit 5c24a94 has been approved by nagisa

bors · 2020-07-09T23:16:24Z

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

bors · 2020-07-09T23:17:23Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Rollup of 14 pull requests #74195

bors · 2020-07-09T23:17:23Z

📌 Commit 5c24a94 has been approved by nagisa

Manishearth · 2020-07-10T07:24:08Z

@bors p=1

bors · 2020-07-10T13:44:17Z

⌛ Testing commit 5c24a94 with merge 9d1583c098bad1b07f1ee7a2c168e8dd21f743e6...

Dylan-DPC-zz · 2020-07-10T13:52:18Z

@bors retry yielding to high priority pr

Manishearth · 2020-07-10T19:05:59Z

@bors rollup=never

Manishearth · 2020-07-10T19:08:39Z

@bors p=0

Manishearth · 2020-07-11T14:55:25Z

Is this rollup=never because it has to be or because we expect it to perhaps fail a rollup? There's a new rollup=iffy for that

nagisa · 2020-07-11T15:00:00Z

Yeah it was rollup=never before iffy existed :)

@bors rollup=iffy

Add support for storing code model to LLVM module IR This patch avoids undefined behavior by linking different object files. Also this would it could be propagated properly to LTO. See https://reviews.llvm.org/D52322 and https://reviews.llvm.org/D52323.

Manishearth · 2020-07-11T15:08:51Z

Expected so, wanted to confirm. Thanks!

Manishearth · 2020-07-11T15:52:10Z

@bors r-

#74243 (comment)

Dylan-DPC-zz · 2020-08-13T00:58:53Z

@kubo39 thanks for the effort but unfortunately I've to close this pr due to inactivity. If you wish and have figured out the failure pasted earlier, you can submit a new PR with the fix and we can get it reviewed. Thanks for taking the time to contribute.

This patch avoids undefined behavior by linking different object files. Also this would it could be propagated properly to LTO. See https://reviews.llvm.org/D52322 and https://reviews.llvm.org/D52323. This patch is based on rust-lang#74002

Add support for storing code model to LLVM module IR This patch avoids undefined behavior by linking different object files. Also this would it could be propagated properly to LTO. See https://reviews.llvm.org/D52322 and https://reviews.llvm.org/D52323. This patch is based on rust-lang#74002

Add support for storing code model to LLVM module IR
825eb47
This patch avoids undefined behavior by linking different object files. Also this would it could be propagated properly to LTO. See https://reviews.llvm.org/D52322 and https://reviews.llvm.org/D52323.

rust-highfive assigned eddyb Jul 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2020

Remove code-model=tiny from module IR test
b99a08a

petrochenkov reviewed Jul 3, 2020
View reviewed changes

Comment threadsrc/librustc_codegen_llvm/context.rs Outdated

Remove duplicated to_llvm_code_model function
4a89c9f

kubo39 force-pushed the set-code-model branch from b67b147 to 4a89c9fCompare July 3, 2020 20:18

nagisa reviewed Jul 4, 2020
View reviewed changes

Fix for reviews
67a5e2b

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 Jul 4, 2020

Manishearth mentioned this pull request Jul 5, 2020
Rollup of 5 pull requests #74054
Closed

bors 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 Jul 5, 2020

Remove target flag from code model test
5c24a94

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 9, 2020

nagisa assigned nagisa and unassigned eddyb Jul 11, 2020

Manishearth mentioned this pull request Jul 11, 2020
Rollup of 10 pull requests #74243
Closed

bors 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 Jul 11, 2020

Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 25, 2020

Dylan-DPC-zz 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2020

Dylan-DPC-zz closed this Aug 13, 2020

Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 13, 2020

kubo39 deleted the set-code-model branch March 12, 2021 01:56

kubo39 mentioned this pull request Mar 12, 2021
Add support for storing code model to LLVM module IR #83044
Merged

Uh oh!

Uh oh!

Conversation

kubo39 commented Jul 3, 2020

Uh oh!

rust-highfive commented Jul 3, 2020

Uh oh!

Uh oh!

nagisa left a comment

Choose a reason for hiding this comment

Uh oh!

nagisaJul 4, 2020

Choose a reason for hiding this comment

Uh oh!

nagisaJul 4, 2020

Choose a reason for hiding this comment

Uh oh!

kubo39Jul 4, 2020

Choose a reason for hiding this comment

Uh oh!

nagisa commented Jul 4, 2020

Uh oh!

bors commented Jul 4, 2020

Uh oh!

JohnTitor commented Jul 5, 2020

Uh oh!

kubo39 commented Jul 5, 2020

Uh oh!

kubo39 commented Jul 9, 2020

Uh oh!

nagisa commented Jul 9, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Jul 9, 2020

Uh oh!

bors commented Jul 9, 2020

Uh oh!

bors commented Jul 9, 2020

Uh oh!

bors commented Jul 9, 2020

Uh oh!

Manishearth commented Jul 10, 2020

Uh oh!

bors commented Jul 10, 2020

Uh oh!

Dylan-DPC-zz commented Jul 10, 2020

Uh oh!

Manishearth commented Jul 10, 2020

Uh oh!

Manishearth commented Jul 10, 2020

Uh oh!

Manishearth commented Jul 11, 2020

Uh oh!

nagisa commented Jul 11, 2020

Uh oh!

Manishearth commented Jul 11, 2020

Uh oh!

Manishearth commented Jul 11, 2020

Uh oh!

Dylan-DPC-zz commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

nagisa commented Jul 9, 2020•
edited
Loading