1360497 - Add -Zdebug-macros to RUSTFLAGS

Status: RESOLVED WONTFIX

(Firefox Build System :: General, enhancement)

Description

[Henri Sivonen (:hsivonen)]

By default, rustc annotates lines expanded from a macro with the location of the macro invocation rather than the location of the line in the macro definition. In case of e.g. encoding_rs, this results in pretty useless panic stacks.

To make panic stacks useful on try and in the crash reporter, we should add -Zdebug-macros to RUSTFLAGS to override the default behavior and to get stacks with usefully actionable location info.

Additionally, accoding to https://github.com/rust-lang/rust/issues/39153#issuecomment-297685269 , it seems that this would be useful for correct profiling attribution, too.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1360497 - Use line numbers from macro definition rather than macro invocation for code expanded from macros.

Review commit: https://reviewboard.mozilla.org/r/134660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134660/

Comment 2

[Henri Sivonen (:hsivonen)]

Hmm. The patch doesn't appear to have the expected effect:
https://treeherder.mozilla.org/logviewer.html#?job_id=95143210&repo=try&lineNumber=2243
(No lines from macros.rs involved.)

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #2)
> Hmm. The patch doesn't appear to have the expected effect:

Because I misread the "if" nesting in a hurry. Sigh.

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 8862777 [details]
Bug 1360497 - Use line numbers from macro definition rather than macro invocation for code expanded from macros.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134660/diff/1-2/

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 8862777 [details]
Bug 1360497 - Use line numbers from macro definition rather than macro invocation for code expanded from macros.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134660/diff/2-3/

Comment 6

[Henri Sivonen (:hsivonen)]

Not what I hoped for in this case, but at least the flag seems to take effect with the current patch:
https://treeherder.mozilla.org/logviewer.html#?job_id=95476817&repo=try&lineNumber=4658

Comment 7

[(not currently active) Ted Mielczarek]

It sounds like we have to make a choice between two suboptimal situations:
a) The current situation, where panic stacks/profiling are not very useful for code involving macros.
b) -Zdebug-macros, where anyone debugging Rust code will wind up stepping through macros from the standard library.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8862777 [details]
Bug 1360497 - Use line numbers from macro definition rather than macro invocation for code expanded from macros.

I'm going to let froynj make the call here.

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> b) -Zdebug-macros, where anyone debugging Rust code will wind up stepping
> through macros from the standard library.

Do we have a lot of code in Firefox where this is a problem? Does gdb's "skip file" not correctly skip over lines from a standard-library macro expansion if you tell it to skip over the file that defines the macro?

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #9)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> > b) -Zdebug-macros, where anyone debugging Rust code will wind up stepping
> > through macros from the standard library.
> 
> Do we have a lot of code in Firefox where this is a problem? Does gdb's
> "skip file" not correctly skip over lines from a standard-library macro
> expansion if you tell it to skip over the file that defines the macro?

FWIW, I think losing the line of assert macro invocation is a more significant adverse outcome of the proposed change than the impact on stepping over write!() and such when debugging.

Ideally, we'd get the macro invocation line for assert macros and the macro definition line for other macros and would use gdb skipping filters to decide at debug time what to skip when stepping.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8862777 [details]
Bug 1360497 - Use line numbers from macro definition rather than macro invocation for code expanded from macros.

https://reviewboard.mozilla.org/r/134660/#review145150

Sorry for taking so long to respond to this.

There is just no good solution here, and I'm not comfortable enabling something that we'll have to throw away shortly before we upgrade Rust, as it sounds like they're disabling `-Z` options in stable.  So we'd have to start with changing the command-line flags to provide a stable interface, and then add the appropriate flags (probably only enabled for `--enable-release` builds).  Permitting people to pass in their own `RUSTFLAGS` would be a reasonable change on its own, though, and then people who wanted `-Z debug-macros` or what have you could get the behavior they like locally.

Comment 12

[Henri Sivonen (:hsivonen)]

Marking WONTFIX per comment 11.

I started a discussion about a better alternative on the Rust Internals forum but the discussion didn't get much interest:
https://internals.rust-lang.org/t/stack-trace-friendly-debug-info-for-macros/5370