Lower #[no_mangle] to #[symbol_name]/#[link_name]

[bjorn3]

View all comments

This reduces the amount of special casing #[no_mangle] needs in the rest of the compiler.

t-lang: nomination comment, example

[rustbot]

miri is developed in its own repository. If possible, consider making this change to rust-lang/miri instead.

cc @rust-lang/miri

[rustbot]

r? @jackh726

rustbot has assigned @jackh726.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: codegen, compiler
• codegen, compiler expanded to 75 candidates
• Random selection from 19 candidates

[bjorn3]

This is a behavior change. We no longer keep the symbol that Rust code refers to unmangled when #[link_ordinal] is used. This matches the behavior on wasm for #[wasm_import_section], but prevents overriding the symbol using a definition with an #[export_name] matching the name we have imported it as in the raw-dylib block. And it is inconsistent with non-ordinal imports through raw-dylib where we could also have used mangled names on our side.

View changes since the review

[mejrs]

I think that makes sense, what's the process here? Nominate for t-lang and let them decide whether to accept this change?

[RalfJung]

Ideally, separate refactors from behavior changes. Doing both in the same PR makes things more complicated to manage.

[bjorn3]

It is non-trivial to keep the old behavior after this refactor. I could open a PR to change the behavior first.

[RalfJung]

I think that would be better, yeah.

And it is inconsistent with non-ordinal imports through raw-dylib where we could also have used mangled names on our side.

It is unclear to me whether you are calling the old or new behavior inconsistent here.

[mejrs]

Ideally, separate refactors from behavior changes.

I think the current PR is small enough to get away with this, no need to split it up here.

However, you can probably simplify much more by lowering into the same attribute during attribute parsing instead. (that would be a more intrusive change where refactors and behavior changes should be split)

[bjorn3]

It is unclear to me whether you are calling the old or new behavior inconsistent here.

The old behavior is consistent with non-ordinal imports on windows. The new behavior is consistent with imports on wasm, but not with non-ordinal imports on windows.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[RalfJung]

@rustbot reroll

[JonathanBrouwer]

@rustbot reroll
Not familiar enough with link_ordinal to feel comfortable reviewing this

[mejrs]

My understanding here is that no_mangle is just a convenience for export_name = "<item_name>", is that correct? What is the interaction with link_name?

Although while reviewing I did notice that this is allowed:

pub trait Trait {
    #[unsafe(export_name = "method")]
    fn method(&self) {}
}

but this emits a warning:

pub trait Trait {
    #[unsafe(no_mangle)]
    fn method(&self) {}
}

This looks like a bug (none or both should be allowed here)

View changes since this review

[mejrs]

In what way do no_mangle and link_name interact when it looks like they're not allowed to be on the same item?

unsafe extern "C" {
    #[unsafe(no_mangle)]
    safe fn name_in_rust();
}

emits the unused_attributes lint, so I expect it to do nothing there.

[bjorn3]

The problem is that no_mangle isn't yet a hard error inside extern blocks. Both export_name (and by extension no_mangle) and link_name write to the same symbol_name field in codegen_fn_attrs, so without this check, I think the following would result in wrong_name as name rather than correct_name as #[unsafe(no_mangle)] runs second and overrides the symbol_name set by #[unsafe(link_name)].

extern "C" {
    #[unsafe(link_name = "correct_name")]
    #[unsafe(no_mangle)]
    fn wrong_name();
}

[mejrs]

That makes sense. Can you add a comment about this?

[bjorn3]

Done

[mejrs]

r=me on the implementation, IMO the new behavior changes make sense also. But I think this sort of thing is ultimately t-lang's decision, can you nominate it for them to discuss?

View changes since this review

[bjorn3]

The exact change nominated for the lang team is #159374 (comment) Basically an item with #[unsafe(link_ordinal)] inside a raw-dylib block on Windows will now always import this ordinal from the target DLL rather than being overridable by a local definition of a symbol with a symbol name matching the item name of the item with #[unsafe(link_ordinal)] (the item name has no relation to the name of the imported symbol, as for ordinal exports there is no symbol name in the first place).

The only uses of #[link_ordinal] I found are: https://github.com/search?q=%2F%5B%5C%5B%5C%28%5Dlink_ordinal%5C%28%5Cd%2B%5C%29%2F+language%3Arust+-is%3Afork+-path%3A%2Ftests%5C%2F%2F+-path%3A%2Fsrc%5C%2Ftest%5C%2F%2F&type=code

• https://github.com/emmachase/raxis/blob/662f22f2ef1c689478a1bb715902120200bd34ce/src/runtime/mod.rs#L533-L537
• https://github.com/compio-rs/winio/blob/b2fa6b17c0244b7e44a7e0a2ce33315b80e9d398/winio-ui-windows-common/src/darkmode/hook.rs#L54-L68

Both of these refer to undocumented functions in uxtheme.dll. I don't think the item names these used are actually their official names as opposed to just a random name picked by the respective crate author that they might change at any time and would thus not be safe to override by defining a function with the same name even if it was possible.

[RalfJung]

Basically an item with #[unsafe(link_ordinal)] inside a raw-dylib block on Windows will now always import this ordinal from the target DLL rather than being overridable by a local definition of a symbol with a symbol name matching the item name of the item with #[unsafe(link_ordinal)] (the item name has no relation to the name of the imported symbol, as for ordinal exports there is no symbol name in the first place).

Can you give an example of a piece of code that changed its meaning, and describe the old and new meaning?

[bjorn3]

mod ffi {
    #[link(name = "foo", kind = "raw-dylib")]
    unsafe extern "C" {
        #[unsafe(link_ordinal(7))] // Perhaps internally called MyFancyFunction when foo.dll was compiled
        pub safe fn barf();
    }
}

#[unsafe(no_mangle)]
extern "C" fn barf() {
    println!("Override called");
}

fn main() {
    ffi::barf();
}

would previously cause the override to be called, but will now call ordinal export 7 of foo.dll like would have been the case without the override.

[RalfJung]

Wow yeah the old behavior seems really strange there. I expected barf to be just a local name for the import that's otherwise irrelevant, given the attribute.

And it is inconsistent with non-ordinal imports through raw-dylib where we could also have used mangled names on our side.

This sounds like we are still doing the strange behavior in some other related case?

[bjorn3]

Currently with raw-dylib the following:

mod ffi {
    #[link(name = "foo", kind = "raw-dylib")]
    unsafe extern "C" {
        pub safe fn bar();
        pub safe fn barf();
    }
}

#[unsafe(no_mangle)]
extern "C" fn bar() {
    println!("Override 1 called");
}

extern "C" fn barf() {
    println!("Override 2 called");
}

fn main() {
    ffi::bar();
    ffi::barf();
}

would produce

bar:
    # print "Override 1 called"

my_crate::barf:
    # print "Override 2 called"

main:
    call bar
    call barf

with an import library

bar -> foo.dll::bar
barf -> foo.dll::barf

so only override 1 would apply. If however raw-dylib mangles symbol names it would result in:

bar:
    # print "Override 1 called"

my_crate::barf:
    # print "Override 2 called"

main:
    call my_crate::ffi::bar
    call my_crate::ffi::barf

with an import library

my_crate::ffi::bar -> foo.dll::bar
my_crate::ffi::barf -> foo.dll::barf

so neither override would be called.

[RalfJung]

Okay, thanks.

I agree that the proposed behavior change is a step in the right direction -- in that example, we are importing by ordinal, not by name, so it's odd that we'd be able to override this by-name.

I also think our handling of overrides for non-ordinal imports, and #155969, are pretty odd, but that's orthogonal and not up for change in this PR. Name-based imports can be overridden by name disregarding the name of the library the import is supposed to come from -- that's surprising but if people truly think this is desirable then it seems like an acceptable end state to have name-based imports overridable even when ordinal-based imports cannot be overridden.

Cc @ChrisDenton

[ChrisDenton]

Both of these refer to undocumented functions in uxtheme.dll. I don't think the item names these used are actually their official names as opposed to just a random name picked by the respective crate author that they might change at any time and would thus not be safe to override by defining a function with the same name even if it was possible.

I assume they cribbed the names from debug symbols. But yes, they are potentially subject to change unless/until they're documented or in public headers.

[ChrisDenton]

I do not find the current behaviour as odd as ralf does (it just looks like normal C semantics to me personally) but I can see how namespacing imports can be beneficial. However, I fear that is drifting off topic from the specific change discussed here.

I do think it unfortunate if we can no longer override certain kinds of imports. I can probably live with it though. Overriding imports is already a bit of a problem, the cli -l rename feature only works on the current crate being compiled (not dependencies) so users can't e.g. override std imports regardless. At least not without resorting to methods outside of rust.

[RalfJung]

It looks like normal C behavior to me too, that's exactly why I think it is terrible. ;)

But the question at hand is whether in this example

    #[link(name = "foo", kind = "raw-dylib")]
    unsafe extern "C" {
        #[unsafe(link_ordinal(7))] // Perhaps internally called MyFancyFunction when foo.dll was compiled
        pub safe fn barf();
    }

barf should be overridable based on the completely arbitrary name barf that was picked here.

I view this as similar to

    #[link(name = "foo", kind = "raw-dylib")]
    unsafe extern "C" {
        #[unsafe(link_name = "function_number_7")]
        pub safe fn barf();
    }

It would be very strange if the name barf could be used to override this import.

[ChrisDenton]

🤷 link_name overrides the C symbol name whereas link_ordinal doesn't.

[RalfJung]

I don't know what you mean by that. I think you are viewing this knowing how it compiles, i.e. that there is an import library being auto-generated that has some C symbols in it, and we pick the names for those symbols in a particular way. But there is absolutely nothing in the source that indicates that. Looking just at the source code the user has written, there is no "import library" or "C symbol name" that would be overriden in one case or the other. In both cases there is some symbol in another library foo, and I am using an attribute (link_name/link_ordinal) to say which symbol I want from that library. I also have to come up with a name for my function (barf) but that's just a way to reference the import I just declared -- I see no way how one could possibly expect that this name has any influence whatsoever on what we import, and I don't see why it would be reasonable to specify behavior as such.

We don't specify language operations by the asm they generate, we specify them by their high-level desired behavior in a way that's consistent across the language and then we generate the asm to make that work. The same should go for linker shenanigans, IMO.

[RalfJung]

The old comments are very hard to make sense of now that there's no NO_MANGLE in the code below any more. (Not just this one, also some of the others below.)

View changes since the review

[RalfJung]

I don't understand how this comment explains why we should keep the symbol name unset here.

View changes since the review

[ChrisDenton]

I don't know what you mean by that.

I mean the symbols written an extern "C" are intentionally C symbol names, no? With all that implies. E.g. you can potentially override memcpy in a such a block precisely because you're playing by linker rules at that point.

Just to spell this out explicitly, there are three names involved here:

• the rust identifier used in rust code
• the symbol name seen by the linker
• the function name (or ordinal) exported from the DLL

By default each of these is automatically derived from the previous one on the list. If you change the identifier you use in rust then, by default, this will change the inferred C name and the inferred exported name.

#[link_name] allow overriding the second (the linker symbol name) without touching the first. Again, by default the exported name will be inferred from that.

#[link_ordinal] modifies only the last one without touching the others. It's one of only a few ways we currently have of explicitly changing that (the others are controlled with import_name_type).

I'm not really sure how to think about these without reference to linkers or how DLLs are loaded because none of this would make any sense without that context.

[RalfJung]

E.g. you can potentially override memcpy in a such a block precisely because you're playing by linker rules at that point.

....? I can override memcpy by defining (aka exporting) a function with no_mangle, but how can I override anything by importing a function?

[bjorn3]

The obvious definition of raw-dylib is that it will import functions from the specified dylib. The fact that this is done using import libraries on Windows and involves linker visible symbol names is an implementation detail.

Also note that the wasm equivalent of raw-dylib has from the very start been mangling the linker visible symbol names precisely to allow importing the same name from different modules without conflicts. And in fact wasi would completely break if we stopped mangling as it reuses the same name across a dozen of modules. And on wasm no import libraries are involved either.

[ChrisDenton]

....? I can override memcpy by defining (aka exporting) a function with no_mangle, but how can I override anything by importing a function?

I merely meant that exporting has a similar concept of a rust identifier and a C symbol name (or linker symbol, if you prefer).

The obvious definition of raw-dylib is that it will import functions from the specified dylib. The fact that this is done using import libraries on Windows and involves linker visible symbol names is an implementation detail.

If it's an implementation detail only then it's a very important one. We could indeed skip generating the C name at all (the middle step) but changing that now seems like a very breaking change (unless it's opt-in of course). For system imports you almost certainly do want the import-merging behaviour (i.e. pick any one).

[bjorn3]

We could indeed skip generating the C name at all (the middle step) but changing that now seems like a very breaking change (unless it's opt-in of course). For system imports you almost certainly do want the import-merging behaviour (i.e. pick any one).

Maybe it could be considered breaking for link_name, but I don't think it is breaking for link_ordinal given that the crate defining the raw-dylib has picked an arbitrary name and is free to rename it anyway.

[ChrisDenton]

We could indeed skip generating the C name at all (the middle step) but changing that now seems like a very breaking change (unless it's opt-in of course). For system imports you almost certainly do want the import-merging behaviour (i.e. pick any one).

Maybe it could be considered breaking for link_name, but I don't think it is breaking for link_ordinal given that the crate defining the raw-dylib has picked an arbitrary name and is free to rename it anyway.

Yes, sorry I was getting side tracked again. I agree it's less of an issue but I would quibble that names are necessarily "arbitrary". There can be a shared understanding of what the function should be called.

[RalfJung]

I merely meant that exporting has a similar concept of a rust identifier and a C symbol name (or linker symbol, if you prefer).

Exporting something under a name makes it globally override that name for anything that imports this name. That's not very surprising.

But nothing in these examples imports barf. So I see nothing in the expected behavior of linking, even when accepting a single flat namespace, that would explain why a barf export has any bearing on what happens.

Anyway you said above you could live with this change so it's probably pointless to keep discussing whether the old behavior made any sense. ;)