Skip to content
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

RFC: Implicit caller location (third try to the unwrap/expect line info problem) #2091

Merged
merged 12 commits into from
Jan 27, 2018

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Jul 31, 2017

Current (Implicit caller location)

Summary

Enable accurate caller location reporting during panic in {Option, Result}::{unwrap, expect} with
the following changes:

  1. Support the #[blame_caller] function attribute, which guarantees a function has access to the
    caller information.
  2. Add an intrinsic function caller_location() (safe wrapper: Location::caller()) to retrieve
    the caller's source location.

Legacy (Semantic inlining):

@jethrogb
Copy link
Contributor

We could change the meaning of file!(), line!() and column!() so they are only converted to real constants after semantic-inlining (a MIR pass) instead of early during macro expansion (an AST pass). Inside #[inline(semantic)] functions, these macros behave as this RFC's core::caller::{FILE, LINE, COLUMN}. This way, we don't need to introduce panic_at_source_location!(). The drawback is losing explicit control of whether we want to report the actual location or the caller's location.

I think the macros should always resolve to the special lang item, and the lang items should also work in non-inlined functions (but you'd get the location of the lang item itself, not the caller). And maybe the lang items could be annotated such that they can be used from within the macros but not written manually.

@kennytm
Copy link
Member Author

kennytm commented Jul 31, 2017

Hmm. Turning file!(), line!() and column!() just into special consts will break backward-compatibility. They need to expand into special literals (or concat! needs to pretend those lang items are literals).

const FILE: &str = file!();
const LINE: u32 = line!();
macro_rules! my_file { () => (FILE) }
macro_rules! my_line { () => (LINE) }
fn main() {
    println!("{}", concat!(file!(), ":", line!()));
    //^ Fine, prints src/main.rs:6
    println!("{}", concat!(my_file!(), ":", my_line!()));
    //~^ ERROR: expected a literal
}

@jethrogb
Copy link
Contributor

jethrogb commented Jul 31, 2017

Uh oh. That might be unintended. At least the docs say it expands to an expression of type &'static str, not a literal. But I suppose we can't change that now.

@kennytm
Copy link
Member Author

kennytm commented Jul 31, 2017

@jethrogb Actually expanding to a literal would be far easier than a special const item. We just add a new LitKind and make file!() and friends (which are procedural macros) the only way to generate such LitKind.

However, doing so would make panic not able to use the static _FILE_LINE_COL = (file!(), line!(), column!()); code-size optimization trick.

@eddyb
Copy link
Member

eddyb commented Jul 31, 2017

Is that optimization worth it? It only means the value is not duplicated between crates.
&(file!(), line!()) hasn't had the values on the stack since before 1.0 (rvalue promotion to 'static - you still need a feature gate to get the borrowck to believe it but the rest of the compiler does it anyway as an optimization) - and we deduplicate constants like that within a crate already.

## Caller location

The core crate provides three magic constants `core::caller::{FILE, LINE, COLUMN}` which resolves to
the caller's location one the function is inlined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have multiple layers of #[inline(semantic)], this might end up being unclear. How about a compile-time backtrace getting built up with all of that info?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk The panic handler will need to be updated to accept an inline stack instead of just the location...

fn panic_impl(fmt: fmt::Arguments, inline_stack: &[(&'static str, u32, u32)]) -> !;

@steveklabnik
Copy link
Member

I just want to say that this is one of the most well-written RFCs I've ever seen. Amazing job @kennytm !


## Why do we use semantic-inlining

If you are learning Rust alongside other languages, you may wonder why Rust obtain the caller
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/obtain/obtains


Rust allows developers to use the `#[inline]` attribute to *hint* the optimizer that a function
should be inlined. However, if we want the precise caller location, a hint is not enough, it needs
to be a requirement. Therefore, the `#[inline(semantic)]` attribute is introduced.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the description here is slightly off - we already have #[inline(always)], and #[inline(semantic)] behaves fundamentally differently than traditional inlining. This feature doesn't even necessarily need to be thought about as an inlining at all, really - you could also phrase it in terms of parameterizing the function over its invocation locations, for example.

assert_eq!(get_caller_loc(), (file!(), line!()));
```

There is also a `caller_location!()` macro to return all three information as a single tuple.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be a macro, right? Could it be a semantically inlined function instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfackler Yes it can be. core::caller::location(), or core::???::Location::caller() if Location is moved into libcore.

`#[inline(with_caller_location)]`?
* Use a different attribute, instead of piggybacking on `#[inline]`?
* `***_at_source_location` is too long?
* Should we move `std::panic::Location` into `core`, and not use a 3-tuple to represent the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact this is what RFC #2070 is suggesting as well

- [Rationale and alternatives](#rationale-and-alternatives)
- [Rationale](#rationale)
- [Alternatives](#alternatives)
- [🚲 Name of everything 🚲](#🚲-name-of-everything-🚲)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link is broken on Github, the anchor is #-name-of-everything-

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ariasuni Thanks. This is a bug in the Markdown TOC generator (alanwalk/markdown-toc#31).

@est31
Copy link
Member

est31 commented Aug 2, 2017

Thanks @kennytm for writing such a well written RFC! It is definitely important to fix this issue of the unwrap function.

First on a matter of coordination. @japaric has opened RFC #2070 recently which concerns enabling panic! to work on no-std applications without using unstable features. As the two RFCs both affect the panic mechanism of Rust, there is some level of overlap here, it would be great if you both could coordinate to make sure the RFCs are compatible with each other.

Semantic inlining vs traditional inlining

I want to emphasize what @sfackler has pointed out above: #[inline(semantic)] is different from traditional inlining. It violates the usual boundary between the calling and the called function to a greater extent than traditional inlining. With traditional inlining, you usually want to retain the separation between the caller and the calee in the stack trace and get annoyed if the compiler/debugger mixes them up, and with #[inline(semantic)] you want the exact opposite. Teaching people that this is the same mechanism will only cause confusion IMO.

Due to the point above, the #[inline(semantic)] name is much greater than the alternatives mentioned. I especially like how "semantic" conveys the meaning that there is an actual "flow of data" from the caller to the callee (the location info), independent from parameters passed. I wouldn't like a rename to #[inline(mir)] because right now, there is no mention of MIR in the language anywhere, and I'd say its just an implementation detail, which it should IMO stay. #[inline(force)] sounds too similar to #[inline(always)], while both are semantically different things.

If the #[inline(semantic)] name can be improved, then by making it less close to the traditional inlining mechanism.

location info lang items

As the file!() macro and friends can't expand to simple literals any more, the RFC proposes the addition of lang items that get classified as constants, but get special meaning as e.g. constant folding must ignore them. They essentially get treated like dummies until the MIR inlining pass. The basic problem here is that you need to have some dummy that the MIR semantic inlining pass can expand to a literal, but which can also be expressed by the token stream API that macros use as input and output. The RFC proposes to solve this via addition of lang items, which is a great way of doing this.

lang items get treated specially by the compiler, so they are always more than just what you call them. You still need to find some category in the existing language where the a feature fits in really well, which you then use to expose it to the remainder of the language. I think the category chosen right now, constants, is not really suiting. For me, an important property of constants is that they are a never changing value, everywhere the same. There are mutable statics, but this is a different thing obviously. You'll have to add an exception to constant folding to specifically avoid them, and you need to define dummy values at the declaration site that might be mistaken by the reader for actual values. And, you have to introduce a new concept of "magic" constants to explain them.

A much greater way of categorizing the lang items would be intrinsic functions marked with #[inline(semantic)]. Intrinsic because they will be implemented in the compiler, and #[inline(semantic)] because the the context of the calling place will affect their evaluation (Of course, it is natural that a call to an intrinsic function gets inlined, but marking it #[inline(semantic)] gives us that additional meaning). Even if the intrinsics that exist in the compiler right now all get converted to llvm intrinsics (so adding an intrinsic that doesn't reach LLVM would be a novelty), the lang items fit much better into the class of intrinsics than into the class of constants.

Forwards compatibility

When I added column info to panic printing, I was pleasantly surprised that panic info printing was forwards compatible to a great degree; I didn't have to avoid breaking any stable API. This property should be preserved by the RFC IMO.

Possible future changes include e.g. include more information from the span, like e.g. the ending line and colum, or crate names, or similar. Of course you can argue how useful these additions are, but I think regardless from one or another being not very useful, keeping forwards compat is important!

The panic hook mechanism has the opaque Location struct which is very forward compatible. If we add a caller_location macro, it should return this struct instead of a tuple.

I haven't found any other places where this RFC is incompatible with future extensions, but if there are any, it would be great to get rid of them.

@nikomatsakis
Copy link
Contributor

I certainly agree this is annoying problem, and one worth solving. It seems to me that the big decision to be made is indeed whether to assume that debuginfo is not a viable path -- I believe that other languages have solved this by having the ability to annotate functions as being "implementation details", and hence these functions get omitted or overlooked in a backtrace (i.e., by the backtrace machinery). But I can't find any citations for this second approach.

In a way, the two are similar. The idea is to label functions that users probably don't want to see. I think one could argue that implementing this via inlining is an 'implementation detail', right? The important bit is that the various new macros in question will work give the position info from the caller, right? (Or are there other things I am overlooking where it is crucial that the function is inlined at the MIR level?)

@est31
Copy link
Member

est31 commented Aug 2, 2017

Or are there other things I am overlooking where it is crucial that the function is inlined at the MIR level

It is important that the function is inlined in MIR instead of waiting for LLVM because the location info is separate for each time a function gets inlined. We want to expand the location info to a literal at compile time and not read it from debuginfo at runtime so that this works with builds whose debuginfo is stripped. IMO we should still offer an option to strip even this info (I think right now its possible to do that via custom panic_fmt lang items, not sure though), but there should be a mode where main debuginfo is being stripped, but the panic location is still being printed.

@main--
Copy link

main-- commented Aug 2, 2017

Inlining is so painful to debug, I'd like to avoid it here if at all possible.

I also don't really agree with your rationale that relying on debug info is not an option. In my experience, the location information from a panic message is rarely useful (at all) in trying to figure out what went wrong - you need at least a backtrace for that.

I just don't see the value in trying to provide this information even when there's no debug info. It's certainly of no immediate use to end users, but I can imagine it being relevant for bug reports. However as I said, a bug report without a backtrace is rarely useful anyways.

From this perspective, I would suggest a very different approach: Rely on debug info entirely. Then, the solution to this problem can be as simple as just always printing more than a single stack frame (say 3) by default (having to re-run from the beginning with RUST_BACKTRACE=1 is always very annoying).

Release builds are of course an issue but I believe there's a much better solution for that: Build with debug symbols but save them externally, in their own file. This also means that bug reports from end users are now much more useful overall as the backtrace can be decoded using the symbol file (even core dumps should work, right?).

Anyways, that's just what I'd like to see from my personal experience.

@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 2, 2017

@est31

It is important that the function is inlined in MIR instead of waiting for LLVM because the location info is separate for each time a function gets inlined.

Can inlining be replaced with a special ABI with modified calling conventions using additional implicit argument for passing (pointer to) the location?
When such functions are converted into function pointers some shims will have to be created though.

@repax
Copy link

repax commented Aug 2, 2017

I like this idea of MIR inlining a lot. As for another use case: It would be nice if the callee could be able to verify, at compile-time, that the caller is eligible, given some policy or otherwise.

I'd also like to see an implementation of this panicking business where this source location data could be stripped from the binary. Points of possible panic are rather prevalent.

@petrochenkov
Copy link
Contributor

@eddyb @kennytm
Could you elaborate why the ABI tweak is a bad alternative compared to inlining?

@eddyb
Copy link
Member

eddyb commented Aug 2, 2017

@petrochenkov It's not as bad as it is involving in the backend. Unless you mean it's all in the Rust compiler, which could make it a MIR pass, which, uhh, would be vaguely equivalent to inlining.
If it's not a new calling convention (see: Swift) but just an extra argument rustc adds, that's fine.

@petrochenkov
Copy link
Contributor

Yeah, I meant normal calling conventions (in low level sense), just an extra argument implicitly added during translation.

@kennytm
Copy link
Member Author

kennytm commented Aug 2, 2017

Thanks for the feedback 🙂 Long post below.


@est31 on Forwards compatibility

The current PoC did not change the how panic works besides relaxing the lifetime requirement of core::panicking::panic, so there should be no problem with future changes of Location and #2070.


@sfackler @est31 on Traditional inlining vs Semantic inlining, and Location info lang item

This RFC itself did not propose changing file!() as friends (yet). The actual file!() was still retained, e.g.

#[inline(semantic)]
fn boom() {
    panic!("kaboom");  // line 3
}
fn main() {
    boom(); // line 6
}

with the current implementation, the panic will still point to line 3. So this is still somewhat like traditional inlining.

To entirely blur the line between caller/callee, we need to change file!() to expand to a special literal (which has no source representation) as mentioned in the first 5 comments. It cannot be an intrinsic or const or static because concat!(line!()) or include_str!(file!()) outside of #[inline(semantic)] should not be affected.

Assuming we don't care about panic's optimization, the only hazard is that,

#[inline(semantic)]
fn foo() -> bool {
    const LINE: u32 = line!();  // <-- this will not be the caller's line
    let l = line!() // <-- this will be the caller's line
    l == LINE // usually false
}

The static/const should trigger an error something similar to E0401. There should also be file!(after_macro_expansion_but_before_inline_semantic) to provide the traditional behavior.

I'm open to keeping core::caller::* and change those into intrinsics, but I'll also see would going through the LitKind route be too dramatic a change. I may modify the RFC after that.


@nikomatsakis @main-- on Debuginfo

Disclaimer: I've never used a debugger on rust generated program for serious debugging because (1) printf-debugging is much more accurate and easier (2) lldb doesn't seem to work properly[a] (3) the only time I've used lldb is to obtain a more correct backtrace, since RUST_BACKTRACE=1 does not work at all on macOS (rust-lang/rust#24346). So I am certainly biased here 😆.

The biggest reason debuginfo is not reliable because it is often absent. And it is absent because it is not generated by default, is huge (>100% size of the executable) and leaks sensitive source code info for propriety programs. Look, even rustc does not ship with any debuginfo

$ find ~/.rustup -name '*.dSYM'

$

💭 Maybe rustup should provide an optional "debuginfo" component 😉

There is also the issue that RUST_BACKTRACE=1 is not enable by default, partly because it is slow (rust-lang/rust#42295), and also because sometimes your system just doesn't have libunwind and you can't even get one level of backtrace.

And I just feel that a systems language with thin runtime shouldn't require a runtime solution to get the caller info.

💭 BTW I wonder why our CI system does not include RUST_BACKTRACE=1? Currently we have logs like https://travis-ci.org/rust-lang/rust/jobs/259620982 (from rust-lang/rust#43506) which fixing requires guesswork.

Note: [a]: lldb "step" jumps around randomly, "print" often don't work probably because the variable is optimized away etc. Sorry I didn't try to reproduce them properly, so no issues filed.


@nikomatsakis on Omitting backtrace (a.k.a. #1744)

Yes implementing this via inlining is an implementation detail. And yes the important bit is the various lang items will work. But I disagree that the idea is to label functions that users don't want to see. In fact, if possible I want unwrap/expect to remain visible in the stack trace, like

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', 1.rs:7:4
stack backtrace:
  10: (garbage omitted for brevity)
  11: <core::option::Option<T>>::unwrap
  12: _1::main
  13: __rust_maybe_catch_panic
  14: std::panicking::try
  15: std::rt::lang_start
  16: main

This could be done by cloning the function's MIR + rewrite + redirect the caller, instead of inlining them. That is more aligned to @sfackler's comment that #[inline(semantic)] is parametrization by call site. But I can't find the way to add a Mir yet; a MirPass can only modify existing MIR. Anyway it is just an implementation detail.

Omitting part of backtrace inside an IDE is not a new idea, Xcode supported this very long time ago, even with dynamic granularity setting. But I'm not aware of any languages besides Swift that allow programs to annotate functions as unimportant themselves, at least not in Python or Java.

The problem isn't that the backtrace is too verbose, but it is not precise enough at the right place due to (traditional) inlining, or that the backtrace does not include the line number and you used more than one unwrap in the function.


@petrochenkov on Special ABI

It won't work because fn(T) and extern "abi" fn(T) are two different types. If they are the same type then it is no different from using an #[attribute].

@main--
Copy link

main-- commented Aug 2, 2017

I have used gdb on Rust a few times and I agree that it could use some improvement. The seemingly random jumps you're seeing are probably due to the fact that the debug symbols emitted by rustc tend to associate almost every instruction with the original statement that caused it to be generated. This is rather problematic as optimizations shuffle things around quite a bit.

But I consider this a bug that should be fixed one day (C/C++ compilers have a decent workaround).


I'm well aware that debug info is rarely shipped in releases. I even believe storing debug info externally (like Microsoft have been doing for ages with their pdb files) is very unconventional in the Unix world today (FWIW Ubuntu do provide symbol files for some of their packages). I wonder why? Is there any big drawback I'm not aware of?

The biggest reason debuginfo is not reliable because it is often absent. And it is absent because it is not generated by default, is huge (>100% size of the executable) and leaks sensitive source code info for propriety programs. Look, even rustc does not ship with any debuginfo

Yes, this is certainly an issue today. My suggestion was not "just use debuginfo", it was "make sure debug symbols exist (throughout the ecosystem), then just use that". Most notably:

  • You would not usually ship a symbol file to end users. In the case of proprietary software with binary distribution this means that it does not leak any information about your source code (not even the the file names and location numbers for panicking).
  • You should still be able to decode a backtrace from those end users on your side using the symbol files (e.g. in a bug report).
  • And in the case of open source software, a user that wants to debug the application can just download the symbol file as well and start debugging.

💭 Maybe rustup should provide an optional "debuginfo" component 😉

👍 It should!

And again, my core argument here is basically that in a world where rustc generates symbol files for every release build by default, not having these symbols always means that showing line info is undesirable.

@matthieu-m
Copy link

First of all, thank you for raising this issue and providing an avenue of discussion; it seems that whenever I write tests I have to have at least this one instance of Option::expect panicking and having to rerun the tests with backtracing enable to get a clue as to where the issue happened.


I think that to solve the optimization issue it could be useful to have a special flag/setting which strips column information from debug information and source locations (making them 0, or even removing them altogether).

This is something that gcc and clang have, with -glevel=1 (0: no debug, 1: minimal debug and 3: full debug). It is an interesting compromise to get some level of backtrace/source-level tracking without completely bloating the generated binary.


With regard to your proposal, I am not a fan of inlining. It seems to me inlining is limited and may cause issues:

  • does not work with recursive functions (mentioned in RFC, but should be added to Drawback section),
  • does not work with polymorphic calls,
  • poorly works with large functions.

The latter can be worked around by having a lint advising users to create a private function with the bulk of the implementation and then using the current public function as a forwarding shell (which can then be inlined without bloat). However it does mean shifting the burden onto the users.

As a result, much as did @petrochenkov, I too wonder whether a different ABI/magic parameter would not be a better solution.


As a strawman proposal, I'll go with the "magic parameter" approach:

#[ghost]
fn panic() -> ! {
    let loc = location!(); // magically retrieves the source location from the ghost argument
}

//  Exposed as parameter when taking a function to pointer, or converting to `Fn`.
static PANIC: fn(&'static ::core::source::Location) -> ! = panic as fn(&'static ::core::source::Location) -> !;

//  Can be passed as regular parameter, even though not part of "official" function signature.
panic(location!());

The essence here is that if we are to be magical, I don't see why we cannot have simili default parameters.

Calling a #[ghost] function from a #[ghost] function should either:

  • magically form a backtrace: would need an array of Location then, and unsure how it would work with polymorphic calls,
  • or simply forward the current location.

The latter is fully decidable at compile-time.

There is one question: how should specialization/implementation of a trait method work. There are two alternatives as far as I can see:

  • If the base method is #[ghost] then the specialization/implementation MUST be #[ghost] too; and vice versa.
  • The specialization/implementation being #[ghost] is independent of the base method:
    • If the base is #[ghost] and specialization/implementation is not, then the specialization/implementation ignores the magic argument (still passed for ABI reasons),
    • If the base is not #[ghost] and the specialization/implementation is, then when called statically it has the caller's location but when called dynamically it has its own location (essentially, two functions are generated).

I am not sure which is best; I would recommend starting with the first one to accumulate experience and check whether this is really an issue to start with.

@kennytm
Copy link
Member Author

kennytm commented Aug 2, 2017

@matthieu-m I have thought of magic parameter and tried to implement that 3 times but it involves too many parts of compilers to make it work 😄 (AST/HIR to split a function into two, and MIR to rewrite the call, probably something more which I forgotten).

Essentially #[ghost] would generate two functions,

#[ghost]
fn x(args: T) -> R { ... }

// =>

#[ghost(redirect="def_id of x$real")]
fn x(args: T) -> R {
    x$real(args, &Location::current())
}

fn x$real(args: T, location: &Location) -> R {
   ...
}

then, after type-checking, any direct call to x would be rewritten to a call to x$real.

#[ghost] cannot use the "and vice versa" option because I want Index::index be #[ghost] too, and that would break downstream source code. Allowing panic to accept an optional parameter is probably not good for inference, since this is basically function overloading which Rust avoids.

Some questions for this:

  • How should visibility be handled (the x$real usually comes from an external crate)
  • How to store the xx$real relationship (queries?)
  • How to do add the x$real without adding an item to the trait
  • What happens when I take a function pointer. Note that the type of Option::unwrap must be coercible to fn(Option<T>) -> T, it cannot be fn(Option<T>, &Location) -> T.
  • Should x be #[inline(always)]

After thinking a bit of these, I find that the effect is very similar to inlining in terms of what I want to solve (unwrap and index), and went with that instead.

@kennytm
Copy link
Member Author

kennytm commented Aug 2, 2017

@main--

I even believe storing debug info externally (like Microsoft have been doing for ages with their pdb files) is very unconventional in the Unix world today (FWIW Ubuntu do provide symbol files for some of their packages).

ELF-based systems like Linux and BSD do embed DWARF section in the binary. But macOS store them externally in a *.dSYM folder. MSVC uses *.pdb as you've mentioned.

@main--
Copy link

main-- commented Aug 2, 2017

ELF-based systems like Linux and BSD do embed DWARF section in the binary.

They traditionally do so in debug builds. And it makes sense there.

But moving them to a separate file is perfectly possible:

objcopy --only-keep-debug my_binary my_binary.sym
strip --strip-debug --strip-unneeded my_binary
objcopy --add-gnu-debuglink=my_binary.sym my_binary

This is what I would suggest for release builds on ELF systems.

@est31
Copy link
Member

est31 commented Aug 2, 2017

@kennytm thanks for your reply. Could you also reply to these suggestions of mine, you seem to have missed them:

  • caller_location should return a Location struct, not a tuple, for forwards compatibility. You just said that your entire RFC is forwards compatible, but caller_location is clearly not.
  • replacing "magic constants" with #[inline(semantic)] intrinsic functions. Fits much better than constants.

@kennytm
Copy link
Member Author

kennytm commented Dec 20, 2017

@pornel #[track_callers] Sounds good to me!

@repax
Copy link

repax commented Jan 1, 2018

I think that the language feature requested in this RFC is both too invasive and too specific. I feel like it's a kludge that, if accepted, will haunt the Rust spec for eternity.

I'd rather see a general solution that allows you to include code in the caller's context that surround the called function (AOP-style but inline), or more work on guaranteeing not just the type of an argument but also its value.

Being able to guarantee values statically (or semi-statically) at the type level is a lot more general and powerful, and something that we would eventually like to tackle. Think of Ada-like value ranges or an effect system encoded in the types of objects and functions.

I'd like to see this proposal postponed until evaluated against long-term goals of the Rust language -- i.e. evolved static guarantees and so on.

@nikomatsakis
Copy link
Contributor

So, we decided to accept this RFC, but in the meantime there was some more feedback. Let me try to summarize the concerns:

I think these concerns can be summarized as "This RFC doesn't do enough to justify its complexity/cost":

I'm nominating for @rust-lang/lang team discussion to decide if we feel like we ought to back up or can proceed towards implementation.

@joshtriplett
Copy link
Member

I think we should continue. We don't always want full backtraces, and just knowing the caller location is useful.

@whitequark
Copy link
Member

I agree that just knowing the caller is extremely useful. On an embedded device I currently work on, there is no working unwinder right now, and an Option::unwrap() panic might mean... anything. And it is actually not trivial to get a working unwinder, e.g. you need a linker script that properly preserves DWARF sections, and even less trivial to debug an unwinder when for some reason it misbehaves. Not to mention that the unwinder, and especially the DWARF tables, are heavy space-wise.

I don't think that everyone who works with embedded Rust must pay the memory cost of having an unwinder, not to mention the mental cost of getting one to work. Also, there's no pure-Rust unwinders right now, so if one wants to do without the C compiler for the target device, backtraces are just impossible.

@aturon
Copy link
Member

aturon commented Jan 27, 2018

In the lang team meeting, the overall feeling was that we should stay the course: an improvement here is badly needed, and this RFC continues to be a highly plausible approach. The concerns that have been raised since FCP closed, while legitimate, are best assessed when we have a working implementation to learn from and iterate on.

Thanks again @kennytm for the RFC!

@aturon aturon merged commit f6004d6 into rust-lang:master Jan 27, 2018
pthariensflame added a commit to pthariensflame/rfcs that referenced this pull request Jan 31, 2018
@KasMA1990
Copy link

The link to the rendered RFC in OP is broken by the way.

@Centril
Copy link
Contributor

Centril commented Mar 4, 2018

@KasMA1990 Thanks, I've fixed it now.

@kennytm kennytm deleted the inline-semantic branch April 26, 2018 15:10
@Centril Centril added A-debugging Debugging related proposals & ideas A-panic Panics related proposals & ideas A-attributes Proposals relating to attributes labels Nov 23, 2018
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 22, 2021
…l, r=davidtwco

Implement -Z location-detail flag

This PR implements the `-Z location-detail` flag as described in rust-lang/rfcs#2091 .

`-Z location-detail=val` controls what location details are tracked when using `caller_location`. This allows users to control what location details are printed as part of panic messages, by allowing them to exclude any combination of filenames, line numbers, and column numbers. This option is intended to provide users with a way to mitigate the size impact of `#[track_caller]`.

Some measurements of the savings of this approach on an embedded binary can be found here: rust-lang#70579 (comment) .

Closes rust-lang#70580 (unless people want to leave that open as a place for discussion of further improvements).

This is my first real PR to rust, so any help correcting mistakes / understanding side effects / improving my tests is appreciated :)

I have one question: RFC 2091 specified this as a debugging option (I think that is what -Z implies?). Does that mean this can never be stabilized without a separate MCP? If so, do I need to submit an MCP now, or is the initial RFC specifying this option sufficient for this to be merged as is, and then an MCP would be needed for eventual stabilization?
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 22, 2021
…l, r=davidtwco

Implement -Z location-detail flag

This PR implements the `-Z location-detail` flag as described in rust-lang/rfcs#2091 .

`-Z location-detail=val` controls what location details are tracked when using `caller_location`. This allows users to control what location details are printed as part of panic messages, by allowing them to exclude any combination of filenames, line numbers, and column numbers. This option is intended to provide users with a way to mitigate the size impact of `#[track_caller]`.

Some measurements of the savings of this approach on an embedded binary can be found here: rust-lang#70579 (comment) .

Closes rust-lang#70580 (unless people want to leave that open as a place for discussion of further improvements).

This is my first real PR to rust, so any help correcting mistakes / understanding side effects / improving my tests is appreciated :)

I have one question: RFC 2091 specified this as a debugging option (I think that is what -Z implies?). Does that mean this can never be stabilized without a separate MCP? If so, do I need to submit an MCP now, or is the initial RFC specifying this option sufficient for this to be merged as is, and then an MCP would be needed for eventual stabilization?
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 23, 2021
…l, r=davidtwco

Implement -Z location-detail flag

This PR implements the `-Z location-detail` flag as described in rust-lang/rfcs#2091 .

`-Z location-detail=val` controls what location details are tracked when using `caller_location`. This allows users to control what location details are printed as part of panic messages, by allowing them to exclude any combination of filenames, line numbers, and column numbers. This option is intended to provide users with a way to mitigate the size impact of `#[track_caller]`.

Some measurements of the savings of this approach on an embedded binary can be found here: rust-lang#70579 (comment) .

Closes rust-lang#70580 (unless people want to leave that open as a place for discussion of further improvements).

This is my first real PR to rust, so any help correcting mistakes / understanding side effects / improving my tests is appreciated :)

I have one question: RFC 2091 specified this as a debugging option (I think that is what -Z implies?). Does that mean this can never be stabilized without a separate MCP? If so, do I need to submit an MCP now, or is the initial RFC specifying this option sufficient for this to be merged as is, and then an MCP would be needed for eventual stabilization?
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 31, 2022
…=davidtwco

`-Z location-detail`: provide option to disable all location details

As reported [here](rust-lang#89920 (comment)), when I first implemented the `-Z location-detail` flag there was a bug, where passing an empty list was not correctly supported, and instead rejected by the compiler. This PR fixes that such that passing an empty list results in no location details being tracked, as originally specified in rust-lang/rfcs#2091 .

This PR also adds a test case to verify that this option continues to work as intended.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Proposals relating to attributes A-debugging Debugging related proposals & ideas A-panic Panics related proposals & ideas Ergonomics Initiative Part of the ergonomics initiative final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.