From 4d95f6f7b8052ca47a8eba10b3a9507d83dc439a Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Mon, 11 Nov 2024 20:00:26 -0500 Subject: [PATCH] Add `--waiver-multiline` for context-sensitive `--waiver-output`. --- Changes | 1 + bin/verilator | 3 +- docs/guide/exe_verilator.rst | 9 ++++ src/V3Ast.cpp | 13 ++--- src/V3Config.cpp | 6 ++- src/V3Error.cpp | 2 + src/V3FileLine.cpp | 34 ++++++++---- src/V3Options.cpp | 1 + src/V3Options.h | 2 + src/V3Waiver.cpp | 53 ++++++++++++++++--- src/V3Waiver.h | 2 +- test_regress/t/t_waiveroutput.out | 9 ++-- test_regress/t/t_waiveroutput.py | 2 +- test_regress/t/t_waiveroutput_allgood.out | 6 +-- test_regress/t/t_waiveroutput_allgood.py | 4 +- test_regress/t/t_waiveroutput_allgood.vlt | 11 ---- test_regress/t/t_waiveroutput_multiline.out | 11 ++++ test_regress/t/t_waiveroutput_multiline.py | 14 ++--- ...ut_wall.py => t_waiveroutput_roundtrip.py} | 10 ++-- ...multiline.v => t_waiveroutput_roundtrip.v} | 0 test_regress/t/t_waiveroutput_wall.out | 10 ---- test_regress/t/t_waiveroutput_wall.vlt | 11 ---- 22 files changed, 133 insertions(+), 81 deletions(-) delete mode 100644 test_regress/t/t_waiveroutput_allgood.vlt create mode 100644 test_regress/t/t_waiveroutput_multiline.out rename test_regress/t/{t_waiveroutput_wall.py => t_waiveroutput_roundtrip.py} (63%) rename test_regress/t/{t_waiveroutput_multiline.v => t_waiveroutput_roundtrip.v} (100%) delete mode 100644 test_regress/t/t_waiveroutput_wall.out delete mode 100644 test_regress/t/t_waiveroutput_wall.vlt diff --git a/Changes b/Changes index c958b258a..ea35c34c9 100644 --- a/Changes +++ b/Changes @@ -17,6 +17,7 @@ Verilator 5.031 devel * Support basic constrained random for multi-dimensional dynamic array and queue (#5591). [Yilou Wang] * Support `pure constraint`. * Add `--no-std-package` as subset-alias of `--no-std`. +* Add `--waiver-multiline` for context-sensitive `--waiver-output`. * Add error on illegal enum base type (#3010). [Iztok Jeras] * Add error on `wait` with missing `.triggered` (#4457). * Add error when improperly storing to parameter (#5147). [Gökçe Aydos] diff --git a/bin/verilator b/bin/verilator index fea7d9008..28acbbcf6 100755 --- a/bin/verilator +++ b/bin/verilator @@ -488,7 +488,8 @@ detailed descriptions of these arguments. +verilog2001ext+ Synonym for +1364-2001ext+ --version Show program version and exits --vpi Enable VPI compiles - --waiver-output Create a waiver file based on the linter warnings + --waiver-multiline Create multiline --match for waivers + --waiver-output Create a waiver file based on linter warnings -Wall Enable all style warnings -Werror- Convert warnings to errors -Wfuture- Disable unknown message warnings diff --git a/docs/guide/exe_verilator.rst b/docs/guide/exe_verilator.rst index d6b979c81..4108f76e4 100644 --- a/docs/guide/exe_verilator.rst +++ b/docs/guide/exe_verilator.rst @@ -1649,6 +1649,15 @@ Summary: Enable the use of VPI and linking against the :file:`verilated_vpi.cpp` files. +.. option:: --waiver-multiline + + When using :vlopt:`--waiver-output \`, include a match + expression that includes the entire multiline error message as a match + regular expression, as opposed to the default of only matching the first + line of the error message. This provides a starting point for creating + complex waivers, but such generated waivers will likely require editing + for brevity before being reused. + .. option:: --waiver-output Generate a waiver file that contains all waiver statements to suppress diff --git a/src/V3Ast.cpp b/src/V3Ast.cpp index e0a15b40d..9a3ce06e8 100644 --- a/src/V3Ast.cpp +++ b/src/V3Ast.cpp @@ -1423,8 +1423,13 @@ string AstNode::instanceStr() const { return ""; } void AstNode::v3errorEnd(std::ostringstream& str) const VL_RELEASE(V3Error::s().m_mutex) { + // Don't look for instance name when warning is disabled. + // In case of large number of warnings, this can + // take significant amount of time + const string instanceStrExtra + = m_fileline->warnIsOff(V3Error::s().errorCode()) ? "" : instanceStr(); if (!m_fileline) { - V3Error::v3errorEnd(str, instanceStr()); + V3Error::v3errorEnd(str, instanceStrExtra); } else { std::ostringstream nsstr; nsstr << str.str(); @@ -1434,11 +1439,7 @@ void AstNode::v3errorEnd(std::ostringstream& str) const VL_RELEASE(V3Error::s(). const_cast(this)->dump(nsstr); nsstr << endl; } - // Don't look for instance name when warning is disabled. - // In case of large number of warnings, this can - // take significant amount of time - m_fileline->v3errorEnd( - nsstr, m_fileline->warnIsOff(V3Error::s().errorCode()) ? "" : instanceStr()); + m_fileline->v3errorEnd(nsstr, instanceStrExtra); } } void AstNode::v3errorEndFatal(std::ostringstream& str) const VL_RELEASE(V3Error::s().m_mutex) { diff --git a/src/V3Config.cpp b/src/V3Config.cpp index b90855564..d561570f5 100644 --- a/src/V3Config.cpp +++ b/src/V3Config.cpp @@ -300,7 +300,11 @@ public: m_lastIgnore.it = m_ignLines.begin(); } void addIgnoreMatch(V3ErrorCode code, const string& match) { - m_waivers.emplace_back(code, match); + // Since Verilator 5.031 the error message compared has context, so + // allow old rules to still match using a final '*' + string newMatch = match; + if (newMatch.empty() || newMatch.back() != '*') newMatch += '*'; + m_waivers.emplace_back(code, newMatch); } void applyBlock(AstNodeBlock* nodep) { diff --git a/src/V3Error.cpp b/src/V3Error.cpp index 60164e66c..d874ed61d 100644 --- a/src/V3Error.cpp +++ b/src/V3Error.cpp @@ -117,6 +117,8 @@ void V3ErrorGuarded::suppressThisWarning() VL_REQUIRES(m_mutex) { // cppcheck-has-bug-suppress constParameter void V3ErrorGuarded::v3errorEnd(std::ostringstream& sstr, const string& extra) VL_REQUIRES(m_mutex) { + // 'extra' is appended to the message, and is is excluded in check for + // duplicate messages. Currently used for reporting instance name. #if defined(__COVERITY__) || defined(__cppcheck__) if (m_errorCode == V3ErrorCode::EC_FATAL) __coverity_panic__(x); #endif diff --git a/src/V3FileLine.cpp b/src/V3FileLine.cpp index 138707039..fd3c04b08 100644 --- a/src/V3FileLine.cpp +++ b/src/V3FileLine.cpp @@ -413,24 +413,36 @@ bool FileLine::warnIsOff(V3ErrorCode code) const { // cppverilator-suppress constParameter void FileLine::v3errorEnd(std::ostringstream& sstr, const string& extra) VL_RELEASE(V3Error::s().m_mutex) { - std::ostringstream nsstr; + // 'extra' is appended to the message, and is is excluded in check for + // duplicate messages. Currently used for reporting instance name. + std::ostringstream nsstr; // sstr with fileline prefix and context + std::ostringstream wsstr; // sstr for waiver (no fileline) with context if (lastLineno()) nsstr << this; nsstr << sstr.str(); + wsstr << sstr.str(); nsstr << "\n"; - std::ostringstream lstr; + wsstr << "\n"; + std::ostringstream extrass; // extra spaced out for prefix if (!extra.empty()) { - lstr << std::setw(ascii().length()) << " " - << ": " << extra; + extrass << std::setw(ascii().length()) << " " + << ": " << extra; } - m_waive = V3Config::waive(this, V3Error::s().errorCode(), sstr.str()); - if (warnIsOff(V3Error::s().errorCode()) || m_waive) { + if (warnIsOff(V3Error::s().errorCode())) { V3Error::s().suppressThisWarning(); - } else if (!V3Error::s().errorContexted()) { - nsstr << warnContextPrimary(); + } else { + if (!V3Error::s().errorContexted()) { + const string add = warnContextPrimary(); + wsstr << add; + nsstr << add; + } + m_waive = V3Config::waive(this, V3Error::s().errorCode(), wsstr.str()); + if (m_waive) { + V3Error::s().suppressThisWarning(); + } else { + V3Waiver::addEntry(V3Error::s().errorCode(), filename(), wsstr.str()); + } } - if (!warnIsOff(V3Error::s().errorCode()) && !m_waive) - V3Waiver::addEntry(V3Error::s().errorCode(), filename(), sstr.str()); - V3Error::v3errorEnd(nsstr, lstr.str()); + V3Error::v3errorEnd(nsstr, extrass.str()); } string FileLine::warnMore() const VL_REQUIRES(V3Error::s().m_mutex) { diff --git a/src/V3Options.cpp b/src/V3Options.cpp index 6d59c99d1..47f60520b 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -1707,6 +1707,7 @@ void V3Options::parseOptsList(FileLine* fl, const string& optdir, int argc, FileLine::globalWarnOff(V3ErrorCode::WIDTH, false); V3Error::pretendError(V3ErrorCode::WIDTH, false); }); + DECL_OPTION("-waiver-multiline", OnOff, &m_waiverMultiline); DECL_OPTION("-waiver-output", Set, &m_waiverOutput); DECL_OPTION("-x-assign", CbVal, [this, fl](const char* valp) { diff --git a/src/V3Options.h b/src/V3Options.h index 74de94871..3b3b6af5e 100644 --- a/src/V3Options.h +++ b/src/V3Options.h @@ -297,6 +297,7 @@ private: bool m_underlineZero = false; // main switch: --underline-zero; undocumented old Verilator 2 bool m_verilate = true; // main switch: --verilate bool m_vpi = false; // main switch: --vpi + bool m_waiverMultiline = false; // main switch: --waiver-multiline bool m_xInitialEdge = false; // main switch: --x-initial-edge bool m_xmlOnly = false; // main switch: --xml-only bool m_jsonOnly = false; // main switch: --json-only @@ -544,6 +545,7 @@ public: bool reportUnoptflat() const { return m_reportUnoptflat; } bool verilate() const { return m_verilate; } bool vpi() const { return m_vpi; } + bool waiverMultiline() const { return m_waiverMultiline; } bool xInitialEdge() const { return m_xInitialEdge; } bool xmlOnly() const { return m_xmlOnly; } bool jsonOnly() const { return m_jsonOnly; } diff --git a/src/V3Waiver.cpp b/src/V3Waiver.cpp index bd4ad0883..ca8d26a73 100644 --- a/src/V3Waiver.cpp +++ b/src/V3Waiver.cpp @@ -19,21 +19,58 @@ #include "V3Waiver.h" #include "V3File.h" +#include "V3Global.h" #include "V3Options.h" #include #include -void V3Waiver::addEntry(V3ErrorCode errorCode, const std::string& filename, const std::string& str) +void V3Waiver::addEntry(V3ErrorCode errorCode, const std::string& filename, const std::string& msg) VL_MT_SAFE_EXCLUDES(s_mutex) { if (filename == V3Options::getStdPackagePath()) return; const V3LockGuard lock{s_mutex}; + + string trimmsg = msg; + if (!v3Global.opt.waiverMultiline()) { + const size_t pos = trimmsg.find('\n'); + trimmsg = trimmsg.substr(0, pos); + if (pos != std::string::npos) trimmsg += '*'; + } + { // Remove line numbers and context "\n [0-9] | ", "\n ^[~]+" + string result; + for (const char* cp = trimmsg.c_str(); *cp; cp = *cp ? cp + 1 : cp) { + while (*cp == ' ' || isdigit(*cp)) ++cp; + if (*cp == '|') ++cp; + // ^~~~~ + while (*cp == ' ' || *cp == '^') ++cp; + while (*cp == '~') ++cp; + while (*cp && *cp != '\n') result += *cp++; + while (*cp == '\n') result += *cp++; + } + trimmsg = result; + } + trimmsg += '*'; + { // "\n"->"*", " *"->"*", "* "->"*" + string result; + string add; + result.reserve(trimmsg.size()); + for (const char& c : trimmsg) { + if (c == '*' || !std::isprint(c)) { + add = "*"; + } else if (c == ' ') { + if (add != "*") add += c; + } else { + result += add + c; + add = ""; + } + } + result += add; + trimmsg = result; + } + std::stringstream entry; - const size_t pos = str.find('\n'); entry << "lint_off -rule " << errorCode.ascii() << " -file \"*" << filename << "\" -match \"" - << str.substr(0, pos); - if (pos != std::string::npos) entry << "*"; - entry << "\""; + << trimmsg << "\""; s_waiverList.push_back(entry.str()); } @@ -46,9 +83,9 @@ void V3Waiver::write(const std::string& filename) VL_MT_SAFE_EXCLUDES(s_mutex) { *ofp << "`verilator_config\n\n"; - *ofp << "// Below you find suggested waivers. You have three options:\n"; - *ofp << "// 1. Fix the reason for the linter warning\n"; - *ofp << "// 2. Keep the waiver permanently if you are sure this is okay\n"; + *ofp << "// Below are suggested waivers. You have three options:\n"; + *ofp << "// 1. Fix the reason for the linter warning in the Verilog sources\n"; + *ofp << "// 2. Keep the waiver permanently if you are sure it is okay\n"; *ofp << "// 3. Keep the waiver temporarily to suppress the output\n\n"; const V3LockGuard lock{s_mutex}; diff --git a/src/V3Waiver.h b/src/V3Waiver.h index 8ccc1f8e9..af2d415cc 100644 --- a/src/V3Waiver.h +++ b/src/V3Waiver.h @@ -30,7 +30,7 @@ class V3Waiver final { static WaiverList s_waiverList VL_GUARDED_BY(s_mutex); public: - static void addEntry(V3ErrorCode errorCode, const string& filename, const std::string& str) + static void addEntry(V3ErrorCode errorCode, const string& filename, const std::string& msg) VL_MT_SAFE_EXCLUDES(s_mutex); static void write(const std::string& filename) VL_MT_SAFE_EXCLUDES(s_mutex); }; diff --git a/test_regress/t/t_waiveroutput.out b/test_regress/t/t_waiveroutput.out index 673c0445a..e111709f1 100644 --- a/test_regress/t/t_waiveroutput.out +++ b/test_regress/t/t_waiveroutput.out @@ -2,9 +2,10 @@ `verilator_config -// Below you find suggested waivers. You have three options: -// 1. Fix the reason for the linter warning -// 2. Keep the waiver permanently if you are sure this is okay +// Below are suggested waivers. You have three options: +// 1. Fix the reason for the linter warning in the Verilog sources +// 2. Keep the waiver permanently if you are sure it is okay // 3. Keep the waiver temporarily to suppress the output -// No waivers needed - great! +// lint_off -rule UNUSEDSIGNAL -file "*t/t_waiveroutput.v" -match "Signal is not used: 'width_warn'*" + diff --git a/test_regress/t/t_waiveroutput.py b/test_regress/t/t_waiveroutput.py index 67b6355da..df3799f05 100755 --- a/test_regress/t/t_waiveroutput.py +++ b/test_regress/t/t_waiveroutput.py @@ -15,7 +15,7 @@ test.top_filename = "t/t_waiveroutput.v" out_filename = test.obj_dir + "/" + test.name + ".waiver_gen.out" waiver_filename = "t/" + test.name + ".vlt" -test.compile(v_flags2=[waiver_filename, '--waiver-output', out_filename]) +test.lint(v_flags2=[waiver_filename, '-Wall', '-Wno-fatal', '--waiver-output', out_filename]) test.files_identical(out_filename, test.golden_filename) diff --git a/test_regress/t/t_waiveroutput_allgood.out b/test_regress/t/t_waiveroutput_allgood.out index 673c0445a..6cba8ab6b 100644 --- a/test_regress/t/t_waiveroutput_allgood.out +++ b/test_regress/t/t_waiveroutput_allgood.out @@ -2,9 +2,9 @@ `verilator_config -// Below you find suggested waivers. You have three options: -// 1. Fix the reason for the linter warning -// 2. Keep the waiver permanently if you are sure this is okay +// Below are suggested waivers. You have three options: +// 1. Fix the reason for the linter warning in the Verilog sources +// 2. Keep the waiver permanently if you are sure it is okay // 3. Keep the waiver temporarily to suppress the output // No waivers needed - great! diff --git a/test_regress/t/t_waiveroutput_allgood.py b/test_regress/t/t_waiveroutput_allgood.py index fd20a2c37..502abc15b 100755 --- a/test_regress/t/t_waiveroutput_allgood.py +++ b/test_regress/t/t_waiveroutput_allgood.py @@ -13,9 +13,9 @@ test.scenarios('vlt') test.top_filename = "t/t_waiveroutput.v" out_filename = test.obj_dir + "/" + test.name + ".waiver_gen.vlt" -waiver_filename = "t/" + test.name + ".vlt" -test.compile(v_flags2=[waiver_filename, '--waiver-output', out_filename]) +# Note no Wall +test.lint(v_flags2=['-Wno-WIDTH', '--waiver-output', out_filename]) test.files_identical(out_filename, test.golden_filename) diff --git a/test_regress/t/t_waiveroutput_allgood.vlt b/test_regress/t/t_waiveroutput_allgood.vlt deleted file mode 100644 index 54c5798f2..000000000 --- a/test_regress/t/t_waiveroutput_allgood.vlt +++ /dev/null @@ -1,11 +0,0 @@ -// DESCRIPTION: Verilator: Verilog Test module -// -// This file ONLY is placed under the Creative Commons Public Domain, for -// any use, without warranty, 2020 by Wilson Snyder. -// SPDX-License-Identifier: CC0-1.0 - -`verilator_config - -lint_off -rule WIDTH -file "*t/t_waiveroutput.v" -match "Operator ASSIGN expects 1 bits on the Assign RHS, but Assign RHS's CONST '2'h3' generates 2 bits." - -lint_off -rule UNUSED -file "*t/t_waiveroutput.v" -match "Signal is not used: 'width_warn'" diff --git a/test_regress/t/t_waiveroutput_multiline.out b/test_regress/t/t_waiveroutput_multiline.out new file mode 100644 index 000000000..ecfc68bf5 --- /dev/null +++ b/test_regress/t/t_waiveroutput_multiline.out @@ -0,0 +1,11 @@ +// DESCRIPTION: Verilator output: Waivers generated with --waiver-output + +`verilator_config + +// Below are suggested waivers. You have three options: +// 1. Fix the reason for the linter warning in the Verilog sources +// 2. Keep the waiver permanently if you are sure it is okay +// 3. Keep the waiver temporarily to suppress the output + +// lint_off -rule UNUSEDSIGNAL -file "*t/t_waiveroutput.v" -match "Signal is not used: 'width_warn'*reg width_warn = 2'b11;*" + diff --git a/test_regress/t/t_waiveroutput_multiline.py b/test_regress/t/t_waiveroutput_multiline.py index 3f3414f68..ddbe93341 100755 --- a/test_regress/t/t_waiveroutput_multiline.py +++ b/test_regress/t/t_waiveroutput_multiline.py @@ -10,15 +10,15 @@ import vltest_bootstrap test.scenarios('vlt') +test.top_filename = "t/t_waiveroutput.v" -out_filename = test.obj_dir + "/" + test.name + "_waiver_gen.vlt" -waiver_filename = test.obj_dir + "/" + test.name + "_waiver.vlt" +out_filename = test.obj_dir + "/" + test.name + ".waiver_gen.out" +waiver_filename = "t/t_waiveroutput.vlt" -test.compile(v_flags2=['--waiver-output', out_filename], fails=True) +test.lint(v_flags2=[ + waiver_filename, '-Wall', '-Wno-fatal', '--waiver-multiline', '--waiver-output', out_filename +]) -test.file_sed(out_filename, waiver_filename, - lambda line: re.sub(r'\/\/ lint_off', 'lint_off', line)) - -test.compile(v_flags2=[waiver_filename]) +test.files_identical(out_filename, test.golden_filename) test.passes() diff --git a/test_regress/t/t_waiveroutput_wall.py b/test_regress/t/t_waiveroutput_roundtrip.py similarity index 63% rename from test_regress/t/t_waiveroutput_wall.py rename to test_regress/t/t_waiveroutput_roundtrip.py index 9128c7d89..39834358b 100755 --- a/test_regress/t/t_waiveroutput_wall.py +++ b/test_regress/t/t_waiveroutput_roundtrip.py @@ -10,13 +10,15 @@ import vltest_bootstrap test.scenarios('vlt') -test.top_filename = "t/t_waiveroutput.v" out_filename = test.obj_dir + "/" + test.name + ".waiver_gen.out" -waiver_filename = "t/" + test.name + ".vlt" +waiver_filename = test.obj_dir + "/" + test.name + "_waiver.vlt" -test.compile(v_flags2=['-Wall', waiver_filename, '--waiver-output', out_filename]) +test.lint(v_flags2=['-Wall', '-Wno-fatal', '--waiver-output', out_filename]) -test.files_identical(out_filename, test.golden_filename) +test.file_sed(out_filename, waiver_filename, + lambda line: re.sub(r'\/\/ lint_off', 'lint_off', line)) + +test.lint(v_flags2=[waiver_filename]) test.passes() diff --git a/test_regress/t/t_waiveroutput_multiline.v b/test_regress/t/t_waiveroutput_roundtrip.v similarity index 100% rename from test_regress/t/t_waiveroutput_multiline.v rename to test_regress/t/t_waiveroutput_roundtrip.v diff --git a/test_regress/t/t_waiveroutput_wall.out b/test_regress/t/t_waiveroutput_wall.out deleted file mode 100644 index 673c0445a..000000000 --- a/test_regress/t/t_waiveroutput_wall.out +++ /dev/null @@ -1,10 +0,0 @@ -// DESCRIPTION: Verilator output: Waivers generated with --waiver-output - -`verilator_config - -// Below you find suggested waivers. You have three options: -// 1. Fix the reason for the linter warning -// 2. Keep the waiver permanently if you are sure this is okay -// 3. Keep the waiver temporarily to suppress the output - -// No waivers needed - great! diff --git a/test_regress/t/t_waiveroutput_wall.vlt b/test_regress/t/t_waiveroutput_wall.vlt deleted file mode 100644 index 54c5798f2..000000000 --- a/test_regress/t/t_waiveroutput_wall.vlt +++ /dev/null @@ -1,11 +0,0 @@ -// DESCRIPTION: Verilator: Verilog Test module -// -// This file ONLY is placed under the Creative Commons Public Domain, for -// any use, without warranty, 2020 by Wilson Snyder. -// SPDX-License-Identifier: CC0-1.0 - -`verilator_config - -lint_off -rule WIDTH -file "*t/t_waiveroutput.v" -match "Operator ASSIGN expects 1 bits on the Assign RHS, but Assign RHS's CONST '2'h3' generates 2 bits." - -lint_off -rule UNUSED -file "*t/t_waiveroutput.v" -match "Signal is not used: 'width_warn'"