-
Notifications
You must be signed in to change notification settings - Fork 390
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
Net type in verilog models not explicitly declared #198
Comments
Originally posted by @tangxifan in #181 (comment) |
FYI, this `default_nettype statement is not required and should be just removed. What created the bug was setting it to "none", but there is no need to setting it to "wire" either. This is usually what is done in traditional libraries. |
Verilog has an implicit default net type of Using
or
Sure, it's possible to just remote the default_nettype statements, but there is likely to be certain cells which require declaring the type anyways (e.g. FFs). One could argue that cell libraries (should) never use multi-bit wires, so the point is moot. I still think that using |
I would tend to disagree here, as I think we should follow what is commonly done in standard cell development. The |
Just because it is typical does not mean it is safe. On one side I see "atypical but safer" and on the other side I see "typical but less safe". Is there any compelling reason to use implicit types (e.g. tool compatibility, correctness, etc.)? Also it's not the ports that need to be declared in FFs, but the internal wires. |
I firmly agree with Edouard. Implicit wires are used everywhere in verilog by everyone. Like everything else in any programming languages, it is just one of the rules you need to remember. There are a vast number of ways to produce verilog that is syntactically correct but functionally dead; failing to explicitly declare wire arrays is just one of them. Also: There are multi-bit wires used in SkyWater standard cells. Only a handful, but see, e.g., muxb16to1 in the HDLL library. |
I don't necessarily disagree with the change as much as I disagree with the reasoning. C++ code is littered with off by 1 errors and unsafe memory accesses. It's something that's "accepted" from the language but yet it causes 70% of security vulnerabilities. It is well known that languages like C++ allow you to shoot yourself in the foot, and as a result there has been a ton of work on linters and sanitizers to try to detect code which is allowed by the language specification but has a high propensity to be incorrect. Why is Verilog any different? There are tons of ways to shoot yourself in the foot with Verilog. I can understand the reasoning of "we don't need this safety feature", assumingly because the potential issues are restricted to a small space, but I can't get behind the reasoning of "this is how everyone does it" or "the language has many other problems so this one isn't worth checking".
Not me 😉 |
Agreeing with Austin on the subject of implicit wires. This is a bad
practice and has been forbidden on my team for 20+ years.
M
…On Fri, Nov 6, 2020 at 1:39 PM Austin Rovinski ***@***.***> wrote:
I don't necessarily disagree with the change as much as I disagree with
the reasoning.
C++ code is littered with off by 1 errors and unsafe memory accesses. It's
something that's "accepted" from the language but yet it causes 70% of
security vulnerabilities
<https://www.zdnet.com/article/microsoft-70-percent-of-all-security-bugs-are-memory-safety-issues/>.
It is well known that languages like C++ allow you to shoot yourself in the
foot, and as a result there has been a ton of work on linters and
sanitizers to try to detect code which is allowed by the language
specification but has a high propensity to be incorrect.
Why is Verilog any different? There are tons of ways to shoot yourself in
the foot with Verilog. default_nettype none is a safety feature exercised
in order to prevent bugs caused by implicit wires. Why would we not
exercise a safety feature if it exists?
I can understand the reasoning of "we don't need *this* safety feature",
assumingly because the potential issues are restricted to a small space,
but I can't get behind the reasoning of "this is how everyone does it" or
"the language has many other problems so this one isn't worth checking".
Implicit wires are used everywhere in verilog by everyone
Not me 😉
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#198 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEFG5AHI7ACP66NUV7R6LH3SORUIZANCNFSM4TGXY7TQ>
.
|
There should not be any implicit wires in the skywater-pdk, any that exist are an error. This is why the file does; `default_nettype none |
FYI, we have an outstanding feature request for a style lint rule on implicit declarations at chipsalliance/verible#217. |
@mithro : I was not aware either that `default_nettype none was supported by iverilog, or that all of the verilog files in the PDK had been corrected for implicit wires. That probably explains why I was seeing errors generated when simulating the I/Os where there were implicit wires. In that case, all is good. |
I am struggling to avoid/ fix this issue, when trying to use Mentor Modelsim. Since The port declaration of the functional and power models of the cells are written in the Verilog-1995 style, which allows implicit wire port types. I do not quite see why multi-bit wires could be implicitly truncated to a single-bit wire as mentioned by @rovinski. The verilog functional models explicitly say "Autogenerated, do not modify directly". So I guess it does not make sense to add a wire declaration for each port inside the cell libraries or set |
This would be very good to get fixed. I have now added FuseSoC support to run the caravel regression tests that use this library, but it only works on icarus. All other simulators complain about this. Can we just do a mass search-and-replace to add wire types? Or regenerate the verilog from some other source? |
If we could get the PDK libraries to explicitly declare wires, then this discussion would be resolved. Better yet, just migrate to the Verilog 2001 convention of fully qualified port names. |
In #181 @tangxifan reported two issues. The include file issue was fixed but the other one was not.
Actual Behavior
Modelsim complains about the following errors:
The text was updated successfully, but these errors were encountered: