Page MenuHomePhabricator

[PowerPC] Make no-PIC default to match GCC - CLANG

Authored by stefanp on Oct 17 2018, 12:38 PM.



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.

NOTE: This patch does not yet include test case updates. Currently working on those updates and will further update this patch at that time. In the meantime I put up this patch to make sure that this is in fact what we want to do.

Diff Detail


Event Timeline

stefanp created this revision.Oct 17 2018, 12:38 PM

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.

  1. This is just what users have come to expect and will typically use -fPIC/-fpic explicitly when they rely on this behaviour.
  2. 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.

stefanp updated this revision to Diff 170400.Oct 22 2018, 6:30 AM

Test cases updated.

stefanp updated this revision to Diff 170926.Oct 24 2018, 10:12 AM

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.

jsji added a comment.Oct 24 2018, 11:33 AM

Agree, I prefer that we don't set either -fPIC or -fPIE by default .

33 ↗(On Diff #170926)

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?

7 ↗(On Diff #170926)

Do we know why this changes due to fPIC default change?
If this is a non-relevant change, maybe deliver in another NFC commit? Thanks.

stefanp added inline comments.Oct 24 2018, 1:08 PM
33 ↗(On Diff #170926)

Good point. I'm going to add another option to check that -fPIC behaves as it did before.

7 ↗(On Diff #170926)

No I don't know exactly why this changes.
I was actually debating in this situation whether or not I should just add -fPIC to the options to keep the behaviour the same as before.

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.

sfertile added inline comments.Oct 25 2018, 6:55 AM
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.

nemanjai added inline comments.Nov 2 2018, 8:29 AM
7 ↗(On Diff #170926)

@stefanp You need to figure out what is going on with this test case for you. I have tried this on PPC as well and the test case did *not* change behaviour.

stefanp added inline comments.Nov 2 2018, 1:42 PM
7 ↗(On Diff #170926)

I got to the bottom of this issue.
It was a mistake on my part. The snapshot that I had for llvm had that test failing before I added my change.
This test has nothing to do with my change at all and I so won't modify it.
Sorry for the confusion... :(

stefanp updated this revision to Diff 172434.Nov 2 2018, 2:24 PM

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.

This revision is now accepted and ready to land.Nov 16 2018, 10:23 AM
This revision was automatically updated to reflect the committed changes.
stefanp reopened this revision.Nov 16 2018, 12:44 PM
This revision is now accepted and ready to land.Nov 16 2018, 12:44 PM
stefanp updated this revision to Diff 175301.Nov 26 2018, 11:09 AM

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.

This revision was automatically updated to reflect the committed changes.
stefanp reopened this revision.Dec 18 2018, 7:07 AM
This revision is now accepted and ready to land.Dec 18 2018, 7:07 AM
This revision was automatically updated to reflect the committed changes.
sfertile added inline comments.Dec 18 2018, 7:17 AM

I think you should mention why BE has to remain pic by default in this comment.