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: Debuggable macro expansions #2117

Closed

Conversation

hsivonen
Copy link
Member

Improves debuggability of, panic stacks for and profiling attribution of code expanded from non-assert-like macros.

By default, annotate code expanded from a macro with debug location info corresponding to the macro definition (i.e. the behavior that's currently available on nightly via -Zdebug-macros). Add an annotation #[collapse_debuginfo] to enable a particular macro definition to opt to have the expansion annotated with debug location info corresponding to the macro invocation (i.e. the current default behavior).

Rust issue, PR that created the current behavior, Internals forum post

Rendered

@withoutboats withoutboats added the T-compiler Relevant to the compiler team, which will review and decide on the RFC. label Aug 18, 2017
@michaelwoerister
Copy link
Member

Thanks a lot for the RFC, @hsivonen! I think this is a good idea as proposed.

Regarding the open questions:

  • I think -Zdebug-macros should go away, once this is stable.
  • I think one should not be able to step into code that was passed as an argument to a #[collapse_debuginfo] macro, just for consistency.

cc @vadimcn and @rust-lang/compiler (and maybe @rust-lang/lang and @rust-lang/dev-tools should take a quick look at this too).

@aturon
Copy link
Member

aturon commented Aug 22, 2017

For context, these issues are a problem for some of our production customers. It'd be good to get feedback from the compiler team soon.

@kennytm
Copy link
Member

kennytm commented Aug 23, 2017

I think the behavior of #[collapse_debuginfo] should be like this:

fn f() {              // 17
    {                 // 18
        one();        // 18
        {             // 18
            two();    // 18
        }             // 18
        {             // 18
            three();  // 19 <--- retain line number if the statement comes from macro invocation
            four();   // 20 <---
        }             // 18
    }                 // 18
}                     // 22

@nrc
Copy link
Member

nrc commented Aug 23, 2017

Related, for error messages, we apply a same-crate heuristic, where if the macro is in the same crate, we give details from inside the macro, and if not we treat it as opaque. If we add an attribute like #[collapse_debuginfo], should it also affect error messages (i.e., replace the same-crate heurictic)?

@vadimcn
Copy link
Contributor

vadimcn commented Aug 24, 2017

I think this proposal looks reasonable. Thanks for writing it up!

Re unsolved questions: if code blocks passed as macro argument are not affected by #[collapse_debuginfo], it might break "step-over" functionality in debuggers, so this scenario needs to be tested if you decide to go this route.

@aturon
Copy link
Member

aturon commented Sep 6, 2017

@michaelwoerister do you want to move toward FCP at this point, to land this ahead of the impl period?

@nrc
Copy link
Member

nrc commented Sep 19, 2017

We discussed this in dev-tools meeting today. We approve of the RFC in principal, but would like to experiment with an implementation. Given the impl period, we'll not do anything with this RFC for now, but consider us approving a hypothetical 'experimental RFC' so that we can experiment with an unstable implementation. We'll revisit this RFC once we have some experience with the implementation.

results for macros that are not assert-like and not print-like but that expand
to substantial code. The current approach of collapsing debug information by
default makes non-assert-like, non-print-like macro usage result in code that
is on debuggable, that doesn't get useful panic/crash stacks in CI or in the
Copy link

Choose a reason for hiding this comment

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

I think "on debuggable" should be changed "not debuggable" here, and "at that" should be changed to "and that" on the subsequent line.

@petrochenkov petrochenkov added T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. and removed T-dev-tools labels Jan 30, 2018
@nrc
Copy link
Member

nrc commented Feb 1, 2018

@michaelwoerister @tromey @hsivonen @vadimcn is anyone interested in doing an initial implementation of this? I think that is the next step here.

@gbutler69
Copy link

@hsivonen

While the distinction between assert-like and non-assert-like and the desired behavior is clear and tied to the nature of the macro and, therefore, it's OK to leave the decision on the debug info behavior to the macro definition site, the desired results for println-like macros are somewhat more dependent on the circumstances of the macro user, but this solution still leaves the decision to the crate that defines a macro as opposed to the crate invoking the macro.

Would it make sense to have an attribute that could annotate the call-site of the macro to select collapsed vs expanded debug info which overrides the setting made at the macro declaration site?

@hsivonen
Copy link
Member Author

hsivonen commented Jun 7, 2018

Would it make sense to have an attribute that could annotate the call-site of the macro to select collapsed vs expanded debug info which overrides the setting made at the macro declaration site?

For the use cases I've encountered so far, the choice has been something that'd belong better on the macro declaration than on the macro invocation.

I don't know if others have use cases where being able to override the choice from the macro invocation site would make sense, but I'm pretty confident that at least the primary choice belongs in the macro declaration.

@Centril Centril added A-macros Macro related proposals and issues A-debugging Debugging related proposals & ideas labels Nov 22, 2018
@Centril Centril added T-lang Relevant to the language team, which will review and decide on the RFC. I-nominated and removed I-nominated labels Jan 14, 2019
@Centril Centril self-assigned this Jan 24, 2019
@ngugc
Copy link

ngugc commented Jul 17, 2020

Hi, any update here? It has been a year and a half since last update.

@pnkfelix
Copy link
Member

Discussed at T-lang backlog bonanza.

Points of feedback:

  • This is a scenario where we agree that a real problem exists. We think the problem might be defined as "it should be easier to debug code that was expanded from macros." Our team would prefer to charter a group to look at the solution space for this problem, rather than trying to evaluate a specific RFC outlining one particular solution.

  • This seems like this could/should be a major change proposal (MCP) for the compiler team, rather than an RFC for the language team.

  • If a solution is reasonably narrow in scope, then it may be able to land without having to go through the bureaucratic overhead of the RFC process.

We recommend you file a compiler-team MCP, if you think you yourself have time to do the associated work associated with implementing the change or chartering a project group to do the prototyping described above. If the implementation effort requires the addition of small attributes or other small changes, those can be discussed by the T-lang design team concurrently with the ongoing implementation effort.

@nikomatsakis
Copy link
Contributor

Based on the above, I'm going to go ahead and close this RFC -- as @pnkfelix noted, I think the best next step is to work towards the implementation side.

@hsivonen
Copy link
Member Author

Filed an MCP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debugging Debugging related proposals & ideas A-macros Macro related proposals and issues T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. 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.