Introduce new gold plugin option "relocation-pic"

[volkalexey]

Summary

With this patch I introduced new gold plugin option "relocation-pic".
This option sets PIC dynamic model for generated code.
This mode is needed for executables on Android.

[volkalexey]

Ping

[volkalexey]

Hi Rafael,

I've rewritten my patch with "-pie" linker option.
Unfortunately I couldn't use isPIEDefault function since PIE will be turned on for all objects generated in Clang::ConstructJob.
Analysis of compiler options is not sufficient since the same object can be used for library or for executable.

[• rafael]

I think this is OK.

Peter, can you just confirm that the handling of hasZeroBaseShadow is
correct? In particular, its interaction with -static.

[pcc]

I'm not sure if this is correct. If an executable is being linked statically I think it will still need to be a PIE if a sanitizer which needs the lower pages is enabled. But then again libc or other system static libraries may have been built as non-PIC so PIE linking might fail. I think the right thing to do might be to always try to link a PIE if the sanitizer needs it, and let the linker complain if it doesn't work, i.e. rewrite like this:

  const bool IsPIE =
		​    !Args.hasArg(options::OPT_shared) &&
		​    (Args.hasArg(options::OPT_pie) || Sanitize.hasZeroBaseShadow() ||
		​     (isAndroid && !Args.hasArg(options::OPT_static)));

Adding Alexey Samsonov, who may be able to confirm the correct behavior here (also since there aren't enough Alexeys on this thread).

[volkalexey]

Unfortunately my last patch cannot work with -Wl compiler option.
E.g. passing -Wl,-shared will not set OPT_shared variable and so -shared and -pie both passed to linker.
Linker will complain about incompatible options.
I am not sure if parsing -Wl option value for shared, static, etc is acceptable here.
In my first patch addition of new gold plugin option does not have such problems.

[volkalexey]

I updated patch to the current trunk.

Alexey, could you check if interaction between -static and Sanitize.hasZeroBaseShadow() is correct?

[samsonov]

In general, sanitizers don't work with -static flag at all, so there shouldn't be any problems with "interaction".

lib/Driver/Tools.cpp

6845

I don't understand this part - effectively you're making -pie the default on Android. Is it really the intended behavior?

If yes, shouldn't you implement it in ToolChain.isPIEDefault() instead?

[volkalexey]

lib/Driver/Tools.cpp

6845

Flag -pie passed to linker solved original problem with LTO on Android.
So it is needed when generating executables on Android with LTO enabled.
Also PIE would be useful for any executables on Android but it will not work for libraries.
Implementing PIE in ToolChain.isPIEDefault() will turn it on for all objects since compiler doesn't know if objects will be used for executable or for library.

[samsonov]

lib/Driver/Tools.cpp

6845

Ok. I also see that you can probably do this because -fpic/-fPIC flags are implied for Android on certain architectures (in Clang::ConstructJob method). But, anyway, unconditionally ignoring the presence/absence of -pie flag on Android is a serious change, which might be surprising for end-user. Is it possible to imply "-pie" iff LTO is enabled?

[volkalexey]

lib/Driver/Tools.cpp

6845

Yes, it is possible to use -pie only with -flto.
Raphael, do you have any objections?

[• rafael]

Ok. I also see that you can probably do this because -fpic/-fPIC flags are implied for Android on certain architectures (in Clang::ConstructJob method). But, anyway, unconditionally ignoring the presence/absence of -pie flag on Android is a serious change, which might be surprising for end-user. Is it possible to imply "-pie" iff LTO is enabled?

Yes, it is possible to use -pie only with -flto.
Raphael, do you have any objections?

I am probably missing something.

From the previous comments I was under the impression that non-pie
executable were not supported at all on android. If that is the case,
passing -pie would be nop, except for working around a linker bug (it
not passing LDPO_PIE to the plugin).

If that is the case, we should just always pass -pie. If that is not
the case, I don't understand the patch. When building a pie executable
the user has to pass -fPIE to clang and -pie gets passed to the linker
already, no?

Cheers,
Rafael

[volkalexey]

On Android user doesn't have to pass -fPIC or -fPIE option.
Clang is turning on PIC mode (not PIE) without any user input when targeting to Android.
However when producing executable compiler has no special options (-pie) for linker.
So linker produces executable from PIC objects without -pie option.
So final executable is not pure PIE, it's executable formed from PIC code.
This is what we have now on Android.
Adding -pie option to linker could change produced code which could be surprising to user.
Since -pie needed only to fix or workaround this bug I propose to have -pie only with LTO enabled.

[volkalexey]

I updated patch with the comment and rebase it to current trunk.

[• rafael]

Getting close. LGTM with a comment about why we can't use
isPIEDefault: It would enable PIE only logic, like which TLS model to
use, when compiling. That actually looks like a bug in the driver, but
can be fixed in another commit.

Cheers,
Rafael

[volkalexey]

2014-04-28 20:38 GMT+04:00 Rafael Ávila de Espíndola <
[e-mail]>:

Getting close. LGTM with a comment about why we can't use
isPIEDefault: It would enable PIE only logic, like which TLS model to
use, when compiling. That actually looks like a bug in the driver, but
can be fixed in another commit.

I don't understand how it could be fixed.
When compiler generates object it has no information regarding further
usage of object: executable or library.
Function isPIEDefault turns on PIE for every object for specific target.
So it seems to be useful only for targets with no shared libraries.

Cheers,
Rafael

http://reviews.llvm.org/D2668

[danalbert]

What happened to guarding this to only be enabled with LTO? Enabling it for all Android targets isn't correct, as anything before Jelly Bean (~15% of devices) doesn't even support PIE. It currently is not possible to build an Android executable with clang that works on ICS or earlier.

Since there is no way to override this behavior, I'd much prefer to just require that build tools supply -pie on their own.

BTW, srhines should really be added as a reviewer for any Android related clang change.

lib/Driver/Tools.cpp

void gnutools::Link::ConstructJob(Compilation &C, const JobAction &JA,
                                   const char *LinkingOutput) const {
   const toolchains::Linux& ToolChain =
     static_cast<const toolchains::Linux&>(getToolChain());
   const Driver &D = ToolChain.getDriver();
   const bool isAndroid =
     ToolChain.getTriple().getEnvironment() == llvm::Triple::Android;
   const bool IsPIE =
     !Args.hasArg(options::OPT_shared) &&
-    (Args.hasArg(options::OPT_pie) || ToolChain.isPIEDefault());
+    !Args.hasArg(options::OPT_static) &&
+    (Args.hasArg(options::OPT_pie) || ToolChain.isPIEDefault() || isAndroid);
 
   ArgStringList CmdArgs;
 
   // Silence warning for "clang -g foo.o -o foo"
   Args.ClaimAllArgs(options::OPT_g_Group);
   // and "clang -emit-llvm foo.o -o foo"
   Args.ClaimAllArgs(options::OPT_emit_llvm);
   // and for "clang -w foo.o -o foo". Other warning options are already

test/Driver/gold-lto.c

 // CHECK-X86-64-COREI7: "-plugin-opt=foo"
 //
 // RUN: %clang -target arm-unknown-linux -### %t.o -flto 2>&1 \
 // RUN:     -march=armv7a -Wl,-plugin-opt=foo \
 // RUN:     | FileCheck %s --check-prefix=CHECK-ARM-V7A
 // CHECK-ARM-V7A: "-plugin" "{{.*}}/LLVMgold.so"
 // CHECK-ARM-V7A: "-plugin-opt=mcpu=cortex-a8"
 // CHECK-ARM-V7A: "-plugin-opt=foo"
+//
+// RUN: %clang -target i686-linux-android -### %t.o -flto 2>&1 \
+// RUN:     | FileCheck %s --check-prefix=CHECK-X86-ANDROID
+// CHECK-X86-ANDROID: "-pie"
+// CHECK-X86-ANDROID: "-plugin" "{{.*}}/LLVMgold.so"