Bump Rust #255
Bump Rust #255
Conversation
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.": fabianschuiki#252
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()`.
fabianschuiki
left a comment
fabianschuiki
left a comment
There was a problem hiding this comment.
LGTM! Thank you very much for getting this back to the present 😁
|
Looks like the lint checks fail -- any chance you could run formatting or adjust the failing lines? |
|
First let me apologize for opening this PR then leaving your comment unaddressed for so long. I have added a commit that fixes all of the format checks (I can't imagine how some of it happened in the first place). Unfortunately this PR doesn't seem to have been updated with that commit. Perhaps it is because it is on a fork of a fork? Something I discovered about github PRs in this process is that once you fork a repo with a given name, you can never fork another repo with the same name. Originally I forked the repo from @stefanlippuner since it was more current. Github did let me open this PR with a branch from my fork of Stefan's fork, but apparently this type of PR doesn't get automatically updated with new commits?!?!? Or am I missing something about how github PRs work? Everything I've read is that when at you add a commit to a branch with an open PR is that the commit is automatically added to the PR. Maybe we should just abandon this PR given the mess I've made of it? The value I see in maintaining this repo vs the ongoing work with Moore and LLHD in the CIRCT project is the Verilog parser written in Rust, making it well suited for procmacros. I've looked at this other one, but the moore parser in this crate gives much better error messages. There is one other parser listed on crates.io, but it seems less developed. This is your work and your time, let me know how you'd like to proceed. If there is a way to add my new commit, we can do that. Or I can close this PR, and either leave it that way or open a new one. |
2 participants
This pull request brings this project up to the latest Rust stable version, 1.87. It is the same as this pull request in Stefan's repo, but applied here now that pull 252 has been merged.
There are three commits in this PR. The first just makes it build with no warnings with the latest stable rust, which is 1.87.0 at the time of this PR. The second fixes the stack overflows mentioned in PR 252. The root cause was that the default derived PartialEq and Hash for &dyn *Node references didn't work the same in newer versions of Rust because of Rust's memory model (see the commit for more details). The final commit just marks up the build commands for LLVM and CIRCT in the Readme.md to use Ninja, triggered by previous updates to CIRCT and LLVM.