From 80fcce64da993191bfb8bd82a2ab316fbb9e90cf Mon Sep 17 00:00:00 2001 From: Mohamed Gaber Date: Sun, 28 Sep 2025 05:50:37 +0300 Subject: [PATCH] pyosys: fix ref-only classes, implicit conversions + cleanup --- .github/workflows/wheels.yml | 1 - Makefile | 2 +- kernel/yosys.cc | 11 ++++ pyosys/generator.py | 120 +++++++++++++++++++++++------------ pyosys/hashlib.h | 23 ++++--- pyosys/wrappers_tpl.cc | 20 +++--- tests/pyosys/test_idict.py | 12 ++-- 7 files changed, 122 insertions(+), 67 deletions(-) diff --git a/.github/workflows/wheels.yml b/.github/workflows/wheels.yml index d6e9832aa..b6c9a51ac 100644 --- a/.github/workflows/wheels.yml +++ b/.github/workflows/wheels.yml @@ -2,7 +2,6 @@ name: Build Wheels for PyPI # run every Sunday at 10 AM on: - push: # TODO: REMOVE THIS, DO NOT MERGE TO UPSTREAM THIS IS JUST SO I DON'T HAVE TO MANUALLY RUN THE WORKFLOW WITH EVERY PUSH workflow_dispatch: schedule: - cron: "0 10 * * 0" diff --git a/Makefile b/Makefile index 22aeee5e8..f2dec8b0f 100644 --- a/Makefile +++ b/Makefile @@ -1017,12 +1017,12 @@ ifeq ($(ENABLE_LIBYOSYS),1) if [ -n "$(STRIP)" ]; then $(INSTALL_SUDO) $(STRIP) -S $(DESTDIR)$(LIBDIR)/libyosys.so; fi ifeq ($(ENABLE_PYOSYS),1) $(INSTALL_SUDO) mkdir -p $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys + $(INSTALL_SUDO) cp pyosys/__init__.py $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys/__init__.py $(INSTALL_SUDO) cp libyosys.so $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys/libyosys.so $(INSTALL_SUDO) cp -r share $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys ifeq ($(ENABLE_ABC),1) $(INSTALL_SUDO) cp yosys-abc $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys/yosys-abc endif - $(INSTALL_SUDO) cp misc/__init__.py $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys/ endif endif ifeq ($(ENABLE_PLUGINS),1) diff --git a/kernel/yosys.cc b/kernel/yosys.cc index 7ed036a02..ca1db3aa6 100644 --- a/kernel/yosys.cc +++ b/kernel/yosys.cc @@ -198,6 +198,15 @@ bool already_shutdown = false; PYBIND11_MODULE(pyosys, m) { m.add_object("__path__", py::list()); } + +// Catch uses of 'import libyosys' which can import libyosys.so, causing a ton +// of symbol collisions and overall weird behavior. +// +// This should not affect using wheels as the dylib has to actually be called +// libyosys_dummy.so for this function to be interacted with at all. +PYBIND11_MODULE(libyosys_dummy, _) { + throw py::import_error("Change your import from 'import libyosys' to 'from pyosys import libyosys'."); +} #endif void yosys_setup() @@ -217,6 +226,8 @@ void yosys_setup() PyImport_AppendInittab((char*)"pyosys.libyosys", PyInit_libyosys); // compatibility with wheels PyImport_AppendInittab((char*)"pyosys", PyInit_pyosys); + // prevent catastrophes + PyImport_AppendInittab((char*)"libyosys", PyInit_libyosys_dummy); Py_Initialize(); PyRun_SimpleString("import sys"); signal(SIGINT, SIG_DFL); diff --git a/pyosys/generator.py b/pyosys/generator.py index e8e0d96fe..15b40a79e 100644 --- a/pyosys/generator.py +++ b/pyosys/generator.py @@ -33,13 +33,13 @@ is a common problem. ``ruff check pyosys/generator.py`` suffices. import os import io import shutil +import argparse from pathlib import Path from sysconfig import get_paths from dataclasses import dataclass, field from typing import Any, Dict, FrozenSet, Iterable, Tuple, Union, Optional, List import pybind11 -import argparse from cxxheaderparser.simple import parse_file, ClassScope, NamespaceScope, EnumDecl from cxxheaderparser.options import ParserOptions from cxxheaderparser.preprocessor import make_gcc_preprocessor @@ -84,6 +84,7 @@ class PyosysClass: :param denylist: If specified, one or more methods can be excluded from wrapping. """ + name: str ref_only: bool = False @@ -101,6 +102,7 @@ class PyosysHeader: :param classes: A list of ``PyosysClass`` classes to be wrapped :param enums: A list of enums to be wrapped """ + name: str classes: List[PyosysClass] = field(default_factory=lambda: []) @@ -110,11 +112,13 @@ class PyosysHeader: for cls in classes: self.classes_by_name[cls.name] = cls + # MARK: Inclusion and Exclusion global_denylist = frozenset( { # deprecated "builtin_ff_cell_types", + "logv_file_error", # no implementation "set_verific_logging", # can't bridge to python cleanly @@ -167,7 +171,11 @@ pyosys_headers = [ } ), ), - PyosysClass("Const", string_expr="s.as_string()", denylist=frozenset({"bits", "bitvectorize"})), + PyosysClass( + "Const", + string_expr="s.as_string()", + denylist=frozenset({"bits", "bitvectorize"}), + ), PyosysClass("AttrObject", denylist=frozenset({"get_blackbox_attribute"})), PyosysClass("NamedObject", denylist=frozenset({"get_blackbox_attribute"})), PyosysClass("Selection"), @@ -207,14 +215,13 @@ pyosys_headers = [ ref_only=True, string_expr="s.name.c_str()", hash_expr="s", - denylist=frozenset({"Pow"}), # has no implementation + denylist=frozenset({"Pow"}), # has no implementation ), PyosysClass( "Design", - ref_only=True, string_expr="s.hashidx_", hash_expr="s", - denylist=frozenset({"selected_whole_modules"}), # deprecated + denylist=frozenset({"selected_whole_modules"}), # deprecated ), ], ), @@ -227,6 +234,7 @@ class PyosysType: Bit of a hacky object all-around: this is more or less used to encapsulate container types so they can be later made opaque using pybind. """ + base: str specialization: Tuple["PyosysType", ...] const: bool = False @@ -340,6 +348,10 @@ class PyosysWrapperGenerator(object): f'\tpy::hashlib::bind_{container.base}<{", ".join(tpl_args)}>(m, "{container.generate_identifier()}");', file=self.f_inc, ) + print( + f"\tpy::implicitly_convertible();", + file=self.f_inc, + ) print(f"}}", file=self.f_inc) # helpers @@ -414,14 +426,21 @@ class PyosysWrapperGenerator(object): self.found_containers.update(self.find_containers(supported, target.type)) # processors - def get_overload_cast(self, function: Function, class_basename: Optional[str]) -> str: + def get_overload_cast( + self, function: Function, class_basename: Optional[str] + ) -> str: is_method = isinstance(function, Method) function_return_type = function.return_type.format() - if class_basename == "Const" and function_return_type in {"iterator", "const_iterator"}: + if class_basename == "Const" and function_return_type in { + "iterator", + "const_iterator", + }: # HACK: qualify Const's iterators function_return_type = f"{class_basename}::{function_return_type}" - pointer_kind = f"{class_basename}::*" if (is_method and not function.static) else "*" + pointer_kind = ( + f"{class_basename}::*" if (is_method and not function.static) else "*" + ) retval = f"static_cast <" retval += function_return_type @@ -437,10 +456,17 @@ class PyosysWrapperGenerator(object): retval += ")" return retval - def get_definition_args(self, function: Function, class_basename: Optional[str], python_name_override: Optional[str] = None) -> List[str]: + def get_definition_args( + self, + function: Function, + class_basename: Optional[str], + python_name_override: Optional[str] = None, + ) -> List[str]: function_basename = function.name.segments[-1].format() - python_function_basename = python_name_override or keyword_aliases.get(function_basename, function_basename) + python_function_basename = python_name_override or keyword_aliases.get( + function_basename, function_basename + ) def_args = [f'"{python_function_basename}"'] def_args.append(self.get_overload_cast(function, class_basename)) @@ -453,7 +479,7 @@ class PyosysWrapperGenerator(object): return def_args - def process_method(self, function: Method, class_basename: str): + def process_method(self, metadata: PyosysClass, function: Method): if ( function.deleted or function.template @@ -473,10 +499,13 @@ class PyosysWrapperGenerator(object): return if function.constructor: - print( - f"\t\t\t.def(py::init<{self.get_parameter_types(function)}>())", - file=self.f, - ) + if ( + not metadata.ref_only + ): # ref-only classes should not be constructed from python + print( + f"\t\t\t.def(py::init<{self.get_parameter_types(function)}>())", + file=self.f, + ) return python_name_override = None @@ -496,15 +525,13 @@ class PyosysWrapperGenerator(object): if function.static: definition_fn = "def_static" - print(f"\t\t\t.{definition_fn}({', '.join(self.get_definition_args(function, class_basename, python_name_override))})", file=self.f) + print( + f"\t\t\t.{definition_fn}({', '.join(self.get_definition_args(function, metadata.name, python_name_override))})", + file=self.f, + ) def process_function(self, function: Function): - if ( - function.deleted - or function.template - or function.vararg - or function.static - ): + if function.deleted or function.template or function.vararg or function.static: return if function.operator is not None: @@ -516,9 +543,12 @@ class PyosysWrapperGenerator(object): self.register_containers(function) - print(f"\t\t\tm.def({', '.join(self.get_definition_args(function, None))});", file=self.f) + print( + f"\t\t\tm.def({', '.join(self.get_definition_args(function, None))});", + file=self.f, + ) - def process_field(self, field: Field, class_basename: str): + def process_field(self, metadata: PyosysClass, field: Field): if field.access != "public": return @@ -544,7 +574,7 @@ class PyosysWrapperGenerator(object): field_python_basename = keyword_aliases.get(field.name, field.name) print( - f'\t\t\t.{definition_fn}("{field_python_basename}", &{class_basename}::{field.name})', + f'\t\t\t.{definition_fn}("{field_python_basename}", &{metadata.name}::{field.name})', file=self.f, ) @@ -558,9 +588,13 @@ class PyosysWrapperGenerator(object): self.register_containers(variable) - definition_fn = f"def_{'readonly' if variable.type.const else 'readwrite'}_static" + definition_fn = ( + f"def_{'readonly' if variable.type.const else 'readwrite'}_static" + ) - variable_python_basename = keyword_aliases.get(variable_basename, variable_basename) + variable_python_basename = keyword_aliases.get( + variable_basename, variable_basename + ) variable_name = variable.name.format() print( @@ -569,21 +603,18 @@ class PyosysWrapperGenerator(object): ) def process_class_members( - self, - metadata: PyosysClass, - cls: ClassScope, - basename: str + self, metadata: PyosysClass, cls: ClassScope, basename: str ): for method in cls.methods: if method.name.segments[-1].name in metadata.denylist: continue - self.process_method(method, basename) + self.process_method(metadata, method) visited_anonymous_unions = set() for field_ in cls.fields: if field_.name in metadata.denylist: continue - self.process_field(field_, basename) + self.process_field(metadata, field_) # Handle anonymous unions for subclass in cls.classes: @@ -594,7 +625,7 @@ class PyosysWrapperGenerator(object): continue visited_anonymous_unions.add(au.id) for subfield in subclass.fields: - self.process_field(subfield, basename) + self.process_field(metadata, subfield) def process_class( self, @@ -627,10 +658,16 @@ class PyosysWrapperGenerator(object): self.process_class_members(metadata, base_scope, basename) if expr := metadata.string_expr: - print(f'\t\t.def("__str__", [](const {basename} &s) {{ return {expr}; }})', file=self.f) + print( + f'\t\t.def("__str__", [](const {basename} &s) {{ return {expr}; }})', + file=self.f, + ) if expr := metadata.hash_expr: - print(f'\t\t.def("__hash__", [](const {basename} &s) {{ return run_hash({expr}); }})', file=self.f) + print( + f'\t\t.def("__hash__", [](const {basename} &s) {{ return run_hash({expr}); }})', + file=self.f, + ) print(f"\t\t;}}", file=self.f) @@ -653,10 +690,12 @@ class PyosysWrapperGenerator(object): enum_class = enum.typename.classkey == "enum class" for value in enum.values: enum_class_qualifier = f"{basename}::" * enum_class - print(f'\t\t\t.value("{value.name}", {enum_class_qualifier}{value.name})', file=self.f) + print( + f'\t\t\t.value("{value.name}", {enum_class_qualifier}{value.name})', + file=self.f, + ) print(f"\t\t\t.finalize();}}", file=self.f) - def process_namespace( self, header: PyosysHeader, @@ -668,7 +707,10 @@ class PyosysWrapperGenerator(object): if len(namespace_components) and (len(ns.functions) + len(ns.variables)): # TODO: Not essential but maybe move namespace usage into # process_function for consistency? - print(f"\t\t{{ using namespace {'::'.join(namespace_components)};", file=self.f) + print( + f"\t\t{{ using namespace {'::'.join(namespace_components)};", + file=self.f, + ) for function in ns.functions: self.process_function(function) for variable in ns.variables: diff --git a/pyosys/hashlib.h b/pyosys/hashlib.h index 02dd75566..386f1c0d8 100644 --- a/pyosys/hashlib.h +++ b/pyosys/hashlib.h @@ -95,8 +95,8 @@ void difference(C &lhs, const iterable &rhs) { template void intersect(C &lhs, const iterable &rhs) { // Doing it in-place is a lot slower - // TODO?: Leave modifying lhs to caller (saves a copy) but complicates - // chaining intersections. + // TODO?: Leave modifying lhs to caller (saves a copy in some cases) + // but complicates chaining intersections. C storage(lhs); for (auto &element_cxx: lhs) { @@ -449,6 +449,13 @@ void bind_idict(module &m, const char *name_cstr) { auto cls = class_(m, name_cstr) .def(init<>()) .def(init()) // copy constructor + .def(init([](const iterable &other){ // copy instructor from arbitrary iterables + auto s = new C(); + for (auto &e: other) { + (*s)(cast(e)); + } + return s; + })) .def("__len__", [](const C &s){ return (size_t)s.size(); }) .def("__getitem__", [](const C &s, int v) { return s[v]; }) .def("__call__", [](C &s, const K &k) { return s(k); }) @@ -480,20 +487,20 @@ void bind_idict(module &m, const char *name_cstr) { .def("items", [](args _){ throw type_error("idicts do not support pairwise iteration"); }) - .def("update", [](C &s, iterable iterable) { - for (auto &e: iterable) { + .def("update", [](C &s, iterable other) { + for (auto &e: other) { s(cast(e)); } }) - .def("__or__", [](const C &s, iterable iterable) { + .def("__or__", [](const C &s, iterable other) { auto result = new C(s); - for (auto &e: iterable) { + for (auto &e: other) { (*result)(cast(e)); } return result; }) - .def("__ior__", [](C &s, iterable iterable) { - for (auto &e: iterable) { + .def("__ior__", [](C &s, iterable other) { + for (auto &e: other) { s(cast(e)); } return s; diff --git a/pyosys/wrappers_tpl.cc b/pyosys/wrappers_tpl.cc index 0c17be4ed..5e77fef21 100644 --- a/pyosys/wrappers_tpl.cc +++ b/pyosys/wrappers_tpl.cc @@ -21,7 +21,6 @@ // #include #include - #include "pyosys/hashlib.h" namespace py = pybind11; @@ -35,7 +34,7 @@ using namespace RTLIL; #include "wrappers.inc.cc" -namespace YOSYS_PYTHON { +namespace pyosys { struct Globals {}; // Trampolines for Classes with Python-Overridable Virtual Methods @@ -160,12 +159,6 @@ namespace YOSYS_PYTHON { } }; - /// @brief Use an auxiliary function to adapt the legacy function. - void log_to_stream(py::object object) - { - // TODO - }; - PYBIND11_MODULE(libyosys, m) { // this code is run on import m.doc() = "python access to libyosys"; @@ -195,9 +188,9 @@ namespace YOSYS_PYTHON { auto global_variables = py::class_(m, "Globals"); // Trampoline Classes - py::class_>(m, "Pass") + py::class_>(m, "Pass") .def(py::init([](std::string name, std::string short_help) { - auto created = new YOSYS_PYTHON::PassTrampoline(name, short_help); + auto created = new pyosys::PassTrampoline(name, short_help); Pass::init_register(); return created; }), py::arg("name"), py::arg("short_help")) @@ -219,9 +212,9 @@ namespace YOSYS_PYTHON { .def("call", py::overload_cast>(&Pass::call)) ; - py::class_(m, "Monitor") + py::class_(m, "Monitor") .def(py::init([]() { - return new YOSYS_PYTHON::MonitorTrampoline(); + return new pyosys::MonitorTrampoline(); })) .def("notify_module_add", &RTLIL::Monitor::notify_module_add) .def("notify_module_del", &RTLIL::Monitor::notify_module_del) @@ -255,6 +248,9 @@ namespace YOSYS_PYTHON { bind_autogenerated_opaque_containers(m); // + + py::implicitly_convertible(); + py::implicitly_convertible(); }; }; diff --git a/tests/pyosys/test_idict.py b/tests/pyosys/test_idict.py index fd04f4b8c..073eed1e5 100644 --- a/tests/pyosys/test_idict.py +++ b/tests/pyosys/test_idict.py @@ -1,11 +1,11 @@ from pyosys import libyosys as ys my_idict = ys.IdstringIdict() -print(my_idict(ys.IdString("\\hello"))) -print(my_idict(ys.IdString("\\world"))) -print(my_idict.get(ys.IdString("\\world"))) +print(my_idict(ys.IdString("\\hello"))) # test explicit IdString construction +print(my_idict("\\world")) +print(my_idict.get("\\world")) try: - print(my_idict.get(ys.IdString("\\dummy"))) + print(my_idict.get("\\dummy")) except IndexError as e: print(f"{repr(e)}") print(my_idict[0]) @@ -22,10 +22,10 @@ current_len = len(my_idict) assert current_len == 2, "copy" my_copy = my_idict.copy() -my_copy(ys.IdString("\\copy")) +my_copy("\\copy") assert len(my_idict) == current_len, "copy seemed to have mutate original idict" assert len(my_copy) == current_len + 1, "copy not behaving as expected" current_copy_len = len(my_copy) -my_copy |= (ys.IdString(e) for e in ("\\the", "\\world")) # 1 new element +my_copy |= ("\\the", "\\world") # 1 new element assert len(my_copy) == current_copy_len + 1, "or operator returned unexpected result"