lld: handling of -flavor / -core command line switches

[iid_iunknown]

Summary

Renato, Shankar,

Would some one of you have time to kindly review my patch please?
I am new to LLVM and need experts help to get my patches through to the trunk.

This patch fixes
20977 - UniversalDriver incorrectly removes the -flavor / -core switches from its command line
20975 - getFlavor: -flavor/-core switches should have higher priority than linker name

Thank you!

[shankarke]

LGTM.

Please clang-format and add a test.

[kledzik]

The original design was that -flavor had to be the first arg (so incrementing argv worked). But now the use of ParseArgs() means that -flavor is being searched for everywhere in the arg list. That is a mismatch.

[iid_iunknown]

In D5384#8, @kledzik wrote:

The original design was that -flavor had to be the first arg (so incrementing argv worked). But now the use of ParseArgs() means that -flavor is being searched for everywhere in the arg list. That is a mismatch.

This is not always under direct user's control, unfortunately. I encountered this problem trying to build Boost with clang/lld using Boost.Build. I am passing arguments to the linker by using the -Xlinker option. -flavor doesn't go first in the lld's command line in my case.

Even if it was the first argument, the algorithm would still malfunction. Assume we have the following command: /some/path/lld -flavor gnu -llib... When lld meets -flavor it does this to skip the switch:

argv += 2;
argc -= 2;

Looks ok, but argv[0] is actually "/some/path/lld", not "-flavor"! After this operation we end up with the command line "gnu -llib...". This malformed line is then passed to GnuLdDriver. The reason GnuLdDriver does not fail on it is mere luck. GnuLdDriver expects argv[0] ("gnu" in our case) to be the program name and skips it :-)

[ruiu]

Nick's concern makes sense. We should allow -flavor only at the first
argument (i.e. argv[1]) because -flavor can be a valid command line
argument of the other linker.

You should process raw argv and argc first for -flavor, and then pass the
rest to parseArgs.

[iid_iunknown]

In D5384#11, @ruiu wrote:

Nick's concern makes sense. We should allow -flavor only at the first
argument (i.e. argv[1]) because -flavor can be a valid command line
argument of the other linker.

You should process raw argv and argc first for -flavor, and then pass the
rest to parseArgs.

Thank you for your remark. There is still a problem with this.

1. I am not sure it can always be controlled by user because the user might not run lld directly. E.g., lld might be executed by Boost.Build. I will play with Boost.Build more tomorrow to check again if it's possible to put -flavor first.
2. Passing "the rest" is still incorrect. E.g., GnuLdDriver addresses argv[0] and expects the program name to be there. Mere arguments skipping by doing += on argv to get the rest won't work.

[kledzik]

1. Don't most build system invoke the compiler to invoke the linker? How are you overriding the linker? Can you redirect to a script that invokes lld with -flavor and $@
2. Yes, shifting argv is not right. We need the -flavor and its arg removed, so argv[0] is still the command name.

[iid_iunknown]

In D5384#14, @kledzik wrote:

1. Don't most build system invoke the compiler to invoke the linker? How are you overriding the linker? Can you redirect to a script that invokes lld with -flavor and $@

Thank you for your attention to this.
I am overriding the linker by symlinking it to the folder where my AArch64 binutils linker is placed (I am crosscompiling).
Yes, clang++ invokes the linker in my case. I tried to pass -flavor using -Xlinker and -Wl options. -flavor never comes first in the linker's command line.

I think it's possible to write a script as you are suggesting, but I personally don't like this idea much. Lld is claimed to be compatible with existing linker options. In fact, I am not able to merely substitute my linker with lld and add -favor for it without the need to write a script to move the options around.

[ruiu]

I see your need. My concern of possible conflict of "-flavor" option with the existing linker is hypothetical, so I think I'm OK to allow -flavor to appear in any place in the command line.

However the current evaluation order doesn't seem correct. I think it make a decision on the flavor in this order.

1. If argv[0] is "ld", the default linker driver is chosen
2. If argv[0] can be interpreted as a triple, the triple is chosen
3. otherwise, look for -flavor (and probably argv[0] is "lld" in this case)

Currently 3 takes precedence over 2, which I feel wrong.

[iid_iunknown]

In D5384#16, @ruiu wrote:

However the current evaluation order doesn't seem correct. I think it make a decision on the flavor in this order.

1. If argv[0] is "ld", the default linker driver is chosen
2. If argv[0] can be interpreted as a triple, the triple is chosen
3. otherwise, look for -flavor (and probably argv[0] is "lld" in this case)

Currently 3 takes precedence over 2, which I feel wrong.

My patch makes the order even more different from that: 3 -> 1 -> 2.

Let me explain my thoughts. If the user explicitly expresses his intentions by providing some command line arguments, it is quite confusing if these explicit settings get silently overridden by lld, making the linker behave differently from what the user wants. Dependence of this logic on the program name, which might be symlinked or even renamed, makes this even more confusing. That's why -flavor's being of the highest priority could make sense.

Please feel free to correct me if this is not a good idea. I'll adjust the code according to your recommendations then.

Thank you.

[ruiu]

My thought on the order that I presented is that: argv[0] is a way to make
a deliberate choice on the flavor, and if it's the same name as a known
linker, it should behave as a drop-in replacement of the linker. Because
the simulating linker is unlikely to have -flavor option, it felt wrong to
recognize that.

However, I see the benefit of accepting -flavor whatever argv[0] is.

So I'm OK with your patch if there's no objection from other LLD engineers.

[iid_iunknown]

Corrections according to the reviewers' remarks.

[iid_iunknown]

In D5384#18, @ruiu wrote:

So I'm OK with your patch if there's no objection from other LLD engineers.

Thank you Rui.
I've clang-formatted the patch and added a simple test for it.

I am using --help in the test to keep it as simple as possible.
In practice, the flavor's need to be the 1st argument may result in more unevident and confusing errors. E.g., when building Boost lld was receiving the command line similar to "ld -v -z relro -flavor gnu...". Without the patch lld output was "lld: cannot find file relro", as UniversalDriver was skipping the "-v -z" part.

[ruiu]

lib/Driver/UniversalDriver.cpp

126

The function name and the arguments aren't aligned with other code in this file, so they look a bit heavier than the others. I'd write

removeArg(llvm::opt::Arg *arg, int &argc, const char **&argv)

129

numberOfItemsToRemove -> numRemove or something

132

IMO the comments in this function don't help readers that much. I'd write a comment about what this function is supposed to do ("Removes argument from argv and update argc.") and remove other comments.

test/Driver/flavor-option.test

9

add newline at end of file

[Bigcheese]

I'm fine with your change. But I'm confused as to why you're not just making a symlink to lld named ld and using that, without passing -flavor. The main lld interface is only intended for testing.

[iid_iunknown]

In D5384#23, @Bigcheese wrote:

I'm fine with your change. But I'm confused as to why you're not just making a symlink to lld named ld and using that, without passing -flavor. The main lld interface is only intended for testing.

Thank you.
Yes, I totally agree with you. This is exactly the way I'm using it.

When I found this problem I didn't know yet that -flavor is optional if lld is symlinked/renamed to ld. Nevertheless, still nothing prevents a user from providing -flavor directly. There is the -core option that has the same problem. Moreover, while symlinking is a good practice of using lld, this is not something that is forced to be done. I spent some time figuring out why lld was showing the errors I mentioned previously. Hope the patch saves someone else's time in the future.

[iid_iunknown]

Corrections according to the reviewers' remarks.

[ruiu]

LGTM

I'll commit this patch soon.

[iid_iunknown]

Committed as r218321.

lib/Driver/UniversalDriver.cpp

 class UniversalDriverOptTable : public llvm::opt::OptTable {
 public:
   UniversalDriverOptTable()
       : OptTable(infoTable, llvm::array_lengthof(infoTable)) {}
 };
 
 enum class Flavor {
   invalid,
-  gnu_ld,       // -flavor gnu
-  win_link,     // -flavor link
-  darwin_ld,    // -flavor darwin
-  core          // -flavor core OR -core
+  gnu_ld,    // -flavor gnu
+  win_link,  // -flavor link
+  darwin_ld, // -flavor darwin
+  core       // -flavor core OR -core
 };
 
 struct ProgramNameParts {
   StringRef _target;
   StringRef _flavor;
 };
 
 } // anonymous namespace
static ProgramNameParts parseProgramName(StringRef programName) {
   using std::end;
 
   // Erase any lld components.
   components.erase(std::remove(components.begin(), components.end(), "lld"),
                    components.end());
 
   // Find the flavor component.
   auto flIter = std::find_if(components.begin(), components.end(),
-                             [](StringRef str)->bool {
+                             [](StringRef str) -> bool {
     return strToFlavor(str) != Flavor::invalid;
   });
 
   if (flIter != components.end()) {
     ret._flavor = *flIter;
     components.erase(flIter);
   }
 
   // Any remaining component must be the target.
   if (components.size() == 1)
     ret._target = components[0];
 
   return ret;
 }
 
+// Removes the argument from argv along with its value, if exists, and updates
+// argc.
+static void removeArg(llvm::opt::Arg *arg, int &argc, const char **&argv) {
+  unsigned int numToRemove = arg->getNumValues() + 1;
+  unsigned int argIndex = arg->getIndex() + 1;
+
+  std::rotate(&argv[argIndex], &argv[argIndex + numToRemove], argv + argc);
+
+  argc -= numToRemove;
+}
+
 static Flavor getFlavor(int &argc, const char **&argv,
                         std::unique_ptr<llvm::opt::InputArgList> &parsedArgs) {
+  if (llvm::opt::Arg *argCore = parsedArgs->getLastArg(OPT_core)) {
+    removeArg(argCore, argc, argv);
+    return Flavor::core;
+  }
+  if (llvm::opt::Arg *argFlavor = parsedArgs->getLastArg(OPT_flavor)) {
+    removeArg(argFlavor, argc, argv);
+    return strToFlavor(argFlavor->getValue());
+  }
+
 #if LLVM_ON_UNIX
   if (llvm::sys::path::filename(argv[0]).equals("ld")) {
 #if __APPLE__
     // On a Darwin systems, if linker binary is named "ld", use Darwin driver.
     return Flavor::darwin_ld;
 #endif
     // On a ELF based systems, if linker binary is named "ld", use gnu driver.
     return Flavor::gnu_ld;
   }
 #endif
-  if (parsedArgs->getLastArg(OPT_core)) {
-    argc--;
-    argv++;
-    return Flavor::core;
-  }
-  if (llvm::opt::Arg *argFlavor = parsedArgs->getLastArg(OPT_flavor)) {
-    argc -= 2;
-    argv += 2;
-    return strToFlavor(argFlavor->getValue());
-  }
+
   StringRef name = llvm::sys::path::stem(argv[0]);
   return strToFlavor(parseProgramName(name)._flavor);
 }
 
 namespace lld {
 
 bool UniversalDriver::link(int argc, const char *argv[],
                            raw_ostream &diagnostics) {
if (missingCount) {
     diagnostics << "error: missing arg value for '"
                 << parsedArgs->getArgString(missingIndex) << "' expected "
                 << missingCount << " argument(s).\n";
     return false;
   }
 
   // Handle --help
   if (parsedArgs->getLastArg(OPT_help)) {
-    table.PrintHelp(llvm::outs(), argv[0], "LLVM Linker", false);
+    table.PrintHelp(llvm::outs(), programName.data(), "LLVM Linker", false);
     return true;
   }
 
   Flavor flavor = getFlavor(argc, argv, parsedArgs);
   std::vector<const char *> args(argv, argv + argc);
 
   // Switch to appropriate driver.
   switch (flavor) {

test/Driver/flavor-option.test

+# a) the -flavor option is position independent and does not need to be the 1st
+#    argument in the command line (bug 20975);
+# b) UniversalDriver correctly removes -flavor along with its value and the
+#    underlying linker does not get a corrupted command line (bug 20977).
+RUN: lld --help -flavor gnu | FileCheck %s
+
+CHECK: --noinhibit-exec
+CHECK: --output-filetype