Page MenuHomePhabricator

New clang option -mpie-copy-relocationss to indicate support for linker copy relocations when linking as PIE
ClosedPublic

Authored by tmsriram on May 5 2016, 2:25 PM.

Details

Summary

With linker copy relocations, PIE can generate better code for external global variable accesses. This patch adds a new option to clang to specify this. Please see http://reviews.llvm.org/D24849 for the patch to LLVM to optimize global accesses when this is available.

Diff Detail

Repository
rL LLVM

Event Timeline

tmsriram updated this revision to Diff 56347.May 5 2016, 2:25 PM
tmsriram retitled this revision from to New clang option -mpiecopyrelocs to indicate support for linker copy relocations when linking as PIE.
tmsriram updated this object.
tmsriram added a reviewer: rnk.
tmsriram added subscribers: cfe-commits, davidxl, rafael.

Is there a gcc option or they just assume they are targeting the
linker that was around when gcc was built?

+ if (Args.hasFlag(options::OPT_mpiecopyrelocs, options::OPT_mno_piecopyrelocs,
+ false)) {
+ CmdArgs.push_back("-piecopyrelocs");
+ }

you don't need the {}

+def piecopyrelocs : Flag<["-"], "piecopyrelocs">,
+ HelpText<"Linker copy relocations support when linking as PIE">;

I think you are missing a verb: Linker copy relocations *are* supported.

But how about just "Position independent executables can have copy relocations"?

Cheers,
Rafael

tmsriram updated this revision to Diff 74757.Oct 14 2016, 5:14 PM
tmsriram retitled this revision from New clang option -mpiecopyrelocs to indicate support for linker copy relocations when linking as PIE to New clang option -mpie-copy-relocationss to indicate support for linker copy relocations when linking as PIE.
tmsriram updated this object.
tmsriram added a reviewer: davide.
  • Option name changed to -mpie-copy-relocations
  • Option sets MCPIECopyRelocations in MCOptions
rnk edited edge metadata.Oct 19 2016, 11:30 AM

Is there a gcc option or they just assume they are targeting the
linker that was around when gcc was built?

IIRC there's no GCC option. They base their decision on the configure-time check like you suggested.

include/clang/Driver/Options.td
1696 ↗(On Diff #74757)

I agree with Rafael, this needs a verb, like "Use copy relocations in PIE builds" or something.

1698 ↗(On Diff #74757)

The "no" option variants don't need to be CC1 options.

1699 ↗(On Diff #74757)

I don't think we need help text for the "no" variant. I think we add help text to "no" flags when the flag is on by default, for example -fno-exceptions.

tmsriram updated this revision to Diff 75179.Oct 19 2016, 11:41 AM
tmsriram edited edge metadata.
  • Changed help text to "Avail copy relocations support for PIE builds"
  • Removed mno-pie... as a CC1 option
  • Removed help text from mno-pie..
tmsriram marked 3 inline comments as done.Oct 19 2016, 11:41 AM
rnk accepted this revision.Oct 19 2016, 11:43 AM
rnk edited edge metadata.

lgtm

include/clang/Driver/Options.td
1696 ↗(On Diff #74757)

I guess "avail" is a verb, but I've never seen anyone use it. I'd really rather use "use". :)

This revision is now accepted and ready to land.Oct 19 2016, 11:43 AM
mgrang added a subscriber: mgrang.Oct 19 2016, 11:48 AM
mgrang added inline comments.
lib/Driver/Tools.cpp
4502 ↗(On Diff #75179)

Please fix indentation.

lib/Frontend/CompilerInvocation.cpp
589 ↗(On Diff #75179)

You should also check for the negative flag here:

Opts.PIECopyRelocations = Args.hasFlag(options::OPT_mpie_copy_relocations, options::OPT_mno_pie_copy_relocations, false);

tmsriram added inline comments.Oct 19 2016, 11:51 AM
lib/Frontend/CompilerInvocation.cpp
589 ↗(On Diff #75179)

mpie_copy_relocations is the only CC1 flag here and that is pushed into CC1 command after checking for the negative.

tmsriram updated this revision to Diff 75189.Oct 19 2016, 11:58 AM
tmsriram edited edge metadata.
  • Fix Help text.
  • Fix indentation.
tmsriram marked 3 inline comments as done.Oct 19 2016, 11:59 AM
This revision was automatically updated to reflect the committed changes.