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

librustc_span has a concurrency bug #69261

Closed
kdy1 opened this issue Feb 18, 2020 · 1 comment · Fixed by #69266
Closed

librustc_span has a concurrency bug #69261

kdy1 opened this issue Feb 18, 2020 · 1 comment · Fixed by #69266
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-parallel Working group: Parallelizing the compiler

Comments

@kdy1
Copy link
Contributor

kdy1 commented Feb 18, 2020

librustc_span contains a concurrency bug. Even though, rustc works without any problem.
(I guess rustc process does not use multiples threads to load file to sourcemap.)

fn try_new_source_file(
&self,
filename: FileName,
src: String,
) -> Result<Lrc<SourceFile>, OffsetOverflowError> {
let start_pos = self.next_start_pos();
// The path is used to determine the directory for loading submodules and
// include files, so it must be before remapping.
// Note that filename may not be a valid path, eg it may be `<anon>` etc,
// but this is okay because the directory determined by `path.pop()` will
// be empty, so the working directory will be used.
let unmapped_path = filename.clone();
let (filename, was_remapped) = match filename {
FileName::Real(filename) => {
let (filename, was_remapped) = self.path_mapping.map_prefix(filename);
(FileName::Real(filename), was_remapped)
}
other => (other, false),
};
let file_id =
StableSourceFileId::new_from_pieces(&filename, was_remapped, Some(&unmapped_path));
let lrc_sf = match self.source_file_by_stable_id(file_id) {
Some(lrc_sf) => lrc_sf,
None => {
let source_file = Lrc::new(SourceFile::new(
filename,
was_remapped,
unmapped_path,
src,
Pos::from_usize(start_pos),
)?);
let mut files = self.files.borrow_mut();
files.source_files.push(source_file.clone());
files.stable_id_to_source_file.insert(file_id, source_file.clone());
source_file
}
};
Ok(lrc_sf)
}

When multiple threads invoked this method at a time, it results in overlapping spans.
(Because the lock is released while analyzing the source file)

Meta

Simple fix: https://github.com/swc-project/swc/pull/672/files#diff-8d69cef1163ff9667c130830b15da62bL181-L188

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 18, 2020
@Mark-Simulacrum Mark-Simulacrum added the WG-compiler-parallel Working group: Parallelizing the compiler label Feb 18, 2020
@Mark-Simulacrum
Copy link
Member

cc @Zoxc

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 20, 2020
…iser

Fix race condition when allocating source files in SourceMap

This makes allocating address space in the source map an atomic operation. `rustc` does not currently do this in parallel, so this bug can't trigger, but parsing files in parallel could trigger it, and that is something we want to do.

Fixes rust-lang#69261.

r? @wesleywiser
@bors bors closed this as completed in 5d285dc Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-parallel Working group: Parallelizing the compiler
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants