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

Add -fPIC for the build system #72

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Conversation

JerrySievert
Copy link
Contributor

This adds -fPIC for the build process, which is needed for any code that needs to be position indepentent, like something that would end up linking for a shared object.

this appears to work correctly on linux and macOS, but I have not been able to confirm under windows.

@bnoordhuis
Copy link
Owner

Please ignore the review; I deleted it again. I clicked cancel because I wanted to ask a general question but GH posted it anyway 🤦

Anyway, the question: does this turn on -fPIC always or only when you pass -Denable-fPIC=ON to cmake?

@JerrySievert
Copy link
Contributor Author

it turns on PIC always. I was struggling for a reason why any build would want PIC disabled. is there a reason why a build would want to disable it? if so, I can make it available with a flag.

@bnoordhuis
Copy link
Owner

It should be a flag. On some architectures (notably 32 bits x86 because of register pressure) position independent code is 5-10% slower than non-PIC code. It's why node didn't turn on PIE for the longest time.

@JerrySievert
Copy link
Contributor Author

okee dokee, can do. my vacation is over, so it might take a couple of days to get the PR updated, in the meantime, I'll convert it into a draft.

@JerrySievert JerrySievert marked this pull request as draft July 10, 2023 19:55
@JerrySievert
Copy link
Contributor Author

changed to -Denable-fPIC=ON - if you want me to change the if() statements to wrap around the actual enable instead of each individual, I can, but even though this is a few more lines of code I felt it was clearer.

happy to make any changes though.

@bnoordhuis bnoordhuis merged commit 90d6634 into bnoordhuis:master Jul 12, 2023
4 checks passed
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