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.

Sign up for GitHub

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

Jump to bottom

Remove serialized_bitcode from LtoModuleCodegen. #129981

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Remove serialized_bitcode from LtoModuleCodegen. #129981

merged 1 commit into from
Sep 10, 2024
+7 −17

Conversation

nnethercote
Copy link
Contributor

It's unused.

r? @bjorn3

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 5, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

antoyo
antoyo approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

LGTM

@GuillaumeGomez
Copy link
Member

@bors r=antoyo

@bors
Copy link
Contributor

bors commented Sep 5, 2024

📌 Commit cd504f2 has been approved by antoyo

It is now in the queue for this repository.

@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 Sep 5, 2024
@bjorn3
Copy link
Member

bjorn3 commented Sep 5, 2024

Are you sure this is safe to remove? It may be necessary to keep it around to avoid dangling references and thus UB. Can't check right now.

@antoyo
Copy link
Contributor

antoyo commented Sep 5, 2024 •
edited
Loading

Also. Pretty sure I should not be the reviewer here. I was just approving the GCC part.

@lqd
Copy link
Member

lqd commented Sep 5, 2024 •
edited
Loading

until bjorn can take a look at the non-gcc parts, or the question is answered, I was asked by @antoyo to @bors r-

@bors 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 Sep 5, 2024
@GuillaumeGomez
Copy link
Member

Went too fast. Thanks for the r-!

@nnethercote
Copy link
Contributor Author

The field was added in ded38db. There appears to be no discussion there about whether anything subtle is happening.

@nnethercote
Copy link
Contributor Author

There is the unsafe LtoModuleTranslation::optimize function nearby, not sure if that's relevant.

@tmiasko
Copy link
Contributor

tmiasko commented Sep 5, 2024

There is rather unhelpful comment about memory management. Can you update it as well?

rust/compiler/rustc_codegen_llvm/src/back/lto.rs

Lines 344 to 347 in eb33b43

// For all serialized bitcode files we parse them and link them in as we did
// above, this is all mostly handled in C++. Like above, though, we don't
// know much about the memory management here so we err on the side of being
// save and persist everything with the original module.

Serialized modules are only used in linker.add(data), and the first thing LLVMRustLinkerAdd does is to make a copy, so I don't see any reason for keeping serialized modules around.

@nnethercote
Copy link
Contributor Author

@tmiasko: can you suggest a rewriting of the comment?

@tmiasko
Copy link
Contributor

tmiasko commented Sep 6, 2024

Sure. I would remove the last sentence.

@nnethercote
Copy link
Contributor Author

I updated with the sentence removed from the comment.

@tmiasko
Copy link
Contributor

tmiasko commented Sep 9, 2024

@bors r=antoyo,tmiasko

@bors
Copy link
Contributor

bors commented Sep 9, 2024

📌 Commit bbe28cf has been approved by antoyo,tmiasko

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 10, 2024
@workingjubilee
Rollup merge of rust-lang#129981 - nnethercote:rm-serialize_bitcode, …
431a890 
…r=antoyo,tmiasko

Remove `serialized_bitcode` from `LtoModuleCodegen`.

It's unused.

r? `@bjorn3`
@workingjubilee workingjubilee mentioned this pull request Sep 10, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 10, 2024
@bors
Auto merge of rust-lang#130177 - workingjubilee:rollup-m4r01yn, r=wor…
2cf47e4 
…kingjubilee

Rollup of 14 pull requests

Successful merges:

 - rust-lang#128316 (Stabilize most of `io_error_more`)
 - rust-lang#129473 (use  `download-ci-llvm=true` in the default compiler config)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129778 (interpret: make typed copies lossy wrt provenance and padding)
 - rust-lang#129981 (Remove `serialized_bitcode` from `LtoModuleCodegen`.)
 - rust-lang#130025 (Also emit `missing_docs` lint with `--test` to fulfil expectations)
 - rust-lang#130040 (unify `llvm-bitcode-linker`, `wasm-component-ld` and llvm-tools logics)
 - rust-lang#130094 (Inform the solver if evaluation is concurrent)
 - rust-lang#130132 ([illumos] enable SIGSEGV handler to detect stack overflows)
 - rust-lang#130146 (bootstrap `naked_asm!` for `compiler-builtins`)
 - rust-lang#130149 (Helper function for formatting with `LifetimeSuggestionPosition`)
 - rust-lang#130152 (adapt a test for llvm 20)
 - rust-lang#130162 (bump download-ci-llvm-stamp)
 - rust-lang#130164 (move some const fn out of the const_ptr_as_ref feature)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee workingjubilee mentioned this pull request Sep 10, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 10, 2024
@bors
Auto merge of rust-lang#130179 - workingjubilee:rollup-l78cv44, r=wor…
0d1946d 
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#128316 (Stabilize most of `io_error_more`)
 - rust-lang#129473 (use  `download-ci-llvm=true` in the default compiler config)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129981 (Remove `serialized_bitcode` from `LtoModuleCodegen`.)
 - rust-lang#130094 (Inform the solver if evaluation is concurrent)
 - rust-lang#130132 ([illumos] enable SIGSEGV handler to detect stack overflows)
 - rust-lang#130146 (bootstrap `naked_asm!` for `compiler-builtins`)
 - rust-lang#130149 (Helper function for formatting with `LifetimeSuggestionPosition`)
 - rust-lang#130152 (adapt a test for llvm 20)
 - rust-lang#130162 (bump download-ci-llvm-stamp)
 - rust-lang#130164 (move some const fn out of the const_ptr_as_ref feature)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 10, 2024
@bors
Auto merge of rust-lang#130179 - workingjubilee:rollup-l78cv44, r=wor…
26b2b8d 
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#128316 (Stabilize most of `io_error_more`)
 - rust-lang#129473 (use  `download-ci-llvm=true` in the default compiler config)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129981 (Remove `serialized_bitcode` from `LtoModuleCodegen`.)
 - rust-lang#130094 (Inform the solver if evaluation is concurrent)
 - rust-lang#130132 ([illumos] enable SIGSEGV handler to detect stack overflows)
 - rust-lang#130146 (bootstrap `naked_asm!` for `compiler-builtins`)
 - rust-lang#130149 (Helper function for formatting with `LifetimeSuggestionPosition`)
 - rust-lang#130152 (adapt a test for llvm 20)
 - rust-lang#130162 (bump download-ci-llvm-stamp)
 - rust-lang#130164 (move some const fn out of the const_ptr_as_ref feature)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 88a2c62 into rust-lang:master Sep 10, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 10, 2024
@rust-timer
Unrolled build for rust-lang#129981
6fb2534 
Rollup merge of rust-lang#129981 - nnethercote:rm-serialize_bitcode, r=antoyo,tmiasko

Remove `serialized_bitcode` from `LtoModuleCodegen`.

It's unused.

r? ``@bjorn3``
@nnethercote nnethercote deleted the rm-serialize_bitcode branch September 10, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoyo antoyo antoyo approved these changes

Assignees

@bjorn3 bjorn3

Labels
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
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
8 participants
@nnethercote @rustbot @GuillaumeGomez @bors @bjorn3 @antoyo @lqd @tmiasko