refactor(rules/verilator): Switch from cmake to json format for Verilator build file parsing#7178
refactor(rules/verilator): Switch from cmake to json format for Verilator build file parsing#7178waruqi merged 5 commits intoxmake-io:devfrom
Conversation
…ator build file parsing 1. The "-make cmake" option of Verilator is deprecated and will be removed in future. See [Deprecations](https://verilator.org/guide/latest/deprecations.html). Use "-make cmake" option in Verilator v5.042 will lead Verilator to return a non-zero value, thus the script reports an error. 2. Verilator can generate build options in json format which can be parsed easier, so we switch from cmake to json format. 3. Add new SystemVerilog language standard `v1800-2023` support. 4. Fix the issue with c++ language standard settings. BREAKING CHANGE: CMake build file parsing is deprecated.
Summary of ChangesHello @24bit-xjkp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Verilator rule to modernize its interaction with the Verilator toolchain. The primary goal is to move away from a deprecated CMake-based build file parsing method to a more maintainable and future-proof JSON-based approach. This change not only resolves compatibility issues with recent Verilator versions but also expands support for the latest SystemVerilog standard and refines C++ language configuration, leading to a more robust and adaptable build system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the Verilator rule to use JSON for build file parsing instead of the deprecated CMake format. This is a great improvement for maintainability and future-proofing. The changes also include support for the new SystemVerilog v1800-2023 standard and a fix for C++ language standard detection.
The overall implementation is good, but I've identified a couple of areas where the new JSON parsing logic could be made more robust. Specifically, the code doesn't handle potential errors during JSON file loading or cases where the file has an unexpected structure. This could lead to build failures. I've provided suggestions to add error handling and safe access patterns to prevent such issues.
Apart from that, the changes are clean and well-aligned with the goals of the pull request.
xmake/rules/verilator/verilator.lua
Outdated
| local json = json.loadfile(jsonfile) | ||
| local global_classes = json["sources"]["global"] | ||
| local classefiles_slow = json["sources"]["classes_slow"] | ||
| local classefiles_fast = json["sources"]["classes_fast"] | ||
| local supportfiles_slow = json["sources"]["support_slow"] | ||
| local supportfiles_fast = json["sources"]["support_fast"] |
There was a problem hiding this comment.
The code does not handle cases where json.loadfile fails or when the sources key is missing in the JSON file. This could lead to a script error if the JSON file is not found, is malformed, or does not have the expected structure. It's safer to check the return value of json.loadfile and provide a default empty table for the sources object to prevent runtime errors.
local data, err = json.loadfile(jsonfile)
if not data then
os.raise("failed to load verilator json file %s: %s", jsonfile, err or "unknown error")
end
local sources = data["sources"] or {}
local global_classes = sources["global"]
local classefiles_slow = sources["classes_slow"]
local classefiles_fast = sources["classes_fast"]
local supportfiles_slow = sources["support_slow"]
local supportfiles_fast = sources["support_fast"]
xmake/rules/verilator/verilator.lua
Outdated
| local json = json.loadfile(jsonfile) | ||
| verilator_root = json["system"]["verilator_root"] | ||
| switches.SC = json["options"]["system_c"] | ||
| switches.COVERAGE = json["options"]["coverage"] | ||
| switches.TIMING = json["options"]["use_timing"] | ||
| switches.THREADS = json["options"]["threads"] | ||
| switches.TRACE_VCD = json["options"]["trace_vcd"] | ||
| switches.TRACE_FST = json["options"]["trace_fst"] | ||
| switches.TRACE_SAIF = json["options"]["trace_saif"] |
There was a problem hiding this comment.
The code does not handle cases where json.loadfile fails or when the expected keys (system, options) are missing in the JSON file. This could lead to a script error. It's safer to check the return value of json.loadfile and ensure nested objects exist by providing default empty tables before accessing them. This change makes the JSON parsing more robust for this function.
local json, err = json.loadfile(jsonfile)
if not json then
os.raise("failed to load verilator json file %s: %s", jsonfile, err or "unknown error")
end
json["system"] = json["system"] or {}
json["options"] = json["options"] or {}
verilator_root = json["system"]["verilator_root"]
switches.SC = json["options"]["system_c"]
switches.COVERAGE = json["options"]["coverage"]
switches.TIMING = json["options"]["use_timing"]
switches.THREADS = json["options"]["threads"]
switches.TRACE_VCD = json["options"]["trace_vcd"]
switches.TRACE_FST = json["options"]["trace_fst"]
switches.TRACE_SAIF = json["options"]["trace_saif"]
先确认下低版本 verilator 是否支持 json ，否则你直接干掉，低版本上会直接 break，应该保持 cmake/json 两者解析同时存在一段时间，如果支持 json ，优先走 json 解析，如果当前 verilator 版本不支持 json 输出，就 fallback 到 cmake 解析 |
v5.036开始支持json，我加一个按版本判断的逻辑吧 |
1. If verilator's version >= 5.036, then we use json file parsing, otherwise, we use cmake file parsing.
xmake/rules/verilator/verilator.lua
Outdated
| --- @return string makefile_type Makefile type, json or cmake | ||
| --- | ||
| function _get_makefile_type(verilator) | ||
| local version = assert(find_programver(verilator), "verilator version is unknown!") |
There was a problem hiding this comment.
应该用 find_tool("xxx", {version = true})， find_programver 一般是不直接用的
xmake/rules/verilator/verilator.lua
Outdated
| --- | ||
| function _get_makefile_type(verilator) | ||
| local version = assert(find_programver(verilator), "verilator version is unknown!") | ||
| local version = assert(find_tool(verilator, {version = true}).version, "verilator version is unknown!") |
There was a problem hiding this comment.
如果没找到，访问 version 会挂
local tool = assert(find_tool("verilator", {program = verilator, version = true}), "verilator not found!")
local version = tool.version
xmake/rules/verilator/verilator.lua
Outdated
| local cxxlang = false | ||
| for _, lang in ipairs(languages) do | ||
| if lang:startswith("xx") or lang:startswith("++") then | ||
| if lang:startswith("cxx") or lang:startswith("c++") then |
There was a problem hiding this comment.
无关的不要改，还有 gnu++20 了，这会 break 一些东西
There was a problem hiding this comment.
这原本是写错了，不管是c++ cxx还是gnuxx gnu++都不是xx或者++开头，会导致设了languages还加一个c++20上去
There was a problem hiding this comment.
那应该用 lang:find("xx", 1, true) ，如果只是判断 cxx/c++ , 还漏了 gnu++ 的
xmake/rules/verilator/verilator.lua
Outdated
| --- @brief get makefile type | ||
| --- @param verilator string verilator program path | ||
| --- @return boolean support_json Whether support json | ||
| --- @return string makefile_type Makefile type, json or cmake |
v1800-2023support.BREAKING CHANGE: CMake build file parsing is deprecated.