Make globals with private linkage unnamed. Fixes #50862. #51007
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I'm not sure if doing this unconditionally (i.e. even under --emit=llvm-ir for example) is a good idea. While |
|
r? @nagisa |
nagisa
reviewed
May 24, 2018
src/librustc_codegen_llvm/declare.rs
Outdated
| /// Use this function when you intend to define a global without a name. | ||
| pub fn define_private_global(cx: &CodegenCx, ty: Type) -> ValueRef { | ||
| unsafe { | ||
| llvm::LLVMRustGetOrInsertGlobal(cx.llmod, ptr::null(), ty.to_ref()) |
There was a problem hiding this comment.
This should probably also set the appropriate linkage, so that the users of this function don’t need to do that.
|
@AstralSorcerer can you make elision of names |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors r+ |
|
📌 Commit f464e66 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit f464e66ecf75032a873173e609af7a428cc2d75b with merge 087dd0b6a5feb6349368ae17efe6433bc6e3867e... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
|
kennytm
added
S-waiting-on-author
|
ping @QuietMisdreavus |
|
...so, this is what happened when i ran the test on my Windows laptop:
Since i don't know much about what's going on the PR, i don't know how well this helps, but if need be i can try to dig farther by running rustc through Visual Studio to see whether i can get it to break on that assertion. |
nikic
reviewed
May 30, 2018
| let using_thin_buffers = config.emit_bc || config.obj_is_bitcode | ||
| || config.emit_bc_compressed || config.embed_bitcode; | ||
| let prepare_for_thin_lto = cgcx.lto == Lto::Thin || cgcx.lto == Lto::ThinLocal | ||
| || using_thin_buffers; |
There was a problem hiding this comment.
Note that PrepareForThinLTO also disables various unrolling / vectorization passes. That's desired if ThinLTO is actually used, but I suspect it's not what you want if we're just embedding bitcode.
There was a problem hiding this comment.
It's probably sufficient for this case to just run the NameAnonGlobals pass (the unnamed globals are what's causing problems with the thin buffers).
There was a problem hiding this comment.
That should indeed be sufficient. I'm wondering though why bytecode emission is still done using ThinLTOBitcodeWriter, even if ThinLTO is not used.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors r+ |
|
📌 Commit e146f3c has been approved by |
bors
removed
the
S-waiting-on-author
pietroalbini
removed
the
S-waiting-on-review
|
Ping from triage @AstralSorcerer! It's been a while since we heard from you, will you have time to work on this again? |
|
Yep! I'd already addressed the changes that were requested, I just hadn't had time to deal with a rebase until recently.
|
pietroalbini
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Emitting LLVM bitcode uses ThinLTOBuffers, so we need to prepare for thin LTO or we will likely cause errors in LLVM.
|
@bors r+ |
|
📌 Commit 3da7c65 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 3da7c65 with merge 15e6466dbe623959e65ac3d456ccda44671e67fb... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-bors
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc @oli-obk @nagisa