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

Improve bitcode generation for Apple platforms #71970

Merged
merged 3 commits into from
May 8, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/librustc_codegen_llvm/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -651,10 +651,10 @@ pub(crate) unsafe fn codegen(
"LLVM_module_codegen_embed_bitcode",
&module.name[..],
);
embed_bitcode(cgcx, llcx, llmod, Some(data));
embed_bitcode(cgcx, llcx, llmod, &config.bc_cmdline, Some(data));
}
} else if config.emit_obj == EmitObj::ObjectCode(BitcodeSection::Marker) {
embed_bitcode(cgcx, llcx, llmod, None);
embed_bitcode(cgcx, llcx, llmod, &config.bc_cmdline, None);
}

if config.emit_ir {
Expand Down Expand Up @@ -777,8 +777,8 @@ pub(crate) unsafe fn codegen(
/// * __LLVM,__cmdline
///
/// It appears *both* of these sections are necessary to get the linker to
/// recognize what's going on. For us though we just always throw in an empty
/// cmdline section.
/// recognize what's going on. A suitable cmdline value is taken from the
/// target spec.
///
/// Furthermore debug/O1 builds don't actually embed bitcode but rather just
/// embed an empty section.
Expand All @@ -789,6 +789,7 @@ unsafe fn embed_bitcode(
cgcx: &CodegenContext<LlvmCodegenBackend>,
llcx: &llvm::Context,
llmod: &llvm::Module,
cmdline: &str,
bitcode: Option<&[u8]>,
) {
let llconst = common::bytes_in_context(llcx, bitcode.unwrap_or(&[]));
Expand All @@ -800,14 +801,15 @@ unsafe fn embed_bitcode(
llvm::LLVMSetInitializer(llglobal, llconst);

let is_apple = cgcx.opts.target_triple.triple().contains("-ios")
|| cgcx.opts.target_triple.triple().contains("-darwin");
|| cgcx.opts.target_triple.triple().contains("-darwin")
|| cgcx.opts.target_triple.triple().contains("-tvos");

let section = if is_apple { "__LLVM,__bitcode\0" } else { ".llvmbc\0" };
llvm::LLVMSetSection(llglobal, section.as_ptr().cast());
llvm::LLVMRustSetLinkage(llglobal, llvm::Linkage::PrivateLinkage);
llvm::LLVMSetGlobalConstant(llglobal, llvm::True);

let llconst = common::bytes_in_context(llcx, &[]);
let llconst = common::bytes_in_context(llcx, cmdline.as_bytes());
let llglobal = llvm::LLVMAddGlobal(
llmod,
common::val_ty(llconst),
Expand Down
4 changes: 4 additions & 0 deletions src/librustc_codegen_ssa/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ pub struct ModuleConfig {
pub emit_ir: bool,
pub emit_asm: bool,
pub emit_obj: EmitObj,
pub bc_cmdline: String,

// Miscellaneous flags. These are mostly copied from command-line
// options.
Expand Down Expand Up @@ -147,6 +148,8 @@ impl ModuleConfig {
|| sess.opts.cg.linker_plugin_lto.enabled()
{
EmitObj::Bitcode
} else if sess.target.target.options.forces_embed_bitcode {
Copy link
Member

Choose a reason for hiding this comment

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

Could this get folded into need_crate_bitcode_for_rlib?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is interesting. Right now I can't see any code path for embedding Marker bitcode. Do we need it? IMO probably not. Would it make any sense for OptLevel to downgrade the wishes of my target spec from Full to Marker? Again probably not. It's certainly surprising, and it complicates the situation for someone debugging Rust in Xcode.

So at the risk of being over-enthusiastic, I've codified this by removing the Marker variant along with the ineffectual OptLevel match. Do you reckon this makes sense?

Copy link
Member

Choose a reason for hiding this comment

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

Oh thanks for the keen eye! I think that may have actually been an issue from an earlier one of my PRs, but at this point I think it's safe to remove. We can always add it back in later if truly necessary.

EmitObj::ObjectCode(BitcodeSection::Full)
} else if need_crate_bitcode_for_rlib(sess) {
let force_full = need_crate_bitcode_for_rlib(sess);
match sess.opts.optimize {
Expand Down Expand Up @@ -211,6 +214,7 @@ impl ModuleConfig {
false
),
emit_obj,
bc_cmdline: sess.target.target.options.bitcode_llvm_cmdline.clone(),

verify_llvm_ir: sess.verify_llvm_ir(),
no_prepopulate_passes: sess.opts.cg.no_prepopulate_passes,
Expand Down
12 changes: 12 additions & 0 deletions src/librustc_target/spec/aarch64_apple_ios.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@ pub fn target() -> TargetResult {
eliminate_frame_pointer: false,
max_atomic_width: Some(128),
abi_blacklist: super::arm_base::abi_blacklist(),
forces_embed_bitcode: true,
// Taken from a clang build on Xcode 11.4.1.
// These arguments are not actually invoked - they just have
// to look right to pass App Store validation.
bitcode_llvm_cmdline: "-triple\0\
arm64-apple-ios11.0.0\0\
-emit-obj\0\
-disable-llvm-passes\0\
-target-abi\0\
darwinpcs\0\
-Os\0"
.to_string(),
..base
},
})
Expand Down
1 change: 1 addition & 0 deletions src/librustc_target/spec/aarch64_apple_tvos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub fn target() -> TargetResult {
eliminate_frame_pointer: false,
max_atomic_width: Some(128),
abi_blacklist: super::arm_base::abi_blacklist(),
forces_embed_bitcode: true,
..base
},
})
Expand Down
10 changes: 10 additions & 0 deletions src/librustc_target/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,10 @@ pub struct TargetOptions {
// If we give emcc .o files that are actually .bc files it
// will 'just work'.
pub obj_is_bitcode: bool,
/// Whether the target requires that emitted object code includes bitcode.
pub forces_embed_bitcode: bool,
/// Content of the LLVM cmdline section associated with embedded bitcode.
pub bitcode_llvm_cmdline: String,

/// Don't use this field; instead use the `.min_atomic_width()` method.
pub min_atomic_width: Option<u64>,
Expand Down Expand Up @@ -939,6 +943,8 @@ impl Default for TargetOptions {
allow_asm: true,
has_elf_tls: false,
obj_is_bitcode: false,
forces_embed_bitcode: false,
bitcode_llvm_cmdline: String::new(),
min_atomic_width: None,
max_atomic_width: None,
atomic_cas: true,
Expand Down Expand Up @@ -1278,6 +1284,8 @@ impl Target {
key!(main_needs_argc_argv, bool);
key!(has_elf_tls, bool);
key!(obj_is_bitcode, bool);
key!(forces_embed_bitcode, bool);
key!(bitcode_llvm_cmdline);
key!(max_atomic_width, Option<u64>);
key!(min_atomic_width, Option<u64>);
key!(atomic_cas, bool);
Expand Down Expand Up @@ -1505,6 +1513,8 @@ impl ToJson for Target {
target_option_val!(main_needs_argc_argv);
target_option_val!(has_elf_tls);
target_option_val!(obj_is_bitcode);
target_option_val!(forces_embed_bitcode);
target_option_val!(bitcode_llvm_cmdline);
target_option_val!(min_atomic_width);
target_option_val!(max_atomic_width);
target_option_val!(atomic_cas);
Expand Down