This is an archive of the discontinued LLVM Phabricator instance.

[X86] enable PIE for functions
ClosedPublic

Authored by AsafBadouh on Apr 18 2016, 9:09 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

AsafBadouh updated this revision to Diff 54072.Apr 18 2016, 9:09 AM
AsafBadouh retitled this revision from to [X86] enable PIE for functions.
AsafBadouh updated this object.
AsafBadouh set the repository for this revision to rL LLVM.
AsafBadouh added a subscriber: llvm-commits.
rnk edited edge metadata.Apr 18 2016, 9:50 AM

Can you add a test for other linkages as Rafael asked?

This is still missing a testcase for other linkages (weak_odr and weak at least).

I would test internal, weak, and weak_odr. Include them even if they generate calls through the PLT.

../llvmOrg/lib/Target/X86/X86Subtarget.cpp
148 ↗(On Diff #54072)

ditto, run the formatter

../llvmOrg/lib/Target/X86/X86Subtarget.h
559 ↗(On Diff #54072)

Is this parameter really necessary? I expect you can remove it, even if you have to change a few test expectations.

561 ↗(On Diff #54072)

Run clang-format

AsafBadouh marked 2 inline comments as done.Apr 19 2016, 3:23 AM
AsafBadouh added inline comments.
../llvmOrg/lib/Target/X86/X86Subtarget.h
559 ↗(On Diff #54072)

I can remove isFast flag but in case it's called from "fastLowerCall" and the return value is "MO_GOTPCREL" I will need to reset it back to zero.
that's because in the fast path we don't handle "NonLazyBind" case in the fast path (see lib/Target/X86/X86ISelLowering.cpp:3333).

do you think we should handle it in the fast path?
or should I remove the flag and reset the return value inside "fastLowerCall"?

AsafBadouh updated this revision to Diff 54171.Apr 19 2016, 3:26 AM
AsafBadouh edited edge metadata.

run clang-format and add other linkage testcases

rnk added a comment.Apr 19 2016, 9:07 AM

Minor issues, I think this is basically ready

../llvmOrg/lib/Target/X86/X86Subtarget.h
559 ↗(On Diff #54171)

Changing fastLowerCall seems nice, it keeps the complexity of what FastISel does local to itself.

../llvmOrg/test/CodeGen/X86/pie.ll
7 ↗(On Diff #54171)

Should this be foo{{$}}?

9 ↗(On Diff #54171)

Interesting. I would've expected a PLT call for such weak functions so that they can be provided by libraries, but it seems you cannot override a non-ODR weak function from an executable with GCC.

GCC generates a direct, non-PLT call in non-PIE mode, and a PLT call with -fPIE. I'm guessing they just don't do this optimization yet.

AsafBadouh added inline comments.Apr 19 2016, 9:24 AM
../llvmOrg/lib/Target/X86/X86Subtarget.h
559 ↗(On Diff #54171)

NP, will upload fix for that shortly.

../llvmOrg/test/CodeGen/X86/pie.ll
7 ↗(On Diff #54171)

yes!

9 ↗(On Diff #54171)

I think they do have it in GCC5.3

AsafBadouh updated this revision to Diff 54213.Apr 19 2016, 9:44 AM

update according to Reid comments.

rnk accepted this revision.Apr 19 2016, 10:52 AM
rnk edited edge metadata.

lgtm, thanks!

This revision is now accepted and ready to land.Apr 19 2016, 10:52 AM
This revision was automatically updated to reflect the committed changes.