Skip to content

Conversation

@donn
Copy link
Contributor

@donn donn commented Oct 21, 2025

What are the reasons/motivation for this change?

  • An overwhelming number of dangling reference-related issues
  • AttrObject methods were not available for Cell

Explain how this is achieved.

  • all class members, including pointers and containers, are now copied by value
    • pybind11 does a decent job bridging them appropriately-- the only surprising element to this is that a.b.append(x) mutates a copy of a.b. but this was the previous behavior.
  • add new overload of RTLIL::Module::addMemory that does not require a "donor" object
    • the idea is Module, Memory, Wire, Cell and Process cannot be directly constructed in Python and can only be added to the existing memory hierarchy in Design using the add methods - Memory requiring a donor object was the odd one out here
  • fix superclass member wrapping only looking at direct superclass for inheritance instead of recursively checking superclasses
  • fix superclass member wrapping not using superclass's denylists
  • fix Design's __str__ function not returning a string
  • fix the generator crashing if there's any std::function in a header
  • misc: add a crude __repr__ based on __str__

Make sure your change comes with tests. If not possible, share how a reviewer might evaluate it.

With a Yosys compiled with ENABLE_PYTHON, python3 ./tests/pyosys/run_tests.py yosys

@donn donn changed the title fix: select bugs and object lifecycle issues fix: select pyosys bugs and object lifecycle issues Oct 21, 2025
@donn donn requested a review from mmicko October 22, 2025 16:52
@mmicko
Copy link
Member

mmicko commented Oct 23, 2025

Would like to ping @jix and @georgerennie on this one as well

@donn donn marked this pull request as draft October 24, 2025 08:13
- consistently use value semantics for objects passed along FFI boundary
  (not ideal but matches previous behavior)
- add new overload of RTLIL::Module: addMemory that does not require a "donor" object
  - the idea is `Module`, `Memory`, `Wire`, `Cell` and `Process` cannot be directly constructed in Python and can only be added to the existing memory hierarchy in `Design` using the `add` methods - `Memory` requiring a donor object was the odd one out here
- fix superclass member wrapping only looking at direct superclass for inheritance instead of recursively checking superclasses
- fix superclass member wrapping not using superclass's denylists
- fix Design's `__str__` function not returning a string
- fix the generator crashing if there's any `std::function` in a header
- misc: add a crude `__repr__` based on `__str__`
@donn donn changed the title fix: select pyosys bugs and object lifecycle issues pyosys: fix a number of regressions from 0.58 Oct 25, 2025
@donn
Copy link
Contributor Author

donn commented Oct 26, 2025

I decided to go with value semantics for now. The reason is this was the ≤0.58 behavior and I think any change requires more thought and JF approval:

# 0.58
d = ys.Design()

class Monitor(ys.Monitor):
	def __init__(self):
		super().__init__()
		self.mods = []

	def py_notify_module_add(self, mod):
		self.mods.append(mod.name.str())

m = Monitor()
d.monitors.append(m)
print(d.monitors) # []

Python really has no strategyy for L-values unless the codebase is already built like Python, i.e., shared pointers all around. In the latter case you can just plug into Python's refcounting, but for values it would actually stores a pointer to the area of memory where the value is. With containers that's particularly bad because you won't even notice a memory issue, you might get an entirely new valid value and spend 6 hours debugging what the heck happened. Hypothetically speaking of course.

I can do something a bit more bespoke but for now I'll err on the side of not breaking existing code.

@donn donn marked this pull request as ready for review October 26, 2025 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants