Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint filter specific warnings or wildcards/regexp #1649

Closed
veripoolbot opened this issue Dec 19, 2019 · 18 comments
Closed

Lint filter specific warnings or wildcards/regexp #1649

veripoolbot opened this issue Dec 19, 2019 · 18 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Dec 19, 2019


Author Name: Stefan Wallentowitz (@wallento)
Original Redmine Issue: 1649 from https://www.veripool.org


Currently we can have linter on/off and specify them in the configuration file, but the latter works on a fileline basis. What we should have additionally is some kind of mechanism that commercial simulators have to filter out warning by adding them to the configuration file. Those could be like the existing ones but without fileline or maybe even a very generic regexp. It probably makes sense to ask a couple of people how they would want to use it. Philipp Wagner of the lowRISC project has proposed something along those lines for example.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 19, 2019


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?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 20, 2019


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

@imphil

This comment has been minimized.

Copy link
Contributor

@imphil imphil commented Dec 23, 2019

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).

@imphil imphil mentioned this issue Dec 23, 2019
1 of 5 tasks complete
@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Dec 23, 2019

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.

@wallento

This comment has been minimized.

Copy link
Contributor

@wallento wallento commented Dec 23, 2019

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.

@wallento

This comment has been minimized.

Copy link
Contributor

@wallento wallento commented Dec 23, 2019

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

@wallento

This comment has been minimized.

Copy link
Contributor

@wallento wallento commented Dec 23, 2019

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?
@imphil

This comment has been minimized.

Copy link
Contributor

@imphil imphil commented Dec 23, 2019

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.
@imphil

This comment has been minimized.

Copy link
Contributor

@imphil imphil commented Dec 23, 2019

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

@wallento

This comment has been minimized.

Copy link
Contributor

@wallento wallento commented Dec 29, 2019

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

@wallento

This comment has been minimized.

Copy link
Contributor

@wallento wallento commented Dec 29, 2019

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..

@imphil

This comment has been minimized.

Copy link
Contributor

@imphil imphil commented Dec 29, 2019

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.

@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Dec 29, 2019

@imphil

This comment has been minimized.

Copy link
Contributor

@imphil imphil commented Dec 29, 2019

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

@wallento

This comment has been minimized.

Copy link
Contributor

@wallento wallento commented Jan 6, 2020

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?

@imphil

This comment has been minimized.

Copy link
Contributor

@imphil imphil commented Jan 6, 2020

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?

@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Jan 7, 2020

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.

@wallento

This comment has been minimized.

Copy link
Contributor

@wallento wallento commented Jan 8, 2020

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.

@wallento wallento self-assigned this Jan 11, 2020
wallento added a commit that referenced this issue Jan 11, 2020
Add configuration file switch '-match' for lint_off. It takes a string
with wildcards allowed and warnings will be matched against it (if
rule and file also match). If it matches, the warning is waived.

Fixes #1649
wallento added a commit that referenced this issue Jan 12, 2020
Add configuration file switch '-match' for lint_off. It takes a string
with wildcards allowed and warnings will be matched against it (if
rule and file also match). If it matches, the warning is waived.

Fixes #1649
wallento added a commit that referenced this issue Jan 12, 2020
Add configuration file switch '-match' for lint_off. It takes a string
with wildcards allowed and warnings will be matched against it (if
rule and file also match). If it matches, the warning is waived.

Fixes #1649
@wallento wallento closed this in fad465a Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.