-
-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Don't let compiler-builtins' weak math definitions shadow system libm
#163084
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| // ignore-tidy-file-filelength | ||
| mod raw_dylib; | ||
|
|
||
| use std::collections::BTreeSet; | ||
|
|
@@ -751,6 +752,72 @@ fn link_rlib<'a>( | |
| /// | ||
| /// There's no need to include metadata in a static archive, so ensure to not link in the metadata | ||
| /// object file (and also don't prepare the archive with a metadata file). | ||
| // Weak f32/f64 math symbols from c-b's `full_availability` module; keep in sync with | ||
| // `compiler-builtins/src/math/mod.rs`. | ||
| const COMPILER_BUILTINS_LIBM_SYMBOLS: &[&str] = &[ | ||
| "cbrtf", | ||
| "ceilf", | ||
| "copysignf", | ||
| "fabsf", | ||
| "fdimf", | ||
| "floorf", | ||
| "fmaf", | ||
| "fmaxf", | ||
| "fminf", | ||
| "fmodf", | ||
| "rintf", | ||
| "roundf", | ||
| "sqrtf", | ||
| "truncf", | ||
| "cbrt", | ||
| "ceil", | ||
| "copysign", | ||
| "fabs", | ||
| "fdim", | ||
| "floor", | ||
| "fma", | ||
| "fmax", | ||
| "fmin", | ||
| "fmod", | ||
| "rint", | ||
| "round", | ||
| "sqrt", | ||
| "trunc", | ||
| ]; | ||
|
|
||
| fn compiler_builtins_libm_members(rlib_path: &Path) -> FxHashSet<String> { | ||
| let Ok(file) = File::open(rlib_path) else { return FxHashSet::default() }; | ||
| let Ok(mmap) = (unsafe { Mmap::map(file) }) else { return FxHashSet::default() }; | ||
| let Ok(archive) = object::read::archive::ArchiveFile::parse(&*mmap) else { | ||
| return FxHashSet::default(); | ||
| }; | ||
| let Some(symbols) = archive.symbols().ok().flatten() else { return FxHashSet::default() }; | ||
|
|
||
| let mut members = FxHashSet::default(); | ||
| for symbol in symbols { | ||
| let Ok(symbol) = symbol else { continue }; | ||
| if !COMPILER_BUILTINS_LIBM_SYMBOLS.iter().any(|&name| name.as_bytes() == symbol.name()) { | ||
| continue; | ||
| } | ||
| if let Ok(member) = archive.member(symbol.offset()) | ||
| && let Ok(name) = str::from_utf8(member.name()) | ||
| { | ||
| members.insert(name.to_string()); | ||
| } | ||
| } | ||
| members | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I understanding correctly that this is collecting the subset of |
||
|
|
||
| // Whether a system libm (`-lm`) is among the native libraries recorded for the final link. | ||
| fn links_libm(crate_info: &CrateInfo, sess: &Session) -> bool { | ||
|
Comment on lines
+811
to
+812
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make this a doc comment |
||
| crate_info | ||
| .native_libraries | ||
| .values() | ||
| .chain(std::iter::once(&crate_info.used_libraries)) | ||
| .flatten() | ||
| .any(|lib| lib.name.as_str() == "m" && lib.kind.is_dllimport() && relevant_lib(sess, lib)) | ||
| } | ||
|
|
||
| fn link_staticlib( | ||
| sess: &Session, | ||
| archive_builder_builder: &dyn ArchiveBuilderBuilder, | ||
|
|
@@ -773,7 +840,15 @@ fn link_staticlib( | |
| ); | ||
| let mut all_native_libs = vec![]; | ||
|
|
||
| // With `-lm` recorded, drop c-b's weak math definitions so libm's strong ones win (#142119). | ||
| let links_libm = links_libm(crate_info, sess); | ||
| let res = each_linked_rlib(crate_info, Some(CrateType::StaticLib), &mut |cnum, path| { | ||
| debug_assert!( | ||
| !links_libm | ||
| || crate_info.compiler_builtins != Some(cnum) | ||
| || ignored_for_lto(sess, crate_info, cnum), | ||
| "compiler-builtins must not participate in LTO for the omit-libm skip to hold" | ||
| ); | ||
| let lto = are_upstream_rust_objects_already_included(sess) | ||
| && !ignored_for_lto(sess, crate_info, cnum); | ||
|
|
||
|
|
@@ -792,6 +867,11 @@ fn link_staticlib( | |
| .enumerate() | ||
| .filter_map(|(i, _)| bundled_filenames.get(i).copied().flatten()) | ||
| .collect(); | ||
| let cb_libm_members = if links_libm && crate_info.compiler_builtins == Some(cnum) { | ||
| compiler_builtins_libm_members(path) | ||
| } else { | ||
| FxHashSet::default() | ||
| }; | ||
| ab.add_archive( | ||
| path, | ||
| AddArchiveKind::Rlib(rmeta_link_cache, &|fname: &str, entry_kind| { | ||
|
|
@@ -810,6 +890,9 @@ fn link_staticlib( | |
| return true; | ||
| } | ||
|
|
||
| if cb_libm_members.contains(fname) { | ||
| return true; | ||
| } | ||
| false | ||
| }), | ||
| ) | ||
|
|
@@ -3036,6 +3119,12 @@ fn linker_with_args( | |
| cmd.link_arg(std::path::absolute(&*sess.target_tlib_path.dir).unwrap()); | ||
| } | ||
|
|
||
| // Emit `-lm` ahead of the rlibs on glibc so its strong definitions win over c-b's weak ones; | ||
| // every other native dylib stays after the rlibs below. | ||
| if sess.target.env == Env::Gnu && links_libm(crate_info, sess) { | ||
| cmd.link_dylib_by_name("m", false, true); | ||
| } | ||
|
|
||
| // Upstream rust crates and their non-dynamic native libraries. | ||
| add_upstream_rust_crates( | ||
| cmd, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| #![no_std] | ||
| #![crate_type = "staticlib"] | ||
|
|
||
| use core::panic::PanicInfo; | ||
|
|
||
| #[panic_handler] | ||
| fn panic(_: &PanicInfo) -> ! { | ||
| loop {} | ||
| } | ||
|
|
||
| // Declaring `ceilf`/`sqrtf` pulls compiler-builtins' weak definitions into the archive, while | ||
| // `#[link(name = "m")]` makes the staticlib record system libm (`-lm`) as a dependency. When | ||
| // libm is recorded, rustc must skip those weak definitions so that a later link against `-lm` | ||
| // resolves the strong libm versions (rust-lang/rust#142119). | ||
| #[link(name = "m")] | ||
| extern "C" { | ||
| fn ceilf(x: f32) -> f32; | ||
| fn sqrtf(x: f32) -> f32; | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| pub extern "C" fn use_mathf(x: f32) -> f32 { | ||
| unsafe { ceilf(x) + sqrtf(x) } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| #![no_std] | ||
| #![crate_type = "staticlib"] | ||
|
|
||
| use core::panic::PanicInfo; | ||
|
|
||
| #[panic_handler] | ||
| fn panic(_: &PanicInfo) -> ! { | ||
| loop {} | ||
| } | ||
|
|
||
| // No `#[link(name = "m")]`: without a system libm recorded, compiler-builtins' weak f32/f64 math | ||
| // definitions are the only provider of `ceilf`/`sqrtf`, so they must be kept. | ||
| extern "C" { | ||
| fn ceilf(x: f32) -> f32; | ||
| fn sqrtf(x: f32) -> f32; | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| pub extern "C" fn use_mathf(x: f32) -> f32 { | ||
| unsafe { ceilf(x) + sqrtf(x) } | ||
| } |
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to update the |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| // compiler-builtins weakly defines the f32/f64 math symbols (its `full_availability` list) so | ||
| // `no_std` targets without a system libm still work. A staticlib that records `-lm` must omit | ||
| // them so libm's strong definitions win at the final link (#142119); the no-`-lm` control keeps | ||
| // its weak fallbacks. `only-gnu` because only gnu libm (glibc) provides these symbols. | ||
|
|
||
| //@ only-gnu | ||
| //@ ignore-cross-compile | ||
|
|
||
| use std::collections::HashSet; | ||
|
|
||
| use run_make_support::object::read::archive::ArchiveFile; | ||
| use run_make_support::object::read::elf::{FileHeader as _, SectionHeader as _, Sym as _}; | ||
| use run_make_support::object::{Endianness, elf}; | ||
| use run_make_support::{rfs, rustc, static_lib_name}; | ||
|
|
||
| // Keep in sync with `COMPILER_BUILTINS_LIBM_SYMBOLS` in rustc_codegen_ssa/src/back/link.rs. | ||
| const LIBM_SYMBOLS: &[&str] = &[ | ||
| "cbrtf", | ||
| "ceilf", | ||
| "copysignf", | ||
| "fabsf", | ||
| "fdimf", | ||
| "floorf", | ||
| "fmaf", | ||
| "fmaxf", | ||
| "fminf", | ||
| "fmodf", | ||
| "rintf", | ||
| "roundf", | ||
| "sqrtf", | ||
| "truncf", | ||
| "cbrt", | ||
| "ceil", | ||
| "copysign", | ||
| "fabs", | ||
| "fdim", | ||
| "floor", | ||
| "fma", | ||
| "fmax", | ||
| "fmin", | ||
| "fmod", | ||
| "rint", | ||
| "round", | ||
| "sqrt", | ||
| "trunc", | ||
| ]; | ||
|
|
||
| fn main() { | ||
| rustc().input("no_libm.rs").crate_type("staticlib").panic("abort").run(); | ||
| let no_libm = static_lib_name("no_libm"); | ||
| let no_libm_defined = defined_global_symbols(&no_libm); | ||
| for name in LIBM_SYMBOLS { | ||
| assert!( | ||
| no_libm_defined.contains(*name), | ||
| "expected weak fallback `{name}` to be kept without `-lm` in `{}`", | ||
| no_libm | ||
| ); | ||
| } | ||
|
|
||
| rustc().input("lib.rs").crate_type("staticlib").panic("abort").run(); | ||
| let with_libm = static_lib_name("lib"); | ||
| let with_libm_defined = defined_global_symbols(&with_libm); | ||
| for name in LIBM_SYMBOLS { | ||
| assert!( | ||
| !with_libm_defined.contains(*name), | ||
| "weak definition `{name}` should be omitted when `-lm` is recorded in `{with_libm}`" | ||
| ); | ||
| } | ||
|
|
||
| // Not a blanket strip: the f16/f128 and integer fallbacks survive. `ceilf16` also guards | ||
| // against matching `ceilf` as a prefix of `ceilf16`. | ||
| assert!( | ||
| with_libm_defined.contains("ceilf16"), | ||
| "compiler-builtins' f16 fallback must survive the omit in `{with_libm}`" | ||
| ); | ||
| assert!( | ||
| with_libm_defined.contains("__floatdidf"), | ||
| "compiler-builtins' integer fallback must survive the omit in `{with_libm}`" | ||
| ); | ||
| } | ||
|
|
||
| /// Every defined global/weak symbol in the archive. The omitted symbols can still appear as | ||
| /// undefined references (e.g. `use_mathf` calling `ceilf`), so filter `SHN_UNDEF` rather than | ||
| /// checking mere presence. | ||
| fn defined_global_symbols(archive_path: &str) -> HashSet<String> { | ||
| let archive_data = rfs::read(archive_path); | ||
| let archive = ArchiveFile::parse(archive_data.as_slice()).unwrap(); | ||
| let mut defined = HashSet::new(); | ||
|
|
||
| for member in archive.members() { | ||
| let member = member.unwrap(); | ||
| let data = member.data(archive_data.as_slice()).unwrap(); | ||
|
|
||
| if let Ok(header) = elf::FileHeader64::<Endianness>::parse(data) { | ||
| collect_elf_defined(header, data, &mut defined); | ||
| } else if let Ok(header) = elf::FileHeader32::<Endianness>::parse(data) { | ||
| collect_elf_defined(header, data, &mut defined); | ||
| } | ||
| } | ||
|
|
||
| defined | ||
| } | ||
|
|
||
| fn collect_elf_defined< | ||
| Elf: run_make_support::object::read::elf::FileHeader<Endian = Endianness>, | ||
| >( | ||
| header: &Elf, | ||
| data: &[u8], | ||
| defined: &mut HashSet<String>, | ||
| ) { | ||
| let Ok(endian) = header.endian() else { return }; | ||
| let Ok(sections) = header.sections(endian, data) else { return }; | ||
|
|
||
| for (si, section) in sections.enumerate() { | ||
| if section.sh_type(endian) != elf::SHT_SYMTAB { | ||
| continue; | ||
| } | ||
| let Ok(symbols) = run_make_support::object::read::elf::SymbolTable::parse( | ||
| endian, data, §ions, si, section, | ||
| ) else { | ||
| continue; | ||
| }; | ||
| let strtab = symbols.strings(); | ||
|
|
||
| for symbol in symbols.symbols() { | ||
| let bind = symbol.st_bind(); | ||
| if bind != elf::STB_GLOBAL && bind != elf::STB_WEAK { | ||
| continue; | ||
| } | ||
| if symbol.st_shndx(endian) == elf::SHN_UNDEF { | ||
| continue; | ||
| } | ||
| let Ok(name_bytes) = symbol.name(endian, strtab) else { continue }; | ||
| if let Ok(name) = str::from_utf8(name_bytes) { | ||
| defined.insert(name.to_string()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?