From 37d90d94f3e9ac45f2dca7de7f6e2d906e6b91a4 Mon Sep 17 00:00:00 2001 From: Griffin Berlstein Date: Tue, 17 Dec 2024 17:55:04 +0000 Subject: [PATCH] [Calyx-utils] Overhaul `GlobalPositionTable` + a bunch of CI nonsense (#2377) A quick touch up of the `GlobalPositionTable` to avoid UB without getting too fancy. Main changes are use of `boxcar::Vec` instead of the standard library Vec, so that we can now have lock-free concurrent appends and accesses. This fits our use-case since we never de-allocate files & positions and frees us from all the issues associated with using locks. Since we're not using locks, we also don't need any unsafe to extend lifetimes, and since `boxcar::Vec` doesn't reallocate stored elements, we don't need to box the stored stuff. I also use `LazyLock` instead of `lazy_static!` which is easier to deal with. The end result is a global position table that can be accessed through associated functions on the `GlobalPositionTable` type, rather than methods on an instance. I also changed the `String`s stored by `File` to `Box`, because we don't ever mutate them. --- In the process of this, I ran face first in CI issues. Turns out that because some of the tests run in the docker container, they aren't actually building using the version of rust in `rust-toolchain.toml` and instead were using the version of rust from the container (1.76). This broke things since `LazyLock` was only stabilized in 1.80. I had a real fight with the CI because it turns out that using the standard actions in the docker container is not a straightforward as one would like. I'll skip a recount of the entire ordeal---peruse the commits to witness my pain---but I resolved things and made these tests consistent with the rest of CI by specifically pulling the toolchain file before running the rust install action. The end result is that even the docker container tests do respect the indicated rust version, and we should be able to bump the version of rust used by _EVERYTHING_ (if this isn't true I may scream) by updating the file. --- .github/workflows/rust.yml | 34 ++++++-- Cargo.lock | 8 +- Cargo.toml | 2 +- calyx-frontend/src/parser.rs | 36 ++++---- calyx-utils/Cargo.toml | 2 +- calyx-utils/src/errors.rs | 2 +- calyx-utils/src/lib.rs | 4 +- calyx-utils/src/position.rs | 118 +++++++++++--------------- cider/Cargo.toml | 3 +- tools/cider-data-converter/Cargo.toml | 2 +- 10 files changed, 107 insertions(+), 104 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index cb2bf4e3c7..1a195e4c5b 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -18,7 +18,7 @@ jobs: steps: - name: Checkout uses: actions/checkout@v4 - - name: Install Rust stable + - name: Install Rust toolchain uses: actions-rust-lang/setup-rust-toolchain@v1 - name: Build calyx dev run: cargo build @@ -107,16 +107,20 @@ jobs: git checkout -f $GITHUB_SHA git clean -fd + - name: Checkout toolchain configuration + uses: actions/checkout@v4 + with: + sparse-checkout: | + rust-toolchain.toml + sparse-checkout-cone-mode: false + - name: Install Rust toolchain + uses: actions-rust-lang/setup-rust-toolchain@v1 - name: Build uses: actions-rs/cargo@v1 with: command: build args: --all --all-features --manifest-path /home/calyx/cider/Cargo.toml - - name: Set location of cider binary - run: | - fud c stages.interpreter.exec /home/calyx/target/debug/cider - - name: Runt tests working-directory: /home/calyx/cider/tests run: | @@ -159,6 +163,16 @@ jobs: git checkout -f $GITHUB_SHA git clean -fd + - name: Checkout toolchain configuration + uses: actions/checkout@v4 + with: + sparse-checkout: | + rust-toolchain.toml + sparse-checkout-cone-mode: false + + - name: Install Rust toolchain + uses: actions-rust-lang/setup-rust-toolchain@v1 + - name: Install calyx-py, MrXL, and queues working-directory: /home/calyx run: | @@ -226,6 +240,16 @@ jobs: git checkout -f $GITHUB_SHA git clean -fd + - name: Checkout toolchain configuration + uses: actions/checkout@v4 + with: + sparse-checkout: | + rust-toolchain.toml + sparse-checkout-cone-mode: false + + - name: Install Rust toolchain + uses: actions-rust-lang/setup-rust-toolchain@v1 + - name: Build uses: actions-rs/cargo@v1 with: diff --git a/Cargo.lock b/Cargo.lock index da298f6330..14f3f7ddb9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -270,6 +270,12 @@ dependencies = [ "syn 2.0.58", ] +[[package]] +name = "boxcar" +version = "0.2.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f839cdf7e2d3198ac6ca003fd8ebc61715755f41c1cad15ff13df67531e00ed" + [[package]] name = "bumpalo" version = "3.15.3" @@ -428,8 +434,8 @@ name = "calyx-utils" version = "0.7.1" dependencies = [ "atty", + "boxcar", "itertools 0.11.0", - "lazy_static", "petgraph", "serde", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index 7478d86c14..8ffea0dca1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,7 +34,7 @@ categories = ["compilers"] homepage = "https://calyxir.org" edition = "2021" version = "0.7.1" -rust-version = "1.67" +rust-version = "1.80" [workspace.dependencies] itertools = "0.11" diff --git a/calyx-frontend/src/parser.rs b/calyx-frontend/src/parser.rs index 47938508dc..13264e9543 100644 --- a/calyx-frontend/src/parser.rs +++ b/calyx-frontend/src/parser.rs @@ -57,16 +57,17 @@ impl CalyxParser { })?; // Add a new file to the position table let string_content = std::str::from_utf8(content)?.to_string(); - let file = GlobalPositionTable::as_mut() - .add_file(path.to_string_lossy().to_string(), string_content); + let file = GlobalPositionTable::add_file( + path.to_string_lossy().to_string(), + string_content, + ); let user_data = UserData { file }; - let gpos_ref = GlobalPositionTable::as_ref(); - let content = gpos_ref.get_source(file).to_owned(); - drop(gpos_ref); + + let content = GlobalPositionTable::get_source(file); // Parse the file let inputs = - CalyxParser::parse_with_userdata(Rule::file, &content, user_data) + CalyxParser::parse_with_userdata(Rule::file, content, user_data) .map_err(|e| e.with_path(&path.to_string_lossy())) .map_err(|e| { calyx_utils::Error::parse_error(e.variant.message()) @@ -96,20 +97,17 @@ impl CalyxParser { )) })?; // Save the input string to the position table - let file = - GlobalPositionTable::as_mut().add_file("".to_string(), buf); + let file = GlobalPositionTable::add_file("".to_string(), buf); let user_data = UserData { file }; - let gpos_ref = GlobalPositionTable::as_ref(); - let content = gpos_ref.get_source(file).to_owned(); - drop(gpos_ref); + let content = GlobalPositionTable::get_source(file); // Parse the input let inputs = - CalyxParser::parse_with_userdata(Rule::file, &content, user_data) + CalyxParser::parse_with_userdata(Rule::file, content, user_data) .map_err(|e| { - calyx_utils::Error::parse_error(e.variant.message()) - .with_pos(&Self::error_span(&e, file)) - })?; + calyx_utils::Error::parse_error(e.variant.message()) + .with_pos(&Self::error_span(&e, file)) + })?; let input = inputs.single().map_err(|e| { calyx_utils::Error::parse_error(e.variant.message()) .with_pos(&Self::error_span(&e, file)) @@ -124,11 +122,7 @@ impl CalyxParser { fn get_span(node: &Node) -> GPosIdx { let ud = node.user_data(); let sp = node.as_span(); - let pos = GlobalPositionTable::as_mut().add_pos( - ud.file, - sp.start(), - sp.end(), - ); + let pos = GlobalPositionTable::add_pos(ud.file, sp.start(), sp.end()); GPosIdx(pos) } @@ -137,7 +131,7 @@ impl CalyxParser { pest::error::InputLocation::Pos(off) => (off, off + 1), pest::error::InputLocation::Span((start, end)) => (start, end), }; - let pos = GlobalPositionTable::as_mut().add_pos(file, start, end); + let pos = GlobalPositionTable::add_pos(file, start, end); GPosIdx(pos) } diff --git a/calyx-utils/Cargo.toml b/calyx-utils/Cargo.toml index 6d3dc94693..1632a1109b 100644 --- a/calyx-utils/Cargo.toml +++ b/calyx-utils/Cargo.toml @@ -23,4 +23,4 @@ string-interner.workspace = true itertools.workspace = true petgraph.workspace = true symbol_table = { version = "0.3", features = ["global"] } -lazy_static.workspace = true +boxcar = "0.2.7" diff --git a/calyx-utils/src/errors.rs b/calyx-utils/src/errors.rs index b560fdeec0..b087d017aa 100644 --- a/calyx-utils/src/errors.rs +++ b/calyx-utils/src/errors.rs @@ -179,7 +179,7 @@ impl Error { post_msg: None, } } - pub fn location(&self) -> (String, usize, usize) { + pub fn location(&self) -> (&str, usize, usize) { self.pos.get_location() } pub fn message(&self) -> String { diff --git a/calyx-utils/src/lib.rs b/calyx-utils/src/lib.rs index 5a9242797c..5ff50868ad 100644 --- a/calyx-utils/src/lib.rs +++ b/calyx-utils/src/lib.rs @@ -18,7 +18,5 @@ pub use math::bits_needed_for; pub use namegenerator::NameGenerator; pub use out_file::OutputFile; pub use pos_string::PosString; -pub use position::{ - FileIdx, GPosIdx, GlobalPositionTable, PosIdx, PositionTable, WithPos, -}; +pub use position::{FileIdx, GPosIdx, GlobalPositionTable, PosIdx, WithPos}; pub use weight_graph::{BoolIdx, Idx, WeightGraph}; diff --git a/calyx-utils/src/position.rs b/calyx-utils/src/position.rs index fb5333897f..8692710061 100644 --- a/calyx-utils/src/position.rs +++ b/calyx-utils/src/position.rs @@ -1,12 +1,8 @@ //! Definitions for tracking source position information of Calyx programs +use std::{cmp, fmt::Write, sync::LazyLock}; + use itertools::Itertools; -use lazy_static::lazy_static; -use std::{ - cmp, - fmt::Write, - sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}, -}; #[derive(Clone, Copy, PartialEq, Eq, Debug)] /// Handle to a position in a [PositionTable] @@ -21,9 +17,9 @@ pub struct FileIdx(u32); /// A source program file struct File { /// Name of the file - name: String, + name: Box, /// The source code of the file - source: String, + source: Box, } struct PosData { @@ -37,11 +33,11 @@ struct PosData { } /// Source position information for a Calyx program. -pub struct PositionTable { +struct PositionTable { /// The source files of the program - files: Vec, + files: boxcar::Vec, /// Mapping from indexes to position data - indices: Vec, + indices: boxcar::Vec, } impl Default for PositionTable { @@ -56,9 +52,9 @@ impl PositionTable { /// Create a new position table where the first file and first position are unknown pub fn new() -> Self { - let mut table = PositionTable { - files: Vec::new(), - indices: Vec::new(), + let table = PositionTable { + files: boxcar::Vec::new(), + indices: boxcar::Vec::new(), }; table.add_file("unknown".to_string(), "".to_string()); let pos = table.add_pos(FileIdx(0), 0, 0); @@ -67,10 +63,12 @@ impl PositionTable { } /// Add a new file to the position table - pub fn add_file(&mut self, name: String, source: String) -> FileIdx { - let file = File { name, source }; - let file_idx = self.files.len(); - self.files.push(file); + pub fn add_file(&self, name: String, source: String) -> FileIdx { + let file = File { + name: name.into(), + source: source.into(), + }; + let file_idx = self.files.push(file); FileIdx(file_idx as u32) } @@ -84,15 +82,9 @@ impl PositionTable { } /// Add a new position to the position table - pub fn add_pos( - &mut self, - file: FileIdx, - start: usize, - end: usize, - ) -> PosIdx { + pub fn add_pos(&self, file: FileIdx, start: usize, end: usize) -> PosIdx { let pos = PosData { file, start, end }; - let pos_idx = self.indices.len(); - self.indices.push(pos); + let pos_idx = self.indices.push(pos); PosIdx(pos_idx as u32) } @@ -104,28 +96,27 @@ impl PositionTable { /// The global position table pub struct GlobalPositionTable; -lazy_static! { - static ref GPOS_TABLE: RwLock = - RwLock::new(PositionTable::default()); -} +static GPOS_TABLE: LazyLock = LazyLock::new(PositionTable::new); impl GlobalPositionTable { - /// Return reference to a global [PositionTable]. - /// - /// # Safety - /// - /// You may not call this function after any call to [`Self::as_ref`]. - pub fn as_mut() -> RwLockWriteGuard<'static, PositionTable> { - GPOS_TABLE - .try_write() - .expect("failed to get write lock for global position table") + fn get_pos(pos: PosIdx) -> &'static PosData { + GPOS_TABLE.get_pos(pos) + } + + fn get_file_data(file: FileIdx) -> &'static File { + GPOS_TABLE.get_file_data(file) } - /// Return an immutable reference to the global position table - pub fn as_ref() -> RwLockReadGuard<'static, PositionTable> { - GPOS_TABLE - .try_read() - .expect("failed to get read lock for global position table") + pub fn get_source(file: FileIdx) -> &'static str { + GPOS_TABLE.get_source(file) + } + + pub fn add_file(name: String, source: String) -> FileIdx { + GPOS_TABLE.add_file(name, source) + } + + pub fn add_pos(file: FileIdx, start: usize, end: usize) -> PosIdx { + GPOS_TABLE.add_pos(file, start, end) } } @@ -157,10 +148,9 @@ impl GPosIdx { /// 1. lines associated with this span /// 2. start position of the first line in span /// 3. line number of the span - fn get_lines(&self) -> (Vec, usize, usize) { - let table = GlobalPositionTable::as_ref(); - let pos_d = table.get_pos(self.0); - let file = &table.get_file_data(pos_d.file).source; + fn get_lines(&self) -> (Vec<&str>, usize, usize) { + let pos_d = GlobalPositionTable::get_pos(self.0); + let file = &*GlobalPositionTable::get_file_data(pos_d.file).source; let lines = file.split('\n').collect_vec(); let mut pos: usize = 0; @@ -186,20 +176,15 @@ impl GPosIdx { pos = next_pos + 1; linum += 1; } - ( - buf.into_iter().map(|str| str.to_owned()).collect(), - out_idx, - out_line, - ) + (buf, out_idx, out_line) } /// returns: /// 1. the name of the file the span is in /// 2. the (inclusive) range of lines within the span - pub fn get_line_num(&self) -> (String, (usize, usize)) { - let table = GlobalPositionTable::as_ref(); - let pos_data = table.get_pos(self.0); - let file_name = table.get_file_data(pos_data.file).name.clone(); + pub fn get_line_num(&self) -> (&str, (usize, usize)) { + let pos_data = GlobalPositionTable::get_pos(self.0); + let file_name = &GlobalPositionTable::get_file_data(pos_data.file).name; let (buf, _, line_num) = self.get_lines(); //reformat to return the range (inclusive) let rng = (line_num, line_num + buf.len() - 1); @@ -208,13 +193,12 @@ impl GPosIdx { /// Format this position with the error message `err_msg` pub fn format_raw>(&self, err_msg: S) -> String { - let table = GlobalPositionTable::as_ref(); - let pos_d = table.get_pos(self.0); + let pos_d = GlobalPositionTable::get_pos(self.0); let (lines, pos, linum) = self.get_lines(); let mut buf = String::new(); - let l = lines[0].as_str(); + let l = lines[0]; let linum_text = format!("{} ", linum); let linum_space: String = " ".repeat(linum_text.len()); let mark: String = "^".repeat(cmp::min( @@ -237,9 +221,8 @@ impl GPosIdx { /// Format this position with filename header and the error message `err_msg` pub fn format>(&self, err_msg: S) -> String { - let table = GlobalPositionTable::as_ref(); - let pos_d = table.get_pos(self.0); - let name = &table.get_file_data(pos_d.file).name; + let pos_d = GlobalPositionTable::get_pos(self.0); + let name = &*GlobalPositionTable::get_file_data(pos_d.file).name; let mut buf = name.to_string(); writeln!(buf).unwrap(); @@ -247,17 +230,16 @@ impl GPosIdx { buf } - pub fn get_location(&self) -> (String, usize, usize) { - let table = GlobalPositionTable::as_ref(); - let pos_d = table.get_pos(self.0); - let name = table.get_file_data(pos_d.file).name.clone(); + pub fn get_location(&self) -> (&str, usize, usize) { + let pos_d = GlobalPositionTable::get_pos(self.0); + let name = &*GlobalPositionTable::get_file_data(pos_d.file).name; (name, pos_d.start, pos_d.end) } /// Visualizes the span without any message or marking pub fn show(&self) -> String { let (lines, _, linum) = self.get_lines(); - let l = lines[0].as_str(); + let l = lines[0]; let linum_text = format!("{} ", linum); format!("{}|{}\n", linum_text, l) } diff --git a/cider/Cargo.toml b/cider/Cargo.toml index 5f560d8a83..892a84e9c8 100644 --- a/cider/Cargo.toml +++ b/cider/Cargo.toml @@ -3,8 +3,7 @@ name = "cider" version = "0.1.1" authors = ["The Calyx authors"] edition = "2021" -rust-version = "1.73" - +rust-version.workspace = true [[bin]] name = "cider" diff --git a/tools/cider-data-converter/Cargo.toml b/tools/cider-data-converter/Cargo.toml index 9557d9c292..21351be5c3 100644 --- a/tools/cider-data-converter/Cargo.toml +++ b/tools/cider-data-converter/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "cider-data-converter" authors.workspace = true -rust-version = "1.73" +rust-version.workspace = true edition.workspace = true version = "0.1.0"