From d119d105695a37b879290bc91e25b91a48301e9b Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Wed, 15 Nov 2017 20:19:12 -0500 Subject: [PATCH] Add BSSPACE and COLONPLUS lint warnings. --- Changes | 2 ++ bin/verilator | 38 ++++++++++++++++++++------ src/V3Error.h | 8 ++++-- src/V3PreLex.l | 11 +++++--- src/verilog.l | 3 ++ test_regress/t/t_lint_bsspace_bad.pl | 24 ++++++++++++++++ test_regress/t/t_lint_bsspace_bad.v | 13 +++++++++ test_regress/t/t_lint_colonplus_bad.pl | 25 +++++++++++++++++ test_regress/t/t_lint_colonplus_bad.v | 14 ++++++++++ 9 files changed, 123 insertions(+), 15 deletions(-) create mode 100755 test_regress/t/t_lint_bsspace_bad.pl create mode 100644 test_regress/t/t_lint_bsspace_bad.v create mode 100755 test_regress/t/t_lint_colonplus_bad.pl create mode 100644 test_regress/t/t_lint_colonplus_bad.v diff --git a/Changes b/Changes index 9c30a88e2..5e997be3f 100644 --- a/Changes +++ b/Changes @@ -12,6 +12,8 @@ The contributors that suggested a given feature are shown in []. Thanks! **** Add error when driving input-only modport. +**** Add BSSPACE and COLONPLUS lint warnings. + **** Fix false unused warning on interfaces, bug1241. [Laurens van Dam] diff --git a/bin/verilator b/bin/verilator index aefe28058..d814e976d 100755 --- a/bin/verilator +++ b/bin/verilator @@ -1251,11 +1251,11 @@ directives in the source, i.e. the warning will still not be printed. =item -Wno-lint Disable all lint related warning messages, and all style warnings. This is -equivalent to "-Wno-ALWCOMBORDER -Wno-CASEINCOMPLETE -Wno-CASEOVERLAP --Wno-CASEX -Wno-CASEWITHX -Wno-CMPCONST -Wno-ENDLABEL -Wno-IMPLICIT --Wno-LITENDIAN -Wno-PINCONNECTEMPTY -Wno-PINMISSING -Wno-SYNCASYNCNET --Wno-UNDRIVEN -Wno-UNSIGNED -Wno-UNUSED -Wno-WIDTH" plus the list shown for -Wno-style. +equivalent to "-Wno-ALWCOMBORDER -Wno-BSSPACE -Wno-CASEINCOMPLETE +-Wno-CASEOVERLAP -Wno-CASEX -Wno-CASEWITHX -Wno-CMPCONST -Wno-COLONPLUS +-Wno-ENDLABEL -Wno-IMPLICIT -Wno-LITENDIAN -Wno-PINCONNECTEMPTY +-Wno-PINMISSING -Wno-SYNCASYNCNET -Wno-UNDRIVEN -Wno-UNSIGNED -Wno-UNUSED +-Wno-WIDTH" plus the list shown for Wno-style. It is strongly recommended you cleanup your code rather than using this option, it is only intended to be use when running test-cases of code @@ -1284,10 +1284,10 @@ Enables the specified warning message. Enable all lint related warning messages (note by default they are already enabled), but do not affect style messages. This is equivalent to -"-Wwarn-ALWCOMBORDER -Wwarn-CASEINCOMPLETE -Wwarn-CASEOVERLAP -Wwarn-CASEX --Wwarn-CASEWITHX -Wwarn-CMPCONST -Wwarn-ENDLABEL -Wwarn-IMPLICIT --Wwarn-LITENDIAN -Wwarn-PINMISSING -Wwarn-REALCVT -Wwarn-UNSIGNED --Wwarn-WIDTH". +"-Wwarn-ALWCOMBORDER -Wwarn-BSSPACE -Wwarn-CASEINCOMPLETE +-Wwarn-CASEOVERLAP -Wwarn-CASEX -Wwarn-CASEWITHX -Wwarn-CMPCONST +-Wwarn-COLONPLUS -Wwarn-ENDLABEL -Wwarn-IMPLICIT -Wwarn-LITENDIAN +-Wwarn-PINMISSING -Wwarn-REALCVT -Wwarn-UNSIGNED -Wwarn-WIDTH". =item -Wwarn-style @@ -3081,6 +3081,17 @@ generally unrolls small loops. You may want to try increasing --unroll-count (and occasionally --unroll-stmts) which will raise the small loop bar to avoid this error. +=item BSSPACE + +Warns that a backslash is followed by a space then a newline. Likely the +intent was to have a backslash directly followed by a newline (e.g. when +making a `define) and there's accidentally whitespace at the end of the +line. If the space is not accidental, suggest removing the backslash in +the code as it serves no function. + +Ignoring this warning will only suppress the lint check, it will simulate +correctly. + =item CASEINCOMPLETE Warns that inside a case statement there is a stimulus pattern for which @@ -3120,6 +3131,15 @@ instead intended is to use a casez with C. Ignoring this warning will only suppress the lint check, it will simulate correctly. +=item COLONPLUS + +Warns that a :+ is seen. Likely the intent was to use +: to select a range +of bits. If the intent was a range that is explicitly positive, suggest +adding a space, e.g. use ": +". + +Ignoring this warning will only suppress the lint check, it will simulate +correctly. + =item CDCRSTLOGIC With --cdc only, warns that asynchronous flop reset terms come from other diff --git a/src/V3Error.h b/src/V3Error.h index a14e0f5ab..1a9fafd37 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -61,6 +61,7 @@ public: BLKANDNBLK, // Blocked and non-blocking assignments to same variable BLKLOOPINIT, // Delayed assignment to array inside for loops BLKSEQ, // Blocking assignments in sequential block + BSSPACE, // Backslash space CASEINCOMPLETE, // Case statement has missing values CASEOVERLAP, // Case statements overlap CASEWITHX, // Case with X values @@ -68,6 +69,7 @@ public: CDCRSTLOGIC, // Logic in async reset path CLKDATA, // Clock used as data CMPCONST, // Comparison is constant due to limited range + COLONPLUS, // :+ instead of +: COMBDLY, // Combinatorial delayed assignment DEFPARAM, // Style: Defparam DECLFILENAME, // Declaration doesn't match filename @@ -125,9 +127,9 @@ public: // Warnings " EC_FIRST_WARN", "ALWCOMBORDER", "ASSIGNDLY", "ASSIGNIN", - "BLKANDNBLK", "BLKLOOPINIT", "BLKSEQ", + "BLKANDNBLK", "BLKLOOPINIT", "BLKSEQ", "BSSPACE", "CASEINCOMPLETE", "CASEOVERLAP", "CASEWITHX", "CASEX", "CDCRSTLOGIC", "CLKDATA", - "CMPCONST", "COMBDLY", "DEFPARAM", "DECLFILENAME", + "CMPCONST", "COLONPLUS", "COMBDLY", "DEFPARAM", "DECLFILENAME", "ENDLABEL", "GENCLK", "IFDEPTH", "IMPERFECTSCH", "IMPLICIT", "IMPURE", "INCABSPATH", "INITIALDLY", @@ -158,9 +160,11 @@ public: // Warnings that are lint only bool lintError() const { return ( m_e==ALWCOMBORDER + || m_e==BSSPACE || m_e==CASEINCOMPLETE || m_e==CASEOVERLAP || m_e==CASEWITHX || m_e==CASEX || m_e==CMPCONST + || m_e==COLONPLUS || m_e==ENDLABEL || m_e==IMPLICIT || m_e==LITENDIAN diff --git a/src/V3PreLex.l b/src/V3PreLex.l index 3e3a9d115..edb6d8bba 100644 --- a/src/V3PreLex.l +++ b/src/V3PreLex.l @@ -70,7 +70,6 @@ wsn [ \t\f] crnl [\r]*[\n] quote [\"] tickquote [`][\"] -backslash [\\] /* Where we use symb/symbdef, we must also look for a `` join */ /* Note in the preprocessor \ESCaped is *not* always special; mantis1537/bug441 */ symb ([a-zA-Z_][a-zA-Z0-9_$]*|\\[^ \t\f\r\n]+) @@ -110,8 +109,9 @@ drop [\032] {crnl} { linenoInc(); yyerrorf("Unterminated string"); BEGIN(INITIAL); } {word} { yymore(); } [^\"\\] { yymore(); } -{backslash}{crnl} { linenoInc(); yymore(); } -{backslash}. { yymore(); } +[\\]{crnl} { linenoInc(); yymore(); } +[\\]{wsn}+{crnl} { yyless(1); LEXP->curFilelinep()->v3warn(BSSPACE, "Backslash followed by whitespace, perhaps the whitespace is accidental?"); } +[\\]. { yymore(); } {quote} { yy_pop_state(); if (LEXP->m_parenLevel || LEXP->m_defQuote) { LEXP->m_defQuote=false; appendDefValue(yytext,yyleng); yyleng=0; } else return (VP_STRING); } @@ -144,7 +144,7 @@ drop [\032] <> { linenoInc(); yyerrorf("EOF in unterminated include filename"); yyleng=0; yyterminate(); } {crnl} { linenoInc(); yyerrorf("Unterminated include filename"); BEGIN(INITIAL); } [^\>\\] { yymore(); } -{backslash}. { yymore(); } +[\\]. { yymore(); } [\>] { yy_pop_state(); return VP_STRING; } /* Reading definition formal parenthesis (or not) to begin formal arguments */ @@ -162,6 +162,7 @@ drop [\032] {drop} { } <> { linenoInc(); yy_pop_state(); yyerrorf("Unterminated ( in define formal arguments."); yyleng=0; return VP_DEFFORM; } {crnl} { linenoInc(); appendDefValue((char*)"\n",1); } /* Include return so can maintain output line count */ +[\\]{wsn}+{crnl} { yyless(1); LEXP->curFilelinep()->v3warn(BSSPACE, "Backslash followed by whitespace, perhaps the whitespace is accidental?"); } [\\]{crnl} { linenoInc(); appendDefValue((char*)"\\\n",2); } /* Include return so can maintain output line count */ {quote} { LEXP->m_defQuote=true; yy_push_state(STRMODE); yymore(); } /* Legal only in default values */ "`\\`\"" { appendDefValue(yytext,yyleng); } /* Maybe illegal, otherwise in default value */ @@ -179,6 +180,7 @@ drop [\032] {drop} { } <> { linenoInc(); yy_pop_state(); yytext=(char*)"\n"; yyleng=1; return (VP_DEFVALUE); } /* Technically illegal, but people complained */ {crnl} { linenoInc(); yy_pop_state(); yytext=(char*)"\n"; yyleng=1; return (VP_DEFVALUE); } +[\\]{wsn}+{crnl} { yyless(1); LEXP->curFilelinep()->v3warn(BSSPACE, "Backslash followed by whitespace, perhaps the whitespace is accidental?"); } [\\]{crnl} { linenoInc(); appendDefValue((char*)"\\\n",2); } /* Return, AND \ is part of define value */ {quote} { LEXP->m_defQuote=true; yy_push_state(STRMODE); yymore(); } [^\/\*\n\r\\\"]+ | @@ -189,6 +191,7 @@ drop [\032] /* - if no \{crnl} ending then the comment belongs to the next line, as a non-embedded comment */ /* - if all but (say) 3rd line is missing \ then it's indeterminate */ "*/" { yy_pop_state(); appendDefValue(yytext,yyleng); } +[\\]{wsn}+{crnl} { yyless(1); LEXP->curFilelinep()->v3warn(BSSPACE, "Backslash followed by whitespace, perhaps the whitespace is accidental?"); } [\\]{crnl} { linenoInc(); LEXP->m_defCmtSlash=true; appendDefValue(yytext,yyleng-2); appendDefValue((char*)"\n",1); } /* Return but not \ */ {crnl} { linenoInc(); yymore(); if (LEXP->m_defCmtSlash) yyerrorf("One line of /* ... */ is missing \\ before newline"); diff --git a/src/verilog.l b/src/verilog.l index fd1f036f2..a80a715fb 100644 --- a/src/verilog.l +++ b/src/verilog.l @@ -783,6 +783,9 @@ vnum {vnum1}|{vnum2}|{vnum3}|{vnum4}|{vnum5} "+:" { FL; return yP_PLUSCOLON; } "-:" { FL; return yP_MINUSCOLON; } ".*" { FL; return yP_DOTSTAR; } + ":+" { FL; yyless(1); + PARSEP->fileline()->v3warn(COLONPLUS, "Perhaps instead of ':+' the intent was '+:'?"); + return ':'; } } /* SystemVerilog Operators */ diff --git a/test_regress/t/t_lint_bsspace_bad.pl b/test_regress/t/t_lint_bsspace_bad.pl new file mode 100755 index 000000000..d00d59f5a --- /dev/null +++ b/test_regress/t/t_lint_bsspace_bad.pl @@ -0,0 +1,24 @@ +#!/usr/bin/perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003 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. + +$Self->{vlt} or $Self->skip("Verilator only test"); + +compile ( + verilator_flags2 => ["--lint-only"], + verilator_make_gcc => 0, + make_top_shell => 0, + make_main => 0, + fails => 1, + expect=> +'%Warning-BSSPACE: t/t_lint_bsspace_bad.v:\d+: Backslash followed by whitespace, perhaps the whitespace is accidental\? +.*%Error: Exiting due to.*', + ); + +ok(1); +1; diff --git a/test_regress/t/t_lint_bsspace_bad.v b/test_regress/t/t_lint_bsspace_bad.v new file mode 100644 index 000000000..b37bdbfa7 --- /dev/null +++ b/test_regress/t/t_lint_bsspace_bad.v @@ -0,0 +1,13 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2017 by Wilson Snyder. + +// Fake binary character here '', so is treated as binary and +// don't get whitespace violation. + +`define FOO blak \ + blak + +module t; +endmodule diff --git a/test_regress/t/t_lint_colonplus_bad.pl b/test_regress/t/t_lint_colonplus_bad.pl new file mode 100755 index 000000000..0218109b6 --- /dev/null +++ b/test_regress/t/t_lint_colonplus_bad.pl @@ -0,0 +1,25 @@ +#!/usr/bin/perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003 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. + +$Self->{vlt} or $Self->skip("Verilator only test"); + +compile ( + verilator_flags2 => ["--lint-only"], + verilator_make_gcc => 0, + make_top_shell => 0, + make_main => 0, + fails => 1, + expect=> +q{%Warning-COLONPLUS: t/t_lint_colonplus_bad.v:\d+: Perhaps instead of ':\+' the intent was '\+:'\? +%Warning-COLONPLUS: Use .* +.*%Error: Exiting due to.*}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_lint_colonplus_bad.v b/test_regress/t/t_lint_colonplus_bad.v new file mode 100644 index 000000000..9bea31d96 --- /dev/null +++ b/test_regress/t/t_lint_colonplus_bad.v @@ -0,0 +1,14 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2017 by Wilson Snyder. + +module t (/*AUTOARG*/ + // Outputs + z + ); + + reg [3:0] r = 4'b1010; + output [2:1] z = r[2 :+ 1]; + +endmodule