Handle --plugin-opt= options as alias options.

[ruiu]

Summary

Previously, we had a loop to iterate over options starting with
--plugin-opt= and parse them by hand. But we can make OptTable
do that job for us.

[pcc]

I think these options can also be spelt -plugin-opt foo as well as -plugin-opt=foo. Can you see a good way to handle that?

[ruiu]

Sure, I think I can just use Eq instead of J, but I'd like to do that in a follow-up patch because the options without = are not handled now.

[pcc]

In D47167#1107249, @ruiu wrote:

Sure, I think I can just use Eq instead of J, but I'd like to do that in a follow-up patch because the options without = are not handled now.

Are you sure? It certainly looks like they are handled, e.g.

$ ra/bin/ld.lld -plugin-opt Of
ra/bin/ld.lld: error: -plugin-opt=Of: number expected, but got 'f'
ra/bin/ld.lld: error: target emulation unknown: -m or at least one .o file required

[ruiu]

Ouch. I don't know if there's a good way to handle options in that style. One way of doing it is to iterate over argv before passing it to OptParser to concatenate --plugin-opt and the following option with =. It doesn't feel that bad, but I'm not very sure if that's a good thing to do.

[pcc]

In D47167#1107268, @ruiu wrote:

Ouch. I don't know if there's a good way to handle options in that style. One way of doing it is to iterate over argv before passing it to OptParser to concatenate --plugin-opt and the following option with =. It doesn't feel that bad, but I'm not very sure if that's a good thing to do.

Yeah that doesn't seem great, but it's the best that I can think of as well.

[ruiu]

• concat --plugin-opt and the following argument

[pcc]

lld/ELF/DriverUtils.cpp

106 ↗

(On Diff #147916)

I don't think this correctly handles the case where the last two arguments are -plugin-opt foo. Won't you end up with an out of bounds access?

[ruiu]

• fix the bug

[pcc]

LGTM

lld/Common/Args.cpp

 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Option/ArgList.h"
 
 using namespace llvm;
 using namespace lld;
 
 int lld::args::getInteger(opt::InputArgList &Args, unsigned Key, int Default) {
-  int V = Default;
-  if (auto *Arg = Args.getLastArg(Key)) {
-    StringRef S = Arg->getValue();
-    if (!to_integer(S, V, 10))
-      error(Arg->getSpelling() + ": number expected, but got '" + S + "'");
-  }
+  auto *A = Args.getLastArg(Key);
+  if (!A)
+    return Default;
+
+  int V;
+  if (to_integer(A->getValue(), V, 10))
     return V;
+
+  StringRef Spelling = Args.getArgString(A->getIndex());
+  error(Spelling + ": number expected, but got '" + A->getValue() + "'");
+  return 0;
 }
 
 std::vector<StringRef> lld::args::getStrings(opt::InputArgList &Args, int Id) {
   std::vector<StringRef> V;
   for (auto *Arg : Args.filtered(Id))
     V.push_back(Arg->getValue());
   return V;
 }

lld/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 parseInt(StringRef S, opt::Arg *Arg) {
-  int V = 0;
-  if (!to_integer(S, V, 10))
-    error(Arg->getSpelling() + "=" + Arg->getValue() +
-          ": number expected, but got '" + S + "'");
-  return V;
+static std::pair<StringRef, StringRef> getOldNewOptions(opt::InputArgList &Args,
+                                                        unsigned Id) {
+  auto *Arg = Args.getLastArg(Id);
+  if (!Arg)
+    return {"", ""};
+
+  StringRef S = Arg->getValue();
+  std::pair<StringRef, StringRef> Ret = S.split(';');
+  if (Ret.second.empty())
+    error(Arg->getSpelling() + " expects 'old;new' format, but got " + S);
+  return Ret;
 }
 
 // Parse the symbol ordering file and warn for any duplicate entries.
 static std::vector<StringRef> getSymbolOrderingFile(MemoryBufferRef MB) {
   SetVector<StringRef> Names;
   for (StringRef S : args::getLines(MB))
     if (!Names.insert(S) && Config->WarnSymbolOrdering)
       warn(MB.getBufferIdentifier() + ": duplicate ordered symbol: " + S);
void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->IgnoreFunctionAddressEquality =
       Args.hasArg(OPT_ignore_function_address_equality);
   Config->Init = Args.getLastArgValue(OPT_init, "_init");
   Config->LTOAAPipeline = Args.getLastArgValue(OPT_lto_aa_pipeline);
   Config->LTODebugPassManager = Args.hasArg(OPT_lto_debug_pass_manager);
   Config->LTONewPassManager = Args.hasArg(OPT_lto_new_pass_manager);
   Config->LTONewPmPasses = Args.getLastArgValue(OPT_lto_newpm_passes);
   Config->LTOO = args::getInteger(Args, OPT_lto_O, 2);
+  Config->LTOObjPath = Args.getLastArgValue(OPT_plugin_opt_obj_path_eq);
   Config->LTOPartitions = args::getInteger(Args, OPT_lto_partitions, 1);
   Config->LTOSampleProfile = Args.getLastArgValue(OPT_lto_sample_profile);
   Config->MapFile = Args.getLastArgValue(OPT_Map);
   Config->MergeArmExidx =
       Args.hasFlag(OPT_merge_exidx_entries, OPT_no_merge_exidx_entries, true);
   Config->NoinhibitExec = Args.hasArg(OPT_noinhibit_exec);
   Config->Nostdlib = Args.hasArg(OPT_nostdlib);
   Config->OFormatBinary = isOutputFormatBinary(Args);
void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->Strip = getStrip(Args);
   Config->Sysroot = Args.getLastArgValue(OPT_sysroot);
   Config->Target1Rel = Args.hasFlag(OPT_target1_rel, OPT_target1_abs, false);
   Config->Target2 = getTarget2(Args);
   Config->ThinLTOCacheDir = Args.getLastArgValue(OPT_thinlto_cache_dir);
   Config->ThinLTOCachePolicy = CHECK(
       parseCachePruningPolicy(Args.getLastArgValue(OPT_thinlto_cache_policy)),
       "--thinlto-cache-policy: invalid cache policy");
+  Config->ThinLTOEmitImportsFiles =
+      Args.hasArg(OPT_plugin_opt_thinlto_emit_imports_files);
+  Config->ThinLTOIndexOnly = Args.hasArg(OPT_plugin_opt_thinlto_index_only) ||
+                             Args.hasArg(OPT_plugin_opt_thinlto_index_only_eq);
+  Config->ThinLTOIndexOnlyArg =
+      Args.getLastArgValue(OPT_plugin_opt_thinlto_index_only_eq);
   Config->ThinLTOJobs = args::getInteger(Args, OPT_thinlto_jobs, -1u);
   ThreadsEnabled = Args.hasFlag(OPT_threads, OPT_no_threads, true);
   Config->Trace = Args.hasArg(OPT_trace);
   Config->Undefined = args::getStrings(Args, OPT_undefined);
   Config->UndefinedVersion =
       Args.hasFlag(OPT_undefined_version, OPT_no_undefined_version, true);
   Config->UnresolvedSymbols = getUnresolvedSymbolPolicy(Args);
   Config->WarnBackrefs =
void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->ZRelro = getZFlag(Args, "relro", "norelro", true);
   Config->ZRetpolineplt = hasZOption(Args, "retpolineplt");
   Config->ZRodynamic = hasZOption(Args, "rodynamic");
   Config->ZStackSize = args::getZOptionValue(Args, OPT_z, "stack-size", 0);
   Config->ZText = getZFlag(Args, "text", "notext", true);
   Config->ZWxneeded = hasZOption(Args, "wxneeded");
 
   // Parse LTO plugin-related options for compatibility with gold.
-  for (auto *Arg : Args.filtered(OPT_plugin_opt)) {
-    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 = parseInt(S.substr(1), Arg);
-    } else if (S.startswith("lto-partitions=")) {
-      Config->LTOPartitions = parseInt(S.substr(15), Arg);
-    } else if (S.startswith("jobs=")) {
-      Config->ThinLTOJobs = parseInt(S.substr(5), Arg);
-    } else if (S.startswith("mcpu=")) {
-      parseClangOption(Saver.save("-" + S), Arg->getSpelling());
-    } else if (S == "new-pass-manager") {
-      Config->LTONewPassManager = true;
-    } else if (S == "debug-pass-manager") {
-      Config->LTODebugPassManager = true;
-    } else if (S.startswith("sample-profile=")) {
-      Config->LTOSampleProfile = S.substr(15);
-    } else if (S.startswith("obj-path=")) {
-      Config->LTOObjPath = S.substr(9);
-    } else if (S == "thinlto-index-only") {
-      Config->ThinLTOIndexOnly = true;
-    } else if (S.startswith("thinlto-index-only=")) {
-      Config->ThinLTOIndexOnly = true;
-      Config->ThinLTOIndexOnlyArg = S.substr(19);
-    } else if (S == "thinlto-emit-imports-files") {
-      Config->ThinLTOEmitImportsFiles = true;
-    } else if (S.startswith("thinlto-prefix-replace=")) {
-      std::tie(Config->ThinLTOPrefixReplace.first,
-               Config->ThinLTOPrefixReplace.second) = S.substr(23).split(';');
-      if (Config->ThinLTOPrefixReplace.second.empty())
-        error("thinlto-prefix-replace expects 'old;new' format, but got " +
-              S.substr(23));
-    } else if (S.startswith("thinlto-object-suffix-replace=")) {
-      std::tie(Config->ThinLTOObjectSuffixReplace.first,
-               Config->ThinLTOObjectSuffixReplace.second) =
-          S.substr(30).split(';');
-      if (Config->ThinLTOObjectSuffixReplace.second.empty())
-        error(
-            "thinlto-object-suffix-replace expects 'old;new' format, but got " +
-            S.substr(30));
-    } else if (!S.startswith("/") && !S.startswith("-fresolution=") &&
-               !S.startswith("-pass-through=") && !S.startswith("thinlto")) {
-      parseClangOption(S, Arg->getSpelling());
-    }
-  }
+           Config->ThinLTOPrefixReplace.second) =
+      getOldNewOptions(Args, OPT_plugin_opt_thinlto_prefix_replace_eq);
+
+      getOldNewOptions(Args, OPT_plugin_opt_thinlto_object_suffix_replace_eq);
+
+  if (auto *Arg = Args.getLastArg(OPT_plugin_opt_mcpu_eq))
+    parseClangOption(Saver.save("-mcpu=" + StringRef(Arg->getValue())),
+                     Arg->getSpelling());
+
+  for (auto *Arg : Args.filtered(OPT_plugin_opt))
+    parseClangOption(Arg->getValue(), Arg->getSpelling());
 
   // Parse -mllvm options.
   for (auto *Arg : Args.filtered(OPT_mllvm))
     parseClangOption(Arg->getValue(), Arg->getSpelling());
 
   if (Config->LTOO > 3)
     error("invalid optimization level for LTO: " + Twine(Config->LTOO));
   if (Config->LTOPartitions == 0)

lld/ELF/Options.td

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">;
 defm thinlto_cache_policy: Eq<"thinlto-cache-policy">,
   HelpText<"Pruning policy for the ThinLTO cache">;
 def thinlto_jobs: J<"thinlto-jobs=">, HelpText<"Number of ThinLTO jobs">;
 
+def plugin_opt_O: J<"plugin-opt=O">, Alias<lto_O>;
+def plugin_opt_debug_pass_manager: F<"plugin-opt=debug-pass-manager">, Alias<lto_debug_pass_manager>;
+def plugin_opt_disable_verify: F<"plugin-opt=disable-verify">, Alias<disable_verify>;
+def plugin_opt_jobs_eq: J<"plugin-opt=jobs=">, Alias<thinlto_jobs>;
+def plugin_opt_lto_partitions_eq: J<"plugin-opt=lto-partitions=">, Alias<lto_partitions>;
+def plugin_opt_mcpu_eq: J<"plugin-opt=mcpu=">;
+def plugin_opt_new_pass_manager: F<"plugin-opt=new-pass-manager">, Alias<lto_new_pass_manager>;
+def plugin_opt_obj_path_eq: J<"plugin-opt=obj-path=">;
+def plugin_opt_sample_profile_eq: J<"plugin-opt=sample-profile=">, Alias<lto_sample_profile>;
+def plugin_opt_save_temps: F<"plugin-opt=save-temps">, Alias<save_temps>;
+def plugin_opt_thinlto_emit_imports_files: F<"plugin-opt=thinlto-emit-imports-files">;
+def plugin_opt_thinlto_index_only: F<"plugin-opt=thinlto-index-only">;
+def plugin_opt_thinlto_index_only_eq: J<"plugin-opt=thinlto-index-only=">;
+def plugin_opt_thinlto_object_suffix_replace_eq: J<"plugin-opt=thinlto-object-suffix-replace=">;
+def plugin_opt_thinlto_prefix_replace_eq: J<"plugin-opt=thinlto-prefix-replace=">;
+
 // 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.
 defm plugin: Eq<"plugin">;
 
+def plugin_opt_fresolution_eq: J<"plugin-opt=-fresolution=">;
+def plugin_opt_pass_through_eq: J<"plugin-opt=-pass-through=">;
+def plugin_opt_thinlto: J<"plugin-opt=thinlto">;
+def plugin_opt_slash: J<"plugin-opt=/">;
+
 // Options listed below are silently ignored for now for compatibility.
 def allow_shlib_undefined: F<"allow-shlib-undefined">;
 def detect_odr_violations: F<"detect-odr-violations">;
 def g: Flag<["-"], "g">;
 def long_plt: F<"long-plt">;
 def no_add_needed: F<"no-add-needed">;
 def no_allow_shlib_undefined: F<"no-allow-shlib-undefined">;
 def no_copy_dt_needed_entries: F<"no-copy-dt-needed-entries">;

lld/test/ELF/lto/thinlto-object-suffix-replace.ll

 ; RUN: -shared %t1.thinlink.bc -o %t3
 ; RUN: diff %t1.o.thinlto.bc.orig %t1.o.thinlto.bc
 
 ; Ensure lld generates error if object suffix replace option does not have 'old;new' format
 ; RUN: rm -f %t1.o.thinlto.bc
 ; RUN: not ld.lld -m elf_x86_64 --plugin-opt=thinlto-index-only \
 ; RUN: --plugin-opt=thinlto-object-suffix-replace="abc:def" -shared %t1.thinlink.bc \
 ; RUN: -o %t3 2>&1 | FileCheck %s --check-prefix=ERR1
-; ERR1: thinlto-object-suffix-replace expects 'old;new' format, but got abc:def
+; ERR1: --plugin-opt=thinlto-object-suffix-replace= expects 'old;new' format, but got abc:def
 
 ; Ensure lld generates error if old suffix doesn't exist in file name
 ; RUN: rm -f %t1.o
 ; RUN: not ld.lld -m elf_x86_64 --plugin-opt=thinlto-index-only \
 ; RUN: --plugin-opt=thinlto-object-suffix-replace=".abc;.o" -shared %t1.thinlink.bc \
 ; RUN: -o %t3 2>&1 | FileCheck %s --check-prefix=ERR2
 ; ERR2: error: -thinlto-object-suffix-replace=.abc;.o was given, but {{.*}} does not end with the suffix
 

lld/test/ELF/lto/thinlto-prefix-replace.ll

 ; REQUIRES: x86
 ; Check that changing the output path via thinlto-prefix-replace works
 ; RUN: mkdir -p %t/oldpath
 ; RUN: opt -module-summary %s -o %t/oldpath/thinlto_prefix_replace.o
 
 ; Ensure that there is no existing file at the new path, so we properly
 ; test the creation of the new file there.
 ; RUN: rm -f %t/newpath/thinlto_prefix_replace.o.thinlto.bc
 ; RUN: ld.lld --plugin-opt=thinlto-index-only --plugin-opt=thinlto-prefix-replace="%t/oldpath/;%t/newpath/" -shared %t/oldpath/thinlto_prefix_replace.o -o %t/thinlto_prefix_replace
 ; RUN: ls %t/newpath/thinlto_prefix_replace.o.thinlto.bc
 
 ; Ensure that lld generates error if prefix replace option does not have 'old;new' format
 ; RUN: rm -f %t/newpath/thinlto_prefix_replace.o.thinlto.bc
 ; RUN: not ld.lld --plugin-opt=thinlto-index-only --plugin-opt=thinlto-prefix-replace=abc:def -shared %t/oldpath/thinlto_prefix_replace.o -o %t/thinlto_prefix_replace 2>&1 | FileCheck %s --check-prefix=ERR
-; ERR: thinlto-prefix-replace expects 'old;new' format, but got abc:def
+; ERR: --plugin-opt=thinlto-prefix-replace= expects 'old;new' format, but got abc:def
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @f() {
 entry:
   ret void
 }