ekiwi / wellen

wellen: waveform datastructures in Rust. Fast VCD, FST and GHW parsing for waveform viewers.
BSD 3-Clause "New" or "Revised" License
41 stars 8 forks source link

Different ghw signal values when loaded by wellen vs gtkwave #12

Closed Gekkio closed 6 months ago

Gekkio commented 6 months ago

I'm seeing some strange signal values when loading a certain ghw file with wellen (0.9.4) and/or surfer (2a75498). Here's a screenshot of the same file opened in surfer and gtkwave:

Screenshot from 2024-04-29 17-56-32

The waveform file was generated by GHDL+VUnit, and state is a 3-bit std_ulogic_vector. For some reason, the middle bit is stuck after reset at 0 in wellen/surfer, while gtkwave shows the correct transitions. I only spotted this by accident when I was wondering why the initial value after reset was 101 while the VHDL code clearly resets it to 111.

Here's the problematic ghw file:

wellen_ghw_bug.zip

And here's a minimal Rust program for printing the state changes of the problematic signal:

use wellen::GetItem;

fn main() {
    let mut wave = wellen::simple::read("wave.ghw").unwrap();

    let var_id = wave
        .hierarchy()
        .lookup_var(&["test_rom_tb", "soc_inst", "core_inst"], &"state")
        .unwrap();
    let signal_ref = wave.hierarchy().get(var_id).signal_ref();
    wave.load_signals_multi_threaded(&[signal_ref]);

    let signal = wave.get_signal(signal_ref).unwrap();
    for change in signal.iter_changes() {
        println!("{}", change.1);
    }
}

The program prints this, which matches the incorrect results seen with surfer, so this definitely seems like a wellen bug:

uuu
101
000
001
000
101
000
001
ekiwi commented 6 months ago

Thanks for the awesome bug report! This is most definitely a bug in wellen. I will investigate over the next week or two and let you know if I have any updates.

ekiwi commented 6 months ago

I am still investigating. Just wanted to note down that this file has a "NULL type signal" which is something that I haven't encountered in my testing yet. So it might be possible that I need to handle that better in wellen.

> ghwdump -h inputs/ghdl/wellen_issue_12.ghw

Warning: ghw_read_hie: NULL type signal 1309d.
Loading this file may take a long time.
design
ekiwi commented 6 months ago

Update: looks like this cursed function is extremely broken: https://github.com/ekiwi/wellen/blob/95ae142fdddc32aeff450c8964319fa7f668dda4/src/wavemem.rs#L999 (Probably doesn't work for any even bit width signals)

Gekkio commented 6 months ago

Thanks for investigating this! I recently upgraded to a development version of VUnit which supports FST output, and I'm now using it instead so this bug is no longer a problem for me :smile: But it causes silent data corruption so I'm glad if it gets fixed so other people won't encounter the same strange problem.

BTW, feel free to store the ghw file or its subset as a test case in the repo if you find it useful. There's nothing confidential or special about it, and you can even find the testbench that generated the file here: https://github.com/Gekkio/gb-research/blob/main/sm83-cpu-core/hdl/simulation/test_rom_tb.vhd (note that actually running the testbench requires a bit of setup)

ekiwi commented 6 months ago

Thanks! This particular bug should be fixed in wellen v0.9.7.

ekiwi commented 6 months ago

In case you are curious: I am now testing the compress function with random inputs through the proptest library: https://github.com/ekiwi/wellen/blob/873a7b330f52b0eb1967e9e602a622d226e5db19/src/wavemem.rs#L1185

ekiwi commented 6 months ago

Here is surfer with the new wellen release: image