steveicarus / iverilog

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

This code stalls instead of erroring #1131

Open parker-research opened 1 month ago

parker-research commented 1 month ago

The following code contains a bug, where signals reference each other in a sort of non-deterministic infinite loop. Currently, IVerilog stalls during the vvp execution stage. I believe that it should instead provide an error during compile, or end in an error during the exeuction.

IVerilog version: OSSCAD Release 2024-05-01

Execution commands:

iverilog -g2012 -Wall -o ./comp.vvp ./attempted_solution_code.sv ./testbench_code.sv
vvp ./comp.vvp

Input File 1: attempted_solution_code.sv (generated by a large language model):

`timescale 1 ps/1 ps

module top_module(
    input mode,
    input too_cold,
    input too_hot,
    input fan_on,
    output heater,
    output aircon,
    output fan
);

    assign heater = (mode && too_cold) || (fan_on && !aircon);
    assign aircon = (!mode && too_hot) || (fan_on && !heater);
    assign fan = (heater || aircon) || fan_on;

endmodule

Input File 2: testbench_code.sv:

`timescale 1 ps/1 ps
`define OK 12
`define INCORRECT 13
module reference_module(
    input mode,
    input too_cold, 
    input too_hot,
    input fan_on,
    output heater,
    output aircon,
    output fan
);

    assign fan = (mode ? too_cold : too_hot) | fan_on;
    assign heater = (mode & too_cold);
    assign aircon = (~mode & too_hot);

endmodule

module stimulus_gen (
    input clk,
    output reg too_cold, too_hot, mode, fan_on,
    output reg[511:0] wavedrom_title,
    output reg wavedrom_enable
);

// Add two ports to module stimulus_gen:
//    output [511:0] wavedrom_title
//    output reg wavedrom_enable

    task wavedrom_start(input[511:0] title = "");
    endtask

    task wavedrom_stop;
        #1;
    endtask 

    initial begin
        {too_cold, too_hot, mode, fan_on} <= 4'b0010;
        @(negedge clk);
        wavedrom_start("Winter");
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0010;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0010;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b1010;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b1011;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0010;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0011;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0010;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0110;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b1110;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0111;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b1111;
        @(negedge clk) wavedrom_stop();

        {too_cold, too_hot, mode, fan_on} <= 4'b0000;
        @(negedge clk);
        wavedrom_start("Summer");
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0000;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0000;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0100;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0101;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0000;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0001;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b0000;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b1000;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b1100;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b1001;
            @(posedge clk) {too_cold, too_hot, mode, fan_on} <= 4'b1101;
        @(negedge clk) wavedrom_stop();

        repeat(200)
            @(posedge clk, negedge clk) {too_cold, too_hot, mode, fan_on} <= $random;

        #1 $finish;
    end

endmodule

module tb();

    typedef struct packed {
        int errors;
        int errortime;
        int errors_heater;
        int errortime_heater;
        int errors_aircon;
        int errortime_aircon;
        int errors_fan;
        int errortime_fan;

        int clocks;
    } stats;

    stats stats1;

    wire[511:0] wavedrom_title;
    wire wavedrom_enable;
    int wavedrom_hide_after_time;

    reg clk=0;
    initial forever
        #5 clk = ~clk;

    logic mode;
    logic too_cold;
    logic too_hot;
    logic fan_on;
    logic heater_ref;
    logic heater_dut;
    logic aircon_ref;
    logic aircon_dut;
    logic fan_ref;
    logic fan_dut;

    wire tb_match;      // Verification
    wire tb_mismatch = ~tb_match;

    initial begin 
        $dumpfile("./new_wave.vcd");
        $dumpvars(1, stim1.clk, tb_mismatch ,mode,too_cold,too_hot,fan_on,heater_ref,heater_dut,aircon_ref,aircon_dut,fan_ref,fan_dut );
    end

    stimulus_gen stim1 (
        .clk,
        .* ,
        .mode,
        .too_cold,
        .too_hot,
        .fan_on );
    reference_module good1 (
        .mode,
        .too_cold,
        .too_hot,
        .fan_on,
        .heater(heater_ref),
        .aircon(aircon_ref),
        .fan(fan_ref) );

    top_module top_module1 (
        .mode,
        .too_cold,
        .too_hot,
        .fan_on,
        .heater(heater_dut),
        .aircon(aircon_dut),
        .fan(fan_dut) );

    bit strobe = 0;
    task wait_for_end_of_timestep;
        repeat(5) begin
            strobe <= !strobe;  // Try to delay until the very end of the time step.
            @(strobe);
        end
    endtask 

    final begin
        if (stats1.errors_heater) $display("Hint: Output '%s' has %0d mismatches. First mismatch occurred at time %0d.", "heater", stats1.errors_heater, stats1.errortime_heater);
        else $display("Hint: Output '%s' has no mismatches.", "heater");
        if (stats1.errors_aircon) $display("Hint: Output '%s' has %0d mismatches. First mismatch occurred at time %0d.", "aircon", stats1.errors_aircon, stats1.errortime_aircon);
        else $display("Hint: Output '%s' has no mismatches.", "aircon");
        if (stats1.errors_fan) $display("Hint: Output '%s' has %0d mismatches. First mismatch occurred at time %0d.", "fan", stats1.errors_fan, stats1.errortime_fan);
        else $display("Hint: Output '%s' has no mismatches.", "fan");

        $display("Hint: Total mismatched samples is %1d out of %1d samples\n", stats1.errors, stats1.clocks);
        $display("Simulation finished at %0d ps", $time);
        $display("Mismatches: %1d in %1d samples", stats1.errors, stats1.clocks);
    end

    // Verification: XORs on the right makes any X in good_vector match anything, but X in dut_vector will only match X.
    assign tb_match = ( { heater_ref, aircon_ref, fan_ref } === ( { heater_ref, aircon_ref, fan_ref } ^ { heater_dut, aircon_dut, fan_dut } ^ { heater_ref, aircon_ref, fan_ref } ) );
    // Use explicit sensitivity list here. @(*) causes NetProc::nex_input() to be called when trying to compute
    // the sensitivity list of the @(strobe) process, which isn't implemented.
    always @(posedge clk, negedge clk) begin

        stats1.clocks++;
        if (!tb_match) begin
            if (stats1.errors == 0) stats1.errortime = $time;
            stats1.errors++;
        end
        if (heater_ref !== ( heater_ref ^ heater_dut ^ heater_ref ))
        begin if (stats1.errors_heater == 0) stats1.errortime_heater = $time;
            stats1.errors_heater = stats1.errors_heater+1'b1; end
        if (aircon_ref !== ( aircon_ref ^ aircon_dut ^ aircon_ref ))
        begin if (stats1.errors_aircon == 0) stats1.errortime_aircon = $time;
            stats1.errors_aircon = stats1.errors_aircon+1'b1; end
        if (fan_ref !== ( fan_ref ^ fan_dut ^ fan_ref ))
        begin if (stats1.errors_fan == 0) stats1.errortime_fan = $time;
            stats1.errors_fan = stats1.errors_fan+1'b1; end

    end
endmodule
caryr commented 1 month ago

Icarus does check for and report some possible infinite loops, but it does not currently search for combinational infinite loops. We have added instrumentation in the code the compiler generates that can help debug these kind of issues when it is enabled, but I can't remember if continuous assignments are instrumented. I think it is currently just procedural code so you would need to change from an assign to always_comb to find the actual loop. The other possible way to track this down is to figure out which transition is triggering the combinational loop and then think about how the variables are changing and how it could create a combinational oscillation.

I've been looking at LLM generated code for a bit and for some things the generated code is okay, but there are often cases where the code is incorrect from an implementation perspective or just plain invalid Verilog. If you tell the LLM it has generated invalid code it will come back and say yes you are correct, but that seems more of a socially acceptable response when the validity of the generated code is challenged.

parker-research commented 1 month ago

Is this code even valid Verilog? I assumed it wasn't (as it's outside of an always_comb, which could be used to make a ring oscillator with this sort of code intentionally), but I could be wrong.