Repository: davidlattimore/wild (rust)

Issue:
Error: Failed to copy section  when linking libclang
On LLVM commit `f83726e6` when testing [wild-proxy](https://github.com/mati865/wild-proxy) by running `PATH=~/Projects/wild-proxy:$PATH CC=wild-gcc CXX=wild-g++ cmake -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_BUILD_TYPE=Release -G Ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON ../llvm` and then `ninja` I expected it to pass, but it failed when linking.

Then also reproduced it using more typical setup:
```
❯ PATH=~/Projects/wild:$PATH cmake -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_BUILD_TYPE=Release -G Ninja ../llvm

❯ PATH=~/Projects/wild:$PATH ninja libclang
...

Error: Failed to copy section ``.text._ZN12_GLOBAL__N_119ARMMachObjectWriter16recordRelocationEPN4llvm16MachObjectWriterERNS1_11MCAssemblerEPKNS1_10MCFragmentERKNS1_7MCFixupENS1_7MCValueERm`` from file lib/libLLVMARMDesc.a @ ARMMachObjectWriter.cpp.o (75777 (296/1))

Caused by:
    Direct relocation (R_X86_64_PC32) to dynamic symbol from non-writable section, but copy relocations are disabled. symbol `llvm::hexdigit(unsigned int, bool)::LUT` (8292 local=2107) in file #2304 (9/0) (lib/libclangAST.a @ ASTContext.cpp.o) (ADDRESS)
```

Pull request description:
Merge symbol visibility when multiple definitions
Fixes #661

Patch:
diff --git a/libwild/src/symbol_db.rs b/libwild/src/symbol_db.rs
index b766fe6..90caf63 100644
--- a/libwild/src/symbol_db.rs
+++ b/libwild/src/symbol_db.rs
@@ -27,6 +27,7 @@ use crate::version_script::VersionScript;
 use ahash::HashMap;
 use ahash::RandomState;
 use anyhow::Context;
+use anyhow::Error;
 use anyhow::bail;
 use crossbeam_queue::SegQueue;
 use itertools::Itertools;
@@ -382,6 +383,29 @@ impl<'data> SymbolDb<'data> {
         symbol_id
     }
 
+    /// Reads the symbol visibility from the original object.
+    fn input_symbol_visibility(&self, symbol_id: SymbolId) -> Visibility {
+        let file_id = self.file_id_for_symbol(symbol_id);
+        match &self.groups[file_id.group()] {
+            Group::Prelude(_) => Visibility::Default,
+            Group::Objects(parsed_input_objects) => {
+                let obj = &parsed_input_objects[file_id.file()];
+                let local_index = symbol_id.to_input(obj.symbol_id_range);
+
+                let Ok(obj_symbol) = obj.object.symbol(local_index) else {
+                    return Visibility::Default;
+                };
+
+                match obj_symbol.st_visibility() {
+                    object::elf::STV_PROTECTED => Visibility::Protected,
+                    object::elf::STV_HIDDEN => Visibility::Hidden,
+                    _ => Visibility::Default,
+                }
+            }
+            Group::Epilogue(_) => Visibility::Default,
+        }
+    }
+
     /// Returns a struct that can be used to print debug information about the specified symbol.
     pub(crate) fn symbol_debug(&self, symbol_id: SymbolId) -> SymbolDebug<'_, 'data> {
         SymbolDebug {
@@ -625,44 +649,27 @@ pub(crate) fn resolve_alternative_symbol_definitions<'data>(
     let mut buckets = take(&mut symbol_db.buckets);
     let error_queue = SegQueue::new();
 
-    let replacements: Vec<Vec<(SymbolId, SymbolId)>> = buckets
+    let replacements: Vec<Vec<SymbolReplacement>> = buckets
         .par_iter_mut()
         .map(|bucket| {
             let mut local_replacements = Vec::new();
 
-            for (first, alternatives) in replace(
+            process_alternatives(
                 &mut bucket.alternative_definitions,
-                HashMap::with_hasher(RandomState::new()),
-            ) {
-                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);
-                    }
-                }
-            }
+                &mut local_replacements,
+                &error_queue,
+                symbol_db,
+                resolved,
+            );
 
-            for (first, alternatives) in replace(
+            process_alternatives(
                 &mut bucket.alternative_versioned_definitions,
-                HashMap::with_hasher(RandomState::new()),
-            ) {
-                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);
-                    }
-                }
-            }
+                &mut local_replacements,
+                &error_queue,
+                symbol_db,
+                resolved,
+            );
+
             local_replacements
         })
         .collect();
@@ -681,8 +688,14 @@ pub(crate) fn resolve_alternative_symbol_definitions<'data>(
         )));
     }
 
-    for (a, b) in replacements.iter().flat_map(|r| r.iter()) {
-        symbol_db.replace_definition(*a, *b);
+    for r in replacements.iter().flat_map(|r| r.iter()) {
+        // TODO: Currently we only make the symbol non-interposable, but we should also actually
+        // change its visibility too. We need somewhere to store this information. We also need
+        // linker-diff to report when we get exported dynamic symbols wrong.
+        if r.visibility != Visibility::Default {
+            symbol_db.symbol_value_flags[r.replace.as_usize()] |= ValueFlags::NON_INTERPOSABLE;
+        }
+        symbol_db.replace_definition(r.replace, r.selected);
     }
 
     symbol_db.buckets = buckets;
@@ -690,6 +703,68 @@ pub(crate) fn resolve_alternative_symbol_definitions<'data>(
     Ok(())
 }
 
+#[derive(Clone, Copy)]
+struct SymbolReplacement {
+    /// The symbol to be replaced.
+    replace: SymbolId,
+
+    /// The symbol that was selected as the definition to use.
+    selected: SymbolId,
+
+    visibility: Visibility,
+}
+
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+enum Visibility {
+    Default,
+    Protected,
+    Hidden,
+}
+
+fn process_alternatives(
+    alternative_definitions: &mut HashMap<SymbolId, Vec<SymbolId>>,
+    local_replacements: &mut Vec<SymbolReplacement>,
+    error_queue: &SegQueue<Error>,
+    symbol_db: &SymbolDb,
+    resolved: &[ResolvedGroup],
+) {
+    for (first, alternatives) in replace(
+        alternative_definitions,
+        HashMap::with_hasher(RandomState::new()),
+    ) {
+        // Compute the most restrictive visibility of any of the alternative definitions. This is
+        // the visibility we'll use for our selected symbol. This seems like odd behaviour, but it
+        // matches what GNU ld appears to do and some programs will fail to link if we don't do
+        // this.
+        let visibility = alternatives
+            .iter()
+            .fold(symbol_db.input_symbol_visibility(first), |vis, id| {
+                vis.max(symbol_db.input_symbol_visibility(*id))
+            });
+
+        match select_symbol(symbol_db, first, &alternatives, resolved) {
+            Ok(selected) => {
+                local_replacements.push(SymbolReplacement {
+                    replace: first,
+                    selected,
+                    visibility,
+                });
+
+                for alt in alternatives {
+                    local_replacements.push(SymbolReplacement {
+                        replace: alt,
+                        selected,
+                        visibility,
+                    });
+                }
+            }
+            Err(err) => {
+                error_queue.push(err);
+            }
+        }
+    }
+}
+
 /// Selects which version of the symbol to use.
 #[inline(always)]
 fn select_symbol(


Test patch:
diff --git a/wild/tests/integration_tests.rs b/wild/tests/integration_tests.rs
index fb88617..bf5c933 100644
--- a/wild/tests/integration_tests.rs
+++ b/wild/tests/integration_tests.rs
@@ -2235,6 +2235,7 @@ fn integration_test(
         "rust-tls.rs",
         "input_does_not_exist.c",
         "ifunc2.c",
+        "visibility-merging.c",
         "tls-local-exec.c",
         "undefined_symbols.c",
         "whole_archive.c",
diff --git a/wild/tests/sources/visibility-merging-1.c b/wild/tests/sources/visibility-merging-1.c
new file mode 100644
index 0000000..3042060
--- /dev/null
+++ b/wild/tests/sources/visibility-merging-1.c
@@ -0,0 +1,8 @@
+// Even though the definition of `data1` from the main file is given priority over this definition,
+// the fact that this definition is hidden causes the symbol to be hidden.
+int data1  __attribute__ ((weak, visibility(("hidden")))) = 0x100;
+
+// Similarly, this symbol makes the symbol of the same name from file protected.
+int data3  __attribute__ ((weak, visibility(("protected")))) = 0x55;
+
+int data4 __attribute__ ((weak, visibility(("hidden")))) = 0x99;
diff --git a/wild/tests/sources/visibility-merging.c b/wild/tests/sources/visibility-merging.c
new file mode 100644
index 0000000..498808d
--- /dev/null
+++ b/wild/tests/sources/visibility-merging.c
@@ -0,0 +1,29 @@
+//#Static:false
+//#LinkArgs:-shared -z now
+//#Object:visibility-merging-1.c
+//#RunEnabled:false
+//#DiffIgnore:section.got
+
+// This symbol is included, but isn't exported as a dynamic symbol because of a second definition in
+// our other file that's marked as hidden.
+int data1 __attribute__ ((weak)) = 0x42;
+
+// This symbol is exported.
+int data2 __attribute__ ((weak)) = 0x88888888;
+
+// This symbol is exported, but is protected, since the definition in the second file is.
+int data3 __attribute__ ((weak)) = 0x55;
+
+// Protected here and hidden in our second object. Hidden should take priority.
+int data4 __attribute__ ((weak, visibility(("protected")))) = 0x99;
+
+// Make sure that direct references to `data1` work on account of it being hidden.
+int get_data1(void) {
+    return data1;
+}
+
+// Note, we don't check direct references to `data2` and `data3`. In the case of `data2`, direct
+// references wouldn't be permitted, since the symbol is interposable. In the case of `data3`, the
+// symbol will be protected, so direct references should be permitted, however GNU ld < 2.40 would
+// error in the case of direct references to protected symbols, so in order to allow our tests to
+// pass with such versions, we don't.