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

Fix for WindowsApps permission denied with run_command(). #4863

Closed
wants to merge 3 commits into from

Conversation

alchitry
Copy link

@alchitry alchitry commented Jan 21, 2025

I've been integrating Yosys into an application (Alchitry Labs) and it fails to run on Windows when installed to WindowsApps. The original error was reported here.

I found that my program could run yosys but when yosys would start yosys-abc it would simply return Access is denied.

After two days of pulling my hair out, I discovered that using CreateProcessA() instead of popen() or system() makes it work. I don't know exactly what the difference is but Windows must be passing something along that doesn't happen otherwise.

This is a Windows only problem and a Windows only solution so a lot was added in an #ifdef block. I couldn't find a way to make it work using the more standard functions.

kernel/yosys.cc Outdated Show resolved Hide resolved
@alchitry
Copy link
Author

alchitry commented Jan 22, 2025

I improved the handling for when process_line is null to be more like system(). I also only redirect stdout not stderr.

system() uses a shell to launch the process while CreateProcess() does not. Using the shell is likely causing the problems but it means that stuff like redirection doesn't work with CreateProcess(). The launch of yosys-abc uses 2>&1 so it might be more accurate to also redirect stderr. It could be conditionally redirected if the command ends with this.

I don't know enough about how this is used elsewhere to make a good decision on how this should be handled.

@alchitry
Copy link
Author

I updated the code so it only uses the new function when needed. This preserves the shell behavior everywhere else.

@whitequark
Copy link
Member

The launch of yosys-abc uses 2>&1 so it might be more accurate to also redirect stderr. It could be conditionally redirected if the command ends with this.

Without the use of a shell, wouldn't this argument (however the redirection is done) be passed to yosys-abc itself, causing issues?

Using the shell is likely causing the problems

Can you please confirm that running cmd.exe via CreateProcess is also causing the error you're observing?

@whitequark
Copy link
Member

I found that my program could run yosys but when yosys would start yosys-abc it would simply return Access is denied.

Are you aware that you can link abc into the main Yosys executable, avoiding the use of popen entirely? On reflection, I think this might be the better way to go to solve this issue.

@alchitry
Copy link
Author

alchitry commented Jan 22, 2025

That would likely fix it. Can it be done using the oss-cad-suite-build? I'm using this to build the tools for windows/linux/mac.

Using cmd.exe to launch the program via CreateProcess gives the access denied error.

In my latest code change I remove the redirection argument if present before running the command.

@whitequark
Copy link
Member

whitequark commented Jan 22, 2025

Can it be done using the oss-cad-suite-build?

Sorry, I'm not familiar with oss-cad-suite-build. I maintain YoWASP tool builds and that's been my primary deployment target since 2020.

In order to enable linking in abc you need to set in yosys/Makefile.conf the following:

LINK_ABC := 1

@alchitry
Copy link
Author

alchitry commented Jan 22, 2025

I believe I was able to add this by adding echo 'LINK_ABC := 1' >> Makefile.conf as line 11 of yosys.sh but this lead to a build error.

/usr/lib/gcc/x86_64-w64-mingw32/13.2.1/../../../../x86_64-w64-mingw32/bin/ld: yosys-libabc.a(sclLiberty.o): in function `Scl_LibertyGlobMatch':
/work/_builds/windows-x64/yosys/yosys/abc/src/map/scl/sclLiberty.c:86: undefined reference to `__imp_PathMatchSpecA'
collect2: error: ld returned 1 exit status
make: *** [Makefile:718: yosys.exe] Error 1

This lead me to line 86 of sclLiberty.c which conveniently had the comment of // if the compiler complains, add "-lshlwapi"

Adding -lshlwapi to LIBS in the makefile did the trick. I need to figure out a cleaner way to only add it to Windows builds but it's a good start.

EDIT: Adding the following two lines to the yosys.sh file got it to compile for windows with ABC linked. This solves my problem without modification to yosys.

	echo 'LINK_ABC := 1' >> Makefile.conf
	echo 'LIBS := -lshlwapi' >> Makefile.conf

@whitequark
Copy link
Member

Adding -lshlwapi to LIBS in the makefile did the trick. I need to figure out a cleaner way to only add it to Windows builds but it's a good start.

That should probably be done as a part of the main Yosys makefile.

@alchitry alchitry closed this by deleting the head repository Jan 22, 2025
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