From d6353929ef414e6059c6da05b472630a801f8cad Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 28 Nov 2019 15:18:31 +0100 Subject: [PATCH 1/4] A heuristic diagnostic to try to help prevent issues related to old GNU ld bugs like issue 61539. --- src/librustc_codegen_ssa/back/link.rs | 98 +++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/src/librustc_codegen_ssa/back/link.rs b/src/librustc_codegen_ssa/back/link.rs index a2b50ea8e2bf7..972780e676bd4 100644 --- a/src/librustc_codegen_ssa/back/link.rs +++ b/src/librustc_codegen_ssa/back/link.rs @@ -460,6 +460,101 @@ fn link_staticlib<'a, B: ArchiveBuilder<'a>>(sess: &'a Session, } } + +/// rust-lang/rust#61539: bugs in older versions of GNU `ld` cause problems that +/// are readily exposed under our default setting of disabling PLT (PR +/// rust-lang/rust#54592). Heuristically detect and warn about this. +fn check_for_buggy_ld_version(sess: &Session, + program_name: &Path, + flavor: LinkerFlavor, + crate_type: config::CrateType) { + debug!("check_for_buggy_ld_version: \ + program_name: {:?} flavor: {:?} crate_type: {:?}", + program_name, flavor, crate_type); + + match crate_type { + config::CrateType::Dylib | + config::CrateType::ProcMacro => (), + + // FIXME: should we include CrateType::Cdylib in the warning? It is not + // clear why we haven't seen it there. + config::CrateType::Cdylib => return, + + // Static objects won't run into this (unless they load a dynamic + // object, which this heuristic is not attempting to detect). + config::CrateType::Executable | + config::CrateType::Rlib | + config::CrateType::Staticlib => return, + }; + + let mut version_cmd = Command::new(program_name); + match flavor { + LinkerFlavor::Gcc => { + // run `gcc -v -Xlinker --version` to query gcc for version info of underlying linker + version_cmd.args(&["-v", "-Xlinker", "--version"]); + } + LinkerFlavor::Ld => { + // run `ld --version` + version_cmd.args(&["--version"]); + } + LinkerFlavor::Msvc | + LinkerFlavor::Em | + LinkerFlavor::Lld(..) | + LinkerFlavor::PtxLinker => { + // Not GNU ld, so don't bother inspecting version. + return; + } + } + let version_check_invocation = format!("{:?}", &version_cmd); + debug!("check_for_buggy_ld_version version_check_invocation: {:?}", + version_check_invocation); + let output = match version_cmd.output() { + Err(_) => { + sess.warn(&format!("Linker version inspection failed to execute: `{}`", + version_check_invocation)); + return; + } + Ok(output) => output, + }; + let out = String::from_utf8_lossy(&output.stdout); + + debug!("check_for_buggy_ld_version out: {:?}", out); + + let parse = |first_line: &str| -> Option<(u32, u32)> { + let suffix = first_line.splitn(2, "GNU ld version ").last()?; + let version_components: Vec<_> = suffix.split('.').collect(); + let major: u32 = version_components.get(0)?.parse().ok()?; + let minor: u32 = version_components.get(1)?.parse().ok()?; + Some((major, minor)) + }; + + let first_line = out.lines().next(); + + debug!("check_for_buggy_ld_version first_line: {:?}", first_line); + + let (major, minor) = match first_line.and_then(parse) { + None => { + sess.warn(&format!("Linker version inspection failed to parse: `{}`, output: {}", + version_check_invocation, out)); + return; + } + Some(version) => version, + }; + debug!("check_for_buggy_ld_version (major, minor): {:?}", (major, minor)); + + let is_old_buggy_version = major < 2 || (major == 2 && minor < 29); + + if is_old_buggy_version { + let diag = sess.diagnostic(); + diag.warn(&format!("Using linker `{}` with Rust dynamic libraries has known bugs.", + first_line.unwrap())); + diag.note_without_error( + "Consider upgrading to GNU ld version 2.29 or newer, or using a different linker."); + diag.note_without_error( + "For more information, see https://github.com/rust-lang/rust/issues/61539"); + } +} + // Create a dynamic library or executable // // This will invoke the system linker/cc to create the resulting file. This @@ -476,6 +571,9 @@ fn link_natively<'a, B: ArchiveBuilder<'a>>(sess: &'a Session, // The invocations of cc share some flags across platforms let (pname, mut cmd) = get_linker(sess, &linker, flavor); + // rust-lang/rust#61539: heuristically inspect ld version to warn about bugs + check_for_buggy_ld_version(sess, &pname, flavor, crate_type); + if let Some(args) = sess.target.target.options.pre_link_args.get(&flavor) { cmd.args(args); } From a29d8056ac11caa87dae65d81067121770657047 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 4 Dec 2019 15:04:44 +0100 Subject: [PATCH 2/4] Refine warning to fire *solely* on dylib, not (dylib|proc-macro), to reduce false-positive rate. --- src/librustc_codegen_ssa/back/link.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/librustc_codegen_ssa/back/link.rs b/src/librustc_codegen_ssa/back/link.rs index 972780e676bd4..516fcae3a6d34 100644 --- a/src/librustc_codegen_ssa/back/link.rs +++ b/src/librustc_codegen_ssa/back/link.rs @@ -473,13 +473,24 @@ fn check_for_buggy_ld_version(sess: &Session, program_name, flavor, crate_type); match crate_type { - config::CrateType::Dylib | - config::CrateType::ProcMacro => (), + // This is the one case that we fire on, because it is the one place we + // know of where using the output in a "supported" fashion (*) can + // trigger the bug in old GNU ld versions. + // + // (*) Of course this raises the question of how much support do we give + // Rust dylibs in the first place + config::CrateType::Dylib => (), // FIXME: should we include CrateType::Cdylib in the warning? It is not // clear why we haven't seen it there. config::CrateType::Cdylib => return, + // We deliberately do not include CrateType::ProcMacro in the warning, + // as it would cause too many false-positives (and ot actually observe + // the known bugs in that context, you would have to be using the + // geneated dylib in an unsupported fashion anyway). + config::CrateType::ProcMacro => return, + // Static objects won't run into this (unless they load a dynamic // object, which this heuristic is not attempting to detect). config::CrateType::Executable | From 357542f3857302af9eabf390b82970071d46d435 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 29 Nov 2019 12:24:09 +0100 Subject: [PATCH 3/4] revised version parsing to handle arbitrary leading strings, rather than assuming it starts with "GNU ld version " --- src/librustc_codegen_ssa/back/link.rs | 40 +++++++++++++++++++-------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/src/librustc_codegen_ssa/back/link.rs b/src/librustc_codegen_ssa/back/link.rs index 516fcae3a6d34..c7cf03e6eecc2 100644 --- a/src/librustc_codegen_ssa/back/link.rs +++ b/src/librustc_codegen_ssa/back/link.rs @@ -531,19 +531,37 @@ fn check_for_buggy_ld_version(sess: &Session, debug!("check_for_buggy_ld_version out: {:?}", out); - let parse = |first_line: &str| -> Option<(u32, u32)> { - let suffix = first_line.splitn(2, "GNU ld version ").last()?; - let version_components: Vec<_> = suffix.split('.').collect(); - let major: u32 = version_components.get(0)?.parse().ok()?; - let minor: u32 = version_components.get(1)?.parse().ok()?; - Some((major, minor)) + let first_line = match out.lines().next() { + Some(line) => line, + None => { + sess.warn(&format!("Linker version inspection had no lines of output: `{}`", + version_check_invocation)); + return; + } }; - - let first_line = out.lines().next(); - debug!("check_for_buggy_ld_version first_line: {:?}", first_line); - let (major, minor) = match first_line.and_then(parse) { + let version_suffix_start = match first_line.find(" 2.") { + None => { + // if we cannot find ` 2.`, then assume that this an ld version that + // does not have the bug; no need for warning. + return; + } + Some(space_version_start) => { + // skip the space character so we are looking at "2.x.y-zzz" + space_version_start + 1 + } + }; + let version_suffix = &first_line[version_suffix_start..]; + debug!("check_for_buggy_ld_version version_suffix: {:?}", version_suffix); + + let parse = |suffix: &str| -> Option<(u32, u32)> { + let mut components = suffix.split('.'); + let major: u32 = components.next()?.parse().ok()?; + let minor: u32 = components.next()?.parse().ok()?; + Some((major, minor)) + }; + let (major, minor) = match parse(version_suffix) { None => { sess.warn(&format!("Linker version inspection failed to parse: `{}`, output: {}", version_check_invocation, out)); @@ -558,7 +576,7 @@ fn check_for_buggy_ld_version(sess: &Session, if is_old_buggy_version { let diag = sess.diagnostic(); diag.warn(&format!("Using linker `{}` with Rust dynamic libraries has known bugs.", - first_line.unwrap())); + first_line)); diag.note_without_error( "Consider upgrading to GNU ld version 2.29 or newer, or using a different linker."); diag.note_without_error( From c4a76c47e418df878b18b05884eb0912d410f63a Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 4 Dec 2019 15:12:53 +0100 Subject: [PATCH 4/4] Explicitly check that we are looking at GNU ld, and otherwise skip the check. (The first version of the PR had this implicitly by always searching for the string literal "GNU ld version " when looking for where to start the search for the version number.) --- src/librustc_codegen_ssa/back/link.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/librustc_codegen_ssa/back/link.rs b/src/librustc_codegen_ssa/back/link.rs index c7cf03e6eecc2..34e0628511a2d 100644 --- a/src/librustc_codegen_ssa/back/link.rs +++ b/src/librustc_codegen_ssa/back/link.rs @@ -541,6 +541,12 @@ fn check_for_buggy_ld_version(sess: &Session, }; debug!("check_for_buggy_ld_version first_line: {:?}", first_line); + if !first_line.contains("GNU ld") { + // If we cannot find "GNU ld" in the version string, then assume that + // this is not actually GNU ld; no need for warning. + return; + } + let version_suffix_start = match first_line.find(" 2.") { None => { // if we cannot find ` 2.`, then assume that this an ld version that