FuseSoC and Github Linter Action #4
Conversation
This seems to have changed
When using the verilator linter, we want to see all warnings, but not have it fail on the warnings (for now).
Verilator now supports waivers for the linter to suppress known and
tolerated warnings. It is too ambitious to solve all Linter warnings
for now, but lets start with a waiver file that waivers all warnings
we see so far.
This file was generated by calling fusesoc and then cleaned up.
fusesoc --cores-root=. run --target=lint
chipsalliance.org:cores:SweRV_EL2 --verilator_options
"--waiver-output verilator_waiver.vlt"
From now on the linter should warn about new warnings, and be pedantic
about it.
This adds the Github Linter Action to be invoked on every push or pull request. It will lint the core and fail when the linter emits any warnings not suppressed by the waiver file. I suggest to not revert the "no-fatal" for now and be pedantic, a linter warning should either be solved or a meaningful entry to the waiver file added.
|
This looks great and will be very useful to keep up code quality. Hopefully we cam reduce the number of existing warnings too. There are quite a few of them but EH1 was verilator clean IIRC so it should be doable |
|
Yeah, I skipped through them. My gut feeling is that half of them are solved easily within an hour (hidden names, widths, etc.) |
|
We use warning disable switches, see tools/Makefile
|
|
That is not what this PR is about, to be frank. At the moment you are suppressing warnings at simulation build. A linter flow will help improve the code quality and not suppress warnings, this is just to kick start a new flow. Plus this PR adds a CI check for new code and code proposals not to add new Linter failures. |
|
Our development/production simulator is xcelium. It compiles our design with no warnings. Compilations with VCS/Questa have no warnings too. Verilator is the only one, having warnings, because is not fully compliant with SV standard. Our design also passed standard lint tools checks, like spyglass and was synthesized with industry leading synthesis tools.
Our goal was to allow simulations of our cores with a free simulation tool – the Verilator. It took quit an effort to make it work. We have no plans to change design sources just to comply 100% with the Verilator deficiencies. Hopefully Verilator will evolve and thus won't warn legitimate code...
|
|
Sorry, @agrobman, but you clearly miss the point. It has nothing to do with SV compliance. If you have any problem with the synthesizable part of SV not working with Verilator, I encourage you to open an issue with Verilator, it will probably be resolved quickly. Both Swerv and Verilator are part of CHIPS Alliance, so there should be a mutual interest here. The signal is defined and not used. Why define it? What purpose does it serve? Spyglass doesn't mention it? Is it really a great idea to use the same identifier again? Is the variable actually defined correctly? So, to come to an end, I genuinely don't care. This is just a service. If you don't believe in style linting at all or have your demo run without trashing the terminal with 514 warnings, fine. |
|
I see two benefits with this patch.
|
|
Hi Stefan and Olof, Thank you. |
|
What verilator version are you using? |
|
@aprnath thanks, just drop me a mail if you have any questions or want to discuss it in general. |
|
From verilator/verilator#2433 where @agrobman asked:
I think those questions are mostly answered in the commits and above.
My suggestion is 2. |
|
I agree with 2) too. Let us know good verilator version to start with. |
|
When anyone from your team finds time and patience, I encourage you to post WIDTH warnings here that you believe are entirely wrong or way too pedantic. I will go through them too, but some prioritization of reviewing them is helpful. |
|
the big one is sized parameters, I mentioned in verilator post. Before I ask designers to look in the warnings I need 'fixed' verilator version. I'd asked them to do this already, but got push back with all these bogus warnings ... Looking at your paper, I think, I don't like waver solutions for us. Our design evolves /changes too often - line based waiver is too much maintenance. BTW, will lint disable comments work with -f verilog option? if compilation files list contains where "disable_lint_file" and "enable_lint_file" contain lint disable and enable comments? to disable linting in part of the design files without touching them... |
This adds a Github action that will automatically check pushes to the repository and pull requests. It will fail when the Verilator linter issues any warning. I propose to use this to have committers either suppress the warnings or add a meaningful message in the waiver file.
You can find a writeup about waiver files in Verilator here: https://wallento.cs.hm.edu/post/20200612-verilator-waivers/
A demo of what the Linter Action produces can be found here for the ibex core, the clou are the source annotations which make it really powerful:
The waiver file looks ugly now, but I see a good path forward solving or waiving the warnings correctly, happy to chat about it.
You don't need to do anything except merging, Actions are a builtin.