Repository: davidlattimore/wild (rust)

Issue:
Report error when there are duplicate definitions of a strong symbol
Error reporting is something that we've neglected a bit. At the moment, we don't report an error when there are multiple definitions of a symbol.

There are a few nuances to multiple-definition rules:

* It's only a problem if there's more than one strong definition. Multiple weak definitions, optionally with one strong definition is fine.
* It's only a problem if the object containing the symbol is loaded. e.g. it's fine to have strong definitions in archive entries that duplicate other symbols and so long as those archive entries aren't loaded, it's OK.

Selecting from multiple symbol definitions is handled in `resolve_alternative_symbol_definitions`, so that would be the place to start looking.

If there are multiple symbols that have duplicate definitions, it might be good to report all of them, although probably not essential.

We should have a test for this. See `undefined_symbols.c` for a similar kind of test to use as a reference.

Pull request description:
better error reporting for duplicate strong symbols
close #521 

The current implementation lacks proper error reporting logic for cases where multiple duplicate strong symbols exist in the link target.

Patch:
diff --git a/libwild/src/symbol_db.rs b/libwild/src/symbol_db.rs
index 4b349de..7344650 100644
--- a/libwild/src/symbol_db.rs
+++ b/libwild/src/symbol_db.rs
@@ -28,6 +28,7 @@ use ahash::HashMap;
 use ahash::RandomState;
 use anyhow::Context;
 use anyhow::bail;
+use crossbeam_queue::SegQueue;
 use itertools::Itertools;
 use object::LittleEndian;
 use object::read::elf::Sym as _;
@@ -37,6 +38,7 @@ use rayon::iter::IntoParallelRefMutIterator;
 use rayon::iter::ParallelIterator;
 use std::collections::hash_map;
 use std::fmt::Display;
+use std::fmt::Write;
 use std::mem::replace;
 use std::mem::take;
 
@@ -596,25 +598,27 @@ pub(crate) fn resolve_alternative_symbol_definitions<'data>(
     resolved: &[ResolvedGroup],
 ) -> Result {
     let mut buckets = take(&mut symbol_db.buckets);
+    let error_queue = SegQueue::new();
 
     let replacements: Vec<Vec<(SymbolId, SymbolId)>> = buckets
         .par_iter_mut()
         .map(|bucket| {
-            let mut replacements = Vec::new();
-
-            let mut add_replacement = |a, b| {
-                replacements.push((a, b));
-            };
+            let mut local_replacements = Vec::new();
 
             for (first, alternatives) in replace(
                 &mut bucket.alternative_definitions,
                 HashMap::with_hasher(RandomState::new()),
             ) {
-                let selected = select_symbol(symbol_db, first, &alternatives, resolved);
-                add_replacement(first, selected);
-
-                for alt in alternatives {
-                    add_replacement(alt, selected);
+                match select_symbol(symbol_db, first, &alternatives, resolved) {
+                    Ok(selected) => {
+                        local_replacements.push((first, selected));
+                        for alt in alternatives {
+                            local_replacements.push((alt, selected));
+                        }
+                    }
+                    Err(err) => {
+                        error_queue.push(err);
+                    }
                 }
             }
 
@@ -622,18 +626,36 @@ pub(crate) fn resolve_alternative_symbol_definitions<'data>(
                 &mut bucket.alternative_versioned_definitions,
                 HashMap::with_hasher(RandomState::new()),
             ) {
-                let selected = select_symbol(symbol_db, first, &alternatives, resolved);
-                add_replacement(first, selected);
-
-                for alt in alternatives {
-                    add_replacement(alt, selected);
+                match select_symbol(symbol_db, first, &alternatives, resolved) {
+                    Ok(selected) => {
+                        local_replacements.push((first, selected));
+                        for alt in alternatives {
+                            local_replacements.push((alt, selected));
+                        }
+                    }
+                    Err(err) => {
+                        error_queue.push(err);
+                    }
                 }
             }
-
-            replacements
+            local_replacements
         })
         .collect();
 
+    let mut duplicate_errors: Vec<anyhow::Error> = error_queue.into_iter().collect();
+    duplicate_errors.sort_by_key(|e| e.to_string());
+
+    if !duplicate_errors.is_empty() {
+        let error_details = duplicate_errors
+            .iter()
+            .map(|e| e.to_string())
+            .collect::<Vec<_>>()
+            .join("\n");
+        return Err(anyhow::Error::msg(format!(
+            "Duplicate symbols detected: {error_details}"
+        )));
+    }
+
     for (a, b) in replacements.iter().flat_map(|r| r.iter()) {
         symbol_db.replace_definition(*a, *b);
     }
@@ -650,23 +672,23 @@ fn select_symbol(
     symbol_id: SymbolId,
     alternatives: &[SymbolId],
     resolved: &[ResolvedGroup],
-) -> SymbolId {
-    let first_strength = symbol_db.symbol_strength(symbol_id, resolved);
-    if first_strength == SymbolStrength::Strong {
-        return symbol_id;
-    }
+) -> Result<SymbolId, anyhow::Error> {
     let mut max_common = None;
-    for &alt in alternatives.iter().rev() {
-        // Dynamic symbols, even strong ones, don't override non-dynamic weak symbols.
+    let mut strong_symbols = Vec::new();
+
+    let all_symbols = std::iter::once(symbol_id).chain(alternatives.iter().copied());
+
+    for alt in all_symbols {
         if symbol_db
             .symbol_value_flags(alt)
             .contains(ValueFlags::DYNAMIC)
         {
             continue;
         }
+
         let strength = symbol_db.symbol_strength(alt, resolved);
         match strength {
-            SymbolStrength::Strong => return alt,
+            SymbolStrength::Strong => strong_symbols.push(alt),
             SymbolStrength::Common(size) => {
                 if let Some((previous_size, _)) = max_common {
                     if size <= previous_size {
@@ -678,19 +700,46 @@ fn select_symbol(
             _ => {}
         }
     }
+
+    if strong_symbols.len() > 1 {
+        let already_defined_in = symbol_db.file_id_for_symbol(*strong_symbols.first().unwrap());
+
+        return Err(anyhow::Error::msg(format!(
+            "{}{}, previously defined in {}",
+            symbol_db.symbol_name_for_display(symbol_id),
+            strong_symbols
+                .iter()
+                .skip(1)
+                .fold(String::new(), |mut output, &s| {
+                    let fid = symbol_db.file_id_for_symbol(s);
+                    write!(output, ", defined in {}", symbol_db.file(fid)).unwrap();
+                    output
+                }),
+            symbol_db.file(already_defined_in)
+        )));
+    }
+
+    if let Some(&strong_symbol) = strong_symbols.first() {
+        return Ok(strong_symbol);
+    }
+
     if let Some((_, alt)) = max_common {
-        return alt;
+        return Ok(alt);
     }
+
+    let first_strength = symbol_db.symbol_strength(symbol_id, resolved);
     if first_strength != SymbolStrength::Undefined {
-        return symbol_id;
+        return Ok(symbol_id);
     }
+
     for &alt in alternatives.iter().rev() {
         let strength = symbol_db.symbol_strength(alt, resolved);
         if strength != SymbolStrength::Undefined {
-            return alt;
+            return Ok(alt);
         }
     }
-    symbol_id
+
+    Ok(symbol_id)
 }
 
 #[derive(PartialEq, Eq, Clone, Copy)]


Test patch:
diff --git a/wild/tests/integration_tests.rs b/wild/tests/integration_tests.rs
index b89ba9c..3490aa9 100644
--- a/wild/tests/integration_tests.rs
+++ b/wild/tests/integration_tests.rs
@@ -2158,7 +2158,8 @@ fn integration_test(
         "tls-local-exec.c",
         "undefined_symbols.c",
         "whole_archive.c",
-        "shared.c"
+        "shared.c",
+        "duplicate_strong_symbols.c"
     )]
     program_name: &'static str,
     #[allow(unused_variables)] setup_symlink: (),
diff --git a/wild/tests/sources/duplicate_strong_symbols.c b/wild/tests/sources/duplicate_strong_symbols.c
new file mode 100644
index 0000000..4cec520
--- /dev/null
+++ b/wild/tests/sources/duplicate_strong_symbols.c
@@ -0,0 +1,13 @@
+//#Config:dup
+//#SkipLinker:ld
+//#Object:duplicate_strong_symbols2.c
+//#Object:exit.c
+//#ExpectError:Duplicate symbols
+
+int test_func(void) {
+    return 0;
+}
+
+void _start(void) {
+    test_func();
+}
diff --git a/wild/tests/sources/duplicate_strong_symbols2.c b/wild/tests/sources/duplicate_strong_symbols2.c
new file mode 100644
index 0000000..4708d41
--- /dev/null
+++ b/wild/tests/sources/duplicate_strong_symbols2.c
@@ -0,0 +1,3 @@
+int test_func(void) {
+    return 1;
+}