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.

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

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

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

widlarizer
Copy link
Collaborator

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
Copy link
Contributor

donn commented Oct 17, 2024

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
Copy link
Contributor

donn commented Oct 17, 2024

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
Copy link
Collaborator Author

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

@widlarizer widlarizer merged commit 7db4c65 into main Oct 21, 2024
40 checks passed
@mmicko mmicko deleted the emil/fix-tcl-args-cxxopts branch October 21, 2024 13:41
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.

cxxopts migration broke Tcl argument passing
2 participants