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

compiler: Use LLVM's Comdat support #131876

Merged
merged 3 commits into from
Oct 20, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) {
.collect::<Vec<_>>();
let initializer = cx.const_array(cx.type_ptr(), &name_globals);

let array = llvm::add_global(cx.llmod, cx.val_ty(initializer), "__llvm_coverage_names");
let array = llvm::add_global(cx.llmod, cx.val_ty(initializer), c"__llvm_coverage_names");
llvm::set_global_constant(array, true);
llvm::set_linkage(array, llvm::Linkage::InternalLinkage);
llvm::set_initializer(array, initializer);
Expand Down
15 changes: 10 additions & 5 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::cell::RefCell;
use std::ffi::CString;

use libc::c_uint;
use rustc_codegen_ssa::traits::{
Expand All @@ -12,6 +13,7 @@ use rustc_middle::mir::coverage::CoverageKind;
use rustc_middle::ty::Instance;
use rustc_middle::ty::layout::HasTyCtxt;
use rustc_target::abi::{Align, Size};
use rustc_target::spec::HasTargetSpec;
use tracing::{debug, instrument};

use crate::builder::Builder;
Expand Down Expand Up @@ -284,10 +286,10 @@ pub(crate) fn save_cov_data_to_mod<'ll, 'tcx>(
cx: &CodegenCx<'ll, 'tcx>,
cov_data_val: &'ll llvm::Value,
) {
let covmap_var_name = llvm::build_string(|s| unsafe {
let covmap_var_name = CString::new(llvm::build_byte_buffer(|s| unsafe {
llvm::LLVMRustCoverageWriteMappingVarNameToString(s);
})
.expect("Rust Coverage Mapping var name failed UTF-8 conversion");
}))
.unwrap();
debug!("covmap var name: {:?}", covmap_var_name);

let covmap_section_name = llvm::build_string(|s| unsafe {
Expand Down Expand Up @@ -322,7 +324,8 @@ pub(crate) fn save_func_record_to_mod<'ll, 'tcx>(
// of descriptions play distinct roles in LLVM IR; therefore, assign them different names (by
// appending "u" to the end of the function record var name, to prevent `linkonce_odr` merging.
let func_record_var_name =
format!("__covrec_{:X}{}", func_name_hash, if is_used { "u" } else { "" });
CString::new(format!("__covrec_{:X}{}", func_name_hash, if is_used { "u" } else { "" }))
.unwrap();
debug!("function record var name: {:?}", func_record_var_name);
debug!("function record section name: {:?}", covfun_section_name);

Expand All @@ -334,7 +337,9 @@ pub(crate) fn save_func_record_to_mod<'ll, 'tcx>(
llvm::set_section(llglobal, covfun_section_name);
// LLVM's coverage mapping format specifies 8-byte alignment for items in this section.
llvm::set_alignment(llglobal, Align::EIGHT);
llvm::set_comdat(cx.llmod, llglobal, &func_record_var_name);
if cx.target_spec().supports_comdat() {
llvm::set_comdat(cx.llmod, llglobal, &func_record_var_name);
}
cx.add_used_global(llglobal);
}

Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_llvm/src/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -787,7 +787,9 @@ fn codegen_msvc_try<'ll>(
let tydesc = bx.declare_global("__rust_panic_type_info", bx.val_ty(type_info));
unsafe {
llvm::LLVMRustSetLinkage(tydesc, llvm::Linkage::LinkOnceODRLinkage);
llvm::SetUniqueComdat(bx.llmod, tydesc);
if bx.cx.tcx.sess.target.supports_comdat() {
llvm::SetUniqueComdat(bx.llmod, tydesc);
}
llvm::LLVMSetInitializer(tydesc, type_info);
}

Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_codegen_llvm/src/llvm/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,7 @@ unsafe extern "C" {
pub type Attribute;
pub type Metadata;
pub type BasicBlock;
pub type Comdat;
}
#[repr(C)]
pub struct Builder<'a>(InvariantOpaque<'a>);
Expand Down Expand Up @@ -1490,6 +1491,9 @@ unsafe extern "C" {
pub fn LLVMSetUnnamedAddress(Global: &Value, UnnamedAddr: UnnamedAddr);

pub fn LLVMIsAConstantInt(value_ref: &Value) -> Option<&ConstantInt>;

pub fn LLVMGetOrInsertComdat(M: &Module, Name: *const c_char) -> &Comdat;
pub fn LLVMSetComdat(V: &Value, C: &Comdat);
}

#[link(name = "llvm-wrapper", kind = "static")]
Expand Down Expand Up @@ -2320,7 +2324,6 @@ unsafe extern "C" {

pub fn LLVMRustPositionBuilderAtStart<'a>(B: &Builder<'a>, BB: &'a BasicBlock);

pub fn LLVMRustSetComdat<'a>(M: &'a Module, V: &'a Value, Name: *const c_char, NameLen: size_t);
pub fn LLVMRustSetModulePICLevel(M: &Module);
pub fn LLVMRustSetModulePIELevel(M: &Module);
pub fn LLVMRustSetModuleCodeModel(M: &Module, Model: CodeModel);
Expand Down
20 changes: 12 additions & 8 deletions compiler/rustc_codegen_llvm/src/llvm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,10 @@ pub fn SetFunctionCallConv(fn_: &Value, cc: CallConv) {
// function.
// For more details on COMDAT sections see e.g., https://www.airs.com/blog/archives/52
pub fn SetUniqueComdat(llmod: &Module, val: &Value) {
unsafe {
let name = get_value_name(val);
LLVMRustSetComdat(llmod, val, name.as_ptr().cast(), name.len());
}
let name_buf = get_value_name(val).to_vec();
let name =
CString::from_vec_with_nul(name_buf).or_else(|buf| CString::new(buf.into_bytes())).unwrap();
set_comdat(llmod, val, &name);
Comment on lines +181 to +184
Copy link
Contributor

@Zalathar Zalathar Oct 19, 2024

Choose a reason for hiding this comment

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

Oh, looks like we need the supports-comdat check here too.

Or maybe this is a sign that the check should be inside set_comdat instead?

Copy link
Contributor

@klensy klensy Oct 19, 2024

Choose a reason for hiding this comment

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

Isn't this because !self.is_like_aix && !self.is_like_osx should be !self.is_like_aix || !self.is_like_osx?

No.

Copy link
Member Author

@workingjubilee workingjubilee Oct 19, 2024

Choose a reason for hiding this comment

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

I mean it's really a sign that "setting a comdat" needs to be a function on the CodegenCx, imo, where we already have acquired all the info, so that it isn't called when we don't have access to that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that makes sense. Happy to leave that for future work.

}

pub fn SetUnnamedAddress(global: &Value, unnamed: UnnamedAddr) {
Expand Down Expand Up @@ -217,8 +217,7 @@ pub fn set_section(llglobal: &Value, section_name: &str) {
}
}

pub fn add_global<'a>(llmod: &'a Module, ty: &'a Type, name: &str) -> &'a Value {
let name_cstr = CString::new(name).expect("unexpected CString error");
pub fn add_global<'a>(llmod: &'a Module, ty: &'a Type, name_cstr: &CStr) -> &'a Value {
unsafe { LLVMAddGlobal(llmod, ty, name_cstr.as_ptr()) }
}

Expand Down Expand Up @@ -252,9 +251,14 @@ pub fn set_alignment(llglobal: &Value, align: Align) {
}
}

pub fn set_comdat(llmod: &Module, llglobal: &Value, name: &str) {
/// Get the `name`d comdat from `llmod` and assign it to `llglobal`.
///
/// Inserts the comdat into `llmod` if it does not exist.
/// It is an error to call this if the target does not support comdat.
pub fn set_comdat(llmod: &Module, llglobal: &Value, name: &CStr) {
unsafe {
LLVMRustSetComdat(llmod, llglobal, name.as_ptr().cast(), name.len());
let comdat = LLVMGetOrInsertComdat(llmod, name.as_ptr());
LLVMSetComdat(llglobal, comdat);
}
}

Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_llvm/src/mono_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ impl<'tcx> PreDefineCodegenMethods<'tcx> for CodegenCx<'_, 'tcx> {
unsafe { llvm::LLVMRustSetLinkage(lldecl, base::linkage_to_llvm(linkage)) };
let attrs = self.tcx.codegen_fn_attrs(instance.def_id());
base::set_link_section(lldecl, attrs);
if linkage == Linkage::LinkOnceODR || linkage == Linkage::WeakODR {
if (linkage == Linkage::LinkOnceODR || linkage == Linkage::WeakODR)
&& self.tcx.sess.target.supports_comdat()
{
llvm::SetUniqueComdat(self.llmod, lldecl);
}

Expand Down
10 changes: 0 additions & 10 deletions compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1658,16 +1658,6 @@ extern "C" void LLVMRustPositionBuilderAtStart(LLVMBuilderRef B,
unwrap(B)->SetInsertPoint(unwrap(BB), Point);
}

extern "C" void LLVMRustSetComdat(LLVMModuleRef M, LLVMValueRef V,
const char *Name, size_t NameLen) {
Triple TargetTriple = Triple(unwrap(M)->getTargetTriple());
GlobalObject *GV = unwrap<GlobalObject>(V);
if (TargetTriple.supportsCOMDAT()) {
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
StringRef NameRef(Name, NameLen);
GV->setComdat(unwrap(M)->getOrInsertComdat(NameRef));
}
}
Comment on lines -1661 to -1669
Copy link
Member Author

@workingjubilee workingjubilee Oct 19, 2024

Choose a reason for hiding this comment

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

afaict the LLVM-C API doesn't have an exact match of the support check we do here, which is why I implemented it via Rust. I'll go fix that.


enum class LLVMRustLinkage {
ExternalLinkage = 0,
AvailableExternallyLinkage = 1,
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_target/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2514,6 +2514,13 @@ fn add_link_args(link_args: &mut LinkArgs, flavor: LinkerFlavor, args: &[&'stati
add_link_args_iter(link_args, flavor, args.iter().copied().map(Cow::Borrowed))
}

impl TargetOptions {
pub fn supports_comdat(&self) -> bool {
// XCOFF and MachO don't support COMDAT.
!self.is_like_aix && !self.is_like_osx
}
}

impl TargetOptions {
fn link_args(flavor: LinkerFlavor, args: &[&'static str]) -> LinkArgs {
let mut link_args = LinkArgs::new();
Expand Down
Loading