Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Python testbenches should not be able to assign combinatorially driven signals #557

Closed
cestrauss opened this issue Dec 8, 2020 · 2 comments · Fixed by #1328
Closed

Python testbenches should not be able to assign combinatorially driven signals #557

cestrauss opened this issue Dec 8, 2020 · 2 comments · Fixed by #1328
Labels
bug simulator:pysim unsoundness
Milestone
0.5

Comments

@cestrauss
Copy link

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
@whitequark
Copy link
Member

whitequark commented Dec 8, 2020 •
edited

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 whitequark changed the title cxxsim: Python process can't override combinatorial assignment pysim: Python testbenches should not be able to assign combinatorially driven signals Dec 8, 2020
@whitequark whitequark changed the title pysim: Python testbenches should not be able to assign combinatorially driven signals Python testbenches should not be able to assign combinatorially driven signals Dec 8, 2020
@whitequark
Copy link
Member

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.

@wanda-phi wanda-phi mentioned this issue Apr 11, 2024
Closed
wanda-phi added a commit to wanda-phi/amaranth that referenced this issue Apr 13, 2024
@wanda-phi
sim: raise an error when overriding a combinationally-driven signal.
ccce9be 
Fixes amaranth-lang#557.
@wanda-phi wanda-phi mentioned this issue Apr 13, 2024
Merged
@whitequark whitequark added this to the 0.5 milestone Apr 13, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 13, 2024
@wanda-phi @whitequark
sim: raise an error when overriding a combinationally-driven signal.
122be78 
Fixes #557.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug simulator:pysim unsoundness
Milestone
0.5
Development

Successfully merging a pull request may close this issue.

sim: raise an error when overriding a combinationally-driven signal. wanda-phi/amaranth
2 participants
@whitequark @cestrauss