New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for RFC 2091: Implicit caller location #47809

Open
aturon opened this Issue Jan 27, 2018 · 13 comments

Comments

Projects
None yet
7 participants
@aturon
Copy link
Member

aturon commented Jan 27, 2018

This is a tracking issue for the RFC "Implicit caller location" (rust-lang/rfcs#2091).

Steps:

Unresolved questions:

  • If we want to support adding #[track_caller] to trait methods, the redirection
    pass/query/whatever should be placed after monomorphization, not before. Currently the RFC
    simply prohibits applying #[track_caller] to trait methods as a future-proofing measure.

  • Diverging functions should be supported.

  • The closure foo::{{closure}} should inherit most attributes applied to the function foo, in
    particular #[inline], #[cold], #[naked] and also the ABI. Currently a procedural macro
    won't see any of these, nor would there be anyway to apply these attributes to a closure.
    Therefore, #[rustc_implicit_caller_location] currently will reject #[naked] and ABI, and
    leaving #[inline] and #[cold] mean no-op. There is no semantic reason why these cannot be
    used though.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Jan 27, 2018

If we want to support adding #[track_caller] to trait methods,

This refers to impl of a trait, not the declarations within the trait, right?

IMO the easiest way to implement this involves no procedural macros or MIR passes.
We'd just pass extra arguments on direct calls, and require a shim to get function pointers of the function. That would also surely work with trait methods, since it'd be post-monomorphization.

To explain, the shim (which we may already have some variation of) would "just" do the direct call to the function, which would pass the location of the shim, the same as the original function.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 29, 2018

This refers to impl of a trait, not the declarations within the trait, right?

I can't remember. =) But I suspect so. I don't think there's anything special about "trait methods" per se -- it's really about dynamic dispatch.

@killercup killercup referenced this issue Feb 8, 2018

Merged

add expect_macro #3

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 25, 2018

@kennytm had a prototype implementation:

https://github.com/kennytm/rust/tree/caller-info-4

Maybe @kennytm you can summarize the approach you took? Do you think you'll have time to rebase etc? I'd like ideally to get @eddyb to buy in to the overall approach. =)

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Apr 25, 2018

The prototype implementation works like this (note that #[track_caller] was called #[implicit_caller_location] there):

  1. First there will be an AST transformation pass (implemented as an attribute proc-macro in src/libsyntax_ext/implicit_caller_location.rs) which expands:

    #[track_caller]
    #[...attributes...]
    fn foo(...) -> T {
        /* code... */
    }

    into

    #[rustc_track_caller]
    #[inline]
    #[...attributes...]
    fn foo(...) -> T {
        let __closure = |__location| { /* code... */ };
        FnOnce::call_once(__closure, intrinsics::caller_location())
    }

    The purpose of this pass is to conveniently make up a new DefId.

  2. Create a new MIR inlining pass (main implementation in src/librustc_mir/transform/inline.rs). This pass does two things:

    1. Force-inline any functions calls foo() where the attributes of the callee foo contains #[rustc_track_caller].

    2. If intrinsics::caller_location() is called :-

      • If it is used from the __closure, replace it by the argument __location. This is to propagate the caller location.
      • Otherwise, replace the call with the caller's location span.
  3. Define the caller_location() intrinsic.

    fn caller_location() -> core::panic::Location<'static>;
  4. Because Location now needs to be known by the compiler, make the Location struct a lang-item.

  5. Update the standard library to use track-caller features:

    1. Change panic! to use caller_location() (or its safe counterpart, Location::caller())
    2. Add #[track_caller] to unwrap() and expect() etc.
  6. Implement the -Z location-detail flag (search for location_detail) so that the filename/line/column can be redacted.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 25, 2018

I think a syntactical transformations is unnecessary because we can instead change the "direct call ABI" (vs reifying to a fn pointer, including trait vtables via miri, which could go through a MIR shim).
MIR inlining should also not be needed.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Apr 26, 2018

@eddyb So you are suggesting to change the extern "Rust" ABI to always pass an extra argument?

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 26, 2018

@kennytm Only for functions declared with the attribute and called through static dispatch, everything else would use a shim when reifying (which I think we do already in some other cases).

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Apr 26, 2018

@eddyb Maybe you could write down some mentoring instructions i.e. which files to look at 😊

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 2, 2018

@eddyb do it! do it! I want this feature.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 1, 2018

@eddyb How are the mentoring instructions coming along?

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Dec 2, 2018

Oh, I don't recall exactly what happened here.

For making static calls do something different, src/librustc_codegen_ssa/mir/block.rs is the place you need to change, while for shims, src/librustc_mir/shim.rs is where their MIR is computed.

But the rest of the pieces, I don't know off the top of my head where they happen.

#54183 added a virtual-call-only shim, which is close to what this needs, so that can be used for inspiration, but here we need both reification to fn pointers and virtual calls.

We could start by disallowing reification/virtualization of such functions, and only implement the static dispatch ABI changes.

@Centril Centril added the E-mentor label Dec 2, 2018

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Dec 2, 2018

We could start by disallowing reification/virtualization of such functions, and only implement the static dispatch ABI changes.

I don't expect this attribute to be usable on "real" functions without us implementing reification.

But for implementing this, I think one good strategy would be:

Not supporting trait methods

For now, I don't think there is sufficient reason to support implicit caller location on trait methods, and supporting that is fairly complicated. So as @aturon said on the head PR, just don't do it.

Step 0: dealing with the attribute.

You should make sure that the #[blame_caller] attribute "works". This means that:

  1. It emits a feature-gate error unless the correct feature-gate is enabled (see the feature guide for more information about feature gates).
  2. It emits an error when used with odd syntax (e.g. #[blame_caller(42)]),
  3. It emits an error when used on something that is not a free fn/inherent impl fn.
  4. It does not emit an error when used "correctly'.
  5. As the RFC says: using #[naked] or extern "ABI" together with #[rustc_implicit_caller_location] should raise an error.

I'm not particularly sure what's the exact way to do this, but you can maybe find some PR that implemented an attribute for that, or ask on Discord.

Make sure to add tests.

Step 1: have reified methods and direct calls go to different LLVM functions

At this stage, I won't even add a parameter, just have the call-sites go through different paths.

So the virtual-call-only shim at #54183 is a good model for how things need to be done. You'll need to add a ReifyShim that is similar to VirtualShim in behavior (see that PR). You'll probably need to split ty::Instance::resolve to ty::Instance::resolve_direct and ty::Instance::resolve_indirect (in addition to the already-existing ty::Instance::resolve_vtable) and adjust the call-sites. Have resolve_indirect return the shim if it is needed (i.e., when calling a #[blame_caller] function).

Step 2: Add a location parameter

Step 2.0: add a location parameter to the MIR of #[blame_caller]

MIR construction is in rustc_mir::build. There's already code there handling implicit arguments, which you could work with.

You'll need to fix ty::Instance::fn_sig to also contain the new type.

Step 2.1: add an "empty" location parameter to calls from the shim.

Change the shim you added in Step 1 to pass an "empty" location parameter - some "dummy" that represents a call that uses a shim. See the other shims for how to generate things in shims. Ask @eddyb if you are fighting const eval.

Step 2.2: add a value to the location parameter to direct calls.

Find all the calls to ty::Instance::resolve_direct you added previously, and make them pass the "correct" location parameter to the function. These callsites can poobably already modify parameters because of InstanceDef::Virtual, you should be able to just follow that.

Make sure that when #[track_caller] calls are nested, you pass the location of your call-site, rather than your location. Add a test for that. Also add a test that when one of the calls in the middle is not #[track_caller], the call-site is not passed.

You should probably add a function on ty::Instance that tells you whether you need to add the caller location.

Step 3: wire up intrinsic::caller_location

See some other intrinsic for how to type-check it. Make sure it can't be called from functions that aren't #[trace_caller]!

I think it might even be the best to do the wiring in MIR construction, like the move_val_init intrinsic - that way you wouldn't even have to convince MIR optimizations not to dead-code-eliminate your new parameter or something evil like that, and to make it not be affected by "plain" MIR inlining you'll only have to change the ty::Instance::resolve_direct callsite there (you did make MIR inlining use resolve_direct, did you?).

Step 4: You have it!

Now you only need to implement the library support.

@flavius

This comment has been minimized.

Copy link

flavius commented Feb 11, 2019

Is anyone working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment