From f4abc83f0e0391e1b20e3f14e7dbe369fb7bf6a9 Mon Sep 17 00:00:00 2001 From: jgreenbaum Date: Tue, 24 Jun 2025 18:49:40 -0700 Subject: [PATCH 1/3] Builds cleanly with rustc 1.87.0 But tests fail as mentioned in the comments to the existing pull request: "Building moore with v1.67 or newer results in stack overflows at runtime (e.g. when running the tests). I think fixing this can be deferred for now.": https://github.com/fabianschuiki/moore/pull/252 --- rust-toolchain.toml | 2 +- src/bin/moore.rs | 2 +- src/circt/src/lib.rs | 2 +- src/svlog/codegen.rs | 6 +++++- src/svlog/context.rs | 6 +++--- src/svlog/lib.rs | 4 +--- src/svlog/mir/visit.rs | 2 +- src/svlog/syntax/lexer.rs | 2 +- src/vhdl/hir/arena.rs | 4 ++-- src/vhdl/hir/misc.rs | 5 +---- src/vhdl/hir/mod.rs | 1 - src/vhdl/hir/prelude.rs | 4 ++-- src/vhdl/scope2.rs | 14 +++++++------- src/vhdl/ty2/subtypes.rs | 2 -- src/vhdl/ty2/types.rs | 2 -- src/vhdl/typeck.rs | 4 +--- 16 files changed, 27 insertions(+), 35 deletions(-) diff --git a/rust-toolchain.toml b/rust-toolchain.toml index c08d70f5a..3249ec2bf 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,3 +1,3 @@ [toolchain] -channel = "1.66" +channel = "1.87" diff --git a/src/bin/moore.rs b/src/bin/moore.rs index c2db647fa..f788d87d3 100644 --- a/src/bin/moore.rs +++ b/src/bin/moore.rs @@ -590,7 +590,7 @@ unsafe extern "C" fn moore_mlir_diagnostic_handler( let sess: &Session = (sess as *const Session).as_ref().unwrap(); // Map the severity from MLIR to Moore. - #[allow(non_upper_case_globals)] + #[allow(non_upper_case_globals,non_snake_case)] let severity = match mlirDiagnosticGetSeverity(diag) { MlirDiagnosticSeverity_MlirDiagnosticError => Severity::Error, MlirDiagnosticSeverity_MlirDiagnosticWarning => Severity::Warning, diff --git a/src/circt/src/lib.rs b/src/circt/src/lib.rs index e34fde01e..eba3e949e 100644 --- a/src/circt/src/lib.rs +++ b/src/circt/src/lib.rs @@ -116,5 +116,5 @@ mod crate_prelude { pub use crate::mlir::*; pub use crate::prelude::*; pub use crate::sys::*; - pub use num::{BigInt, BigRational, One, ToPrimitive, Zero}; + pub use num::{BigInt, BigRational, ToPrimitive, Zero}; } diff --git a/src/svlog/codegen.rs b/src/svlog/codegen.rs index 530d71fa8..2ae40875d 100644 --- a/src/svlog/codegen.rs +++ b/src/svlog/codegen.rs @@ -3743,7 +3743,11 @@ pub enum ModulePortKind<'a> { Port, /// An interface signal. IntfSignal { - intf: &'a ast::Interface<'a>, + /* jack@greenbaum.org 20 June 2025 + * Clippy says this is never read. That is sort of correct, only intf.ast is read. Grrr. + */ + #[allow(dead_code)] + intf: &'a ast::Interface<'a>, env: ParamEnv, decl_id: NodeId, }, diff --git a/src/svlog/context.rs b/src/svlog/context.rs index e65ecc7a6..bb78f6173 100644 --- a/src/svlog/context.rs +++ b/src/svlog/context.rs @@ -334,17 +334,17 @@ impl<'t> GlobalArenas<'t> { } /// Allocate an AST root. - pub fn alloc_ast_root(&'t self, ast: ast::Root<'t>) -> &'t ast::Root { + pub fn alloc_ast_root(&'t self, ast: ast::Root<'t>) -> &'t ast::Root<'t> { self.ast_roots.alloc(ast) } /// Allocate an AST type. - pub fn alloc_ast_type(&'t self, ast: ast::Type<'t>) -> &'t ast::Type { + pub fn alloc_ast_type(&'t self, ast: ast::Type<'t>) -> &'t ast::Type<'t> { self.ast_types.alloc(ast) } /// Allocate an AST expression. - pub fn alloc_ast_expr(&'t self, ast: ast::Expr<'t>) -> &'t ast::Expr { + pub fn alloc_ast_expr(&'t self, ast: ast::Expr<'t>) -> &'t ast::Expr<'t> { self.ast_exprs.alloc(ast) } diff --git a/src/svlog/lib.rs b/src/svlog/lib.rs index 743528afb..6fb5351e3 100644 --- a/src/svlog/lib.rs +++ b/src/svlog/lib.rs @@ -287,9 +287,7 @@ mod queries { value::*, }; use std::{ - cell::RefCell, - collections::{HashMap, HashSet}, - sync::Arc, + cell::RefCell, collections::{HashMap, HashSet}, sync::Arc, }; moore_derive::derive_query_db! { diff --git a/src/svlog/mir/visit.rs b/src/svlog/mir/visit.rs index 6fb4e4ea4..64bed1257 100644 --- a/src/svlog/mir/visit.rs +++ b/src/svlog/mir/visit.rs @@ -49,7 +49,7 @@ impl<'a, T: WalkVisitor<'a>> WalkVisitor<'a> for Vec { impl<'a, T: WalkVisitor<'a>> WalkVisitor<'a> for Option { fn walk(&'a self, visitor: &mut dyn Visitor<'a>) { - for x in self { + if let Some(x) = self { x.walk(visitor); } } diff --git a/src/svlog/syntax/lexer.rs b/src/svlog/syntax/lexer.rs index f3e23f794..d780315b4 100644 --- a/src/svlog/syntax/lexer.rs +++ b/src/svlog/syntax/lexer.rs @@ -20,7 +20,7 @@ pub struct Lexer<'a> { } impl<'a> Lexer<'a> { - pub fn new(input: Preprocessor<'a>) -> Lexer { + pub fn new(input: Preprocessor<'a>) -> Lexer<'a> { Lexer { input: input, peek: [(CatTokenKind::Eof, INVALID_SPAN); 4], diff --git a/src/vhdl/hir/arena.rs b/src/vhdl/hir/arena.rs index 31a5e2860..2666d2035 100644 --- a/src/vhdl/hir/arena.rs +++ b/src/vhdl/hir/arena.rs @@ -14,12 +14,12 @@ make_arenas!( package: Package2<'t>, type_decl: TypeDecl2<'t>, subtype_ind: SubtypeInd2<'t>, - const_decl: ConstDecl<'t>, + const_decl: crate::hir::obj_decl::ConstDecl<'t>, lit_expr: LitExpr, package_slot: Slot<'t, Package2<'t>>, type_decl_slot: Slot<'t, TypeDecl2<'t>>, subtype_ind_slot: Slot<'t, SubtypeInd2<'t>>, - const_decl_slot: Slot<'t, ConstDecl<'t>>, + const_decl_slot: Slot<'t, crate::hir::obj_decl::ConstDecl<'t>>, } ); diff --git a/src/vhdl/hir/misc.rs b/src/vhdl/hir/misc.rs index 88bcea563..cd461076d 100644 --- a/src/vhdl/hir/misc.rs +++ b/src/vhdl/hir/misc.rs @@ -1,7 +1,5 @@ // Copyright (c) 2016-2021 Fabian Schuiki -use std; - use crate::hir::prelude::*; pub fn apply_use_clauses<'a, I>(clauses: I, context: AllocContext) @@ -9,8 +7,7 @@ where I: IntoIterator, { for u in clauses.into_iter() { - let e = apply_use_clause(u, context); - std::mem::forget(e); + let _ = apply_use_clause(u, context); } } diff --git a/src/vhdl/hir/mod.rs b/src/vhdl/hir/mod.rs index 463955a08..c7b246083 100644 --- a/src/vhdl/hir/mod.rs +++ b/src/vhdl/hir/mod.rs @@ -33,7 +33,6 @@ pub use self::expr::*; pub use self::lib::*; pub use self::misc::*; pub use self::node::*; -pub use self::obj_decl::*; pub use self::pkg::*; pub use self::slot::*; pub use self::subtype_decl::*; diff --git a/src/vhdl/hir/prelude.rs b/src/vhdl/hir/prelude.rs index 0de31e433..a7e17d9b8 100644 --- a/src/vhdl/hir/prelude.rs +++ b/src/vhdl/hir/prelude.rs @@ -6,7 +6,7 @@ pub use crate::common::score::Result; pub use crate::common::source::{Span, Spanned}; pub use crate::common::NodeId; -pub use crate::arenas::{Alloc, AllocInto, AllocOwned, AllocOwnedInto, AllocOwnedSelf, AllocSelf}; +pub use crate::arenas::{Alloc, AllocInto, AllocOwned, AllocOwnedInto}; pub use crate::hir::node::*; pub use crate::hir::slot::*; pub use crate::hir::visit::Visitor; @@ -14,4 +14,4 @@ pub use crate::hir::AllocContext; pub use crate::scope2::{Def2, ScopeContext, ScopeData, TypeVariantDef}; pub use crate::score::ResolvableName; pub use crate::syntax::ast; -pub use crate::ty2::{self, Type}; +pub use crate::ty2::Type; diff --git a/src/vhdl/scope2.rs b/src/vhdl/scope2.rs index c9bdabeb0..a06775b77 100644 --- a/src/vhdl/scope2.rs +++ b/src/vhdl/scope2.rs @@ -67,12 +67,12 @@ impl<'t> Def2<'t> { impl<'t> PartialEq for Def2<'t> { fn eq(&self, other: &Self) -> bool { match (*self, *other) { - (Def2::Node(a), Def2::Node(b)) => (a as *const _ == b as *const _), - (Def2::Lib(a), Def2::Lib(b)) => (a as *const _ == b as *const _), - (Def2::Pkg(a), Def2::Pkg(b)) => (a as *const _ == b as *const _), - (Def2::Type(a), Def2::Type(b)) => (a as *const _ == b as *const _), - (Def2::Enum(a), Def2::Enum(b)) => (a == b), - (Def2::Unit(a), Def2::Unit(b)) => (a == b), + (Def2::Node(a), Def2::Node(b)) => std::ptr::addr_eq(a as *const _, b as *const _), + (Def2::Lib(a), Def2::Lib(b)) => a as *const _ == b as *const _, + (Def2::Pkg(a), Def2::Pkg(b)) => std::ptr::addr_eq(a as *const _, b as *const _), + (Def2::Type(a), Def2::Type(b)) => std::ptr::addr_eq(a as *const _, b as *const _), + (Def2::Enum(a), Def2::Enum(b)) => a == b, + (Def2::Unit(a), Def2::Unit(b)) => a == b, _ => false, } } @@ -250,7 +250,7 @@ impl<'t> fmt::Debug for TypeVariantDef<'t> { impl<'t> PartialEq for TypeVariantDef<'t> { fn eq(&self, other: &Self) -> bool { - self.0 as *const _ == other.0 as *const _ && self.1 == other.1 + std::ptr::addr_eq(self.0 as *const _, other.0 as *const _) && self.1 == other.1 } } diff --git a/src/vhdl/ty2/subtypes.rs b/src/vhdl/ty2/subtypes.rs index fba704e93..caf942046 100644 --- a/src/vhdl/ty2/subtypes.rs +++ b/src/vhdl/ty2/subtypes.rs @@ -4,8 +4,6 @@ use std::fmt::{Debug, Display}; -pub use num::BigInt; - use crate::ty2::marks::*; use crate::ty2::range::*; use crate::ty2::types::*; diff --git a/src/vhdl/ty2/types.rs b/src/vhdl/ty2/types.rs index 811c04135..1de784329 100644 --- a/src/vhdl/ty2/types.rs +++ b/src/vhdl/ty2/types.rs @@ -5,8 +5,6 @@ use std::borrow::Borrow; use std::fmt::{self, Debug, Display}; -pub use num::BigInt; - use crate::ty2::access::*; use crate::ty2::enums::*; use crate::ty2::floats::*; diff --git a/src/vhdl/typeck.rs b/src/vhdl/typeck.rs index 354a5cb5a..354554481 100644 --- a/src/vhdl/typeck.rs +++ b/src/vhdl/typeck.rs @@ -610,9 +610,7 @@ macro_rules! impl_typeck_err { ($slf:tt, $id:ident: $id_ty:ty => $blk:block) => { impl<'sbc, 'lazy, 'sb, 'ast, 'ctx> Typeck<$id_ty> for TypeckContext<'sbc, 'lazy, 'sb, 'ast, 'ctx> { fn typeck(&$slf, $id: $id_ty) { - use std; - let res = (move || -> Result<()> { $blk })(); - std::mem::forget(res); + let _ = (move || -> Result<()> { $blk })(); } } } From 3cfd5adbdded756823fdf0ebce8e77f191f3236d Mon Sep 17 00:00:00 2001 From: jgreenbaum Date: Tue, 24 Jun 2025 18:57:10 -0700 Subject: [PATCH 2/3] Fix stack overflows, use *Node.id() instead of ref All the tests in `test/run.sh` pass now. Using procout I was able to debug the output of derive::query::derive_query_db. What I saw was that key comparison functions were "aliasing" in the caches, so it kept retrieving the same scope over and over until the stack overflowed. Looking at the code I saw PartialEq and Hash being derived for keys with the arg signature of `&dyn *Node`, so any of the different Node traits. While debugging I could see that the references were identical for different `Node::id` values. This makes some sense, Rust doesn't guarantee that a reference to an object remains unchanged unless you `Pin` it. It looks like it kept reusing the same memory in different iterations of a loop. The solution I implemented replaces the derived `PartialEq` and `Hash` for keys with a single arg that is a `&dyn *Node`, and instead generates implementations that use the `Node::id()` instead of the reference. I'm not sure this is what is expected. I see comments in the code about moving away from node ids. But unless you put `Box::pin` objects into the cache you need to use a memory independent id like `Node::id()`. --- src/derive/query.rs | 53 ++++++++++++++++++++++++++++++++++++++++--- src/svlog/resolver.rs | 15 +++++++++++- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/derive/query.rs b/src/derive/query.rs index d4ddb224d..5ffee7e1d 100644 --- a/src/derive/query.rs +++ b/src/derive/query.rs @@ -2,12 +2,13 @@ use heck::CamelCase; use proc_macro::TokenStream; +use proc_macro2::Span; use quote::{format_ident, quote, ToTokens}; use std::{ cell::RefCell, - collections::{BTreeSet, HashSet}, + collections::{BTreeSet, HashSet}, }; -use syn::visit::Visit; +use syn::{visit::Visit, Ident}; // CAUTION: This is all wildly unstable and relies on the compiler maintaining // a certain order between proc macro expansions. So this could break any @@ -155,9 +156,37 @@ pub(crate) fn derive_query_db(input: TokenStream) -> TokenStream { let key_name = format_ident!("{}QueryKey", name.to_string().to_camel_case()); let key_indices = (0..arg_types.len()).map(syn::Index::from); let doc = format!("The arguments passed to the `{}` query.", name); + // If there is one arg and it is a &dyn *Node then the default derived `PartialEq` and `Hash` + // traits aren't valid for the way nodes are used in the caches. They need to depend on + // the node `id()` instead. + // let mut derive_list: Vec = Vec::new(); + // derive_list.push(proc_macro2::Literal::string("Clone")); + // let mut derive_list: proc_macro2::TokenStream = "Clone, PartialEq, Eq, Hash".parse().unwrap(); + let mut derive_list: Vec = + vec!["Clone", "PartialEq", "Eq", "Hash"].into_iter().map(|i| Ident::new(i, Span::call_site()).into()).collect(); + let mut derive_hash_eq = false; + if arg_types.len() == 1 { + if let syn::Type::Reference(ref_type) = &arg_types[0] { + if let syn::Type::TraitObject(t_obj) = ref_type.elem.as_ref() { + if t_obj.dyn_token.is_some() { + if t_obj.bounds.len() == 1 { + if let syn::TypeParamBound::Trait(bound) = t_obj.bounds.first().unwrap() { + let t_ident = bound.path.segments.last().unwrap().ident.to_string(); + if t_ident.contains("Node") { + derive_hash_eq = true; + derive_list = + vec!["Clone", "Eq"].into_iter().map(|i| Ident::new(i, Span::call_site()).into()).collect(); + } + } + } + } + } + } + } + let derive_tokens = derive_list; //proc_macro2::TokenStream::from_iter(derive_list); keys.push(quote! { #[doc = #doc] - #[derive(Clone, PartialEq, Eq, Hash)] + #[derive (#(#derive_tokens),*) ] pub struct #key_name #key_generics (#(pub #arg_types),*); impl #key_generics std::fmt::Debug for #key_name #key_generics { @@ -166,6 +195,20 @@ pub(crate) fn derive_query_db(input: TokenStream) -> TokenStream { } } }); + if derive_hash_eq { + keys.push(quote! { + impl #key_generics PartialEq for #key_name #key_generics { + fn eq(&self, other: &Self) -> bool { + self.0.id() == other.0.id() + } + } + impl #key_generics std::hash::Hash for #key_name #key_generics { + fn hash(&self, state: &mut H) { + self.0.id().hash(state); + } + } + }); + } // Determine the cache field name. let cache_name = format_ident!("cached_{}", name); @@ -305,6 +348,10 @@ pub(crate) fn derive_query_db(input: TokenStream) -> TokenStream { #(#keys)* }); + // Uncomment for debugging with procout + // let module_ident = syn::Ident::new("query", proc_macro2::Span::mixed_site()); + // procout::procout(&output, Some(module_ident), Some("/tmp/query_procout.rs")); + // Produce some output. // println!("{}", output); output.into() diff --git a/src/svlog/resolver.rs b/src/svlog/resolver.rs index 9269abceb..bc57defc7 100644 --- a/src/svlog/resolver.rs +++ b/src/svlog/resolver.rs @@ -1109,7 +1109,7 @@ pub(crate) fn scope_location<'a>( } /// A location of a node within its enclosing scope. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, Copy, Eq)] pub struct ScopeLocation<'a> { /// The node which generates the enclosing scope. pub scope: &'a dyn ScopedNode<'a>, @@ -1117,6 +1117,19 @@ pub struct ScopeLocation<'a> { pub order: usize, } +impl<'a> PartialEq for ScopeLocation<'a> { + fn eq(&self, other: &Self) -> bool { + self.scope.id() == other.scope.id() && self.order == other.order + } +} + +impl<'a> Hash for ScopeLocation<'a> { + fn hash(&self, state: &mut H) { + self.scope.id().hash(state); + self.order.hash(state); + } +} + /// Resolve a local name in a scope. /// /// This traverses up the scope tree until a definition with visibility `LOCAL` From a774e03eb33cff501418e449c3461c6c99d869b6 Mon Sep 17 00:00:00 2001 From: jgreenbaum Date: Tue, 24 Jun 2025 19:27:20 -0700 Subject: [PATCH 3/3] Update LLVM and CIRCT build commands for Ninja --- README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 5207d6091..8db30f87d 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ And then follow these steps: mkdir -p circt/llvm/build pushd circt/llvm/build - cmake ../llvm \ + cmake -G Ninja ../llvm \ -DCMAKE_BUILD_TYPE=Release \ -DLLVM_BUILD_EXAMPLES=OFF \ -DLLVM_ENABLE_ASSERTIONS=ON \ @@ -32,19 +32,19 @@ And then follow these steps: -DLLVM_INSTALL_UTILS=ON \ -DLLVM_OPTIMIZED_TABLEGEN=ON \ -DLLVM_TARGETS_TO_BUILD="host" - cmake --build . + ninja -j$(nproc) popd #### Build CIRCT mkdir -p circt/build pushd circt/build - cmake .. \ + cmake -G Ninja .. \ -DCMAKE_BUILD_TYPE=Release \ -DMLIR_DIR=$PWD/../llvm/build/lib/cmake/mlir \ -DLLVM_DIR=$PWD/../llvm/build/lib/cmake/llvm \ -DLLVM_ENABLE_ASSERTIONS=ON - cmake --build . + ninja -j$(nproc) popd #### Build Moore