# Dealing with multiple imports of the same external symbol

Rust provides syntax for importing the same external symbol multiple times>

mod A {
    extern "C" {
        fn my_symbol(x: i32) -> i32;
    }
}

mod B {
    extern "C" {
        fn my_symbol(x: std::num::NonZeroI32) -> std::num::NonZeroI32;
    }
}

However, this is causing trouble with our backends:
- LLVM can only represent a single import of a given symbol. When the second import gets codegen'd, it simply overwrite the first.
- Cranelift can only represent a single import of a given symbol. The backend errors when encountering a second, incompatible import.

The choice of the LLVM backend to have one import overwrite the other leads to surprising UB:

fn call_my_symbol(x: i32) -> i32 {
    unsafe { A::my_symbol(x) }
}

Asuming `my_symbol` is the identity function, we now have a problem.
If `B::my_symbol` gets codegen'd later (e.g. because it is called by some other function), the import of `my_symbol` with have `nonnull` attributes on its argument and return value, making it UB to do `call_my_symbol(0)`.
In other words, the declaration of `B::my_symbol`, which might never even be executed in this program, has introduced UB into our program!

This is the fundamental problem tracked at https://github.com/rust-lang/rust/issues/46188.

(There's another possible cause of UB here which is basically the same issue, but inside LLVM rather than in our codegen backend: when LLVM merges two modules for LTO, if they both declare the same symbol but with different sigantures, one of the declarations has to "win". This can leak one module's attributes into calls that originate from the other module.)

We have to decide what we want to happen in this case.
Here are the options I can imagine:
- This is UB.
- This should be compiled successfully without UB.
- This should be a compilation error.

Let's go over them.

## This is UB

I would say this would be a pretty sad outcome.
This kind of UB is quite non-local and hard to protect against: in principle, any `extern "C"` declaration in any crate can cause UB anywhere else in the crate graph!
The proof obligation attached with a declaration is roughly: the signature must be valid for *every* call to the declared function *everywhere* in this crate graph, even if the call does not go through this import.
(I don't think there is any UB from mismatching declarations if the function is never actually called.)
A remedying factor is that `extern` blocks are becoming `unsafe` in the next edition; we can interpret that as reflecting the proof obligation. I don't know if there are any plans to warn against non-`unsafe` blocks in previous editions, but if we make this UB in all editions, then arguably we should make it `unsafe` in all editions.

That said, this would not strictly be the first case of "dead code can cause UB":
- Declaring an `extern static` also has proof obligations attached with it even when it is not used.
However, they are less non-local the the ones we are discussing here: the obligation is that the name of the static resolves to an address that has the right alignment and that the address is derefrenceable for sufficiently many bytes (as given by the size of the `extern static`).
- `#[no_mangle]` definitions can be seen as having a similar problem (where they "intercept" function calls that logically should go to a different function), but they are much less common. As [unsafe attributes](https://github.com/rust-lang/rust/issues/123757) progresses we will hopefully see warnings telling people to use `#[unsafe(no_mangle)]` in all editions.

If we say this is UB, we need to extend the [UB docs](https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html), and we need to decide which exact cases we are making UB.
For instance, the example above could be made non-UB by simply not emitting `nonnull` attributes on declarations, and only emitting them on call sites.

## This should be compiled successfully without UB

On cranelift this is apparently not possible, but I spent a bunch of time trying to get this to work on LLVM.
(On GCC it seems this [might be possible](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Different.20imports.20of.20the.20same.20function.20vs.20LLVM/near/454508833) but it has not been tried.)
The most promising approach was to declare all functions as `void @fun()`, which is also what clang uses for an "unknown signature".
This had some minor and one major problem.

The minor problems are all about cases where on a function call, LLVM looks at the attributes of the callee rather than the attributes of the call site.
Some of them got fixed by @nikic, but one case (which caused a noticeable code size regression related to unwind information) is so deep in the backend that a fix is non-obvious.

The main problem, however, is that some calling conventions have mandatory name mangling that includes information about the arguments.
Specifically this is a Windows thing, [affecting](https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170#FormatC) stdcall, fastcall, and vectorcall.
It may be possible to somehow work around this, but at this point I gave up in the relevant [PR](https://github.com/rust-lang/rust/issues/46188) since it seemed increasingly like we are trying to force a round peg into a square hole -- without proper LLVM support for importing the same symbol under different local names, we'd likely keep running into ncreasingly obscure issues.
(And linking is already littered with obscure issues...)

## This should be a compilation error

That leaves us with one more option: if we can't correctly compile such code, can we at least detect the error and thus protect the user from UB?
All the relevant information is available to rustc, it "just" needs to do a full scan of the entire crate graph and ensure all imports are mutually compatible.
Surely, stopping compilation in that case is better than having UB?

Of course the devil is in the details.
rustc already has a lint detecting incompatible declarations *in the same crate*; the example above actually causes a warning:

warning: `my_symbol` redeclared with a different signature
 --> src/lib.rs:9:9
  |
3 |         fn my_symbol(x: i32) -> i32;
  |         --------------------------- `my_symbol` previously declared here
...
9 |         fn my_symbol(x: std::num::NonZeroI32) -> std::num::NonZeroI32;
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
  |
  = note: expected `unsafe extern "C" fn(i32) -> i32`
             found `unsafe extern "C" fn(NonZero<i32>) -> NonZero<i32>`
  = note: `#[warn(clashing_extern_declarations)]` on by default

This is based on a notion of two types being "structurally the same", implemented [here](https://github.com/rust-lang/rust/blob/42a901acd9c9d3a0c9ca7adf2470b789f9c81a5b/compiler/rustc_lint/src/foreign_modules.rs#L218).
This compares types recursively, skipping transparent newtypes and considering structs equal if they have the same layout and the same fields in the same order.

We could start by turning this lint into a hard error.
The more tricky question is what to do with cross-crate situations.
If one import is in your crate and one import in another crate, you could at least still fix the error by adjusting the signature to match that other crate.
(Except if the import uses a private non-`repr(C)` type, which makes it impossible to declare a compatible signature. But that would be a buggy import.)
If the two conflicting imports are in different crates, there is simply no way to combine those two crates without risking UB, but the local crate author can do nothing about this.
We should still at least warn about this I think (so the crate author can file issues). Making it a hard error is likely going to be frustrating, but the UB could be even more frustrating...

So the design questions are:
- Which signatures should we consider "conflicting"? Note that ABI compatibility is not sufficient with the current implementation, e.g. `i32` and `NonZeroI32` are ABI-compatible but generate different LLVM signatures. However, we could adjust codegen to guarantee producing the same signature for ABI-compatible declarations (i.e., omit `nonnull` and similar attributes from declarations). The current lint accepts many mismatches that are not ABI-compatible (e.g. when `repr(C)` structs differ in having an `i32` field vs a `NonZeroI32` field) but rejects some that are ABI-compatible (e.g. when pointers have completely different pointee types).
- What should we do about semver concerns? The current lint recurses into private fields of types declared in other crates.
- When should the lint be warn-by-default / deny-by-default / a hard error? In particular, what should we do when there is a conflict of two imports that are not both in the local crate?