From f049d5df10fc169d12ae825b5a5e78e47a0195bd Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 31 Mar 2023 15:06:08 +1100 Subject: [PATCH 1/2] Remove an unnecessary use of `with_session_globals`. We can easily pass in the source map. --- compiler/rustc_expand/src/proc_macro.rs | 10 +++++++--- compiler/rustc_span/src/profiling.rs | 16 +++++----------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_expand/src/proc_macro.rs b/compiler/rustc_expand/src/proc_macro.rs index ddba14417195b..26bc216f678cf 100644 --- a/compiler/rustc_expand/src/proc_macro.rs +++ b/compiler/rustc_expand/src/proc_macro.rs @@ -54,7 +54,7 @@ impl base::BangProcMacro for BangProcMacro { ) -> Result { let _timer = ecx.sess.prof.generic_activity_with_arg_recorder("expand_proc_macro", |recorder| { - recorder.record_arg_with_span(ecx.expansion_descr(), span); + recorder.record_arg_with_span(ecx.sess.source_map(), ecx.expansion_descr(), span); }); let proc_macro_backtrace = ecx.ecfg.proc_macro_backtrace; @@ -85,7 +85,7 @@ impl base::AttrProcMacro for AttrProcMacro { ) -> Result { let _timer = ecx.sess.prof.generic_activity_with_arg_recorder("expand_proc_macro", |recorder| { - recorder.record_arg_with_span(ecx.expansion_descr(), span); + recorder.record_arg_with_span(ecx.sess.source_map(), ecx.expansion_descr(), span); }); let proc_macro_backtrace = ecx.ecfg.proc_macro_backtrace; @@ -134,7 +134,11 @@ impl MultiItemModifier for DeriveProcMacro { let stream = { let _timer = ecx.sess.prof.generic_activity_with_arg_recorder("expand_proc_macro", |recorder| { - recorder.record_arg_with_span(ecx.expansion_descr(), span); + recorder.record_arg_with_span( + ecx.sess.source_map(), + ecx.expansion_descr(), + span, + ); }); let proc_macro_backtrace = ecx.ecfg.proc_macro_backtrace; let strategy = exec_strategy(ecx); diff --git a/compiler/rustc_span/src/profiling.rs b/compiler/rustc_span/src/profiling.rs index 0ab890b9f0121..66e5369da3ae1 100644 --- a/compiler/rustc_span/src/profiling.rs +++ b/compiler/rustc_span/src/profiling.rs @@ -1,3 +1,5 @@ +use crate::source_map::SourceMap; + use std::borrow::Borrow; use rustc_data_structures::profiling::EventArgRecorder; @@ -11,25 +13,17 @@ pub trait SpannedEventArgRecorder { /// /// Note: when self-profiling with costly event arguments, at least one argument /// needs to be recorded. A panic will be triggered if that doesn't happen. - fn record_arg_with_span(&mut self, event_arg: A, span: crate::Span) + fn record_arg_with_span(&mut self, source_map: &SourceMap, event_arg: A, span: crate::Span) where A: Borrow + Into; } impl SpannedEventArgRecorder for EventArgRecorder<'_> { - fn record_arg_with_span(&mut self, event_arg: A, span: crate::Span) + fn record_arg_with_span(&mut self, source_map: &SourceMap, event_arg: A, span: crate::Span) where A: Borrow + Into, { self.record_arg(event_arg); - - let span_arg = crate::with_session_globals(|session_globals| { - if let Some(source_map) = &*session_globals.source_map.borrow() { - source_map.span_to_embeddable_string(span) - } else { - format!("{span:?}") - } - }); - self.record_arg(span_arg); + self.record_arg(source_map.span_to_embeddable_string(span)); } } From 4e63ab6fc11ab942052c485ed121a606f0a7d319 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 31 Mar 2023 14:54:25 +1100 Subject: [PATCH 2/2] Improve `with_source_map`. Rename `with_source_map` as `set_source_map`. Because `with` functions (e.g. `with_session_globals`, `scoped_tls::ScopedKey::with`) are for *getting* a value for the duration of a closure, and `set` functions (e.g. `set_session_globals_then` `scoped_tls::ScopedKey::with`) are for *setting* a value for the duration of a closure. Also fix up the comment, which is wrong: - The bit about `TyCtxt` is wrong. - `span_debug1` doesn't exist any more. - There's only one level of fallback, not two. (This is effectively a follow-up to the changes in #93936.) Also add a comment explaining that `SessionGlobals::source_map` should only be used when absolutely necessary. --- compiler/rustc_interface/src/interface.rs | 2 +- compiler/rustc_span/src/lib.rs | 23 +++++++++++++---------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_interface/src/interface.rs b/compiler/rustc_interface/src/interface.rs index 5e38ca034ac1f..be7fa9378ca66 100644 --- a/compiler/rustc_interface/src/interface.rs +++ b/compiler/rustc_interface/src/interface.rs @@ -292,7 +292,7 @@ pub fn run_compiler(config: Config, f: impl FnOnce(&Compiler) -> R + Se override_queries: config.override_queries, }; - rustc_span::with_source_map(compiler.sess.parse_sess.clone_source_map(), move || { + rustc_span::set_source_map(compiler.sess.parse_sess.clone_source_map(), move || { let r = { let _sess_abort_error = OnDrop(|| { compiler.sess.finish_diagnostics(registry); diff --git a/compiler/rustc_span/src/lib.rs b/compiler/rustc_span/src/lib.rs index 02cffc762bed3..e14760aa01885 100644 --- a/compiler/rustc_span/src/lib.rs +++ b/compiler/rustc_span/src/lib.rs @@ -87,6 +87,14 @@ pub struct SessionGlobals { symbol_interner: symbol::Interner, span_interner: Lock, hygiene_data: Lock, + + /// A reference to the source map in the `Session`. It's an `Option` + /// because it can't be initialized until `Session` is created, which + /// happens after `SessionGlobals`. `set_source_map` does the + /// initialization. + /// + /// This field should only be used in places where the `Session` is truly + /// not available, such as `::fmt`. source_map: Lock>>, } @@ -1013,16 +1021,9 @@ impl Decodable for Span { } } -/// Calls the provided closure, using the provided `SourceMap` to format -/// any spans that are debug-printed during the closure's execution. -/// -/// Normally, the global `TyCtxt` is used to retrieve the `SourceMap` -/// (see `rustc_interface::callbacks::span_debug1`). However, some parts -/// of the compiler (e.g. `rustc_parse`) may debug-print `Span`s before -/// a `TyCtxt` is available. In this case, we fall back to -/// the `SourceMap` provided to this function. If that is not available, -/// we fall back to printing the raw `Span` field values. -pub fn with_source_map T>(source_map: Lrc, f: F) -> T { +/// Insert `source_map` into the session globals for the duration of the +/// closure's execution. +pub fn set_source_map T>(source_map: Lrc, f: F) -> T { with_session_globals(|session_globals| { *session_globals.source_map.borrow_mut() = Some(source_map); }); @@ -1041,6 +1042,8 @@ pub fn with_source_map T>(source_map: Lrc, f: F) -> impl fmt::Debug for Span { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // Use the global `SourceMap` to print the span. If that's not + // available, fall back to printing the raw values. with_session_globals(|session_globals| { if let Some(source_map) = &*session_globals.source_map.borrow() { write!(f, "{} ({:?})", source_map.span_to_diagnostic_string(*self), self.ctxt())