[ELF] - LTO: Try to be option compatible with the gold plugin.

[grimar]

Summary

This is relative to PR30720.

Previously we ignored all --plugin-opt arguments.
Patch adds support for them.

Patch does not add any new LTO options,
and just implements mapping from --plugin-opt to existent ones.

I am not sure if we want to support any other --plugin-opt options.

[ruiu]

ELF/Driver.cpp

621

Rename getLTOValue.

623

This function is unnecessarily too complicated. Don't mix Key with other plugin options. Handle Key first, and then if Key does not exist, handle others. I.e.

if (Args.hasArg(Key))
  return getInteger(Args, Key, Default);
...

[davide]

This patch lies in the limbo between fully supported and completely unsupported.
As such, I'm not sure I like it.
Previously, we just ignored every possible argument to --plugin-opt (or plugin-opt entirely).
I guess we should at least error out in case the argument can't be translated (or add a test for that).

[grimar]

In D36227#830068, @davide wrote:

This patch lies in the limbo between fully supported and completely unsupported.
As such, I'm not sure I like it.

I was not sure how to support other ones and if we want to support it.
For example gold-plugin (which is my reference, https://github.com/llvm-mirror/llvm/blob/master/tools/gold/gold-plugin.cpp#L190)
supports --plugin-opt=mcpu=FOO, which then assigned to lto::Config::CPU field.
But LLD does not have separate command line option for setting that and it is not clear for me
if it is desirable. It is unclear also if we need separate option or can live with --option-opt=mcpu (in case
if we want to support it). In latter case I would move initialization of lto::Config struct fields to Driver.cpp
probably, but that is story for different patch.

Another exaple is "thinlto-index-only=" option, looking on implementation it does not set any Config field,
but do something else and I am not yet familar with gold plugin logic and so would prefer to implement it separatelly.

Because of all above I started from supporting things that we already have (and which are clear in part of implementation for me).
I think patch supports all LTO options that we have atm in command line and do that "at once for all", what is probably
good for start.

Previously, we just ignored every possible argument to --plugin-opt (or plugin-opt entirely).
I guess we should at least error out in case the argument can't be translated (or add a test for that).

Ok, I agree. Not sure about error, I think warning for start can be enough probably. And we can change it to error later
when it be clear which options we support/ignore and which not.

[grimar]

ELF/Driver.cpp

623

If I do that then mix of options will work incorrectly, example:

ld.lld --lto-O0 --plugin-opt=O6 ...

will set optimization level 0 instead of 6

[ruiu]

No, you don't have to worry too much about the case where users use both gold plugin options and lld options.

[grimar]

Reimplemented:

• Ignore some options passed by gcc.
• Error out on unsupported --plugin-opt= options.
• Don't mix regular commandline and --plugin-opt commandline handling.

[ruiu]

ELF/Driver.cpp

614

Please do not use function overloading unless you really need it. We already have the function with the same name, and that function is different from this. When you write a new function with a different meaning/semantics, you usually have to give it a new name.

You are not using Arg except for generating an error message. Please change the signature to

static int parseInt(StringRef S, opt::Arg *Arg)

635–637

Negate the condition and remove continue.

725

Inline this function.

ELF/Options.td

363

80 cols

[grimar]

*All review comments addressed.

[ruiu]

ELF/Driver.cpp

725

plugin_opt_eq is an alias for plugin_opt. Do you have to pass both?

738

What is '/'?

[grimar]

ELF/Driver.cpp

725

I'll recheck.

738

Line Rafael mentioned earlier:

"Note that gcc has be bad habit of always passing some options, so we
pretty much have to ignore those:
-plugin-opt=/usr/libexec/gcc/x86_64-redhat-linux/7/lto-wrapper
...
"

It is LTO wrapper which we should ignore:
https://stackoverflow.com/questions/19807107/what-is-gcc-lto-wrapper

[grimar]

ELF/Driver.cpp

725

plugin_opt_eq is an alias for plugin_opt. Do you have to pass both?

Yes, so if you have --plugin-opt=aaa --plugin-opt bbb, then
Args.filtered(OPT_plugin_opt) will return only bbb and
Args.filtered(OPT_plugin_opt_eq) will return only aaa.

Not sure if it is a bug or by design of filtered(), but atm we need both it seems.

[ruiu]

ELF/Driver.cpp

725

OK, maybe it cannot handle aliases correctly. It is OK for now.

732

drop_front(N) is the same as substr(N), right? Please use substr.

[grimar]

• Used substr() instead of drop_front().

[ruiu]

LGTM

ELF/Driver.cpp

if (S == "none")
     return false;
   if (S != "zlib")
     error("unknown --compress-debug-sections value: " + S);
   if (!zlib::isAvailable())
     error("--compress-debug-sections: zlib is not available");
   return true;
 }
 
+static int getInteger(opt::Arg *Arg, StringRef S) {
+  int V = 0;
+  if (!to_integer(S, V, 10))
+    error(Arg->getSpelling() + ": number expected, but got '" + S + "'");
+  return V;
+}
+
+void scanLTOPluginOptions(opt::InputArgList &Args) {
+  for (auto *Arg : Args.filtered(OPT_plugin_opt, OPT_plugin_opt_eq)) {
+    StringRef S = Arg->getValue();
+    if (S == "disable-verify")
+      Config->DisableVerify = true;
+    else if (S == "save-temps")
+      Config->SaveTemps = true;
+    else if (S.startswith("O"))
+      Config->LTOO = getInteger(Arg, S.drop_front(1));
+    else if (S.startswith("lto-partitions="))
+      Config->LTOPartitions = getInteger(Arg, S.drop_front(15));
+    else if (S.startswith("jobs="))
+      Config->ThinLTOJobs = getInteger(Arg, S.drop_front(5));
+    // Ignore some options always passed by gcc.
+    else if (S.startswith("/") || S.startswith("-fresolution=") ||
+             S.startswith("-pass-through="))
+      continue;
+    else
+      error(Arg->getSpelling() + ": unknown option: " + S);
+  }
+}
+
 // Initializes Config members by the command line options.
 void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->AllowMultipleDefinition = Args.hasArg(OPT_allow_multiple_definition);
   Config->AuxiliaryList = getArgs(Args, OPT_auxiliary);
   Config->Bsymbolic = Args.hasArg(OPT_Bsymbolic);
   Config->BsymbolicFunctions = Args.hasArg(OPT_Bsymbolic_functions);
   Config->Chroot = Args.getLastArgValue(OPT_chroot);
   Config->CompressDebugSections = getCompressDebugSections(Args);
void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->ZNow = hasZOption(Args, "now");
   Config->ZOrigin = hasZOption(Args, "origin");
   Config->ZRelro = !hasZOption(Args, "norelro");
   Config->ZRodynamic = hasZOption(Args, "rodynamic");
   Config->ZStackSize = getZOptionValue(Args, "stack-size", 0);
   Config->ZText = !hasZOption(Args, "notext");
   Config->ZWxneeded = hasZOption(Args, "wxneeded");
 
+  scanLTOPluginOptions(Args);
   if (Config->LTOO > 3)
-    error("invalid optimization level for LTO: " +
-          Args.getLastArgValue(OPT_lto_O));
+    error("invalid optimization level for LTO: " + Twine(Config->LTOO));
   if (Config->LTOPartitions == 0)
     error("--lto-partitions: number of threads must be > 0");
   if (Config->ThinLTOJobs == 0)
     error("--thinlto-jobs: number of threads must be > 0");
 
   if (auto *Arg = Args.getLastArg(OPT_m)) {
     // Parse ELF{32,64}{LE,BE} and CPU type.
     StringRef S = Arg->getValue();
     std::tie(Config->EKind, Config->EMachine, Config->OSABI) =
         parseEmulation(S);
     Config->MipsN32Abi = (S == "elf32btsmipn32" || S == "elf32ltsmipn32");
     Config->Emulation = S;
   }
 
   if (Args.hasArg(OPT_print_map))
     Config->MapFile = "-";
 
   // --omagic is an option to create old-fashioned executables in which
   // .text segments are writable. Today, the option is still in use to

ELF/Options.td

 def lto_partitions: J<"lto-partitions=">,
   HelpText<"Number of LTO codegen partitions">;
 def disable_verify: F<"disable-verify">;
 def mllvm: S<"mllvm">;
 def opt_remarks_filename: Separate<["--"], "opt-remarks-filename">,
   HelpText<"YAML output file for optimization remarks">;
 def opt_remarks_with_hotness: Flag<["--"], "opt-remarks-with-hotness">,
   HelpText<"Include hotness informations in the optimization remarks file">;
+defm plugin_opt: Eq<"plugin-opt">, HelpText<"specifies LTO options for compatibility with GNU linkers">;
 def save_temps: F<"save-temps">;
 def thinlto_cache_dir: J<"thinlto-cache-dir=">,
   HelpText<"Path to ThinLTO cached object file directory">;
 def thinlto_cache_policy: S<"thinlto-cache-policy">,
   HelpText<"Pruning policy for the ThinLTO cache">;
 def thinlto_jobs: J<"thinlto-jobs=">, HelpText<"Number of ThinLTO jobs">;
 
 // Ignore LTO plugin-related options.
 // clang -flto passes -plugin and -plugin-opt to the linker. This is required
 // for ld.gold and ld.bfd to get LTO working. But it's not for lld which doesn't
 // rely on a plugin. Instead of detecting which linker is used on clang side we
 // just ignore the option on lld side as it's easier. In fact, the linker could
 // be called 'ld' and understanding which linker is used would require parsing of
 // --version output.
 def plugin: S<"plugin">;
 def plugin_eq: J<"plugin=">;
-def plugin_opt: S<"plugin-opt">;
-def plugin_opt_eq: J<"plugin-opt=">;
 
 // Options listed below are silently ignored for now for compatibility.
 def allow_shlib_undefined: F<"allow-shlib-undefined">;
 def cref: F<"cref">;
 def detect_odr_violations: F<"detect-odr-violations">;
 def g: Flag<["-"], "g">;
 def no_add_needed: F<"no-add-needed">;
 def no_allow_shlib_undefined: F<"no-allow-shlib-undefined">;

test/ELF/basic.s

 # DUP:      duplicate symbol: _start
 # DUP-NEXT: >>> defined at {{.*}}:(.text+0x0)
 # DUP-NEXT: >>> defined at {{.*}}:(.text+0x0)
 
 # RUN: not ld.lld %t -o %t -m wrong_emul_fbsd 2>&1 | FileCheck --check-prefix=UNKNOWN_EMUL %s
 # UNKNOWN_EMUL: unknown emulation: wrong_emul_fbsd
 
 # RUN: not ld.lld %t --lto-partitions=0 2>&1 | FileCheck --check-prefix=NOTHREADS %s
+# RUN: not ld.lld %t --plugin-opt=lto-partitions=0 2>&1 | FileCheck --check-prefix=NOTHREADS %s
 # NOTHREADS: --lto-partitions: number of threads must be > 0
 
 # RUN: not ld.lld %t --thinlto-jobs=0 2>&1 | FileCheck --check-prefix=NOTHREADSTHIN %s
+# RUN: not ld.lld %t --plugin-opt=jobs=0 2>&1 | FileCheck --check-prefix=NOTHREADSTHIN %s
 # NOTHREADSTHIN: --thinlto-jobs: number of threads must be > 0

test/ELF/lto-plugin-ignore.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: ld.lld %t -plugin-opt=/foo/bar -plugin-opt=-fresolution=zed \
+# RUN:   -plugin-opt=-pass-through=-lgcc -o /dev/null

test/ELF/lto/opt-level.ll

 ; REQUIRES: x86
 ; RUN: llvm-as -o %t.o %s
 ; RUN: ld.lld -o %t0 -m elf_x86_64 -e main --lto-O0 %t.o
 ; RUN: llvm-nm %t0 | FileCheck --check-prefix=CHECK-O0 %s
+; RUN: ld.lld -o %t0 -m elf_x86_64 -e main --plugin-opt=O0 %t.o
+; RUN: llvm-nm %t0 | FileCheck --check-prefix=CHECK-O0 %s
 ; RUN: ld.lld -o %t2 -m elf_x86_64 -e main --lto-O2 %t.o
 ; RUN: llvm-nm %t2 | FileCheck --check-prefix=CHECK-O2 %s
 ; RUN: ld.lld -o %t2a -m elf_x86_64 -e main %t.o
 ; RUN: llvm-nm %t2a | FileCheck --check-prefix=CHECK-O2 %s
+; RUN: ld.lld -o %t2 -m elf_x86_64 -e main --plugin-opt=O2 %t.o
+; RUN: llvm-nm %t2 | FileCheck --check-prefix=CHECK-O2 %s
 
 ; Reject invalid optimization levels.
 ; RUN: not ld.lld -o %t3 -m elf_x86_64 -e main --lto-O6 %t.o 2>&1 | \
-; RUN:   FileCheck --check-prefix=INVALID %s
-; INVALID: invalid optimization level for LTO: 6
+; RUN:   FileCheck --check-prefix=INVALID1 %s
+; INVALID1: invalid optimization level for LTO: 6
+; RUN: not ld.lld -o %t3 -m elf_x86_64 -e main --plugin-opt=O6 %t.o 2>&1 | \
+; RUN:   FileCheck --check-prefix=INVALID1 %s
+; RUN: not ld.lld -o %t3 -m elf_x86_64 -e main --plugin-opt=Ofoo %t.o 2>&1 | \
+; RUN:   FileCheck --check-prefix=INVALID2 %s
+; INVALID2: --plugin-opt=: number expected, but got 'foo'
+
 ; RUN: not ld.lld -o %t3 -m elf_x86_64 -e main --lto-O-1 %t.o 2>&1 | \
-; RUN:   FileCheck --check-prefix=INVALIDNEGATIVE %s
-; INVALIDNEGATIVE: invalid optimization level for LTO: -1
+; RUN:   FileCheck --check-prefix=INVALIDNEGATIVE1 %s
+; INVALIDNEGATIVE1: invalid optimization level for LTO: 4294967295
+; RUN: not ld.lld -o %t3 -m elf_x86_64 -e main --plugin-opt=O-1 %t.o 2>&1 | \
+; RUN:   FileCheck --check-prefix=INVALIDNEGATIVE2 %s
+; INVALIDNEGATIVE2: invalid optimization level for LTO: 4294967295
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; CHECK-O0: foo
 ; CHECK-O2-NOT: foo
 define internal void @foo() {
   ret void
 }
 
 define void @main() {
   call void @foo()
   ret void
 }

test/ELF/lto/save-temps.ll

 ; REQUIRES: x86
 ; RUN: cd %T
 ; RUN: rm -f a.out a.out.lto.bc a.out.lto.o
 ; RUN: llvm-as %s -o %t.o
 ; RUN: llvm-as %p/Inputs/save-temps.ll -o %t2.o
 ; RUN: ld.lld -shared -m elf_x86_64 %t.o %t2.o -save-temps
 ; RUN: llvm-nm a.out | FileCheck %s
 ; RUN: llvm-nm a.out.0.0.preopt.bc | FileCheck %s
 ; RUN: llvm-nm a.out.lto.o | FileCheck %s
 ; RUN: llvm-dis a.out.0.0.preopt.bc
 
+; RUN: rm -f a.out a.out.lto.bc a.out.lto.o
+; RUN: ld.lld -shared -m elf_x86_64 %t.o %t2.o --plugin-opt=save-temps
+; RUN: llvm-nm a.out | FileCheck %s
+; RUN: llvm-nm a.out.0.0.preopt.bc | FileCheck %s
+; RUN: llvm-nm a.out.lto.o | FileCheck %s
+; RUN: llvm-dis a.out.0.0.preopt.bc
+
 target triple = "x86_64-unknown-linux-gnu"
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
 define void @foo() {
   ret void
 }
 
 ; CHECK: T bar
 ; CHECK: T foo

test/ELF/lto/verify-invalid.ll

 ; REQUIRES: x86
 ; RUN: llvm-as %s -o %t.o
 ; RUN: ld.lld -m elf_x86_64 %t.o -o %t2 -mllvm -debug-pass=Arguments \
 ; RUN:   2>&1 | FileCheck -check-prefix=DEFAULT %s
 ; RUN: ld.lld -m elf_x86_64 %t.o -o %t2 -mllvm -debug-pass=Arguments \
 ; RUN:   -disable-verify 2>&1 | FileCheck -check-prefix=DISABLE %s
+; RUN: ld.lld -m elf_x86_64 %t.o -o %t2 -mllvm -debug-pass=Arguments \
+; RUN:   --plugin-opt=disable-verify 2>&1 | FileCheck -check-prefix=DISABLE %s
 
 target triple = "x86_64-unknown-linux-gnu"
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
 define void @_start() {
   ret void
 }
 
 ; -disable-verify should disable the verification of bitcode.
 ; DEFAULT:     Pass Arguments: {{.*}} -verify {{.*}} -verify
 ; DISABLE-NOT: Pass Arguments: {{.*}} -verify {{.*}} -verify