#45 no warning on negative delay, related lockup

open
nobody
5
2012-09-04
2012-05-13
Iztok Jeras
No

Hi,

Icarus does not produce a warning if a negative delay is used. But the larger issue I encountered was some kind of lockup. I did not create a simple example, but here is the code triggering the issue:
git clone git://github.com/jeras/sigrok-dump.git
cd sigrok-dump
git checkout 6c67d17bdb
cd onewire/verilog/
./onewire_iverilog.sh

The negative delay is in onewire/verilog/onewire_master.sv (line 72), but the lockup issue is only triggered if the sampler is also active onewire/verilog/onewire_tb.sv (line 54).

Regards,
Iztok Jeras

Discussion

1 2 > >> (Page 1 of 2)
  • Cary R.
    Cary R.
    2012-05-14

    I have not looked at the example, but here's something to consider. I believe all delays are interpreted as positive except for the new style timing checks which actually use negative values. If a negative value is given/used for a delay then the delay is actually the twos-complement value. The apparent lock up could be because you have a huge delay value and then a clock that is producing lots of evens. You can use ^c and continue a few times to see if time is actually advancing or not. There are also a few other debug tools in the development branch for this kind of problem.

    It's possible that SystemVerilog redefined how this is supposed to work, but I doubt it would change something so fundamental.

     
  • Iztok Jeras
    Iztok Jeras
    2012-05-14

    Hi,

    I was able to see the time advancing in the produced waveforms, so it is possible that the twos-complement value was used for the delay. I am not sure this is a bug but I would expect unsigned values to be used for delays, or they are and this is the source of the 2'complement?
    The idea for a warning if the delay is negative comes from simulating the same design in ModelSim.
    I still think there is a co dependency between the delay and another process running at the same time. Since if I disable the binary dump process (initial with while loop) the test ends quickly, and the waveform does not show an enormous delay.

    To reproduce the issue in addition to the steps in the first post something must be present in the while loop (onewire_tb.sv line 58), the #1us delay is enough.

    If it is not worth looking deeper into this, could you at least consider adding a warning for the negative delay? I should simplify debugging similar issues. Or would such a check impact simulation performance?

    Regards,
    Iztok Jeras

     
  • I have checked the example, and the problem is indeed that the negative delay is being converted to a very large positive number. If you increase the delay in your while loop to a big enough value (e.g. 10000s), you will find your test eventually finishes.

    This behaviour is required by the standard. From 1364-2005 section 9.7.1:

    "If the delay expression evaluates to a negative value, it shall be interpreted as a twos-complement unsigned integer of the same size as a time variable."

    The reason for the co-dependency with your while loop is that Icarus is an event driven simulator. Without the while loop, it has nothing to do until the large delay has elapsed. With the while loop, it must keep executing the while loop. With the while loop delay being 1us, and the converted negative delay being ~2^64 ps, that's a lot of iterations of the while loop...

    I'm sympathetic to the request for a warning. I think we could safely say that any delay >= 2^63 is most likely a mistake (obviously this doesn't apply to timing checks where negative delays are allowed).

     
  • Cary R.
    Cary R.
    2012-05-14

    I have not looked, but I expect it would require some work to get a check that only looked for negative values since the delay elements always assume a positive value we would need to create a new set of functions that are used when the input is signed. This doesn't catch the case of assigning a signed value to a time variable, etc. What we may be able to easily add is a compiler warning triggered by -W<something> (signed-delays?) if a signed variable is used in a delay context. If we had SystemVerilog casting implemented then these warnings could be cast away by the user if they had confirmed a negative value never occurred.

     
  • Iztok Jeras
    Iztok Jeras
    2012-05-15

    Hi,

    All my questions have been answered. Please close this bug, unless you wish to keep it as a reminder to add a warning for negative delays.

    Regards,
    Iztok Jeras

     
  • Cary,

    My thought was that rather than look/warn specifically for negative delays, we should warn about extremely large positive delays. If bit 63 of a delay value is set, it is very likely that the value resulted from conversion of a negative number or from underflow in an unsigned expression. If we made the warning optional, that would deal with the rare case that such a large delay was really required.

    I think I'd only want to do this at compile time, and not take the performance hit of a run-time check.

     
  • Cary R.
    Cary R.
    2012-05-18

    The problem I see with just checking for the 64 bit MSB during compilation is it misses the run time checks and to be honest I believe that's the more likely case to have a problem. This also creates an asymmetry in the check since we are only checking compile time constant values. To me that could also create confusion. I think the only thing that we can do that would be useful is to report when a signed value was used in a delay context and I'm not certain this is worth the effort. As a minimum this report will be moved to the feature request list.

    I would appreciate comments on how everyone thinks we should proceed.

     
  • For me, dynamic delays are a rare use case, so a compile time check would catch most errors. But I accept that for other people this may not be the case.

    A run-time check wouldn't really be that big an overhead. As vvp has different instructions for static and dynamic delays, constant delay values could be checked at compile time, so it would only be dynamic delays that would have the overhead. All it requires is a single bit test and branch on a value that will be loaded into the CPU anyway.

    So I remain sympathetic to the request, but have no strong opinion on whether it should be done or not.

     
  • Cary R.
    Cary R.
    2012-09-04

    • milestone: 530321 -->
     
  • Cary R.
    Cary R.
    2012-09-04

    I'm moving this to the feature request tracker since Icarus is working correctly.

     
1 2 > >> (Page 1 of 2)