Skip to content

Change clippy's symbol interning strategy to be reentrant #60907

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ __pycache__/
.project
.settings/
.valgrindrc
.vscode/
.vscode
.favorites.json
/*-*-*-*/
/*-*-*/
Expand Down
32 changes: 18 additions & 14 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1852,8 +1852,11 @@ pub fn rustc_optgroups() -> Vec<RustcOptGroup> {
}

// Convert strings provided as --cfg [cfgspec] into a crate_cfg
pub fn parse_cfgspecs(cfgspecs: Vec<String>) -> FxHashSet<(String, Option<String>)> {
syntax::with_globals(move || {
pub fn parse_cfgspecs(
driver_symbols: &[&str],
cfgspecs: Vec<String>,
) -> FxHashSet<(String, Option<String>)> {
syntax::with_globals(driver_symbols, move || {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this one needs the extra symbols.
It's a "leaf" function that shouldn't delegate to any tool-specific functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not need any symbols at all I believe. Would you mind if I did that change in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

It needs symbols because it parses paths and therefore needs to use at least self/super/etc.
I guess it's ok to leave this as is because I'm not entirely sure that parsing cannot be intercepted by a tool now or in the future.

let cfg = cfgspecs.into_iter().map(|s| {
let sess = parse::ParseSess::new(FilePathMapping::empty());
let filename = FileName::cfg_spec_source_code(&s);
Expand Down Expand Up @@ -1918,6 +1921,7 @@ pub fn get_cmd_lint_options(matches: &getopts::Matches,
}

pub fn build_session_options_and_crate_config(
driver_symbols: &[&str],
matches: &getopts::Matches,
) -> (Options, FxHashSet<(String, Option<String>)>) {
let color = match matches.opt_str("color").as_ref().map(|s| &s[..]) {
Expand Down Expand Up @@ -2296,7 +2300,7 @@ pub fn build_session_options_and_crate_config(
})
.collect();

let cfg = parse_cfgspecs(matches.opt_strs("cfg"));
let cfg = parse_cfgspecs(driver_symbols, matches.opt_strs("cfg"));
let test = matches.opt_present("test");

let is_unstable_enabled = nightly_options::is_unstable_enabled(matches);
Expand Down Expand Up @@ -2735,13 +2739,13 @@ mod tests {
// When the user supplies --test we should implicitly supply --cfg test
#[test]
fn test_switch_implies_cfg_test() {
syntax::with_globals(|| {
syntax::with_globals(&[], || {
let matches = &match optgroups().parse(&["--test".to_string()]) {
Ok(m) => m,
Err(f) => panic!("test_switch_implies_cfg_test: {}", f),
};
let registry = errors::registry::Registry::new(&[]);
let (sessopts, cfg) = build_session_options_and_crate_config(matches);
let (sessopts, cfg) = build_session_options_and_crate_config(&[], matches);
let sess = build_session(sessopts, None, registry);
let cfg = build_configuration(&sess, to_crate_config(cfg));
assert!(cfg.contains(&(Symbol::intern("test"), None)));
Expand All @@ -2753,14 +2757,14 @@ mod tests {
#[test]
fn test_switch_implies_cfg_test_unless_cfg_test() {
use syntax::symbol::sym;
syntax::with_globals(|| {
syntax::with_globals(&[], || {
let matches = &match optgroups().parse(&["--test".to_string(),
"--cfg=test".to_string()]) {
Ok(m) => m,
Err(f) => panic!("test_switch_implies_cfg_test_unless_cfg_test: {}", f),
};
let registry = errors::registry::Registry::new(&[]);
let (sessopts, cfg) = build_session_options_and_crate_config(matches);
let (sessopts, cfg) = build_session_options_and_crate_config(&[], matches);
let sess = build_session(sessopts, None, registry);
let cfg = build_configuration(&sess, to_crate_config(cfg));
let mut test_items = cfg.iter().filter(|&&(name, _)| name == sym::test);
Expand All @@ -2771,28 +2775,28 @@ mod tests {

#[test]
fn test_can_print_warnings() {
syntax::with_globals(|| {
syntax::with_globals(&[], || {
let matches = optgroups().parse(&["-Awarnings".to_string()]).unwrap();
let registry = errors::registry::Registry::new(&[]);
let (sessopts, _) = build_session_options_and_crate_config(&matches);
let (sessopts, _) = build_session_options_and_crate_config(&[], &matches);
let sess = build_session(sessopts, None, registry);
assert!(!sess.diagnostic().flags.can_emit_warnings);
});

syntax::with_globals(|| {
syntax::with_globals(&[], || {
let matches = optgroups()
.parse(&["-Awarnings".to_string(), "-Dwarnings".to_string()])
.unwrap();
let registry = errors::registry::Registry::new(&[]);
let (sessopts, _) = build_session_options_and_crate_config(&matches);
let (sessopts, _) = build_session_options_and_crate_config(&[], &matches);
let sess = build_session(sessopts, None, registry);
assert!(sess.diagnostic().flags.can_emit_warnings);
});

syntax::with_globals(|| {
syntax::with_globals(&[], || {
let matches = optgroups().parse(&["-Adead_code".to_string()]).unwrap();
let registry = errors::registry::Registry::new(&[]);
let (sessopts, _) = build_session_options_and_crate_config(&matches);
let (sessopts, _) = build_session_options_and_crate_config(&[], &matches);
let sess = build_session(sessopts, None, registry);
assert!(sess.diagnostic().flags.can_emit_warnings);
});
Expand Down Expand Up @@ -3390,7 +3394,7 @@ mod tests {
let matches = optgroups()
.parse(&["--edition=2018".to_string()])
.unwrap();
let (sessopts, _) = build_session_options_and_crate_config(&matches);
let (sessopts, _) = build_session_options_and_crate_config(&[], &matches);
assert!(sessopts.edition == Edition::Edition2018)
}
}
7 changes: 5 additions & 2 deletions src/librustc_driver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ impl Callbacks for DefaultCallbacks {}
// The FileLoader provides a way to load files from sources other than the file system.
pub fn run_compiler(
args: &[String],
driver_symbols: &'static [&'static str],
callbacks: &mut (dyn Callbacks + Send),
file_loader: Option<Box<dyn FileLoader + Send + Sync>>,
emitter: Option<Box<dyn Write + Send>>
Expand All @@ -135,7 +136,7 @@ pub fn run_compiler(

install_panic_hook();

let (sopts, cfg) = config::build_session_options_and_crate_config(&matches);
let (sopts, cfg) = config::build_session_options_and_crate_config(driver_symbols, &matches);

let mut dummy_config = |sopts, cfg, diagnostic_output| {
let mut config = interface::Config {
Expand All @@ -149,6 +150,7 @@ pub fn run_compiler(
diagnostic_output,
stderr: None,
crate_name: None,
driver_symbols,
lint_caps: Default::default(),
};
callbacks.config(&mut config);
Expand Down Expand Up @@ -222,6 +224,7 @@ pub fn run_compiler(
diagnostic_output,
stderr: None,
crate_name: None,
driver_symbols,
lint_caps: Default::default(),
};

Expand Down Expand Up @@ -1176,7 +1179,7 @@ pub fn main() {
&format!("Argument {} is not valid Unicode: {:?}", i, arg))
}))
.collect::<Vec<_>>();
run_compiler(&args, &mut DefaultCallbacks, None, None)
run_compiler(&args, &[], &mut DefaultCallbacks, None, None)
}).and_then(|result| result);
process::exit(match result {
Ok(_) => EXIT_SUCCESS,
Expand Down
7 changes: 5 additions & 2 deletions src/librustc_interface/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ pub struct Config {
pub file_loader: Option<Box<dyn FileLoader + Send + Sync>>,
pub diagnostic_output: DiagnosticOutput,

pub driver_symbols: &'static [&'static str],

/// Set to capture stderr output during compiler execution
pub stderr: Option<Arc<Mutex<Vec<u8>>>>,

Expand Down Expand Up @@ -136,15 +138,16 @@ where
let stderr = config.stderr.take();
util::spawn_thread_pool(
config.opts.debugging_opts.threads,
config.driver_symbols,
&stderr,
|| run_compiler_in_existing_thread_pool(config, f),
)
}

pub fn default_thread_pool<F, R>(f: F) -> R
pub fn default_thread_pool<F, R>(driver_symbols: &[&str], f: F) -> R
where
F: FnOnce() -> R + Send,
R: Send,
{
util::spawn_thread_pool(None, &None, f)
util::spawn_thread_pool(None, driver_symbols, &None, f)
}
6 changes: 4 additions & 2 deletions src/librustc_interface/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ pub fn scoped_thread<F: FnOnce() -> R + Send, R: Send>(cfg: thread::Builder, f:
#[cfg(not(parallel_compiler))]
pub fn spawn_thread_pool<F: FnOnce() -> R + Send, R: Send>(
_threads: Option<usize>,
driver_symbols: &[&str],
stderr: &Option<Arc<Mutex<Vec<u8>>>>,
f: F,
) -> R {
Expand All @@ -178,7 +179,7 @@ pub fn spawn_thread_pool<F: FnOnce() -> R + Send, R: Send>(
}

scoped_thread(cfg, || {
syntax::with_globals( || {
syntax::with_globals(driver_symbols, || {
ty::tls::GCX_PTR.set(&Lock::new(0), || {
if let Some(stderr) = stderr {
io::set_panic(Some(box Sink(stderr.clone())));
Expand All @@ -192,6 +193,7 @@ pub fn spawn_thread_pool<F: FnOnce() -> R + Send, R: Send>(
#[cfg(parallel_compiler)]
pub fn spawn_thread_pool<F: FnOnce() -> R + Send, R: Send>(
threads: Option<usize>,
driver_symbols: &[&str],
stderr: &Option<Arc<Mutex<Vec<u8>>>>,
f: F,
) -> R {
Expand All @@ -213,7 +215,7 @@ pub fn spawn_thread_pool<F: FnOnce() -> R + Send, R: Send>(

let with_pool = move |pool: &ThreadPool| pool.install(move || f());

syntax::with_globals(|| {
syntax::with_globals(driver_symbols, || {
syntax::GLOBALS.with(|syntax_globals| {
syntax_pos::GLOBALS.with(|syntax_pos_globals| {
// The main handler runs for each Rayon worker thread and sets up
Expand Down
12 changes: 8 additions & 4 deletions src/librustc_macros/src/symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,14 @@ pub fn symbols(input: TokenStream) -> TokenStream {
}

impl Interner {
pub fn fresh() -> Self {
Interner::prefill(&[
#prefill_stream
])
/// If your driver adds more symbols, this is the first index you may use.
/// Do not leave holes in the indexing scheme and add all symbols to the `Interner`
/// created in the closure argument to `Interner::fresh`.
pub const FIRST_DRIVER_INDEX: u32 = #counter;
/// It is the driver's responsibility to not preeintern any symbols that are already
/// in the list given to the closure.
pub fn fresh(driver_symbols: &[&str]) -> Self {
Interner::prefill(&[#prefill_stream], driver_symbols)
}
}
});
Expand Down
14 changes: 7 additions & 7 deletions src/librustdoc/clean/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ mod test {

#[test]
fn test_cfg_not() {
with_globals(|| {
with_globals(&[], || {
assert_eq!(!Cfg::False, Cfg::True);
assert_eq!(!Cfg::True, Cfg::False);
assert_eq!(!word_cfg("test"), Cfg::Not(Box::new(word_cfg("test"))));
Expand All @@ -484,7 +484,7 @@ mod test {

#[test]
fn test_cfg_and() {
with_globals(|| {
with_globals(&[], || {
let mut x = Cfg::False;
x &= Cfg::True;
assert_eq!(x, Cfg::False);
Expand Down Expand Up @@ -536,7 +536,7 @@ mod test {

#[test]
fn test_cfg_or() {
with_globals(|| {
with_globals(&[], || {
let mut x = Cfg::True;
x |= Cfg::False;
assert_eq!(x, Cfg::True);
Expand Down Expand Up @@ -588,7 +588,7 @@ mod test {

#[test]
fn test_parse_ok() {
with_globals(|| {
with_globals(&[], || {
let mi = dummy_meta_item_word("all");
assert_eq!(Cfg::parse(&mi), Ok(word_cfg("all")));

Expand Down Expand Up @@ -622,7 +622,7 @@ mod test {

#[test]
fn test_parse_err() {
with_globals(|| {
with_globals(&[], || {
let mi = attr::mk_name_value_item(
DUMMY_SP,
Ident::from_str("foo"),
Expand Down Expand Up @@ -661,7 +661,7 @@ mod test {

#[test]
fn test_render_short_html() {
with_globals(|| {
with_globals(&[], || {
assert_eq!(
word_cfg("unix").render_short_html(),
"Unix"
Expand Down Expand Up @@ -741,7 +741,7 @@ mod test {

#[test]
fn test_render_long_html() {
with_globals(|| {
with_globals(&[], || {
assert_eq!(
word_cfg("unix").render_long_html(),
"This is supported on <strong>Unix</strong> only."
Expand Down
3 changes: 2 additions & 1 deletion src/librustdoc/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,8 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt

let config = interface::Config {
opts: sessopts,
crate_cfg: config::parse_cfgspecs(cfgs),
crate_cfg: config::parse_cfgspecs(&[], cfgs),
driver_symbols: &[],
input,
input_path: cpath,
output_file: None,
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ pub fn main() {
rustc_driver::set_sigpipe_handler();
env_logger::init();
let res = std::thread::Builder::new().stack_size(thread_stack_size).spawn(move || {
rustc_interface::interface::default_thread_pool(move || {
rustc_interface::interface::default_thread_pool(&[], move || {
get_args().map(|args| main_args(&args)).unwrap_or(1)
})
}).unwrap().join().unwrap_or(rustc_driver::EXIT_FAILURE);
Expand Down
10 changes: 7 additions & 3 deletions src/librustdoc/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ pub fn run(options: Options) -> i32 {

let config = interface::Config {
opts: sessopts,
crate_cfg: config::parse_cfgspecs(options.cfgs.clone()),
crate_cfg: config::parse_cfgspecs(&[], options.cfgs.clone()),
driver_symbols: &[],
input,
input_path: None,
output_file: None,
Expand Down Expand Up @@ -279,7 +280,8 @@ fn run_test(test: &str, cratename: &str, filename: &FileName, line: usize,

let config = interface::Config {
opts: sessopts,
crate_cfg: config::parse_cfgspecs(cfgs),
crate_cfg: config::parse_cfgspecs(&[], cfgs),
driver_symbols: &[],
input,
input_path: None,
output_file: Some(output_file.clone()),
Expand Down Expand Up @@ -385,7 +387,9 @@ pub fn make_test(s: &str,

// Uses libsyntax to parse the doctest and find if there's a main fn and the extern
// crate already is included.
let (already_has_main, already_has_extern_crate, found_macro) = crate::syntax::with_globals(|| {
let (
already_has_main, already_has_extern_crate, found_macro,
) = crate::syntax::with_globals(&[],|| {
use crate::syntax::{parse::{self, ParseSess}, source_map::FilePathMapping};
use errors::emitter::EmitterWriter;
use errors::Handler;
Expand Down
10 changes: 6 additions & 4 deletions src/libsyntax/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,23 @@ pub struct Globals {
}

impl Globals {
fn new() -> Globals {
/// See documentation on `syntax_pos::Globals` for information on the slice.
/// It is solely forwareded to `syntax_pos::Globals::new`
fn new(driver_symbols: &[&str]) -> Globals {
Globals {
// We have no idea how many attributes their will be, so just
// initiate the vectors with 0 bits. We'll grow them as necessary.
used_attrs: Lock::new(GrowableBitSet::new_empty()),
known_attrs: Lock::new(GrowableBitSet::new_empty()),
syntax_pos_globals: syntax_pos::Globals::new(),
syntax_pos_globals: syntax_pos::Globals::new(driver_symbols),
}
}
}

pub fn with_globals<F, R>(f: F) -> R
pub fn with_globals<F, R>(driver_symbols: &[&str], f: F) -> R
Copy link
Contributor

Choose a reason for hiding this comment

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

with_globals could probably keep its interface and delegate to a new with_globals_xxx function taking &[&str].

Too bad Rust still doesn't have default fn parameters to write fn foo(driver_symbols: &[&str] = &[]).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering that, but it seemed too error prone (since it's too easy to forget to do somewhere).

where F: FnOnce() -> R
{
let globals = Globals::new();
let globals = Globals::new(driver_symbols);
GLOBALS.set(&globals, || {
syntax_pos::GLOBALS.set(&globals.syntax_pos_globals, f)
})
Expand Down
4 changes: 2 additions & 2 deletions src/libsyntax/mut_visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1343,7 +1343,7 @@ mod tests {

// make sure idents get transformed everywhere
#[test] fn ident_transformation () {
with_globals(|| {
with_globals(&[], || {
let mut zz_visitor = ToZzIdentMutVisitor;
let mut krate = string_to_crate(
"#[a] mod b {fn c (d : e, f : g) {h!(i,j,k);l;m}}".to_string());
Expand All @@ -1358,7 +1358,7 @@ mod tests {

// even inside macro defs....
#[test] fn ident_transformation_in_defs () {
with_globals(|| {
with_globals(&[], || {
let mut zz_visitor = ToZzIdentMutVisitor;
let mut krate = string_to_crate(
"macro_rules! a {(b $c:expr $(d $e:token)f+ => \
Expand Down
Loading