Add support for storing code model to LLVM module IR #74002

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)

src/librustc_codegen_llvm/context.rs

nagisa

r? @nagisa

r=me after pub → crate

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

src/librustc_codegen_llvm/back/write.rs

@@ -108,7 +108,7 @@ fn to_llvm_relocation_model(relocation_model: RelocModel) -> llvm::RelocModel {
    }
 }

-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

src/rustllvm/PassWrapper.cpp

+//  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

src/librustc_codegen_llvm/context.rs Outdated Show resolved Hide resolved

Remove duplicated to_llvm_code_model function

4a89c9f

kubo39 force-pushed the set-code-model branch from b67b147 to 4a89c9f Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for storing code model to LLVM module IR #74002

Add support for storing code model to LLVM module IR #74002

kubo39 commented Jul 3, 2020

rust-highfive commented Jul 3, 2020

nagisa left a comment

nagisa Jul 4, 2020

nagisa Jul 4, 2020

kubo39 Jul 4, 2020

nagisa commented Jul 4, 2020

bors commented Jul 4, 2020

JohnTitor commented Jul 5, 2020

kubo39 commented Jul 5, 2020

kubo39 commented Jul 9, 2020

nagisa commented Jul 9, 2020 •

edited

bors commented Jul 9, 2020

bors commented Jul 9, 2020

bors commented Jul 9, 2020

bors commented Jul 9, 2020

Manishearth commented Jul 10, 2020

bors commented Jul 10, 2020

Dylan-DPC-zz commented Jul 10, 2020

Manishearth commented Jul 10, 2020

Manishearth commented Jul 10, 2020

Manishearth commented Jul 11, 2020

nagisa commented Jul 11, 2020

Manishearth commented Jul 11, 2020

Manishearth commented Jul 11, 2020

Dylan-DPC-zz commented Aug 13, 2020

Add support for storing code model to LLVM module IR #74002

Add support for storing code model to LLVM module IR #74002

Conversation

kubo39 commented Jul 3, 2020

rust-highfive commented Jul 3, 2020

nagisa left a comment

Choose a reason for hiding this comment

nagisa Jul 4, 2020

Choose a reason for hiding this comment

nagisa Jul 4, 2020

Choose a reason for hiding this comment

kubo39 Jul 4, 2020

Choose a reason for hiding this comment

nagisa commented Jul 4, 2020

bors commented Jul 4, 2020

JohnTitor commented Jul 5, 2020

kubo39 commented Jul 5, 2020

kubo39 commented Jul 9, 2020

nagisa commented Jul 9, 2020 • edited

bors commented Jul 9, 2020

bors commented Jul 9, 2020

bors commented Jul 9, 2020

bors commented Jul 9, 2020

Manishearth commented Jul 10, 2020

bors commented Jul 10, 2020

Dylan-DPC-zz commented Jul 10, 2020

Manishearth commented Jul 10, 2020

Manishearth commented Jul 10, 2020

Manishearth commented Jul 11, 2020

nagisa commented Jul 11, 2020

Manishearth commented Jul 11, 2020

Manishearth commented Jul 11, 2020

Dylan-DPC-zz commented Aug 13, 2020

nagisa commented Jul 9, 2020 •

edited