Bug at file sc_trace_file_base.cpp

[Guedes]

I think I just found a bug at the file sc_trace_file_base.cpp on version 2.3.2. It was recommended to me to report it here.

The problem is at the function set_time_unit, line 193, where you can read that the parameters of the function are v and tu:

void
sc_trace_file_base::set_time_unit( double v, sc_time_unit tu )

As you can see, there are no restriction for the use of value v. On the code I was working at, the value was 0.5, and that is ok for SystemC 2.3.1a.

However, at 2.3.2, any value of v that it is not a multiple of 10 will make the function fail in an internal assertion because trace_unit_fs = v * unit_to_fs(tu):
 

    trace_unit_fs = static_cast<unit_type>(v * unit_to_fs(tu));

Then, at the line 213, fs_unit_to_str is called:

      ss << fs_unit_to_str(trace_unit_fs)

And in fs_unit_to_str:

it:std::string
sc_trace_file_base::fs_unit_to_str(sc_trace_file_base::unit_type tu)
{
    switch (tu)
    {
        case UINT64_C(1): return "1 fs";
        case UINT64_C(10): return "10 fs";
        case UINT64_C(100): return "100 fs";
        case UINT64_C(1000): return "1 ps";
        case UINT64_C(10000): return "10 ps";
        case UINT64_C(100000): return "100 ps";
        case UINT64_C(1000000): return "1 ns";
        case UINT64_C(10000000): return "10 ns";
        case UINT64_C(100000000): return "100 ns";
        case UINT64_C(1000000000): return "1 us";
        case UINT64_C(10000000000): return "10 us";
        case UINT64_C(100000000000): return "100 us";
        case UINT64_C(1000000000000): return "1 ms";
        case UINT64_C(10000000000000): return "10 ms";
        case UINT64_C(100000000000000): return "100 ms";
        case UINT64_C(1000000000000000): return "1 sec";
        case UINT64_C(10000000000000000): return "10 sec";
        case UINT64_C(100000000000000000): return "100 sec";
        default: sc_assert(0); return "";
    }
}

If the variable v is different of a multiple of 10, the function fs_unit_to_str will necessarily fail. For v = 0.5, it will always get to the default and sc_assert(0) will be activated. 

This function should receive the value of unit_to_fs(tu) as a parameter, or change the way this block of code works altogether. 

Could someone confirm if this is a bug indeed, and propose a fix for that?

 

Thank you very much.

 

[Roman Popov]

Thanks a lot for reporting!  I can confirm that 2.3.2 does not supports v that it is not a multiple of 10.   I agree that set_time_unit API is bad (error-prone), and should be changed.

Good news that you've received a runtime error early and did not spent lots of time .  

Do you need support for fractional timeunits? Or just more clear API? 

I've also checked 2.3.1. It does not works properly with 0.5 either:

int sc_main (int argc, char **argv) {
    sc_trace_file* tf;
    tf = sc_create_vcd_trace_file("traces");
    sc_signal<int> isig{"isig",0};
    tf->set_time_unit(0.5, SC_NS);
    sc_trace(tf, isig, "isig" );
    sc_start(0.5, SC_NS);
    isig = 1;
    sc_start(2.5, SC_NS);
    isig = 2;
    sc_start(3.5, SC_NS);
    isig = 4;
    sc_start(3.5, SC_NS);
    sc_close_vcd_trace_file(tf);
    return 0;
}

 

VCD is incorrect:

$date
     Dec 05, 2017       11:50:52
$end

$version
 SystemC 2.3.1-Accellera --- Dec  5 2017 11:40:15
$end

$scope module SystemC $end
$var wire   32  aaaaa  isig [31:0]  $end
$upscope $end
$enddefinitions  $end

$comment
All initial values are dumped below at time 0 sec = 0 timescale units.
$end

$dumpvars
b0 aaaaa
$end

#1
b1 aaaaa

#6
b10 aaaaa

#13
b100 aaaaa

 

So 2.3.2 is better. But not perfect :(

[Philipp A Hartmann]

Quoting IEEE 1666-2011, 8.1.2 (color highlighting mine):

Quote

Member function set_time_unit shall be overridden in the derived class to set the time unit for the trace file. The value of the double argument shall be positive and shall be a power of 10. In the absence of any call function set_time_unit, the default trace file time unit shall be 1 picosecond.

So I think, the implementation in SystemC 2.3.2 is in line with the SystemC standard.

Instead of failing with an assertion, a more explanatory error message would be helpful, of course.

Greetings from Duisburg,
  Philipp

[Roman Popov]

I've looked again at VCD generated by 2.3.1.  It has no time unit mentioned at all.

So it is actually correct : if you substitute 0.5 NS as a timeunit, you will have correct timestamps.  Gtkwave renders them as 1ns.  But other VCD viewers I've tried just don't show time unit at all in this case.

[Roman Popov]

Quote

 

$date
     Dec 05, 2017       12:21:02
$end

$version
 SystemC 2.3.1-Accellera --- Dec  5 2017 11:40:15
$end

$timescale
     500 ps
$end

$scope module SystemC $end
$var wire   32  aaaaa  isig [31:0]  $end
$upscope $end
$enddefinitions  $end

$dumpvars
b0 aaaaa
$end
#1
b1 aaaaa
#3
b10 aaaaa
#7
b100 aaaaa

 

GtkWave renders correctly with 500 ps timeunit. But commercial viewer I have crashes. 

[Philipp A Hartmann]

I can also add a quote from the Verilog Standard IEEE 1364-2005, 18.2.3.5 $timescale, describing the grammar rules for the timescale support in VCD:

Quote

vcd_declaration_timescale ::=
    $timescale time_number time_unit $end
time_number ::=
    1 | 10 | 100
time_unit ::=
    s | ms | us | ns | ps | fs

So even here, only powers of 10 are allowed.

[Guedes]

I think a clear API should be fine, considering that IEEE 1666-2011, 8.1.2 explicitly defines it as a power of 10.

Maybe an assert checking if (v % 10 == 0) would be interesting here, with a more direct message about the wrong parameterization. 

About my specific issue, I will change the code here properly. 

Thanks.