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

Code in block passed to macro has line number of macro invocation #39153

Closed
hsivonen opened this issue Jan 18, 2017 · 26 comments · Fixed by #120845
Closed

Code in block passed to macro has line number of macro invocation #39153

hsivonen opened this issue Jan 18, 2017 · 26 comments · Fixed by #120845
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hsivonen
Copy link
Member

It appears that (at least on Linux) if you pass a block of code to a macro, the line numbers for code in the block become the line of the start of the macro invocation rather than the actual lines within the block. This makes debugging hard.

Consider:

call_macro_defined_somewhere!({
  one();
  two();
  three();
});

Line reported for one(), two() and three() is the line that call_macro_defined_somewhere!({ is on. Expected line number for one() to be the line number for call_macro_defined_somewhere!({ plus one, etc.

@alexcrichton
Copy link
Member

cc @michaelwoerister

@alexcrichton alexcrichton added the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Jan 18, 2017
@michaelwoerister
Copy link
Member

At the moment, this is the intended behavior unfortunately. See #35238 for some discussion.

cc @vadimcn

@vadimcn
Copy link
Contributor

vadimcn commented Jan 18, 2017

@hsivonen: you can use -Zdebug-macros to turn this off.

@hsivonen
Copy link
Member Author

you can use -Zdebug-macros to turn this off.

This helps thank you.

It surprises me that the other issue isn't a solved problem on the gdb/DWARF level, considering that C macros allow adjacent statements to be sourced from different places, too.

@tromey
Copy link
Contributor

tromey commented Feb 2, 2017

It surprises me that the other issue isn't a solved problem on the gdb/DWARF level, considering that C macros allow adjacent statements to be sourced from different places, too.

The rust compiler can emit DWARF saying where each line comes from and gdb should do the right thing when stepping. The keyword to look for in the DWARF standard is DW_LNS_set_file. Also, FWIW, I think there is at least one other bug open about debuginfo and macros. #35238 talks about pretending that macros are the result of function inlining, but I don't understand why that would be necessary, or generally even desirable. I feel though that I am probably missing something.

@vadimcn
Copy link
Contributor

vadimcn commented Feb 2, 2017

@tromey: We wanted to be able to step over macros like println!(), which are far more common in Rust than they are in C/C++.

I found two ways of achieving that:

  • tag all code produced by a macro with the location of the macro expansion site (this is what is currently implemented). Unfortunately, this means that you cannot step through the macro guts even when that's desired. (Well, you can revert to the old behavior by compiling with -Zdebug-macros).
  • tag all expanded code with true source locations, but then pretend that it ended up at the expansion site as a result of inlining (which macros are a form of, actually). This would be the preferred solution, since you'd be able to step over or into as needed; however, this turned out to be impossible to express using LLVM debug metadata.

@tromey
Copy link
Contributor

tromey commented Feb 6, 2017

but then pretend that it ended up at the expansion site as a result of inlining (which macros are a form of, actually)

This is the part I don't think is desirable. Inlining implies that there is a new function scope, which isn't necessarily the case in reality. It seems like this could make debugging weird unless you take extra effort to inject variables from the function into this new scope.

I tend to think this is another area where some Rust DWARF extension would be preferable; though maybe there are other halfway solutions, like a different heuristic for what locations to emit (e.g., if the macro invocation contains significant amounts of user code, emit locations for that code from the expansion, so that stepping "works").

@vadimcn
Copy link
Contributor

vadimcn commented Feb 9, 2017

Inlining implies that there is a new function scope, which isn't necessarily the case in reality.

Yeah, that's basically the reason we didn't pursue this route.

I hope my PR #39678 will sufficiently address the issue with include!() and other macros like that.

@julian-seward1
Copy link

This is also a problem when profiling Rust code, at least with the
Valgrind/Callgrind/KCachegrind stack, since they (as a result of the dwarf
src loc info) wind up attributing all costs in the included file to the include
macro invokation itself, which is pretty much useless.

@michaelwoerister
Copy link
Member

@vadimcn Didn't you add a command line flag to control how macro source locations are handled?

@julian-seward1
Copy link

@michaelwoerister yes, -Zdebug-macros does fix it. It would be nicer if this
was the default setting, though. At least from a profiling point of view, where
seeing inside the macro is important.

@vadimcn
Copy link
Contributor

vadimcn commented Apr 27, 2017

@julian-seward1: You scenario is exactly why I added that flag. However, I fail to see why would we want to make it the default. The change was made for the sake of sane debugging experience. Very few people want to land in the middle of a macro definition in libstd (for which we don't even emit line numbers), when trying to step over a println!() in their source.

@hsivonen
Copy link
Member Author

I, too, think that -Zdebug-macros behavior should be the default. There are many non-macro cases where one wouldn't want to step into standard-library code. It seems to me that some kind of debugger-side ignore filter for the standard-library paths would be a better solution than the compiler obfuscating the location info.

@vadimcn
Copy link
Contributor

vadimcn commented Apr 28, 2017

I, too, think that -Zdebug-macros behavior should be the default.

@hsivonen: I can understand that this may be annoying when debugging macro-heavy code (I am guessing you are referring to this?), but you've gotta agree that such code is pretty uncommon.

There are many non-macro cases where one wouldn't want to step into standard-library code.

But non-macro cases never were a problem - step-over works just fine for plain functions.

It seems to me that some kind of debugger-side ignore filter for the standard-library paths would be a better solution.

That would be debugger-dependent and everybody would have to take adjust their config to set up such exclusions. I am not even sure it would work. Debuggers typically allow one to exclude functions from being stepped in, however in this case, all code belongs to the function where the macro was expanded.

All things considered, I believe that the current default is correct.

That's not to say we should not try to improve. Can we identify more cases when we can be certain that stepping-in is the desired behavior, like this one? Perhaps an attribute can be added that allows to opt out of debug location override? I'll be happy to hear about any ideas that do not involve flipping the global default.

PS: Or, maybe, someone wants to design a better debug info support for macros and pitch it to the DWARF Standards Committee?

@hsivonen
Copy link
Member Author

(I am guessing you are referring to this?)

Yes.

I filed a bug to get -Zdebug-macros turned on in the Firefox build system.

I'll be happy to hear about any ideas that do not involve flipping the global default.

Would giving the current default treatment only to macros from the standard library meet your goals?

@whitequark
Copy link
Member

This would be the preferred solution, since you'd be able to step over or into as needed; however, this turned out to be impossible to express using LLVM debug metadata.

There's one option I don't see anyone explore in this thread. Namely, why not just fix LLVM? LLVM Debug info is unstable enough to make this change at this point.

@hsivonen
Copy link
Member Author

Would giving the current default treatment only to macros from the standard library meet your goals?

It seems that -Zdebug-macros has adverse effects on the line info for assert!, panic! and unreachable! macros in stack traces, so, indeed, it seems that there isn't one approach that's the best for all macros or even all macros in a given program. (That is, I now wish I could get the default behavior for assert!, panic! and unreachable! but the -Zdebug-macros behavior for other macros.)

@hsivonen
Copy link
Member Author

hsivonen commented Jul 3, 2017

Per f2f discussion with @michaelwoerister last week, I propose this be fixed like I suggested on the internals forum:

  • Add an annotation that can be applied to a macro declaration to request that the macro be given what's currently the default behavior: the location info for code expanded from the macro is the location of the outermost macro invocation with this annotation applied to it.
  • Annotate the standard-library assertion-like macros with this annotation, because for assert-like things you want to see the assertion invocation site in the stack trace.
  • Annotate the standard-library print-like macros with this annotation, because people like these to be stepped over when debugging.
  • Give macros that aren't annotated with this new annotation the behavior of -Zdebug-macros, i.e. make the location info come the macro definition rather than the macro invocation.

Use cases:

  • Useful stack traces in Firefox CI (currently one could do a try push with -Zdebug-macros but then assertion-like macros don't have useful location info).
  • Useful stack traces in the field from Firefox crash reporter when there's (non-assertion-like) code expanded from macros on the stack.
  • Useful profiling attribution for code expanded from macros when running Gecko Profiler on a vanilla Firefox build (i.e. Nightly as shipped and not a custom build with -Zdebug-macros).

@michaelwoerister
Copy link
Member

How do the @rust-lang/lang and @rust-lang/compiler teams feel about @hsivonen's proposal? I imagine it to looks something like:

#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
#[collapse_debuginfo] //                             <- this is new
macro_rules! assert {
    ($cond:expr) => ( /* ... */ );
    ($cond:expr, $($arg:tt)+) => ( /* ... */ );
}

Would we need an RFC for this? It wouldn't have to be stabilized immediately, since the most important macros in questions are all in the standard library.

@vadimcn
Copy link
Contributor

vadimcn commented Jul 6, 2017

Would we need an RFC for this?

I think we should. Would be nice to also have an implementation too, so we could compare debugging experience with and without it.

@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 26, 2017
@hsivonen
Copy link
Member Author

I'll write an RFC.

@wesleywiser
Copy link
Member

Visited during wg-debugging triage. We don't have any concrete next steps at this time but an idea was floated during the meeting that inferring which kind of line annotation a macro should use might be possible based on the kinds of tokens the macro captures. For example, macros that only capture idents/literals would have the current behavior and macros that capture token trees would use the proposed behavior by default. We need to survey macro uses in the ecosystem to see if this (or something like it) might make sense.

@eddyb
Copy link
Member

eddyb commented Jul 18, 2022

Relatedly, whether to treat the macro as opaque has come up in:

(And macros like file!/line! would likely also want a consistent behavior with that)

I don't have a strong opinion, but I agree it's an issue and it would be good to have a common solution.

@davidtwco
Copy link
Member

I did a little survey of the macros provided by the standard library/compiler to check @wesleywiser's hypothesis that we could do something intelligent here based on what token kinds are used by a macro:

Macro Token kinds Desirable behaviour (collapsed/not collapsed)
concat_bytes! literal Collapsed
concat_idents! ident Collapsed
concat_format_args! expr / expr + tt Collapsed
format_args_nl! expr / expr + tt Collapsed
log_syntax! tt Collapsed
trace_macros! N/A Collapsed
assert! expr / expr + tt Collapsed
assert_eq! expr + expr / expr + expr + tt Collapsed
assert_ne! expr + expr / expr + expr + tt Collapsed
cfg! tt Collapsed
column! N/A Collapsed
compile_error! expr Collapsed
concat! expr Collapsed
dbg! expr Collapsed
debug_assert! tt Collapsed
debug_assert_eq! tt Collapsed
debug_assert_ne! tt Collapsed
env! expr / expr + expr Collapsed
eprint! tt Collapsed
eprintln! tt Collapsed
file! N/A Collapsed
format! tt Collapsed
format_args! expr / expr + tt Collapsed
include! expr Collapsed
include_bytes! expr Collapsed
include_str! expr Collapsed
is_x86_feature_detected! tt Collapsed
line! N/A Collapsed
matches! expr + pat_param + expr Collapsed
module_path! N/A Collapsed
option_env! expr Collapsed
panic! tt Collapsed
print! tt Collapsed
println! tt Collapsed
stringify! tt Collapsed
thread_local! Various Collapsed
todo! tt Collapsed
try! expr Collapsed
unimplemented! tt Collapsed
unreachable! tt Collapsed
vec! expr + expr / expr Collapsed
write! expr + tt Collapsed
writeln! expr / expr + tt Collapsed

Overwhelmingly, these macros benefit from debuginfo being collapsed. However, it's clear that std's macros aren't entirely representative of macro_rules! use in general - just searching through std for uses of macro_rules! internally (mostly in tests and benchmarks) turns up macros which are used to deduplicate fragments of code, where uncollapsed debuginfo would be best.

I think the original suggestion from this issue, a #[collapse_debuginfo] attribute, would be the best way forward here - so I've re-opened and seconded rust-lang/compiler-team#386 and implemented that in #99556.

@eddyb
Copy link
Member

eddyb commented Jul 21, 2022

Do we have any debuginfo attributes at all today? I'm worried about the ad-hoc nature of #[collapse_debuginfo].

As per #39153 (comment), I'm wondering if tackling this from the #[track_caller]/Location::caller() side of things might result in a better interface for e.g. macro authors, and then debuginfo just follows from there.

At the very least, I don't think it would be a good idea to consider stabilizing something here (whatever the timeline for that may be, but still) without being certain whether desired behaviors for #[track_caller] and debuginfo align or not.


There's also the matter of always having full debugger awareness of macro expansion layers (like rustc diagnostics with -Z macro-backtrace), but I don't see it discussed since #39153 (comment) and I'm guessing there's practical reasons why it's not being pursued? (it's not super clear from the comments in this thread what exactly is wrong there, my best guess is existing debugger behavior isn't very friendly to such an approach?)


Since I mentioned -Z macro-backtrace, we should probably also make that the default in pretty much any case where the macro author doesn't want to hide it from the macro user (whichever heuristic we may end up using for that) - it's a shame we have all that functionality implemented and keep it locked away behind a flag.

@vadimcn
Copy link
Contributor

vadimcn commented Jul 21, 2022

There's also the matter of always having full debugger awareness of macro expansion layers ...

Technically this is possible, but hard to accomplish in practice. One would need to:

  1. Come up with a novel representation for macro expansions for both debug info formats (DWARF and PDB)
    1.1 Convince DWARF Committee and Microsoft to accept it into their standards.
  2. Modify Rust's private LLVM to emit this information.
    2.1 Upstream it to mainline LLVM.
  3. Modify all debuggers out there (well, at least GDB, LLDB and WinDbg) to interpret this information.

(2.1) and (3) might get pushback without (1.1)
The whole procedure is likely to take years.

IMO, a more practical approach would be emitting debug information that is equivalent to an inlined lambda function that captures all variables in the surrounding scope. Might need to extend LLVM's DIBuilder API to support this scenario, but otherwise should be feasible to do.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 13, 2022
…ywiser

ssa: implement `#[collapse_debuginfo]`

cc rust-lang#39153 rust-lang/compiler-team#386

Debuginfo line information for macro invocations are collapsed by default - line information are replaced by the line of the outermost expansion site. Using `-Zdebug-macros` disables this behaviour.

When the `collapse_debuginfo` feature is enabled, the default behaviour is reversed so that debuginfo is not collapsed by default. In addition, the `#[collapse_debuginfo]` attribute is available and can be applied to macro definitions which will then have their line information collapsed.

r? rust-lang/wg-debugging
bjorn3 pushed a commit to bjorn3/rust that referenced this issue Oct 23, 2022
…ywiser

ssa: implement `#[collapse_debuginfo]`

cc rust-lang#39153 rust-lang/compiler-team#386

Debuginfo line information for macro invocations are collapsed by default - line information are replaced by the line of the outermost expansion site. Using `-Zdebug-macros` disables this behaviour.

When the `collapse_debuginfo` feature is enabled, the default behaviour is reversed so that debuginfo is not collapsed by default. In addition, the `#[collapse_debuginfo]` attribute is available and can be applied to macro definitions which will then have their line information collapsed.

r? rust-lang/wg-debugging
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 12, 2024
debuginfo: Stabilize `-Z debug-macros`, `-Z collapse-macro-debuginfo` and `#[collapse_debuginfo]`

`-Z debug-macros` is "stabilized" by enabling it by default and removing.

`-Z collapse-macro-debuginfo` is stabilized as `-C collapse-macro-debuginfo`.
It now supports all typical boolean values (`parse_opt_bool`) in addition to just yes/no.

Default value of `collapse_debuginfo` was changed from `false` to `external` (i.e. collapsed if external, not collapsed if local) - rust-lang#100758 (comment) describes some debugging scenarios that motivate this default as reasonable.
`#[collapse_debuginfo]` attribute without a value is no longer supported to avoid guessing the default.

Closes rust-lang#100758
Closes rust-lang#41743
Closes rust-lang#39153

TODO: Stabilization report
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 25, 2024
debuginfo: Stabilize `-Z debug-macros`, `-Z collapse-macro-debuginfo` and `#[collapse_debuginfo]`

`-Z debug-macros` is "stabilized" by enabling it by default and removing.

`-Z collapse-macro-debuginfo` is stabilized as `-C collapse-macro-debuginfo`.
It now supports all typical boolean values (`parse_opt_bool`) in addition to just yes/no.

Default value of `collapse_debuginfo` was changed from `false` to `external` (i.e. collapsed if external, not collapsed if local) - rust-lang#100758 (comment) describes some debugging scenarios that motivate this default as reasonable.
`#[collapse_debuginfo]` attribute without a value is no longer supported to avoid guessing the default.

Stabilization report: rust-lang#120845 (comment)

Closes rust-lang#100758
Closes rust-lang#41743
Closes rust-lang#39153
@bors bors closed this as completed in 6acb9e7 Apr 26, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Apr 26, 2024
debuginfo: Stabilize `-Z debug-macros`, `-Z collapse-macro-debuginfo` and `#[collapse_debuginfo]`

`-Z debug-macros` is "stabilized" by enabling it by default and removing.

`-Z collapse-macro-debuginfo` is stabilized as `-C collapse-macro-debuginfo`.
It now supports all typical boolean values (`parse_opt_bool`) in addition to just yes/no.

Default value of `collapse_debuginfo` was changed from `false` to `external` (i.e. collapsed if external, not collapsed if local) - rust-lang/rust#100758 (comment) describes some debugging scenarios that motivate this default as reasonable.
`#[collapse_debuginfo]` attribute without a value is no longer supported to avoid guessing the default.

Stabilization report: rust-lang/rust#120845 (comment)

Closes rust-lang/rust#100758
Closes rust-lang/rust#41743
Closes rust-lang/rust#39153
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet