Associated functions that contain extern indicator or have #[rustc_std_internal_symbol] are reachable

[ghost]

Previously these fails to link with undefined reference to `foo':

Example 1

struct AssocFn;

impl AssocFn {
    #[no_mangle]
    fn foo() {}
}

fn main() {
    extern "Rust" {
        fn foo();
    }
    unsafe { foo() }
}

(Playground)

Example 2

#![crate_name = "lib"]
#![crate_type = "lib"]

struct AssocFn;

impl AssocFn {
    #[no_mangle]
    fn foo() {}
}

extern crate lib;

fn main() {
    extern "Rust" {
        fn foo();
    }
    unsafe { foo() }
}

But I believe they should link successfully, because this works:

#[no_mangle]
fn foo() {}

fn main() {
    extern "Rust" {
        fn foo();
    }
    unsafe { foo() }
}

(Playground)

This PR fixes the problem, by adding associated functions that have "custom linkage" to reachable_set, just like normal functions.

I haven't tested whether #76211 and Miri are fixed by this PR yet, but I'm submitting this anyway since this fixes the examples above.

I added a run-pass test that combines my two examples above, but I'm not sure if that's the right way to test this. Maybe I should add / modify an existing codegen test (src/test/codegen/export-no-mangle.rs?) instead?

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

I don't think #[no_mangle] is supposed to work with associated functions at all.
Are there practical use cases / uses in the wild?

All the checks and logic mentioning sym::no_mangle that I can find in the compiler only apply to free items.

[petrochenkov]

I'm still not sure whether the label combination S-waiting-on-team + T-lang is enough for the lang team to triage a PR, so I'm going to nominate as well.

[ghost]

I don't think #[no_mangle] is supposed to work with associated functions at all.
Are there practical use cases / uses in the wild?

I don't know. ^{I found it supported by reading this code. (EDIT: Never mind. That code does not seem very convincing.)} I'm experimenting with it fully because I need to know whether Miri should support this: I found that it is working on public associated functions in library crates, and the unused_attributes lint is explicitly not fired for #[no_mangle] on associated functions:

rust/compiler/rustc_passes/src/check_attr.rs

Line 1149 in 6a540bd

Target::Static | Target::Fn | Target::Method(..) => {}

[Mark-Simulacrum]

This was discussed during the last several language team meetings. There doesn't seem to be a lot of strong opinions in either direction on this, but the overall consensus seems to be that we should make #[no_mangle] work (i.e., produce a non-mangled symbol in the final binary, along with export/reachability) on functions inside impl Foo. This seems to be consistent with a general perspective that the context where a function is placed should not, generally, restrict what attributes can be placed on it so long as they are supportable in that context.

There was some debate over whether this is practically useful, but generally it seems like the answer is at least maybe yes - the description of #76211 cites a concrete case where this was unexpectedly not working, for example. Ultimately, it seems relatively harmless to support this, and fairly likely to do what users expect.

---

We did not discuss whether it makes sense to feature gate this to start -- I suspect that would in practice break some code in the wild, and this seems largely like a bugfix, so perhaps we will opt to skip that. In that case we'll want an FCP so I'll bring this up on Zulip so that someone on T-lang can hopefully kick that off.

[nikomatsakis]

@rfcbot fcp merge

Per @Mark-Simulacrum's summary, I propose to merge this. I think we should skip the feature gate, as I consider this a bug fix.

[rfcbot]

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

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

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!

See this document for info about what commands tagged team members can give me.

[cramertj]

Sharing in case others have the same thought process:

At first I thought that it was odd to have a #[no_mangle] associated function use the fn <...> name, since I think of the name of those functions as including the type-component of the path. That is, I think of the function above as AssocFn::foo rather than a function named foo. However, the more I consider it, the more it makes sense to think of it just like we would mod assoc_fn { #[no_mangle] fn foo() { ... } }-- the type is part of the path / item container, just as any other path element such as a crate or module would be.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[petrochenkov]

If the intent is to support #[no_mangle] on associated items, then this PR is not enough.
You can search for sym::no_mangle in rustc codebase - all those places need to be updated to account for no_mangle on non-items.

[bors]

📌 Commit 9315a0c has been approved by petrochenkov

[bors]

⌛ Testing commit 9315a0c with merge 3266d01a40ef990ea7c029906fed132f9d2e4f8e...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[petrochenkov]

@bors retry

[bors]

⌛ Testing commit 9315a0c with merge 5a19ffe...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 5a19ffe to master...

[rust-highfive]

📣 Toolstate changed by #86492!

Tested on commit 5a19ffe.
Direct link to PR: #86492

💔 miri on windows: test-pass → test-fail (cc @eddyb @RalfJung @oli-obk).
💔 miri on linux: test-pass → test-fail (cc @eddyb @RalfJung @oli-obk).

[ghost]

I think this change (unintentionally) made reachable_set no longer include any #[no_mangle] static item. Opened #88032 to fix this.