Rollup merge of #87092 - ricobbe:fix-raw-dylib-multiple-definitions, r=petrochenkov

Remove nondeterminism in multiple-definitions test

Compare all fields in `DllImport` when sorting to avoid nondeterminism in the error for multiple inconsistent definitions of an extern function.  Restore the multiple-definitions test.

Resolves #87084.

compiler/rustc_codegen_cranelift/src/lib.rs

@@ -221,9 +221,7 @@ impl CodegenBackend for CraneliftCodegenBackend {
             sess,
             &codegen_results,
             outputs,
-        );
+        )
-
-        Ok(())
     }
 }
 

compiler/rustc_codegen_llvm/src/lib.rs

@@ -292,9 +292,7 @@ impl CodegenBackend for LlvmCodegenBackend {
 
         // Run the linker on any artifacts that resulted from the LLVM run.
         // This should produce either a finished executable or library.
-        link_binary::<LlvmArchiveBuilder<'_>>(sess, &codegen_results, outputs);
+        link_binary::<LlvmArchiveBuilder<'_>>(sess, &codegen_results, outputs)
-
-        Ok(())
     }
 }
 

compiler/rustc_codegen_ssa/src/back/link.rs

@@ -1,9 +1,9 @@
-use rustc_data_structures::fx::{FxHashMap, FxHashSet};
+use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
 use rustc_data_structures::temp_dir::MaybeTempDir;
-use rustc_errors::Handler;
+use rustc_errors::{ErrorReported, Handler};
 use rustc_fs_util::fix_windows_verbatim_for_gcc;
 use rustc_hir::def_id::CrateNum;
-use rustc_middle::middle::cstore::{DllCallingConvention, DllImport};
+use rustc_middle::middle::cstore::DllImport;
 use rustc_middle::middle::dependency_format::Linkage;
 use rustc_session::config::{self, CFGuard, CrateType, DebugInfo, LdImpl, Strip};
 use rustc_session::config::{OutputFilenames, OutputType, PrintRequest};
@@ -35,7 +35,6 @@ use object::{Architecture, BinaryFormat, Endianness, FileFlags, SectionFlags, Se
 use tempfile::Builder as TempFileBuilder;
 
 use std::ffi::OsString;
-use std::iter::FromIterator;
 use std::path::{Path, PathBuf};
 use std::process::{ExitStatus, Output, Stdio};
 use std::{ascii, char, env, fmt, fs, io, mem, str};
@@ -54,7 +53,7 @@ pub fn link_binary<'a, B: ArchiveBuilder<'a>>(
     sess: &'a Session,
     codegen_results: &CodegenResults,
     outputs: &OutputFilenames,
-) {
+) -> Result<(), ErrorReported> {
     let _timer = sess.timer("link_binary");
     let output_metadata = sess.opts.output_types.contains_key(&OutputType::Metadata);
     for &crate_type in sess.crate_types().iter() {
@@ -95,11 +94,17 @@ pub fn link_binary<'a, B: ArchiveBuilder<'a>>(
             match crate_type {
                 CrateType::Rlib => {
                     let _timer = sess.timer("link_rlib");
-                    link_rlib::<B>(sess, codegen_results, RlibFlavor::Normal, &out_filename, &path)
+                    link_rlib::<B>(
-                        .build();
+                        sess,
+                        codegen_results,
+                        RlibFlavor::Normal,
+                        &out_filename,
+                        &path,
+                    )?
+                    .build();
                 }
                 CrateType::Staticlib => {
-                    link_staticlib::<B>(sess, codegen_results, &out_filename, &path);
+                    link_staticlib::<B>(sess, codegen_results, &out_filename, &path)?;
                 }
                 _ => {
                     link_natively::<B>(
@@ -145,6 +150,8 @@ pub fn link_binary<'a, B: ArchiveBuilder<'a>>(
             }
         }
     });
+
+    Ok(())
 }
 
 pub fn each_linked_rlib(
@@ -220,7 +227,7 @@ fn link_rlib<'a, B: ArchiveBuilder<'a>>(
     flavor: RlibFlavor,
     out_filename: &Path,
     tmpdir: &MaybeTempDir,
-) -> B {
+) -> Result<B, ErrorReported> {
     info!("preparing rlib to {:?}", out_filename);
     let mut ab = <B as ArchiveBuilder>::new(sess, out_filename, None);
 
@@ -259,7 +266,7 @@ fn link_rlib<'a, B: ArchiveBuilder<'a>>(
     }
 
     for (raw_dylib_name, raw_dylib_imports) in
-        collate_raw_dylibs(sess, &codegen_results.crate_info.used_libraries)
+        collate_raw_dylibs(sess, &codegen_results.crate_info.used_libraries)?
     {
         ab.inject_dll_import_lib(&raw_dylib_name, &raw_dylib_imports, tmpdir);
     }
@@ -312,7 +319,7 @@ fn link_rlib<'a, B: ArchiveBuilder<'a>>(
             }
         }
     }
-    return ab;
+    return Ok(ab);
 
     // For rlibs we "pack" rustc metadata into a dummy object file. When rustc
     // creates a dylib crate type it will pass `--whole-archive` (or the
@@ -454,65 +461,40 @@ fn link_rlib<'a, B: ArchiveBuilder<'a>>(
 fn collate_raw_dylibs(
     sess: &Session,
     used_libraries: &[NativeLib],
-) -> Vec<(String, Vec<DllImport>)> {
+) -> Result<Vec<(String, Vec<DllImport>)>, ErrorReported> {
-    let mut dylib_table: FxHashMap<String, FxHashSet<DllImport>> = FxHashMap::default();
+    // Use index maps to preserve original order of imports and libraries.
+    let mut dylib_table = FxIndexMap::<String, FxIndexMap<Symbol, &DllImport>>::default();
 
     for lib in used_libraries {
         if lib.kind == NativeLibKind::RawDylib {
-            let name = lib.name.unwrap_or_else(||
+            let ext = if matches!(lib.verbatim, Some(true)) { "" } else { ".dll" };
-                bug!("`link` attribute with kind = \"raw-dylib\" and no name should have caused error earlier")
+            let name = format!("{}{}", lib.name.expect("unnamed raw-dylib library"), ext);
-            );
+            let imports = dylib_table.entry(name.clone()).or_default();
-            let name = if matches!(lib.verbatim, Some(true)) {
+            for import in &lib.dll_imports {
-                name.to_string()
+                if let Some(old_import) = imports.insert(import.name, import) {
-            } else {
+                    // FIXME: when we add support for ordinals, figure out if we need to do anything
-                format!("{}.dll", name)
+                    // if we have two DllImport values with the same name but different ordinals.
-            };
+                    if import.calling_convention != old_import.calling_convention {
-            dylib_table.entry(name).or_default().extend(lib.dll_imports.iter().cloned());
+                        sess.span_err(
-        }
+                            import.span,
-    }
+                            &format!(
-
+                                "multiple declarations of external function `{}` from \
-    // Rustc already signals an error if we have two imports with the same name but different
+                                 library `{}` have different calling conventions",
-    // calling conventions (or function signatures), so we don't have pay attention to those
+                                import.name, name,
-    // when ordering.
+                            ),
-    // FIXME: when we add support for ordinals, figure out if we need to do anything if we
+                        );
-    // have two DllImport values with the same name but different ordinals.
+                    }
-    let mut result: Vec<(String, Vec<DllImport>)> = dylib_table
-        .into_iter()
-        .map(|(lib_name, import_table)| {
-            let mut imports = Vec::from_iter(import_table.into_iter());
-            imports.sort_unstable_by_key(|x: &DllImport| x.name.as_str());
-            (lib_name, imports)
-        })
-        .collect::<Vec<_>>();
-    result.sort_unstable_by(|a: &(String, Vec<DllImport>), b: &(String, Vec<DllImport>)| {
-        a.0.cmp(&b.0)
-    });
-    let result = result;
-
-    // Check for multiple imports with the same name but different calling conventions or
-    // (when relevant) argument list sizes.  Rustc only signals an error for this if the
-    // declarations are at the same scope level; if one shadows the other, we only get a lint
-    // warning.
-    for (library, imports) in &result {
-        let mut import_table: FxHashMap<Symbol, DllCallingConvention> = FxHashMap::default();
-        for import in imports {
-            if let Some(old_convention) =
-                import_table.insert(import.name, import.calling_convention)
-            {
-                if import.calling_convention != old_convention {
-                    sess.span_fatal(
-                        import.span,
-                        &format!(
-                            "multiple definitions of external function `{}` from library `{}` have different calling conventions",
-                            import.name,
-                            library,
-                    ));
                 }
             }
         }
     }
-
+    sess.compile_status()?;
-    result
+    Ok(dylib_table
+        .into_iter()
+        .map(|(name, imports)| {
+            (name, imports.into_iter().map(|(_, import)| import.clone()).collect())
+        })
+        .collect())
 }
 
 /// Create a static archive.
@@ -531,9 +513,9 @@ fn link_staticlib<'a, B: ArchiveBuilder<'a>>(
     codegen_results: &CodegenResults,
     out_filename: &Path,
     tempdir: &MaybeTempDir,
-) {
+) -> Result<(), ErrorReported> {
     let mut ab =
-        link_rlib::<B>(sess, codegen_results, RlibFlavor::StaticlibBase, out_filename, tempdir);
+        link_rlib::<B>(sess, codegen_results, RlibFlavor::StaticlibBase, out_filename, tempdir)?;
     let mut all_native_libs = vec![];
 
     let res = each_linked_rlib(&codegen_results.crate_info, &mut |cnum, path| {
@@ -581,6 +563,8 @@ fn link_staticlib<'a, B: ArchiveBuilder<'a>>(
             print_native_static_libs(sess, &all_native_libs);
         }
     }
+
+    Ok(())
 }
 
 fn escape_stdout_stderr_string(s: &[u8]) -> String {

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -1581,7 +1581,7 @@ impl EncodeContext<'a, 'tcx> {
     fn encode_native_libraries(&mut self) -> Lazy<[NativeLib]> {
         empty_proc_macro!(self);
         let used_libraries = self.tcx.native_libraries(LOCAL_CRATE);
-        self.lazy(used_libraries.iter().cloned())
+        self.lazy(used_libraries.iter())
     }
 
     fn encode_foreign_modules(&mut self) -> Lazy<[ForeignModule]> {

compiler/rustc_middle/src/middle/cstore.rs

@@ -64,7 +64,7 @@ pub enum LinkagePreference {
     RequireStatic,
 }
 
-#[derive(Clone, Debug, Encodable, Decodable, HashStable)]
+#[derive(Debug, Encodable, Decodable, HashStable)]
 pub struct NativeLib {
     pub kind: NativeLibKind,
     pub name: Option<Symbol>,
@@ -75,7 +75,7 @@ pub struct NativeLib {
     pub dll_imports: Vec<DllImport>,
 }
 
-#[derive(Clone, Debug, PartialEq, Eq, Encodable, Decodable, Hash, HashStable)]
+#[derive(Clone, Debug, Encodable, Decodable, HashStable)]
 pub struct DllImport {
     pub name: Symbol,
     pub ordinal: Option<u16>,
@@ -92,7 +92,7 @@ pub struct DllImport {
 ///
 /// The usize value, where present, indicates the size of the function's argument list
 /// in bytes.
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Encodable, Decodable, Hash, HashStable)]
+#[derive(Clone, PartialEq, Debug, Encodable, Decodable, HashStable)]
 pub enum DllCallingConvention {
     C,
     Stdcall(usize),

src/test/ui/rfc-2627-raw-dylib/multiple-declarations.rs

@@ -0,0 +1,19 @@
+// only-i686-pc-windows-msvc
+// compile-flags: --crate-type lib --emit link
+#![allow(clashing_extern_declarations)]
+#![feature(raw_dylib)]
+//~^ WARN the feature `raw_dylib` is incomplete
+#[link(name = "foo", kind = "raw-dylib")]
+extern "C" {
+    fn f(x: i32);
+}
+
+pub fn lib_main() {
+    #[link(name = "foo", kind = "raw-dylib")]
+    extern "stdcall" {
+        fn f(x: i32);
+        //~^ ERROR multiple declarations of external function `f` from library `foo.dll` have different calling conventions
+    }
+
+    unsafe { f(42); }
+}

src/test/ui/rfc-2627-raw-dylib/multiple-declarations.stderr

@@ -0,0 +1,17 @@
+warning: the feature `raw_dylib` is incomplete and may not be safe to use and/or cause compiler crashes
+  --> $DIR/multiple-declarations.rs:4:12
+   |
+LL | #![feature(raw_dylib)]
+   |            ^^^^^^^^^
+   |
+   = note: `#[warn(incomplete_features)]` on by default
+   = note: see issue #58713 <https://github.com/rust-lang/rust/issues/58713> for more information
+
+error: multiple declarations of external function `f` from library `foo.dll` have different calling conventions
+  --> $DIR/multiple-declarations.rs:14:9
+   |
+LL |         fn f(x: i32);
+   |         ^^^^^^^^^^^^^
+
+error: aborting due to previous error; 1 warning emitted
+