the #[track_caller] shim should not inherit #[no_mangle]

[folkertdev]

fixes #143162

builds on #143293 which introduced a mechanism to strip attributes from shims.

I'm now stripping specifically the attributes that modify the symbol from the #[track_caller] shim. Maybe I missed some though, and maybe we really should be stripping more things from either the #[track_caller] shim, or shims in general.

cc @Jules-Bertholet @workingjubilee @bjorn3

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
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

[rustbot]

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[Jules-Bertholet]

I think this may be the exact opposite of what we want to do—the #[no_mangle] should probably apply only to the shim. (I think we actually need the lang team to weigh in on this… #143162 (comment))

[folkertdev]

Having the shim "steal" the attribute breaks this code:

https://github.com/rust-lang/rust/blob/master/tests/ui/rfcs/rfc-2091-track-caller/track-caller-ffi.rs

extern "Rust" {
    #[track_caller]
    fn rust_track_caller_ffi_test_tracked() -> &'static Location<'static>;
    fn rust_track_caller_ffi_test_untracked() -> &'static Location<'static>;
}

mod provides {
    use std::panic::Location;
    #[track_caller] // UB if we did not have this!
    #[no_mangle]
    fn rust_track_caller_ffi_test_tracked() -> &'static Location<'static> {
        Location::caller()
    }
    #[no_mangle]
    fn rust_track_caller_ffi_test_untracked() -> &'static Location<'static> {
        Location::caller()
    }
}

in other words, it is current behavior that you are able to export a symbol that takes the implicit location argument.

The #[track_caller] attribute is only allowed on functions that use the (unstable) rust ABI, so only interop with other rust code is relevant, and then it does make sense to me that you'd want to be able to export the "tracked" version. Users can always write their own shim if they want to export a symbol that does not have the implicit location argument.

edit: so basically I implemented what Björn suggests at #143162 (comment).

edit2: also Jubilee did not think that T-lang needed to be involved #143162 (comment). Given the above that seems right to me: if we want to keep existing code working, we need the shim to drop these attributes. That seems fine, because only extern "Rust" can be used in combination with #[track_caller].

[Jules-Bertholet]

in other words, it is current behavior that you are able to export a symbol that takes the implicit location argument.

Ah, I didn’t know that was actually intended to work and that we had a test for it.

also Jubilee did not think that T-lang needed to be involved #143162 (comment).

Yes, but she was advocating for the exact opposite resolution than you are proposing here! So I think it would actually have confirmed that they did need to be involved. Except that, at the time, neither she nor I knew that the lang team had already made this decision. So yes, in light of this new-to-me information, there’s no need for lang involvement, and we should just merge this PR.

[Jules-Bertholet]

You should also add a test that casting a #[track_caller] fn from an extern block to a function pointer works:

mod inner {
    #[unsafe(no_mangle)]
    #[track_caller]
    extern "Rust" fn foo() {
        println!("hello world");
    }
}

unsafe extern "Rust" {
    #[track_caller]
    safe fn foo();
}

fn main() {
    foo();
    let fnptr = foo as fn();
    fnptr();
}

[folkertdev]

It does work, though it uses the location of the safe fn ... definition. That does make sense in a way, but I'm not sure that is correct.

[Jules-Bertholet]

It does work, though it uses the location of the safe fn ... definition.

This has a subtle interaction with #[align(…)].

mod inner {
    #[unsafe(no_mangle)]
    #[track_caller]
    #[align(32)]
    fn foo() {
        println!("hello world");
    }
}

unsafe extern "Rust" {
    #[track_caller]
    safe fn foo();
}

fn main() {
    let fn_addr = foo as fn() as usize;
    assert_eq!(fn_addr & 31, 0);
}

The assertion would likely pass without track_caller, but fails with it. I think this is actually fine, we shouldn’t guarantee that the assertion passes; I’ll update the RFC.

[fee1-dead]

it is current behavior that you are able to export a symbol that takes the implicit location argument

I don't think that's the case here? Given that #[no_mangle] already doesn't work with #[track_caller], it seems that no one can actually use #[track_caller] inside extern "Rust" blocks (since no one can actually export them). So I do see this as a picking one from two choices:

• Disallow #[track_caller] or make #[track_caller] a no-op in extern "Rust" and then make the shim use the no_mangle name (the inner function that has the extra argument does not have no_mangle). I don't think this would break anyone but we could do a crater run for this just to make sure.
• Allow #[track_caller] in extern "Rust", make the underlying function (which has extra argument) #[no_mangle] but not the shim. (this PR)

I personally think we should be picking the first option, as that is what the original RFC specifies:

Currently the foo::{{closure}} cannot inherit attributes defined on the main function. To prevent
problems regarding ABI, using #[naked] or extern "ABI" together with
#[rustc_implicit_caller_location] should raise an error.

[folkertdev]

Given that #[no_mangle] already doesn't work with #[track_caller]

That is not the full picture, the test shows that they do in fact work together, but then creating a ReifyShim (e.g. by making a function pointer) causes the error.

Doing some digging, the test was added in #70916, based on the comments this behavior was stabilized very deliberately (though without a very clear motivation, other than that the alternative seemed odd).

We could make a different choice now but then we're firmly in T-lang territory I think.

[fee1-dead]

Let's nominate this for lang then

[Jules-Bertholet]

If you look at the git history of the test and related discussions, it’s clear that lang already decided this. It’s even documented in the Reference:

When applied to a function in an extern block the attribute must also be applied to any linked implementations, otherwise undefined behavior results. When applied to a function which is made available to an extern block, the declaration in the extern block must also have the attribute, otherwise undefined behavior results.

[folkertdev]

Right, the thing I'm not 100% clear about is the interaction with #[no_mangle]. The RFC apparently suggests that you should not be able to apply ABI-changing attributes in combination with #[track_caller]? (it's kind of hard to parse, to me at least).

I believe (but do not know for sure) that disallowing #[no_mangle] breaks the FFI test (and would in effect violate the reference). At least, it would break the test in spirit, because there is no guarantee for the mangled symbol at the definition and in the extern block to match up (though because they are in the same file/CGU here, it might still work in this concrete case).

---

The reference also documents the behavior that we're seeing with the location reported by the shim, so I think that's all good then

https://doc.rust-lang.org/reference/attributes/codegen.html#r-attributes.codegen.track_caller.decay

[Jules-Bertholet]

Yes, the decision was made post-RFC, and is contrary to it. Reviewing the discussions again, I’m not actually sure whether the entire lang team was fully aware.

We could change the behavior without breaking code by simply ignoring #[track_caller] in extern blocks. Alternately, as I suggested in the other thread, it should be possible to support linking #[track_caller] #[unsafe(no_mangle)] fn foo() {} with both extern "Rust" { fn foo(); } and extern "Rust" { #[track_caller] fn foo(); }, by generating 2 symbols—but removing #[track_caller] from the original declaration would still be breaking.