-
-
Notifications
You must be signed in to change notification settings - Fork 610
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 issue 19116 - Segfaults on recent Linux 32 bits distros #9981
base: master
Are you sure you want to change the base?
Conversation
|
I think it would be much better to find out what the PIE code gen issue is. |
|
@WalterBright There is no codegen issue. LDC turned on PIC by default ldc-developers/ldc#1618 & ldc-developers/ldc#1664 |
|
The rust compiler runs the linker with |
|
@SSoulaimane uhm thanks a lot for this work, but I'm not sure this is a good investment of your time. As this has been broken for years, it clearly shows that either no one uses DMD 32-bit anymore on Linux or they switched to LDC. In either case, the 32-bit |
|
@wilzbach Thanks. It's not a hard problem though. We just need to make a decision, we either turn PIC on, or read the linker output. |
|
@SSoulaimane I suggest to turn PIC on by default and provide the option to disable it for the rare cases where this may be necessary. |
9674369 to
b0dc2c7
Compare
|
@PetarKirov I thought about it but then if we had a working option that turns PIC off we could just use it and keep PIC off by default. |
Uhm I'm not sure how I am supposed to know this, though most/all? major Linux distributions have switched to PIC only quite a while ago, so I wouldn't be surprised if non-PIC is no longer tested nor supported unofficially. There would be little point in fixing it in this case. |
|
@wilzbach It's fixed now I don't know if you want to keep the circle ci tests or not. |
b0dc2c7 to
9f423c8
Compare
|
Thanks for your pull request and interest in making D better, @SSoulaimane! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#9981" |
| FILE* fp = popen(cmd.peekChars(), "r"); | ||
| scope(exit) pclose(fp); | ||
| return 1 == findStr(fp, "--enable-default-pie"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the overhead in opening this process on every compilation? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linker is slow anyway would be an argument.
But maybe we can pessimize older cc versions instead of the new ones i.e optimistically assume that cc can handle the -no-pie switch and retry if it fails.
9f423c8 to
af3fead
Compare
Check the linker (cc) on linux if it assumes PIE by default to turn it off when linking non-PIC code. We check the linker build flags since this option only applies to GCC 6 and newer.
Pass the
-no-pieflag toccon Linux 32 bits.Re-enables CircleCi 32 bits tests which were previously disabled in #9499.