leonardt / fault

A Python package for testing hardware (part of the magma ecosystem)
BSD 3-Clause "New" or "Revised" License
41 stars 13 forks source link

get_value does not handle signed values properly #265

Open sgherbst opened 4 years ago

sgherbst commented 4 years ago

Just noticed that when get_value is used in conjunction with SInt types, the values are treated as unsigned. An example is shown below, where the value 214 is returned instead of -42. It appears the reason is that the generated testbench code does not attach the signed attribute to signed signals. However, this only ends up impacting get_value, not expect.

test.py

from pathlib import Path
import fault
import magma as m

class dut(m.Circuit):
    io = m.IO(
        a=m.In(m.SInt[8]),
        y=m.Out(m.SInt[8]),
        clk=m.ClockIn
    )

t = fault.Tester(dut, dut.clk)

t.poke(dut.a, -42)
t.step(2)
y = t.get_value(dut.a)

t.step(2)

t.compile_and_run(
    target='system-verilog',
    simulator='iverilog',
    ext_srcs=[Path('dut.sv').resolve()],
    ext_model_file=True
)

print('y', y.value)

dut.sv

module dut (
    input signed [7:0] a,
    input clk,
    output reg signed [7:0] y
);
    always @(posedge clk) begin
        y <= a;
    end
endmodule
leonardt commented 4 years ago

The expect logic calls process_value (https://github.com/leonardt/fault/blob/b634f16d145d6b2113e51e0d68b643e558c8b0f1/fault/system_verilog_target.py#L516) which will convert the expected value from SInt to unsigned for the comparison (https://github.com/leonardt/fault/blob/b634f16d145d6b2113e51e0d68b643e558c8b0f1/fault/system_verilog_target.py#L334-L337), so the rest of the logic just assumes unsigned.

I looked at the generated TB:

`timescale 1ns/1ns
module dut_tb;
    reg [7:0] a;
    wire [7:0] y;
    reg clk;
    integer get_value_file_file;

    dut #(

    ) dut (
        .a(a),
        .y(y),
        .clk(clk)
    );

    initial begin
        get_value_file_file = $fopen("/private/tmp/build/get_value_file.txt", "w");
        clk <= 1'b0;
        a <= 8'd214;
        #5 clk ^= 1;
        #5 clk ^= 1;
        $fwrite(get_value_file_file, "%0d\n", a);
        #5 clk ^= 1;
        #5 clk ^= 1;
        $fclose(get_value_file_file);
        #20 $finish;
    end

endmodule

would changing reg [7:0] a to reg signed [7:0] a resolve the issue?

I think the logic could be improved here: https://github.com/leonardt/fault/blob/b634f16d145d6b2113e51e0d68b643e558c8b0f1/fault/system_verilog_target.py#L594 to insert the signed modifier for SInt types, but then I think the above expect code (and probably poke code, which I think also uses process_value, so I think just updating process_value might work) would need to be changed to not convert the value to the unsigned representation.