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 ↗

(On Diff #8367)

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 ↗

(On Diff #8367)

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 ↗

(On Diff #8367)

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 ↗

(On Diff #8367)

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.

tools/gold/gold-plugin.cpp

namespace options {
   enum generate_bc { BC_NO, BC_ALSO, BC_ONLY };
   static bool generate_api_file = false;
   static generate_bc generate_bc_file = BC_NO;
   static std::string bc_path;
   static std::string obj_path;
   static std::string extra_library_path;
   static std::string triple;
   static std::string mcpu;
+  static bool relocation_pic = false;
   // Additional options to pass into the code generator.
   // Note: This array will contain all plugin options which are not claimed
   // as plugin exclusive to pass to the code generator.
   // For example, "generate-api-file" and "as"options are for the plugin
   // use only and will not be passed.
   static std::vector<std::string> extra;
 
   static void process_plugin_option(const char* opt_)
   {
     if (opt_ == NULL)
       return;
     llvm::StringRef opt = opt_;
 
     if (opt == "generate-api-file") {
       generate_api_file = true;
     } else if (opt.startswith("mcpu=")) {
       mcpu = opt.substr(strlen("mcpu="));
     } else if (opt.startswith("extra-library-path=")) {
       extra_library_path = opt.substr(strlen("extra_library_path="));
     } else if (opt.startswith("mtriple=")) {
       triple = opt.substr(strlen("mtriple="));
+    } else if (opt == "relocation-pic") {
+      relocation_pic = true;
     } else if (opt.startswith("obj-path=")) {
       obj_path = opt.substr(strlen("obj-path="));
     } else if (opt == "emit-llvm") {
       generate_bc_file = BC_ONLY;
     } else if (opt == "also-emit-llvm") {
       generate_bc_file = BC_ALSO;
     } else if (opt.startswith("also-emit-llvm=")) {
       llvm::StringRef path = opt.substr(strlen("also-emit-llvm="));
for (unsigned i = 0, e = I->syms.size(); i != e; i++) {
           api_file << I->syms[i].name << "\n";
       }
     }
   }
 
   if (options::generate_api_file)
     api_file.close();
 
+  if (options::relocation_pic)
+    output_type = LTO_CODEGEN_PIC_MODEL_DYNAMIC;
   lto_codegen_set_pic_model(code_gen, output_type);
   lto_codegen_set_debug_model(code_gen, LTO_DEBUG_MODEL_DWARF);
   if (!options::mcpu.empty())
     lto_codegen_set_cpu(code_gen, options::mcpu.c_str());
 
   // Pass through extra options to the code generator.
   if (!options::extra.empty()) {
     for (std::vector<std::string>::iterator it = options::extra.begin();