verilator
verilator copied to clipboard
verilator
Add new warning on unused waivers
When you're implementing something in stages, it's quite common to have signals tied off that you'll connect to in future. Verilator (and other tools) support magic signals starting with unused, which squash the resulting warnings. For example:
logic unused_foo;
assign unused_foo = ^{foo[1:0]};
will squash any warnings about us not using bits 1 and 0 of foo.
The problem is that it's easy to forget to remove the waiver. This leaves unnecessary cruft at best and, at worst, squashes helpful lint warnings (if we thought we were using foo correctly now, but actually never connected up foo[0], for example).
Is there a form of this waiver that will squash warnings for the signal being unused, but will trigger a warning if it is, in fact, used? If not, how hard would something like this be to implement?
It doesn't seem overly complex, but it touches parts of Verilator that are currently not part of the waiver infrastructure. I would suggest adding a special "unused" waiver type for that, that marks unused assignments and then can check in some rather large stages if they are actually unused.
A methodology I recommend and we use in my company is simply:
assign unused_foo = ^{foo[1:0]}; // FIXME
Then grep for FIXME near the end of a project. This allows you to comment anything with FIXME so has a lot more flexibility.
Note the Verilator source tree uses a similar convention, with a test_regress/t/t_dist_fixme.pl test to check for the FIXMEs.
Could a "pedantic waivers" option that triggers a warnings/error for all waivers that didn't match be helpful?
@wallento: That would be perfect!
@wsnyder: That's neat, but is a slightly awkward fit when you've got a long development cycle (like we're seeing with OpenTitan). There, it's nice if the lint flow can catch stuff before final signoff. Maybe there are other TODO/FIXME comments that should stay, but I'd still like to be reminded that I've messed up my waivers.
@wallento/@rwarbrick perhaps you'd be willing to contribute a patch to add the new "unused waiver" warning?
Reading back on this I'm not quite clear how this was suggested to be used, @rswarbrick can you provide an example use case please?
Maybe this is not helpful to answer what you are actually asking, but I think the intention here was to raise [a new type of] warning, if the waiver contradicts the design. The story goes:
I implemented foo, but have rest of my module/design still stubbed out and not ready yet:
wire [1:0] foo = something; // Unused, so yields UNUSED warning
I need to context switch my mind and do something else right now because there is a fire at the customer, so I need to park this, I will just sign off the warning so it doesn't pollute every other developer's world:
wire [1:0] foo = something;
wire unused_will_come_back_to_this_later = &{1'b0, foo}; // No more UNUSED warning
Ok, I am back, I have put out the fire, but I already forgot half the code, but I think I implemented the remaining:
wire [1:0] foo = something;
wire unused_will_come_back_to_this_later = &{1'b0, foo}; // No more UNUSED warning
wire result = ~foo[1];
In the above, I forgot that I signed off the foo, and I'm under the impression that I finished my job, but I never actually used foo[0].
The idea in this issue was that with the last snippet we should give a warning saying "you say foo[1] is unused, but actually it is used, it's on the right hand side of the assignment to result." That will prod the user to remove the unused_will_come_back_to_this_later "waiver", which at that point was cruft in the best case, and in the worst it hid the unused warning on foo[0].
In general, a waiver that does not actually waive anything should itself be a warning.
This somewhat relates to the idea of the NOEFFECT warning on things that are accepted but are ignored, that I described in https://github.com/verilator/verilator/issues/3421#issuecomment-1126974326 in the sense that both relates to making the user aware if they provided spurious input (i.e.: input that does not do what it appears to be doing)
Unused_foo is just a signal like any other, it's not specifying a waiver. So I'm still not sure what's requested.
If we have vlt waivers e.g.
lint_off -rule WIDTH -file "t/t_vlt_warn.v"
We can easily add a warning e/g/ WAIVERUNUSED if this doesn't suppress something.
However this would be very hard/slow to know it doesn't apply:
// verilator lint_off WIDTH
And I'm still not sure how the e.g. WAIVERUNUSED warning helps the original use case.
Unused_foo is just a signal like any other, it's not specifying a waiver.
I think that is a quirk of how this is expressed. Signals with unused in the name suppress the UNUSED warning, so they are effectively used as a waiver. unused_foo is never consumed in the design otherwise.
True but typically there's a signal that likely needs to remain in the unused list even when the design is complete, so this proposed warning won't fire. Also the problem as originally stated included forgetting to remove a waiver on a bit that really was unused but shouldn't have been.
I still think the solution as stated is to properly use FIXMEs. This is well proven to work well in large organizations and code bases.
assign unused= &{1'b0,
need_to_finish_design, //FIXME
upper_bit_unused_even_when_finished[15]};
Not that auditing unused waivers isn't also useful, but that seems a different problem than originally requested.
True but typically there's a signal that likely needs to remain in the unused list even when the design is complete, so this proposed warning won't fire.
That's fine, the warning should fire only if the signoff is inconsistent.
Also the problem as originally stated included forgetting to remove a waiver on a bit that really was unused but shouldn't have been.
I think the idea here was that by being forced to audit your waivers, you have a better chance of discovering mistakes. The mistake here is that you did not wire up foo[0] to its consumer, only foo[1], but because you waived both (and you don't remember that you did waive both), you don't notice. The new warning would highlighting that that your waiver is still there but no longer consistent, and prod you to audit it, at which point hopefully you will think hard about what to do with foo[0] (leave it signed off as it is indeed expected to be signed off at the end of the project, or go and fix your mistake of not consuming it). You are right however that this does not save you from never coming back to finishing the job, but might help if you come back and get it wrong.
Not that auditing unused waivers isn't also useful, but that seems a different problem than originally requested.
I'm not sure any more what we consider the "original problem" here.
Reading back on this I'm not quite clear how this was suggested to be used, @rswarbrick can you provide an example use case please?
About the original "issue": it was a while ago that I ran into this and I'm not completely sure what concrete use case I had in mind. Probably the best thing is to either close the issue report or morph it to represent what Geza was talking about. (So I don't make the wrong choice, please feel free! :-D)
If things come up more concretely in a couple of months, I can always open a new issue... (possibly including a patch this time!)