rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.9k stars 1.56k forks source link

[RFC] Add `#[export_ordinal(n)]` attribute #3641

Open MeguminSama opened 4 months ago

MeguminSama commented 4 months ago

This RFC adds an attribute that allows specifying the ordinal of an exported function when creating cdylibs on windows.

We already have a #[link_ordinal(n)] attribute which allows importing functions from DLLs by its ordinal, but we don't have any way of doing the opposite - specifying the ordinal of a function when we're making our own DLLs.

Currently, you need to create a lib.def file and specify the ordinals, and then link it with cargo:rustc-cdylib-link-arg=/DEF.

The biggest downside of the current method is that once you specify a .def file, you will have to specify an ordinal for every function that you want to export from the DLL, or else it won't be present in the generated .lib file. This can become very overwhelming if you have a lot of exported functions.

This RFC would allow you to specify exported function ordinals like so:

// Export this function at Ordinal 1
#[no_mangle]
#[export_ordinal(1)]
pub extern "C" fn hello() {
    println!("Hello, World!");
}

Rendered

programmerjake commented 4 months ago

you should probably specify that the attribute is ignored everywhere other than Windows (or similar) targets.

MeguminSama commented 4 months ago

@programmerjake Thanks, I wasn't sure if ordinals were a thing on other targets. I'll update it accordingly.

joshtriplett commented 3 months ago

@rfcbot merge

rfcbot commented 3 months ago

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.

tmandry commented 3 months ago

Can we reuse #[link_ordinal(n)] for this purpose, the same way extern "C" can be used both for "importing" and "exporting" functions? I don't think there's any ambiguity based on context.

Should this attribute be unsafe once unsafe attributes are implemented? I'm unsure if we can guarantee there will be no collisions with anything else linked into the program (including native libraries).

MeguminSama commented 3 months ago

Can we reuse #[link_ordinal(n)] for this purpose, the same way extern "C" can be used both for "importing" and "exporting" functions?

I think this could be a good idea if there is no ambiguity. It could be confusing at a glance if a project has multiple #[link_ordinal(n)], one for importing, and one for exporting, but it's probably a non-issue.

Should this attribute be unsafe once unsafe attributes are implemented?

I'm not sure about how export_ordinal would be implemented, but if there's the chance of collisions, then I agree it should be marked as unsafe.

MeguminSama commented 3 months ago

In this case is the externally-declared function somehow being re-exported?

@tmandry I took into consideration that some libraries could possibly be statically linked, and the possibility that someone may want to re-export a function. If this isn't a valid concern I'm happy to remove it - I just tried to think of every possibility.

RalfJung commented 3 months ago

Are reexports even possible? Existing attributes like link_name use the same name for imports and exports so it would seem odd to use a different name here.

tmandry commented 3 months ago

Existing attributes like link_name use the same name for imports and exports so it would seem odd to use a different name here.

I don't think that's true; if I try to use it on an export I get a warning:

#[link_name = "actual_symbol_name"]
pub extern "C" fn name_in_rust() {}
warning: attribute should be applied to a foreign function or static
 --> src/lib.rs:1:1
  |
1 | #[link_name = "actual_symbol_name"]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2 | #[no_mangle]
3 | pub extern "C" fn name_in_rust() {}
  | ----------------------------------- not a foreign function or static
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: `#[warn(unused_attributes)]` on by default

Regardless, agreed that we should be consistent: Either link_name and link_ordinal work for both imports and exports, or we would have export_name and export_ordinal. I still lean toward the former unless it's possible in some executable format to have a re-exported function under a different name.

RalfJung commented 3 months ago

Seems like I misremembered... I thought there was a way to set the link name independent of the item name. Maybe there isn't.

ChrisDenton commented 3 months ago

Were you thinking of export_name? E.g.

#[export_name = "exported_symbol_name"]
pub extern "C" fn name_in_rust() {}
MeguminSama commented 3 months ago

If we already use export_name, I think it'd be best to stick with export_ordinal?

RalfJung commented 3 months ago

Yeah, that makes sense.

tmandry commented 3 months ago

@rfcbot reviewed

Agreed that we should stick with export_ordinal to be consistent with export_name.

MeguminSama commented 2 months ago

I noticed that the edition guide for unsafe attributes got merged.

It says that export_name must be marked as unsafe. I think that if this is the case, export_ordinal should definitely be unsafe.

Should I update the RFC to remove it from the unanswered questions, and clarify that in 2024 edition it should be marked as unsafe?

Edit: I don't entirely understand how editions would work - if export_ordinal got added to rust, would it only be available in 2024 edition? Or would it be available in prior editions, and just marked as unsafe in the 2024 edition?

RalfJung commented 2 months ago

If it has the same risks as export_name, then yeah it should be unsafe.

MeguminSama commented 2 months ago

It's entirely possible that a dependency could mark something as a duplicate ordinal, so I believe the same risks apply.

I'm not sure whether to word it as "in 2024 edition" or not though.

RalfJung commented 2 months ago

I don't see a reason to make this unsafe only in some editions. We do that for old attributes solely due to backwards compatibility. New things should be properly unsafe from the start.

MeguminSama commented 2 months ago

Thanks for the clarification. I'll work on the changes in a bit.

MeguminSama commented 1 month ago

I have updated the RFC to mark export_ordinal as unsafe.

I have also removed the section on re-exporting bindings, as after testing, I've confirmed that this is not valid code.