Skip to content

Disable STRIP operations when appropriate. #5127

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

Merged
merged 1 commit into from
May 26, 2025

Conversation

RonxBulld
Copy link
Contributor

What are the reasons/motivation for this change?
It was always difficult to debug the logic in yosys when using it as a dynamic library, and it was further discovered that this was because STRIP was always used to remove symbol information in yosys when installing yosys, regardless of how ENABLE_DEBUG was set. I don't know if this is by design or just an unexpected behavior.

Explain how this is achieved.
The STRIP variable is cleared when ENABLE_DEBUG is set to 1, and validity checks are performed everywhere the STRIP variable is used.

If applicable, please suggest to reviewers how they can test the change.
Build with ENABLE_DEBUG=1 and inspect with a debug symbol viewing tool.

@KrystalDelusion KrystalDelusion requested a review from mmicko May 19, 2025 22:47
Copy link
Collaborator

@widlarizer widlarizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core of the issue is that make install strips installed yosys and libyosys.so, regardless of ENABLE_DEBUG. This seems wrong, and this PR is a valid solution to this

I see a potential problem with somebody setting a custom STRIP variable regardless of if they set ENABLE_DEBUG, but also, I'm willing to bet that no such person exists, so I don't mind.

Also, beware: strip -S creates binaries that file reports as not stripped... terrifying

@RonxBulld
Copy link
Contributor Author

The core of the issue is that make install strips installed yosys and libyosys.so, regardless of ENABLE_DEBUG. This seems wrong, and this PR is a valid solution to this

I see a potential problem with somebody setting a custom STRIP variable regardless of if they set ENABLE_DEBUG, but also, I'm willing to bet, so I don't mind.

Also, beware: strip -S creates binaries that file reports as not stripped... terrifying

You summed it up more clearly than I could.
Yes, I also noticed the "not stripped" problem you mentioned. The file tool does not seem to think that ELF without DWARF is "stripped".
For the other question, if the user executes "make" and simply leaves STRIP empty in the hope that STRIP will not be executed, it will cause another problem: the shell tries to execute a command called "-S".

@widlarizer
Copy link
Collaborator

I thought of an evil alternative, by the way: if STRIP is set to true, the true shell builtin will be called, which does nothing and exits with 0. So stripping may be disabled by setting STRIP=true - by the user, or automatically when ENABLE_DEBUG=1. This is a neat solution but very confusing as well

@widlarizer widlarizer added the merge-after-jf Merge: PR will be merged after the next Dev JF unless concerns are raised label May 22, 2025
@widlarizer widlarizer merged commit 4f7ea38 into YosysHQ:main May 26, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-after-jf Merge: PR will be merged after the next Dev JF unless concerns are raised
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants