-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
std: avoid tearing dbg! prints
#149869
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
base: main
Are you sure you want to change the base?
std: avoid tearing dbg! prints
#149869
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ use clippy_utils::macros::{MacroCall, macro_backtrace}; | |
| use clippy_utils::source::snippet_with_applicability; | ||
| use rustc_data_structures::fx::FxHashSet; | ||
| use rustc_errors::Applicability; | ||
| use rustc_hir::{Closure, ClosureKind, CoroutineKind, Expr, ExprKind, LetStmt, LocalSource, Node, Stmt, StmtKind}; | ||
| use rustc_hir::{Arm, Closure, ClosureKind, CoroutineKind, Expr, ExprKind, LetStmt, LocalSource, Node, Stmt, StmtKind}; | ||
| use rustc_lint::{LateContext, LateLintPass, LintContext}; | ||
| use rustc_session::impl_lint_pass; | ||
| use rustc_span::{Span, SyntaxContext, sym}; | ||
|
|
@@ -90,33 +90,27 @@ impl LateLintPass<'_> for DbgMacro { | |
| (macro_call.span, String::from("()")) | ||
| } | ||
| }, | ||
| // dbg!(1) | ||
| ExprKind::Match(val, ..) => ( | ||
| macro_call.span, | ||
| snippet_with_applicability(cx, val.span.source_callsite(), "..", &mut applicability) | ||
| .to_string(), | ||
| ), | ||
| // dbg!(2, 3) | ||
| ExprKind::Tup( | ||
| [ | ||
| Expr { | ||
| kind: ExprKind::Match(first, ..), | ||
| .. | ||
| }, | ||
| .., | ||
| Expr { | ||
| kind: ExprKind::Match(last, ..), | ||
| .. | ||
| }, | ||
| ], | ||
| ) => { | ||
| let snippet = snippet_with_applicability( | ||
| cx, | ||
| first.span.source_callsite().to(last.span.source_callsite()), | ||
| "..", | ||
| &mut applicability, | ||
| ); | ||
| (macro_call.span, format!("({snippet})")) | ||
| ExprKind::Match(first, arms, _) => { | ||
| let vals = collect_vals(first, arms); | ||
| let suggestion = match vals.as_slice() { | ||
| // dbg!(1) => 1 | ||
| &[val] => { | ||
| snippet_with_applicability(cx, val.span.source_callsite(), "..", &mut applicability) | ||
| .to_string() | ||
| } | ||
| // dbg!(2, 3) => (2, 3) | ||
| &[first, .., last] => { | ||
| let snippet = snippet_with_applicability( | ||
| cx, | ||
| first.span.source_callsite().to(last.span.source_callsite()), | ||
| "..", | ||
| &mut applicability, | ||
| ); | ||
| format!("({snippet})") | ||
| } | ||
| _ => unreachable!(), | ||
| }; | ||
| (macro_call.span, suggestion) | ||
| }, | ||
| _ => unreachable!(), | ||
| }; | ||
|
|
@@ -169,3 +163,33 @@ fn is_async_move_desugar<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx | |
| fn first_dbg_macro_in_expansion(cx: &LateContext<'_>, span: Span) -> Option<MacroCall> { | ||
| macro_backtrace(span).find(|mc| cx.tcx.is_diagnostic_item(sym::dbg_macro, mc.def_id)) | ||
| } | ||
|
|
||
| /// Extracts all value expressions from the `match`-tree generated by `dbg!`. | ||
| /// | ||
| /// E.g. from | ||
| /// ```rust, ignore | ||
| /// match 1 { | ||
| /// tmp_1 => match 2 { | ||
| /// tmp_2 => { | ||
| /// /* printing */ | ||
| /// (tmp_1, tmp_2) | ||
| /// } | ||
| /// } | ||
| /// } | ||
| /// ``` | ||
| /// this extracts `1` and `2`. | ||
| fn collect_vals<'hir>(first: &'hir Expr<'hir>, mut arms: &'hir [Arm<'hir>]) -> Vec<&'hir Expr<'hir>> { | ||
| let mut vals = vec![first]; | ||
| loop { | ||
| let [arm] = arms else { unreachable!("dbg! macro expansion only has single-arm matches") }; | ||
|
|
||
| match is_async_move_desugar(arm.body).unwrap_or(arm.body).peel_drop_temps().kind { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure whether this is really necessary, I just copied this from above.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is needed here. You can probably use takes_async_fn(async move |val| { dbg!(val, val + 1); val });
//~^ dbg_macro |
||
| ExprKind::Block(..) => return vals, | ||
| ExprKind::Match(val, a, _) => { | ||
| vals.push(val); | ||
| arms = a; | ||
| } | ||
| _ => unreachable!("dbg! macro expansion only results in block or match expressions"), | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this actually help avoid tearing?
So we should expect this to lower to a series of writes to stderr, i.e., it will in fact tear if there's concurrency. We'd need to add a buffer somewhere to avoid that.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, we're taking a lock in fmt::Write for Stderr before calling write_fmt.
Should we avoid the matches and just make dbg! expand to
write!(std::io::stderr().lock(), ...)then? That seems simpler and achieves the same goal, right? I guess it'd need to inline the capturing (print_to_buffer_if_capture_used)...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mark-Simulacrum What would you propose would go in that
...? That's precisely what the recursivematches generate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today, we expand dbg!(a, b, c) to:
That causes tearing because eprintln! ends up calling
global_s().write_fmt(args), which is juststd::io::stderr().write_fmt(args), i.e., we take the stderr lock three times.It seems to me that we ought to be able to ensure unique lock ownership if we added a wrapping:
because the stderr lock is re-entrant, so it's fine to take it twice on the same thread (as this would be doing), we just need to prevent other threads from acquiring it while printing our dbg! parts.
I think this seems simpler than the PR's implementation -- at the very least it's less nesting, and presumably no change in diagnostics / error messages? It does mean we don't get this benefit for the libtest output capture, but that seems OK to me, and if we wanted it we could replace locking stderr with a dedicated lock_acquire() function of some kind.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in my original PR this approach leads to deadlocks and is unacceptable. The "we just need to prevent other threads from acquiring it while printing our dbg!" part is impossible. For example when you write
dbg!(f())thenf()may block until another thread does some task (e.g. could be as simple and innocent asdbg!("foo", chan.recv())) . And that other task may also calldbg!, at which point you've deadlocked.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's true, though that's already the case for the formatting itself (Debug impls using dbg! can lead to deadlocks today), right? In order to avoid it we'd need to serialize to a temporary buffer before writing to stderr (perhaps optionally -- if locking fails -- but still).
That also implies a change in behavior if we land this PR: today
dbg!("foo", chan.recv())prints foo and then waits, whereas with this PR we'd only print foo after both are complete, right? I'm not sure that's an improvement, i.e., it's not obvious that we should fix the issue. Do we have a T-libs-api decision somewhere on what the behavior of multi-expression dbg! should be with regards to concurrency?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if the
Debugimpl causesdbg!to be called on another thread, which is rather unusual, but in theory yes. But I don't think it's a fair comparison at all, the situation you describe is incredibly niche, whereas my example can be trivially caused if for example you userayonto parallelize some computation you're debugging.Yes. I honestly think the old behavior is more surprising than this new behavior, and trivial to fix if undesirable. On the other hand torn multi-expression
dbg!statements can be very confusing, and it is 'impossible' to fix (you can wrap the entire thing in a tuple but that also changes the output, or write your own macro of course which defeats the point of quick debugging).Not that I'm aware of. You can nominate the issue if you feel this change is controversial.