Ran a quick test and determined that for GCC the default option is -fno-PIC for both ELFv1 and ELFv2. We are currently using -fPIC as the default for LLVM.
Would like to revert to what GCC is doing on PowerPC.
Details
- Reviewers
nemanjai kbarton sfertile syzaara lei hfinkel joerg echristo - Commits
- rG4810420ca11d: [PowerPC] Make no-PIC default to match GCC - CLANG
rGc75a9651d716: [PowerPC] Make no-PIC default to match GCC - CLANG
rG3bb8c70dfabf: [PowerPC] Make no-PIC default to match GCC - CLANG
rC349489: [PowerPC] Make no-PIC default to match GCC - CLANG
rL349489: [PowerPC] Make no-PIC default to match GCC - CLANG
rC348299: [PowerPC] Make no-PIC default to match GCC - CLANG
rL348299: [PowerPC] Make no-PIC default to match GCC - CLANG
rC347070: [PowerPC] Make no-PIC default to match GCC - CLANG
rL347070: [PowerPC] Make no-PIC default to match GCC - CLANG
Diff Detail
Event Timeline
TL; DR: +1 from me for proceeding with this patch. If you don't hear any strong objections before next week, I'd say please update the test cases and we'll go ahead with this.
I think it is important to match what GCC is doing for two reasons.
- This is just what users have come to expect and will typically use -fPIC/-fpic explicitly when they rely on this behaviour.
- Most build systems that build both static and shared versions will use -fPIC explicitly for the shared library case and just omit the option (rather than adding -fno-pic for the static library case).
For the most part, things work the same on PPC between the two cases, but there are some important differences - not the least of which being tail-call optimizations. We do not want the PIC default to take away those optimization opportunities in builds where PIC truly doesn't matter.
Default on all Power PC platforms is now no-PIC.
If any bugs are exposed by this change we will fix them as they are exposed.
Agree, I prefer that we don't set either -fPIC or -fPIE by default .
test/Driver/ppc-abi.c | ||
---|---|---|
33–65 | No sure whether this is correct place to add, but maybe we should add some test to make sure that adding -fPIC option will set the pic-level the same as before? | |
test/Driver/print-multi-directory.c | ||
7 ↗ | (On Diff #170926) | Do we know why this changes due to fPIC default change? |
test/Driver/ppc-abi.c | ||
---|---|---|
33–65 | Good point. I'm going to add another option to check that -fPIC behaves as it did before. | |
test/Driver/print-multi-directory.c | ||
7 ↗ | (On Diff #170926) | No I don't know exactly why this changes. Unfortunately I can't put this in a later NFC change because I would leave a failing test. I either have to adjust the test or I would have to add -fPIC. |
test/Driver/print-multi-directory.c | ||
---|---|---|
7 ↗ | (On Diff #170926) | This test changing behavior is suspect, the run line uses a target that is not powerpc and if the test somehow depended on the hosts default pic value wouldn't it be failing for mips64 as well? FWIW I applied this change to top-of-trunk and ran the test an X86 machine and the test behavior didn't change. I think it worth investigating to find out why this test changes behavior due to this change. |
test/Driver/print-multi-directory.c | ||
---|---|---|
7 ↗ | (On Diff #170926) | I got to the bottom of this issue. |
Added the PIC version to the second test as well.
Removed the third test. It has nothing to do with this change and I should not have modified that test in the first place.
Patch is now only for Little Endian.
Big Endian will remain PIC by default as it was before. This is due to a bug found on BE where -fsanitize=leak is not detecting leaks correctly. As Little Endian is a higher priority we will add that code first and then look at the BE issue at a later date.
cfe/trunk/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
2438 ↗ | (On Diff #178669) | I think you should mention why BE has to remain pic by default in this comment. |
No sure whether this is correct place to add, but maybe we should add some test to make sure that adding -fPIC option will set the pic-level the same as before?