steveicarus / iverilog

Icarus Verilog
https://steveicarus.github.io/iverilog/
GNU General Public License v2.0
2.8k stars 521 forks source link

Add some lint-like coding style warnings #359

Open cuijialang opened 4 years ago

cuijialang commented 4 years ago

Description 1、Case statement does not have a ‘default’-- “others => null” branch; 2、Variables not used 3、Assign to the same signal more than once within the same statement block. 4、Sensitivity lists are complete and do not include unnecessary signals. Missing Signals, Unneeded Signals, Sliced Signals, Duplicate Signals

Expected behaviour iverilog should report an error or give a warning

the code is below

module up3down5(clock, data_in, up, down, carry_out, borrow_out, count_out, parity_out);

input [8:0] data_in;
input clock, up, down;

output reg [8:0] count_out;
output reg carry_out, borrow_out, parity_out;

reg [9:0] cnt_up, cnt_dn;
reg [9:0] cnt_up1, cnt_dn1;                         ///////////error 2
reg [8:0] count_nxt;

always @(posedge clock)
begin
    cnt_dn = count_out - 3'b 101;
    cnt_up = count_out + 2'b 11;
case ({up,down})
    2'b 00 : count_nxt = data_in;
    2'b 01 : count_nxt = cnt_dn;
    2'b 10 : count_nxt = cnt_up;
    2'b 11 : count_nxt = count_out;
    //default : count_nxt = 9'bX;                  ///////////error 1
endcase

parity_out  <= ^count_nxt;
carry_out   <= up & cnt_up[9];
borrow_out  <= down & cnt_dn[9];
count_out   <= count_nxt;
cnt_up1 <= count_nxt;                             ///////////error 3
cnt_up1 <= cnt_up;                                ///////////error 4
end

endmodule

error1: Case statement does not have a ‘default’-- “others => null” branch; error2: Variables not used; error3/error4:Assign to the same signal more than once within the same statement block.

caryr commented 4 years ago

I think the title for this is misleading. At the most these should be warnings and many of them are invalid. For example:

Case 1 could only be a warning when this is in an always_comb and even then may be correct because the default value could be assigned before the case. It is not valid for other processes because this could be indicating a latch/flop. Also there is already a SV keyword to trigger a warning for this case, but unfortunately Icarus does not currently support it.

Case 2 could be a warning, but it would have to be during the elaboration phase and even that warning may not be correct. For example the signal could be driven from the VPI.

Case 3 may be correct for a non-blocking assignment. It is certainly not an issue for a blocking assignment. For the non-blocking case you have complications because the duplication may be conditional. You could likely do analysis like was done for delays in an always and report that at compile time. Anything more precise would need to be done at the runtime and I expect that would have significant performance impacts.

For case 4 how do you distinguish missing signals from for example a flop? It would make sense to warn on duplicate signals. Unused signals are a bit more tricky since technically the clock for a flop is not used in the block so is unused. Slices of a signal are only an issue when @ is used. We already report a warning when an array slice is used inside an @.

martinwhitaker commented 4 years ago

Agreed, none of these are language errors, and most have valid uses. Title adjusted accordingly.