Open MikeOpenHWGroup opened 2 years ago
This is the exception to the rule. We have a cfg parameter that is random constrained: cfg.drv_idle_mode
. One of the options is to drive random data on idle.
I think it's a waive, on a case-per-case basis
Can you give a bit more context on this one @datum-dpoulin?
"... lint error because class members randomized with these calls cannot be constrained"
cfg.drv_idle_mode is being constrained to create this random behavior. That's why I'd say that this is the exception to the linter rule.
"I also worry that invocations of $urandom() may impair random stability (that is, the ability to reproduce results from the same seed)."
Why would $urandom() be different than other randomization functions?
FYI the new OBI agent architecture doesn't have problems like this because the driver gets a new sequence item from the sequencer on every clock. Just puts the contents onto the wires.
I believe you are right @datum-dpoulin, it looks like an exception safe to waive. You do have the drv_idle_mode knob that already controls the behavior... to some extent... in the worst case "a control freak" could use factories to override uvma_obi_memory_drv_c with a child class that overrides uvma_obi_memory_drv_c::drv_mstr_idle(). Of course it would have been nicer to only have to override a drv_mstr_idle_random() containing just the UVMA_OBI_MEMORY_DRV_IDLE_RANDOM case item code that sets $urandom() values, but it could be over-engineering, maybe a comment with the solution is enough.
+You could use some inline code waivers here: https://dvteclipse.com/documentation/svlinter/Inline_Lint_Waivers.html
I don't think random stability is an issue.
OK, I'll roll over on this one. Please add an inline waiver as shown above.
So, that covers the example in the original issue report. There are still 7 other files with SVTB.29.1.3.1 lint errors.
@strichmo, I think you have already addressed the ones attributed to you. @datum-dpoulin and @silabs-oysteink, please have a look at yours.
I did not address this before my departure but here's my 2 cents:
OK, it appears that a consensus is developing that in these instances it is safe to waive SVTB.29.1.3.1, and in some cases desirable from a performance standpoint. I would like to add guidance on this in our newly minted coding style guidelines. The Verissimo rule is:
Do not use $urandom, $urandom_range $random, $dist_uniform, $dist_normal, $dist_exponential,
$dist_poisson, $dist_chi_square, $dist_t, $dist_erlang because members randomized with these
calls cannot be constrained.
How about we add this to our coding stlye guidelines?
Use of $urandom, $urandom_range $random, $dist_uniform, $dist_normal, $dist_exponential,
$dist_poisson, $dist_chi_square, $dist_t, $dist_erlang should be avoided because members randomized
with these calls cannot be constrained.
This rule may be waived if the randomization of a member using one of these system methods is controlled
by a member that is constrainable. For example:
if (cfg.drive_random_rdata) begin
slv_mp.drv_slv_cb.rdata <= $urandom();
end
Note that the use of the verbs "should" and "may" is consistent with the guidelines.
In core-v-verif there are 8 files that use some variation of
$urandom()
. You are in the "assignees" list of this issue because git believes you authored one or more of these.The Verissimo linter flags these as violation SVTB.29.1.3.1, which is a lint error because class members randomized with these calls cannot be constrained. I also worry that invocations of $urandom() may impair random stability (that is, the ability to reproduce results from the same seed).
Possible resolutions:
Example fixes
From uvma_obi_memory_drv.sv. Replace this:
...with this:
Steps to Reproduce
Lint checks are run every six hours - the link above will show all violations of this rule.
Additional context
This is a set of linter issues identified by the Verissimo SystemVerilog Testbench Linter. Please reach out to me directly if you have questions about the tool.