Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-19T23:01:14Z

For the projects I personally lead, I require all disables to be inline code, as then it's all in one spot, making it visible to the person reading/auditing the code, and the suppressions will automatically float with any code changes.

However I understand this isn't always possible.

Anyhow, the current lint_off has the file and line as optional, and supports wildcards. What additionally do you suggest?

Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-20T20:31:05Z

I mainly see it when I add Verilator to some IP that has not been used with Verilator before. It's probably a tradeoff between fixing the lint errors all at once vs. pushing a lot warnings upstream. Adding a configuration file may ease the adoption, with the risk of hiding potential for improvements. There are also still a couple of things that are hard to fix easily like loops on arrays. Not sure how representative this scenario is, but it can help with adoption without changing the main code (not that that is an issue of Verilator ;)

The problem with file+lineno is that it has to be changed on file changes, plus we could go for a more generic use of regex's. I know from commercial tools that they have features to hide/waive "known" warnings to tolerate them and not trash logs.

My personal experience is too limited tbh to say this is useful and worth the effort. I will try to get other people to comment about cases where it can be useful. It does not seem

Thanks for opening this issue @wallento!

This request comes from the use of Verilator together with proprietary lint solutions for the OpenTitan (https://github.com/lowRISC/opentitan) and Ibex (https://github.com/lowRISC/ibex) code bases.

I agree that waivers within the source code are beneficial for discoverability. We still chose to use external waiver files for these three (arguably subjective) reasons:

• We'd like to support multiple lint tools and avoid cluttering source files with multiple tool-specific comments/annotations.
• We'd like to ease the audit and sign-off off all lint waivers. Having them in one file makes this easier.
• We typically need to waive individual occurrences of lint errors, since all errors are individually reviewed.

Verilator already supports waivers with file/line and rule type. We are currently using this approach in Ibex, and that's the waiver file: https://github.com/lowRISC/ibex/blob/master/lint/verilator_waiver.vlt. Since we introduced this waiver I've learned two things:

• It's fragile. Line numbers change regularly with unrelated code changes (e.g. insert a new line somewhere). The change history of the waiver file illustrates that.
• It's not self-documenting. Typically, the lint error message is needed to understand a waiver, file and line number are less useful pieces of information. We therefore copy/paste the error message into the waiver file as comment.

Other tools support match/wildcard/regex waivers directly on the error message. Even though that doesn't seem to be a particularly beautiful approach, it works rather well.

For example:

That's the current waiver:

// Bits of signal are not used: boot_addr_i[7:0]
// Boot address is 256B aligned, cleaner to pass all bits in
lint_off -msg UNUSED -file "*/rtl/ibex_if_stage.sv" -lines 40

If we can match the error message, it could be something like:

// PROPOSED SYNTAX

// e.g. full string match
lint_off -msg UNUSED -file "*/rtl/ibex_if_stage.sv" -match "Bits of signal are not used: boot_addr_i[7:0]"

// or regex match
lint_off -msg UNUSED -file "*/rtl/ibex_if_stage.sv" -regex "Bits of signal are not used: boot_addr_i.*"

(Side remark: I always found "-msg" slightly misnamed, other tools use "rule" for this piece of information.)

So I'm proposing to add the ability to match a lint waiver against the error message. The specific type of match is up for discussion. In my experience, some kind of wildcard matching is very helpful (e.g. glob-style matches), full regex support is nice-to-have (especially since many people know the syntax). But if of course matters what you'd like to introduce as external dependency (e.g. depend on PCRE, or copy in a simple glob-style wildcard implementation).

Good points.

1. I think a patch to use --rule instead of --ms makes sense. We could keep --msg in one test, and mark it in docs under the --rule descrption as deprecated?

2. I understand now what you mean by matching the error itself. I think this would be a good enhancement. I'd suggest use the existing V3String::wildmatch for now.

A few thoughts that come to my mind: wouldn't it be sufficient to waive the triplet (module, sig, msg)? With wildcard of course. If msg is not unique number them?

This stuff is sitting in a hot path, but even a regex can be implemented so that it doesn't impact users that don't have them. Anyhow it may be noticable to people using a few of those on a large design. As verilator generates the thing you want to regex against from all information it has at hand, it seems to me the favorable way to match against such triplets.

I mean like: lint_off -module "t" -var "X" -msg UNUSED

Ah, nevermind, spent too much time on linkparser. We would probably want to do it as a filter on the emitted messages simply.

So, I propose to implement two things:

• add the notation I proposed. This is simply done. (see below)
• add Phillip's proposal but with a different keyword I suppose (waive?) a we probably want to have -wildcard and not -regex, right?

Thanks Stefan and Wilson! Two comments:

• Waiving based on rule type (msg), module, and signal is a step forward, but not sufficient. In my example, I'd like to waive the message that bits 7:0 are unused. But I still like to get warnings if, for example, bit 10 is unused. I realize that a wildcard match on an error message string are a bit ugly, but a solution which uses fully structured data is hard to achieve for little gain.
• I had a look at the code a couple weeks ago, and I also came to the conclusion that postprocessing (filtering before messages are displayed) seems like the best implementation approach.

Opened #2068 for the msg -> rule argument renaming.

@imphil I see, but you still only need wildcards only, right? No full blown regex matching in opposite, I mean.

I will start implementing this soon:

lint_waive -rule <rule> -file "<filename>" -match "<string>"
lint_waive -rule <rule> -file "<filename>" -wildmatch "<wildcardstring>"

Good? Things can still be easily renamed..

Yes, wildcards as in V3String::wildmatch are sufficient for now. If real-world experience indicates a need for anything beyond that (e.g. full regex support) we can easily add it as second step.

Works for me, and would be in line with the behavior of -file, which also uses wildmatch().

I have started adding this on top of #2072. I assume it makes sense to have it as

lint_waive [-rule <rule>] [-file "<filenamewc>"] -match "<matchstr>"

So any combination of rule and file or both not is valid?

I did miss that before, but do you plan to add a new lint_waive next to the existing lint_off? I don't see a need for that, it would be easier for users to just extend the existing lint_off directive.

I wouldn't place additional requirements on the needed arguments, expect for "at least one argument must be provided". If more than one argument is provided, they are combined with AND.

lint_off [-rule <rule>] [-file "<filenamewc>"] [-match "<matchstr>"] [-lines <line> [ - <line> ]]]

I'm not sure how to best integrate/align that with lint_on, which is currently supported. Leave lint_on as is? Is the resulting behavior sequential, i.e. can a lint_on override a previous lint_off?

I agree that overloading lint_off seems nicer.

lint_on makes a lot of sense when used inline in the Verilog code. It was in the .vlt for symmetry, and for cases where a warning might be off by default but people want it on for specific files.

Given the --match is applied AFTER a warning has actually been built it might make sense not to support --match on lint_on.

A prototype can be found in https://github.com/verilator/verilator/tree/issue-1649, but it is based on the ongoing work of supporting more configuration directives. I think I will converge on both soon.