Driver: support detecting driver mode when clang has a version suffix without dash (PR21094)

[hans]

Summary

Clang would previously not get into C++ mode when invoked as 'clang++3.6' (though clang++-3.6 would work).

I found the previous loop logic in this function confusing; hopefully this makes it clearer.

[dblaikie]

Just some thoughts - mostly quite optional.

test/Driver/parse-progname.c

1 ↗

(On Diff #15045)

Yay tests!

Is "REQUIRES: shell" sufficient to get ln? I would wonder if we'd get a shell on Windows (mingw, perhaps) where symlinks weren't possible - but I guess maybe mingw fakes it somehow.

tools/driver/driver.cpp

209 ↗

(On Diff #15045)

possible cleanup: could remove the duplicated "--driver-mode=" from these strings & just build it into the use of these strings instead.

222 ↗

(On Diff #15045)

The array itself could be moved inside this function if it returned a const DriverSuffix*, which seems like a nice API for it. (null if not found, presumably)

223 ↗

(On Diff #15045)

I still rather like my std::find_if (& it'd be simpler once the struct had a name), but I get that it's not everyone's cup of tea.

239 ↗

(On Diff #15045)

Prefer copy/move initialization over direct initialization (makes it clear to the reader that no explicit conversions are happening here)?

245 ↗

(On Diff #15045)

Prefer copy/move initialization over direct initialization (makes it clear to the reader that no explicit conversions are happening here)?

249 ↗

(On Diff #15045)

this seems like a bit of a special case (& rather repetitious) - could we not just have a single call to FindDriverSuffix search for an arbitrary substring rather than a suffix?

What if someone has clang++-my-branch ? (perhaps not very common, I suppose - but still, a substring search seems simpler and more accepting - would it go wrong somewhere?)

284 ↗

(On Diff #15045)

Hardcoding the size of the array (while obvious in the previous line) isn't ideal - the begin/end formulation I committed or the prior array_lengthof use, seem preferable.

[hans]

Thanks for the review!

test/Driver/parse-progname.c

1 ↗

(On Diff #15045)

I hope it's enough. I was inspired by some other test that required shell and seems to successfully copy stuff around and use sed.

In an msys shell, there is ln for example, though I don't know if it is faking it or uses some kind of magic.

tools/driver/driver.cpp

209 ↗

(On Diff #15045)

I'd rather skip on this, since it will require allocating storage for the string concatenation somewhere, so making this code simpler will make the code that adds the flag to the arguments less straight-forward.

222 ↗

(On Diff #15045)

Yes, that's much nicer. Done.

223 ↗

(On Diff #15045)

I think my loop is easier to read, but I'm pretty old-fashioned :)

239 ↗

(On Diff #15045)

Done.

245 ↗

(On Diff #15045)

Done.

249 ↗

(On Diff #15045)

I'm hesitant to change the existing suffix-matching logic too much. clang++-my-branch won't work, but it didn't work before my patch either.

Also it wouldn't be completely straight-forward: cl is a substring of clang, so we'd have to be somewhat smart, and with a target prefix mixed in... hmm.

284 ↗

(On Diff #15045)

Yes, the std::begin and end were much nicer. Done.

[hans]

Addressing comments.

[dblaikie]

tools/driver/driver.cpp

209 ↗

(On Diff #15045)

Oh, fair enough - that makes sense. Thanks for the explanation.

249 ↗

(On Diff #15045)

OK then - but at this point I'm a bit too confused by the logic here (test if it's a suffix, if it isn't, strip off any digits and dots (a version suffix), try to find the suffix again, then strip off any -foo component and try again...

Oh, I see one problem then, maybe... (next comment)

250 ↗

(On Diff #15045)

Hmm, pity this is essentially std::string's find_last_not_of, except that you can't readily pass the set of all digits and '.'. I could imagine some other STL algorithms that I might find more readable, but probably only about as much as the std::find_if discussed previously.

251 ↗

(On Diff #15045)

the extra parens around the isDigit call are a bit confusing/misleading (I saw the end ')' and thought it was maybe grouped with the empty() call above for some reason)

254 ↗

(On Diff #15045)

Should this line be inside the prior if (but after/outside the loop)?

[hans]

tools/driver/driver.cpp

251 ↗

(On Diff #15045)

The extra parenthesis are around the || expression.

Anyway, I've rewritten this with less parenthesis, and fitting on one line.

254 ↗

(On Diff #15045)

D'oh, yes it should. Done.

[hans]

Address comments.

[dblaikie]

Looks good

tools/driver/driver.cpp

251 ↗

(On Diff #15045)

Oh, right - I just can't count. Sorry about that.

The new version isn't necessarily 'portable' to interesting digit characters in other languages? Maybe? Dunno if that's important.

If you're going to write it that way, I'll at least optionally suggest that you could use StringRef's find_last_not_of to do that search.

[dblaikie]

tools/driver/driver.cpp

258 ↗

(On Diff #15056)

Might help if these comments were maybe illustrated with an example:

// Strip trailing version: clang-3.4 -> clang

// Strip trailing -component: clang-foo -> clang

[hans]

tools/driver/driver.cpp

258 ↗

(On Diff #15056)

Done.

251 ↗

(On Diff #15045)

I found that StringRef has an rtrim(); I think that's what we want here.

[hans]

Closed by commit rL220052 (authored by @hans).

cfe/trunk/test/Driver/parse-progname.c

+// REQUIRES: shell, arm-registered-target
+
+
+
+// RUN: ln -fs %clang %T/clang++
+// RUN: ln -fs %clang %T/clang++3.5.0
+// RUN: ln -fs %clang %T/clang++-3.5
+// RUN: ln -fs %clang %T/clang++-tot
+// RUN: ln -fs %clang %T/clang-c++
+// RUN: ln -fs %clang %T/clang-g++
+// RUN: ln -fs %clang %T/c++
+// RUN: ln -fs %clang %T/foo-clang++
+// RUN: ln -fs %clang %T/foo-clang++-3.5
+// RUN: ln -fs %clang %T/foo-clang++3.5
+// RUN: %T/clang++          -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// RUN: %T/clang++3.5.0     -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// RUN: %T/clang++-3.5      -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// RUN: %T/clang++-tot      -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// RUN: %T/clang-c++        -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// RUN: %T/clang-g++        -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// RUN: %T/c++              -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// RUN: %T/foo-clang++      -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// RUN: %T/foo-clang++-3.5  -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// RUN: %T/foo-clang++3.5   -### %s 2>&1 | FileCheck -check-prefix=CXXMODE %s
+// CXXMODE: "-x" "c++"
+
+
+// RUN: ln -fs %clang %T/clang-cl
+// RUN: ln -fs %clang %T/cl
+// RUN: ln -fs %clang %T/cl.exe
+// RUN: ln -fs %clang %T/clang-cl3.5
+// RUN: ln -fs %clang %T/clang-cl-3.5
+// RUN: %T/clang-cl         -### %s 2>&1 | FileCheck -check-prefix=CLMODE %s
+// RUN: %T/cl               -### %s 2>&1 | FileCheck -check-prefix=CLMODE %s
+// RUN: %T/cl.exe           -### %s 2>&1 | FileCheck -check-prefix=CLMODE %s
+// RUN: %T/clang-cl3.5      -### %s 2>&1 | FileCheck -check-prefix=CLMODE %s
+// RUN: %T/clang-cl-3.5     -### %s 2>&1 | FileCheck -check-prefix=CLMODE %s
+// CLMODE: "-fdiagnostics-format" "msvc"
+
+
+// RUN: ln -fs %clang %T/clang-cpp
+// RUN: ln -fs %clang %T/cpp
+// RUN: %T/clang-cpp        -### %s 2>&1 | FileCheck -check-prefix=CPPMODE %s
+// RUN: %T/cpp              -### %s 2>&1 | FileCheck -check-prefix=CPPMODE %s
+// CPPMODE: "-E"
+
+
+// RUN: ln -fs %clang %T/cl-clang
+// RUN: %T/cl-clang        -### %s 2>&1 | FileCheck -check-prefix=CMODE %s
+// CMODE: "-x" "c"
+// CMODE-NOT: "-fdiagnostics-format" "msvc"
+
+
+// RUN: ln -fs %clang %T/arm-linux-gnueabi-clang
+// RUN: %T/arm-linux-gnueabi-clang -### %s 2>&1 | FileCheck -check-prefix=TARGET %s
+// TARGET: Target: arm--linux-gnueabi

cfe/trunk/tools/driver/driver.cpp

static void ApplyQAOverride(SmallVectorImpl<const char*> &Args,
   }
 }
 
 extern int cc1_main(ArrayRef<const char *> Argv, const char *Argv0,
                     void *MainAddr);
 extern int cc1as_main(ArrayRef<const char *> Argv, const char *Argv0,
                       void *MainAddr);
 
-static void ParseProgName(SmallVectorImpl<const char *> &ArgVector,
-                          std::set<std::string> &SavedStrings,
-                          Driver &TheDriver)
-{
-  // Try to infer frontend type and default target from the program name.
-
-  // suffixes[] contains the list of known driver suffixes.
-  // Suffixes are compared against the program name in order.
-  // If there is a match, the frontend type is updated as necessary (CPP/C++).
-  // If there is no match, a second round is done after stripping the last
-  // hyphen and everything following it. This allows using something like
-  // "clang++-2.9".
-
-  // If there is a match in either the first or second round,
-  // the function tries to identify a target as prefix. E.g.
-  // "x86_64-linux-clang" as interpreted as suffix "clang" with
-  // target prefix "x86_64-linux". If such a target prefix is found,
-  // is gets added via -target as implicit first argument.
-  static const struct {
-    const char *Suffix;
-    const char *ModeFlag;
-  } suffixes [] = {
-    { "clang",     nullptr },
-    { "clang++",   "--driver-mode=g++" },
-    { "clang-c++", "--driver-mode=g++" },
-    { "clang-cc",  nullptr },
-    { "clang-cpp", "--driver-mode=cpp" },
-    { "clang-g++", "--driver-mode=g++" },
-    { "clang-gcc", nullptr },
-    { "clang-cl",  "--driver-mode=cl"  },
-    { "cc",        nullptr },
-    { "cpp",       "--driver-mode=cpp" },
-    { "cl" ,       "--driver-mode=cl"  },
-    { "++",        "--driver-mode=g++" },
+struct DriverSuffix {
+};
+
+static const DriverSuffix *FindDriverSuffix(StringRef ProgName) {
+  // A list of known driver suffixes. Suffixes are compared against the
+  // program name in order. If there is a match, the frontend type if updated as
+  // necessary by applying the ModeFlag.
+  static const DriverSuffix DriverSuffixes[] = {
+      {"clang", nullptr},
+      {"clang++", "--driver-mode=g++"},
+      {"clang-c++", "--driver-mode=g++"},
+      {"clang-cc", nullptr},
+      {"clang-cpp", "--driver-mode=cpp"},
+      {"clang-g++", "--driver-mode=g++"},
+      {"clang-gcc", nullptr},
+      {"clang-cl", "--driver-mode=cl"},
+      {"cc", nullptr},
+      {"cpp", "--driver-mode=cpp"},
+      {"cl", "--driver-mode=cl"},
+      {"++", "--driver-mode=g++"},
   };
-  std::string ProgName(llvm::sys::path::stem(ArgVector[0]));
+
+  for (size_t i = 0; i < llvm::array_lengthof(DriverSuffixes); ++i)
+    if (ProgName.endswith(DriverSuffixes[i].Suffix))
+      return &DriverSuffixes[i];
+  return nullptr;
+}
+
+static void ParseProgName(SmallVectorImpl<const char *> &ArgVector,
+                          std::set<std::string> &SavedStrings) {
+  // Try to infer frontend type and default target from the program name by
+  // comparing it against DriverSuffixes in order.
+
+  // If there is a match, the function tries to identify a target as prefix.
+  // E.g. "x86_64-linux-clang" as interpreted as suffix "clang" with target
+  // prefix "x86_64-linux". If such a target prefix is found, is gets added via
+  // -target as implicit first argument.
+
+  std::string ProgName =llvm::sys::path::stem(ArgVector[0]);
 #ifdef LLVM_ON_WIN32
   // Transform to lowercase for case insensitive file systems.
-  std::transform(ProgName.begin(), ProgName.end(), ProgName.begin(),
-                 toLowercase);
+  ProgName = StringRef(ProgName).lower();
 #endif
-  StringRef ProgNameRef(ProgName);
-  StringRef Prefix;
 
-  for (int Components = 2; Components; --Components) {
-    auto I = std::find_if(std::begin(suffixes), std::end(suffixes),
-                          [&](decltype(suffixes[0]) &suffix) {
-      return ProgNameRef.endswith(suffix.Suffix);
-    });
+  StringRef ProgNameRef = ProgName;
+  const DriverSuffix *DS = FindDriverSuffix(ProgNameRef);
 
-    if (I != std::end(suffixes)) {
-      if (I->ModeFlag) {
-        auto it = ArgVector.begin();
-        if (it != ArgVector.end())
-          ++it;
-        ArgVector.insert(it, I->ModeFlag);
-      }
-      StringRef::size_type LastComponent = ProgNameRef.rfind('-',
-        ProgNameRef.size() - strlen(I->Suffix));
-      if (LastComponent != StringRef::npos)
-        Prefix = ProgNameRef.slice(0, LastComponent);
-      break;
+  if (!DS) {
+    // Try again after stripping any trailing version number:
+    // clang++3.5 -> clang++
+    ProgNameRef = ProgNameRef.rtrim("0123456789.");
+    DS = FindDriverSuffix(ProgNameRef);
+  }
+
+  if (!DS) {
+    // Try again after stripping trailing -component.
+    // clang++-tot -> clang++
+    ProgNameRef = ProgNameRef.slice(0, ProgNameRef.rfind('-'));
+    DS = FindDriverSuffix(ProgNameRef);
+  }
+
+  if (DS) {
+    if (const char *Flag = DS->ModeFlag) {
+      // Add Flag to the arguments.
+      ArgVector.insert(it, Flag);
     }
 
-    StringRef::size_type LastComponent = ProgNameRef.rfind('-');
+    StringRef::size_type LastComponent = ProgNameRef.rfind(
+        '-', ProgNameRef.size() - strlen(DS->Suffix));
     if (LastComponent == StringRef::npos)
-      break;
-    ProgNameRef = ProgNameRef.slice(0, LastComponent);
-  }
-
-  if (Prefix.empty())
       return;
 
+    // Infer target from the prefix.
+    StringRef Prefix = ProgNameRef.slice(0, LastComponent);
     std::string IgnoredError;
     if (llvm::TargetRegistry::lookupTarget(Prefix, IgnoredError)) {
       auto it = ArgVector.begin();
       if (it != ArgVector.end())
         ++it;
-    const char* Strings[] =
-      { GetStableCStr(SavedStrings, std::string("-target")),
-        GetStableCStr(SavedStrings, Prefix) };
-    ArgVector.insert(it, std::begin(Strings), std::end(Strings));
+      const char *arr[] = { "-target", GetStableCStr(SavedStrings, Prefix) };
+      ArgVector.insert(it, std::begin(arr), std::end(arr));
+    }
   }
 }
 
 namespace {
   class StringSetSaver : public llvm::cl::StringSaver {
   public:
     StringSetSaver(std::set<std::string> &Storage) : Storage(Storage) {}
     const char *SaveString(const char *Str) override {
int main(int argc_, const char **argv_) {
 
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagClient);
   ProcessWarningOptions(Diags, *DiagOpts, /*ReportDiags=*/false);
 
   Driver TheDriver(Path, llvm::sys::getDefaultTargetTriple(), Diags);
   SetInstallDir(argv, TheDriver);
 
   llvm::InitializeAllTargets();
-  ParseProgName(argv, SavedStrings, TheDriver);
+  ParseProgName(argv, SavedStrings);
 
   SetBackdoorDriverOutputsFromEnvVars(TheDriver);
 
   std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(argv));
   int Res = 0;
   SmallVector<std::pair<int, const Command *>, 4> FailingCommands;
   if (C.get())
     Res = TheDriver.ExecuteCompilation(*C, FailingCommands);