YosysHQ / yosys

Yosys Open SYnthesis Suite
https://yosyshq.net/yosys/
ISC License
3.5k stars 895 forks source link

driver: fix special args passing to tcl and python #4672

Closed widlarizer closed 1 month ago

widlarizer commented 1 month ago

Fixes #4669. To test the python side this needs the changes from #4671 but even with that I get a segfault with valgrind ./yosys -y foo starting with this:

Invalid read of size 8
   at 0x4D033A3: PyList_New.constprop.0 (in /nix/store/sxr2igfkwhxbagri49b8krmcqz168sim-python3-3.11.8/lib/libpython3.11.so.1.0)
   by 0x4CEB408: PyImport_Import (in /nix/store/sxr2igfkwhxbagri49b8krmcqz168sim-python3-3.11.8/lib/libpython3.11.so.1.0)
   by 0x4CEB6DF: PyImport_ImportModule (in /nix/store/sxr2igfkwhxbagri49b8krmcqz168sim-python3-3.11.8/lib/libpython3.11.so.1.0)
   by 0xDA234D: Yosys::init_share_dirname() (kernel/yosys.cc:1017)
   by 0xDA0DCF: Yosys::yosys_setup() (kernel/yosys.cc:550)
   by 0xCC1FCC: main (kernel/driver.cc:497)
 Address 0x10 is not stack'd, malloc'd or (recently) free'd

@donn any ideas?

donn commented 1 month ago

I broke this by accident in #4643 by the looks of it, oops. Because init_share_dirname depends on Python now when ENABLE_PYTHON is defined, Python needs to be initialized first.

This patch should fix it.

diff --git a/kernel/yosys.cc b/kernel/yosys.cc
index 374b07d06..774c7f37d 100644
--- a/kernel/yosys.cc
+++ b/kernel/yosys.cc
@@ -547,12 +547,6 @@ void yosys_setup()
    if(already_setup)
        return;
    already_setup = true;
-   init_share_dirname();
-   init_abc_executable_name();
-
-#define X(_id) RTLIL::ID::_id = "\\" # _id;
-#include "kernel/constids.inc"
-#undef X

 #ifdef WITH_PYTHON
    // With Python 3.12, calling PyImport_AppendInittab on an already
@@ -566,6 +560,13 @@ void yosys_setup()
    }
 #endif

+   init_share_dirname();
+   init_abc_executable_name();
+
+#define X(_id) RTLIL::ID::_id = "\\" # _id;
+#include "kernel/constids.inc"
+#undef X
+
    Pass::init_register();
    yosys_design = new RTLIL::Design;
    yosys_celltypes.setup();
diff --git a/setup.py b/setup.py
index b3a6a9280..b39b579e0 100644
--- a/setup.py
+++ b/setup.py
@@ -76,7 +76,7 @@ class libyosys_so_ext(Extension):
         # yosys-abc
         yosys_abc_target = os.path.join(pyosys_path, "yosys-abc")
         shutil.copy("yosys-abc", yosys_abc_target)
-        bext.spawn(["strip", "-S", "yosys-abc"])
+        bext.spawn(["strip", "-S", yosys_abc_target])

         # share directory
         share_target = os.path.join(pyosys_path, "share")

I'll go ahead and test that wheels are still okay.

donn commented 1 month ago

Wheels tested and pass with this change, as does this:

./yosys -y <(echo "
import libyosys as ys

d = ys.Design()
ys.run_pass('help', d)
print(sys.path)
print(sys.argv)") -- test argv

Super, super sorry for the inconvenience.

widlarizer commented 1 month ago

Don't worry about it, I'm just glad it's a quick fix