amaranth icon indicating copy to clipboard operation
amaranth copied to clipboard

Python testbenches should not be able to assign combinatorially driven signals

Open cestrauss opened this issue 5 years ago • 2 comments

Found this by accident, as I don't really have a use case for overriding driven signals. Nevertheless, pysim seems to allow this.

Consider:

from nmigen import Signal, Module
from nmigen.sim import Simulator

m = Module()
s = Signal()
m.d.comb += s.eq(1)


def process():
    yield s.eq(0)


sim = Simulator(m, engine="cxxsim")
sim.add_process(process)
sim.run()

I get:

$ python bug16.py 
Traceback (most recent call last):
  File "bug16.py", line 15, in <module>
    sim.run()
  File "/home/cstrauss/src/nmigen/nmigen/sim/core.py", line 168, in run
    while self.advance():
  File "/home/cstrauss/src/nmigen/nmigen/sim/core.py", line 159, in advance
    return self._engine.advance()
  File "/home/cstrauss/src/nmigen/nmigen/sim/cxxsim.py", line 241, in advance
    self._step()
  File "/home/cstrauss/src/nmigen/nmigen/sim/cxxsim.py", line 231, in _step
    process.run()
  File "/home/cstrauss/src/nmigen/nmigen/sim/_pycoro.py", line 123, in run
    self.coroutine.throw(exn)
  File "/home/cstrauss/src/nmigen/nmigen/sim/core.py", line 84, in wrapper
    yield from process()
  File "bug16.py", line 10, in process
    yield s.eq(0)
  File "/home/cstrauss/src/nmigen/nmigen/sim/_pycoro.py", line 76, in run
    self.exec_locals)
  File "<string>", line 1, in <module>
  File "/home/cstrauss/src/nmigen/nmigen/sim/cxxsim.py", line 38, in next
    value |= part.next
  File "/home/cstrauss/src/nmigen/nmigen/sim/_cxxrtl.py", line 115, in next
    value |= self._next[chunk]
ValueError: NULL pointer access

cestrauss avatar Dec 08 '20 11:12 cestrauss

I'm fairly certain this doesn't actually work in PySim either (the "override" is silently reverted the next time any inputs to the combinatorial function change), so I would classify the fact that it seems to be allowed in PySim as a bug.

whitequark avatar Dec 08 '20 11:12 whitequark

Though, both PySim and CXXSim need to be fixed: the former so that it is actually an error, the latter so that the error is nicer.

whitequark avatar Dec 08 '20 11:12 whitequark