From 3c964147bec515d625887e40e94c6148a873e234 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sat, 1 Jul 2023 10:45:25 -0400 Subject: [PATCH] Add MISINDENT lint warning for misleading indentation. --- Changes | 1 + docs/guide/exe_verilator.rst | 9 ++-- docs/guide/warnings.rst | 45 ++++++++++++++++++++ src/V3Error.h | 9 ++-- src/V3FileLine.cpp | 6 +++ src/V3FileLine.h | 1 + src/V3LinkParse.cpp | 56 +++++++++++++++++++++++++ src/V3String.cpp | 9 ++++ src/V3String.h | 2 + test_regress/t/t_lint_misindent_bad.out | 27 ++++++++++++ test_regress/t/t_lint_misindent_bad.pl | 20 +++++++++ test_regress/t/t_lint_misindent_bad.v | 48 +++++++++++++++++++++ 12 files changed, 225 insertions(+), 8 deletions(-) create mode 100644 test_regress/t/t_lint_misindent_bad.out create mode 100755 test_regress/t/t_lint_misindent_bad.pl create mode 100644 test_regress/t/t_lint_misindent_bad.v diff --git a/Changes b/Changes index e3086370a..ce964fe9a 100644 --- a/Changes +++ b/Changes @@ -16,6 +16,7 @@ Verilator 5.013 devel * Deprecation planned for 32-bit pointer -m32 mode (#4268). * Support some stream operations on queues (#4292). [Ryszard Rozak, Antmicro Ltd] * Add GENUNNAMED lint warning. [Srinivasan Venkataramanan, Deepa Palaniappan] +* Add MISINDENT lint warning for misleading indentation. * Fix 'VlForkSync' redeclaration (#4277). [Krzysztof Bieganski, Antmicro Ltd] * Fix processes that can outlive their parents (#4253). [Krzysztof Boronski, Antmicro Ltd] * Fix duplicate fork names (#4295). [Ryszard Rozak, Antmicro Ltd] diff --git a/docs/guide/exe_verilator.rst b/docs/guide/exe_verilator.rst index faede475a..6ec962e1e 100644 --- a/docs/guide/exe_verilator.rst +++ b/docs/guide/exe_verilator.rst @@ -1562,10 +1562,11 @@ Summary: Enable all lint-related warning messages (note that by default, they are already enabled), but do not affect style messages. This is equivalent to - ``-Wwarn-ALWCOMBORDER -Wwarn-BSSPACE -Wwarn-CASEINCOMPLETE - -Wwarn-CASEOVERLAP -Wwarn-CASEX -Wwarn-CASTCONST -Wwarn-CASEWITHX -Wwarn-CMPCONST - -Wwarn-COLONPLUS -Wwarn-IMPLICIT -Wwarn-ASCRANGE - -Wwarn-PINMISSING -Wwarn-REALCVT -Wwarn-UNSIGNED -Wwarn-WIDTH``. + ``-Wwarn-ALWCOMBORDER -Wwarn-ASCRANGE -Wwarn-BSSPACE -Wwarn-CASEINCOMPLETE + -Wwarn-CASEOVERLAP -Wwarn-CASEWITHX -Wwarn-CASEX -Wwarn-CASTCONST -Wwarn-CMPCONST + -Wwarn-COLONPLUS -Wwarn-IMPLICIT -Wwarn-IMPLICITSTATIC -Wwarn-LATCH -Wwarn-MISINDENT + -Wwarn-NEWERSTD -Wwarn-PINMISSING -Wwarn-REALCVT -Wwarn-STATICVAR -Wwarn-UNSIGNED + -Wwarn-WIDTHTRUNC -Wwarn-WIDTHEXPAND -Wwarn-WIDTHXZEXPAND``. .. option:: -Wwarn-style diff --git a/docs/guide/warnings.rst b/docs/guide/warnings.rst index 94af80a31..21c582de9 100644 --- a/docs/guide/warnings.rst +++ b/docs/guide/warnings.rst @@ -1039,6 +1039,51 @@ List Of Warnings unsupported. Verilator uses only the typical delay value. +.. option:: MISINDENT + + Warns that the indentation of a statement is misleading, suggesting the + statement is part of a previous :code:`if` or :code:`while` block while + it is not. + + Verilator suppresses this check when there is an inconsistent mix of + spaces and tabs, as it cannot ensure the width of tabs. Verilator also + ignores blocks with :code:`begin`/:code:`end`, as the :code:`end` + visually indicates the earlier statement's end. + + Ignoring this warning will only suppress the lint check; it will + simulate correctly. + + For example + + .. code-block:: sv + :linenos: + :emphasize-lines: 3 + + if (something) + statement_in_if; + statement_not_in_if; //<--- Warning + + Results in: + + .. code-block:: + + %Warning-MISINDENT: example.v:3:9: Misleading indentation + + To fix this repair the indentation to match the correct earlier + statement, for example: + + .. code-block:: sv + :linenos: + :emphasize-lines: 3 + + if (something) + statement_in_if; + statement_not_in_if; //<--- Repaired + + Other tools with similar warnings: GCC -Wmisleading-indentation, + clang-tidy readability-misleading-indentation. + + .. option:: MODDUP .. TODO better example diff --git a/src/V3Error.h b/src/V3Error.h index 264f8c0c1..5f402e481 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -115,6 +115,7 @@ public: LATCH, // Latch detected outside of always_latch block LITENDIAN, // Little endian, renamed to ASCRANGE MINTYPMAXDLY, // Unsupported: min/typ/max delay expressions + MISINDENT, // Misleading indentation MODDUP, // Duplicate module MULTIDRIVEN, // Driven from multiple blocks MULTITOP, // Multiple top level modules @@ -198,7 +199,7 @@ public: "IFDEPTH", "IGNOREDRETURN", "IMPERFECTSCH", "IMPLICIT", "IMPLICITSTATIC", "IMPORTSTAR", "IMPURE", "INCABSPATH", "INFINITELOOP", "INITIALDLY", "INSECURE", - "LATCH", "LITENDIAN", "MINTYPMAXDLY", "MODDUP", + "LATCH", "LITENDIAN", "MINTYPMAXDLY", "MISINDENT", "MODDUP", "MULTIDRIVEN", "MULTITOP", "NEWERSTD", "NOLATCH", "NULLPORT", "PINCONNECTEMPTY", "PINMISSING", "PINNOCONNECT", "PINNOTFOUND", "PKGNODECL", "PROCASSWIRE", "PROFOUTOFDATE", "PROTECTED", "RANDC", "REALCVT", "REDEFMACRO", "RISEFALLDLY", @@ -237,9 +238,9 @@ public: return (m_e == ALWCOMBORDER || m_e == ASCRANGE || m_e == BSSPACE || m_e == CASEINCOMPLETE || m_e == CASEOVERLAP || m_e == CASEWITHX || m_e == CASEX || m_e == CASTCONST || m_e == CMPCONST || m_e == COLONPLUS || m_e == IMPLICIT || m_e == IMPLICITSTATIC - || m_e == LATCH || m_e == NEWERSTD || m_e == PINMISSING || m_e == REALCVT - || m_e == STATICVAR || m_e == UNSIGNED || m_e == WIDTH || m_e == WIDTHTRUNC - || m_e == WIDTHEXPAND || m_e == WIDTHXZEXPAND); + || m_e == LATCH || m_e == MISINDENT || m_e == NEWERSTD || m_e == PINMISSING + || m_e == REALCVT || m_e == STATICVAR || m_e == UNSIGNED || m_e == WIDTH + || m_e == WIDTHTRUNC || m_e == WIDTHEXPAND || m_e == WIDTHXZEXPAND); } // Warnings that are style only bool styleError() const VL_MT_SAFE { diff --git a/src/V3FileLine.cpp b/src/V3FileLine.cpp index 06421f12a..5de357085 100644 --- a/src/V3FileLine.cpp +++ b/src/V3FileLine.cpp @@ -433,6 +433,12 @@ string FileLine::source() const VL_MT_SAFE { } // LCOV_EXCL_STOP return m_contentp->getLine(m_contentLineno); } +string FileLine::sourcePrefix(int toColumn) const VL_MT_SAFE { + const std::string src = source(); + if (toColumn > static_cast(src.length())) toColumn = static_cast(src.length()); + if (toColumn < 1) return ""; + return src.substr(0, toColumn - 1); +} string FileLine::prettySource() const VL_MT_SAFE { string out = source(); // Drop ignore trailing newline diff --git a/src/V3FileLine.h b/src/V3FileLine.h index 8f1cf140b..cb29f24f9 100644 --- a/src/V3FileLine.h +++ b/src/V3FileLine.h @@ -241,6 +241,7 @@ public: // as the parser errors etc generally make more sense pointing at the last parse point int lineno() const VL_MT_SAFE { return m_lastLineno; } string source() const VL_MT_SAFE; + string sourcePrefix(int toColumn) const VL_MT_SAFE; string prettySource() const VL_MT_SAFE; // Source, w/stripped unprintables and newlines FileLine* parent() const VL_MT_SAFE { return m_parent; } V3LangCode language() const { return singleton().numberToLang(filenameno()); } diff --git a/src/V3LinkParse.cpp b/src/V3LinkParse.cpp index 9d146d264..84be83294 100644 --- a/src/V3LinkParse.cpp +++ b/src/V3LinkParse.cpp @@ -127,6 +127,58 @@ private: && !nodep->stmtsp()->nextp()); // Has only one item } + void checkIndent(AstNode* nodep, AstNode* childp) { + // Try very hard to avoid false positives + AstNode* nextp = nodep->nextp(); + if (!childp) return; + if (!nextp && VN_IS(nodep, While) && VN_IS(nodep->backp(), Begin)) + nextp = nodep->backp()->nextp(); + if (!nextp) return; + if (VN_IS(childp, Begin)) return; + FileLine* const nodeFlp = nodep->fileline(); + FileLine* const childFlp = childp->fileline(); + FileLine* const nextFlp = nextp->fileline(); + // UINFO(0, "checkInd " << nodeFlp->firstColumn() << " " << nodep << endl); + // UINFO(0, " child " << childFlp->firstColumn() << " " << childp << endl); + // UINFO(0, " next " << nextFlp->firstColumn() << " " << nextp << endl); + // Same filename, later line numbers (no macro magic going on) + if (nodeFlp->filenameno() != childFlp->filenameno()) return; + if (nodeFlp->filenameno() != nextFlp->filenameno()) return; + if (nodeFlp->lastLineno() >= childFlp->firstLineno()) return; + if (childFlp->lastLineno() >= nextFlp->firstLineno()) return; + // This block has indent 'a' + // Child block has indent 'b' where indent('b') > indent('a') + // Next block has indent 'b' + // (note similar code below) + if (!(nodeFlp->firstColumn() < childFlp->firstColumn() + && nextFlp->firstColumn() >= childFlp->firstColumn())) + return; + // Might be a tab difference in spaces up to the node prefix, if so + // just ignore this warning + // Note it's correct we look at nodep's column in all of these + const std::string nodePrefix = nodeFlp->sourcePrefix(nodeFlp->firstColumn()); + const std::string childPrefix = childFlp->sourcePrefix(nodeFlp->firstColumn()); + const std::string nextPrefix = nextFlp->sourcePrefix(nodeFlp->firstColumn()); + if (childPrefix != nodePrefix) return; + if (nextPrefix != childPrefix) return; + // Some file lines start after the indentation, so make another check + // using actual file contents + const std::string nodeSource = nodeFlp->source(); + const std::string childSource = childFlp->source(); + const std::string nextSource = nextFlp->source(); + if (!(VString::leadingWhitespaceCount(nodeSource) + < VString::leadingWhitespaceCount(childSource) + && VString::leadingWhitespaceCount(nextSource) + >= VString::leadingWhitespaceCount(childSource))) + return; + nextp->v3warn(MISINDENT, + "Misleading indentation\n" + << nextp->warnContextPrimary() << '\n' + << nodep->warnOther() + << "... Expected indentation matching this earlier statement's line:\n" + << nodep->warnContextSecondary()); + } + // VISITs void visit(AstNodeFTask* nodep) override { if (!nodep->user1SetOnce()) { // Process only once. @@ -517,6 +569,7 @@ private: VL_RESTORER(m_insideLoop); { m_insideLoop = true; + checkIndent(nodep, nodep->stmtsp()); iterateChildren(nodep); } } @@ -533,6 +586,7 @@ private: VL_RESTORER(m_insideLoop); { m_insideLoop = true; + checkIndent(nodep, nodep->stmtsp()); iterateChildren(nodep); } } @@ -628,6 +682,7 @@ private: } void visit(AstGenIf* nodep) override { cleanFileline(nodep); + checkIndent(nodep, nodep->elsesp() ? nodep->elsesp() : nodep->thensp()); const bool nestedIf = (VN_IS(nodep->backp(), Begin) && nestedIfBegin(VN_CAST(nodep->backp(), Begin))); if (nestedIf) { @@ -674,6 +729,7 @@ private: } void visit(AstIf* nodep) override { cleanFileline(nodep); + checkIndent(nodep, nodep->elsesp() ? nodep->elsesp() : nodep->thensp()); iterateChildren(nodep); } void visit(AstPrintTimeScale* nodep) override { diff --git a/src/V3String.cpp b/src/V3String.cpp index 5dcf0f63d..9c1365c60 100644 --- a/src/V3String.cpp +++ b/src/V3String.cpp @@ -158,6 +158,15 @@ bool VString::isWhitespace(const string& str) { return true; } +string::size_type VString::leadingWhitespaceCount(const string& str) { + string::size_type result = 0; + for (const char c : str) { + ++result; + if (!std::isspace(c)) break; + } + return result; +} + double VString::parseDouble(const string& str, bool* successp) { char* const strgp = new char[str.size() + 1]; char* dp = strgp; diff --git a/src/V3String.h b/src/V3String.h index bd31909cc..cbca5de56 100644 --- a/src/V3String.h +++ b/src/V3String.h @@ -113,6 +113,8 @@ public: static string removeWhitespace(const string& str); // Return true if only whitespace or "" static bool isWhitespace(const string& str); + // Return number of spaces/tabs leading in string + static string::size_type leadingWhitespaceCount(const string& str); // Return double by parsing string static double parseDouble(const string& str, bool* successp); // Replace all occurrences of the word 'from' in 'str' with 'to'. A word is considered diff --git a/test_regress/t/t_lint_misindent_bad.out b/test_regress/t/t_lint_misindent_bad.out new file mode 100644 index 000000000..145c4b9f3 --- /dev/null +++ b/test_regress/t/t_lint_misindent_bad.out @@ -0,0 +1,27 @@ +%Warning-MISINDENT: t/t_lint_misindent_bad.v:14:9: Misleading indentation + 14 | $display("bad1"); + | ^~~~~~~~ + t/t_lint_misindent_bad.v:12:7: ... Expected indentation matching this earlier statement's line: + 12 | if (0) + | ^~ + ... For warning description see https://verilator.org/warn/MISINDENT?v=latest + ... Use "/* verilator lint_off MISINDENT */" and lint_on around source to disable this message. +%Warning-MISINDENT: t/t_lint_misindent_bad.v:20:9: Misleading indentation + 20 | $display("bad2"); + | ^~~~~~~~ + t/t_lint_misindent_bad.v:16:7: ... Expected indentation matching this earlier statement's line: + 16 | if (0) + | ^~ +%Warning-MISINDENT: t/t_lint_misindent_bad.v:24:9: Misleading indentation + 24 | $display("bad3"); + | ^~~~~~~~ + t/t_lint_misindent_bad.v:22:7: ... Expected indentation matching this earlier statement's line: + 22 | for (;0;) + | ^~~ +%Warning-MISINDENT: t/t_lint_misindent_bad.v:28:9: Misleading indentation + 28 | $display("bad4"); + | ^~~~~~~~ + t/t_lint_misindent_bad.v:26:7: ... Expected indentation matching this earlier statement's line: + 26 | while (0) + | ^~~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_lint_misindent_bad.pl b/test_regress/t/t_lint_misindent_bad.pl new file mode 100755 index 000000000..4ad25735e --- /dev/null +++ b/test_regress/t/t_lint_misindent_bad.pl @@ -0,0 +1,20 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003-2009 by Wilson Snyder. This program is free software; you +# can redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(vlt => 1); + +lint( + fails => 1, + verilator_flags2 => ["--lint-only -Wall -Wno-DECLFILENAME"], + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_lint_misindent_bad.v b/test_regress/t/t_lint_misindent_bad.v new file mode 100644 index 000000000..5f6b6cab8 --- /dev/null +++ b/test_regress/t/t_lint_misindent_bad.v @@ -0,0 +1,48 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +// Do not reindent - spaces are critical to this test + +module t (/*AUTOARG*/); + + initial begin + if (0) + $display("ok"); + $display("bad1"); // <--- Bad + + if (0) + $display("ok"); + else + $display("ok"); + $display("bad2"); // <--- Bad + + for (;0;) + $display("ok"); + $display("bad3"); // <--- Bad + + while (0) + $display("ok"); + $display("bad4"); // <--- Bad + + // Normal styles + if (0) $display("ok"); + $display("ok"); + for (;0;) $display("ok"); + $display("ok"); + while (0) $display("ok"); + $display("ok"); + + // Questionable but pops up in some cases e.g. SweRV + // (all statements have similar indent) + if (0) + begin + $display("ok"); + end + $display("ok"); + + end + +endmodule