[clang][driver] Only warn once about invalid -stdlib value

[tbaeder]

Summary

Since ToolChain::GetCXXStdlibType() is a simple getter that might emit the "invalid library name in argument" warning, it can conceivably be called several times while initializing the build pipeline.

Before this patch, a simlpe 'clang++ -stdlib=foo ./test.cpp' would print the warning twice.

Change this and always only print the warning once. Keep the rest of the semantics of the function.

E.g, before:

$ clang++ -stdlib=foo ./test.cpp
clang-10: error: invalid library name in argument '-stdlib=foo'
clang-10: error: invalid library name in argument '-stdlib=foo'

After:

$ bin/clang++ -stdlib=foo ./test.cpp
clang-12: error: invalid library name in argument '-stdlib=foo'

[tbaeder]

Noticed similar/worse behavior for -rtlib. Expanded the patch to that and -unwindlib as well.

[jdoerfert]

I don't believe a static variable is a good idea. Also the name is not really informative. Members in ToolChain with a proper names would be nicer (IMHO).

Tests missing. Also, now you don't warn for different missing runtimes, which seems odd.

[yaxunl]

In D95915#2539483, @jdoerfert wrote:

I don't believe a static variable is a good idea. Also the name is not really informative. Members in ToolChain with a proper names would be nicer (IMHO).

Tests missing. Also, now you don't warn for different missing runtimes, which seems odd.

+1

I agree with Johannes about static variable usage. We should avoid using static variables since they are not thread safe and become hurdles when we want to parallelize the driver.

Is it possible to emit the diag in a lambda and use std::call_once with it?

[tbaeder]

In D95915#2539483, @jdoerfert wrote:

Also, now you don't warn for different missing runtimes, which seems odd.

What do you mean? Is the value returned from GetRuntimeLibType() ever going to change?

[Hahnfeld]

My proposal would be to cache the return value of the three routines in ToolChain. This has the advantage that the values get parsed only once and there is at most one warning. I don't know how this plays with parallelization efforts, but I don't think we should worry about this right now, given the current code.

[tbaeder]

In D95915#2541129, @Hahnfeld wrote:

My proposal would be to cache the return value of the three routines in ToolChain. This has the advantage that the values get parsed only once and there is at most one warning. I don't know how this plays with parallelization efforts, but I don't think we should worry about this right now, given the current code.

That's what I was looking at right now as well, since using std::call_once() already means the methods can't be const anymore anyway. Might as well just cache the value. It is still slightly ugly with the current code in that subclasses can override them and then the caching is gone.

[Hahnfeld]

In D95915#2541132, @tbaeder wrote:

That's what I was looking at right now as well, since using std::call_once() already means the methods can't be const anymore anyway. Might as well just cache the value.

You can make the caching members mutable, IIRC that's a common pattern in Clang code.

It is still slightly ugly with the current code in that subclasses can override them and then the caching is gone.

True, but not really of concern since their implementation is usually a constant value and not emitting warnings.

[tbaeder]

Here a version without the local static bools. If this is good to go, I can add some tests.

[yaxunl]

LGTM. It also avoid redundant check.

[jdoerfert]

Test still missing.

[Hahnfeld]

clang/include/clang/Driver/ToolChain.h

169–177

Maybe llvm::Optional for each type instead of having two members?

[tbaeder]

Switched to llvm::Optional and added some tests

[tbaeder]

Is this good to go now? Or still something left? @yaxunl?

[yaxunl]

LGTM. Thanks.

[tbaeder]

Looks like this broke the tests because -stdlib is not a thing on Windows? Would passing a linux -target x86_64-linux-gnu be an appropriate fix for the tests?

[dyung]

It also failed on the PS4 linux bot: http://lab.llvm.org:8014/#/builders/125/builds/125

clang/include/clang/Driver/ToolChain.h

private:
   /// The effective clang triple for the current Job.
   mutable llvm::Triple EffectiveTriple;
 
   /// Set the toolchain's effective clang triple.
   void setEffectiveTriple(llvm::Triple ET) const {
     EffectiveTriple = std::move(ET);
   }
 
+  mutable llvm::Optional<CXXStdlibType> cxxStdlibType;
+  mutable llvm::Optional<RuntimeLibType> runtimeLibType;
+  mutable llvm::Optional<UnwindLibType> unwindLibType;
+
 protected:
   MultilibSet Multilibs;
   Multilib SelectedMultilib;
 
   ToolChain(const Driver &D, const llvm::Triple &T,
             const llvm::opt::ArgList &Args);
 
   void setTripleEnvironment(llvm::Triple::EnvironmentType Env);
 
   virtual Tool *buildAssembler() const;
   virtual Tool *buildLinker() const;
   virtual Tool *buildStaticLibTool() const;
   virtual Tool *getTool(Action::ActionClass AC) const;

clang/lib/Driver/ToolChain.cpp

void ToolChain::addProfileRTLibs(const llvm::opt::ArgList &Args,
   if (!needsProfileRT(Args) && !needsGCovInstrumentation(Args))
     return;
 
   CmdArgs.push_back(getCompilerRTArgString(Args, "profile"));
 }
 
 ToolChain::RuntimeLibType ToolChain::GetRuntimeLibType(
     const ArgList &Args) const {
+  if (runtimeLibType)
+    return *runtimeLibType;
+
   const Arg* A = Args.getLastArg(options::OPT_rtlib_EQ);
   StringRef LibName = A ? A->getValue() : CLANG_DEFAULT_RTLIB;
 
   // Only use "platform" in tests to override CLANG_DEFAULT_RTLIB!
   if (LibName == "compiler-rt")
-    return ToolChain::RLT_CompilerRT;
+    runtimeLibType = ToolChain::RLT_CompilerRT;
   else if (LibName == "libgcc")
-    return ToolChain::RLT_Libgcc;
+    runtimeLibType = ToolChain::RLT_Libgcc;
   else if (LibName == "platform")
-    return GetDefaultRuntimeLibType();
-
+    runtimeLibType = GetDefaultRuntimeLibType();
+  else {
     if (A)
-    getDriver().Diag(diag::err_drv_invalid_rtlib_name) << A->getAsString(Args);
+      getDriver().Diag(diag::err_drv_invalid_rtlib_name)
+          << A->getAsString(Args);
 
-  return GetDefaultRuntimeLibType();
+    runtimeLibType = GetDefaultRuntimeLibType();
+  }
+
+  return *runtimeLibType;
 }
 
 ToolChain::UnwindLibType ToolChain::GetUnwindLibType(
     const ArgList &Args) const {
+  if (unwindLibType)
+    return *unwindLibType;
+
   const Arg *A = Args.getLastArg(options::OPT_unwindlib_EQ);
   StringRef LibName = A ? A->getValue() : CLANG_DEFAULT_UNWINDLIB;
 
   if (LibName == "none")
-    return ToolChain::UNW_None;
+    unwindLibType = ToolChain::UNW_None;
   else if (LibName == "platform" || LibName == "") {
     ToolChain::RuntimeLibType RtLibType = GetRuntimeLibType(Args);
     if (RtLibType == ToolChain::RLT_CompilerRT)
-      return ToolChain::UNW_None;
+      unwindLibType = ToolChain::UNW_None;
     else if (RtLibType == ToolChain::RLT_Libgcc)
-      return ToolChain::UNW_Libgcc;
+      unwindLibType = ToolChain::UNW_Libgcc;
   } else if (LibName == "libunwind") {
     if (GetRuntimeLibType(Args) == RLT_Libgcc)
       getDriver().Diag(diag::err_drv_incompatible_unwindlib);
-    return ToolChain::UNW_CompilerRT;
+    unwindLibType = ToolChain::UNW_CompilerRT;
   } else if (LibName == "libgcc")
-    return ToolChain::UNW_Libgcc;
-
+    unwindLibType = ToolChain::UNW_Libgcc;
+  else {
     if (A)
       getDriver().Diag(diag::err_drv_invalid_unwindlib_name)
           << A->getAsString(Args);
 
-  return GetDefaultUnwindLibType();
+    unwindLibType = GetDefaultUnwindLibType();
+  }
+
+  return *unwindLibType;
 }
 
 ToolChain::CXXStdlibType ToolChain::GetCXXStdlibType(const ArgList &Args) const{
+  if (cxxStdlibType)
+    return *cxxStdlibType;
+
   const Arg *A = Args.getLastArg(options::OPT_stdlib_EQ);
   StringRef LibName = A ? A->getValue() : CLANG_DEFAULT_CXX_STDLIB;
 
   // Only use "platform" in tests to override CLANG_DEFAULT_CXX_STDLIB!
   if (LibName == "libc++")
-    return ToolChain::CST_Libcxx;
+    cxxStdlibType = ToolChain::CST_Libcxx;
   else if (LibName == "libstdc++")
-    return ToolChain::CST_Libstdcxx;
+    cxxStdlibType = ToolChain::CST_Libstdcxx;
   else if (LibName == "platform")
-    return GetDefaultCXXStdlibType();
-
+    cxxStdlibType = GetDefaultCXXStdlibType();
+  else {
     if (A)
-    getDriver().Diag(diag::err_drv_invalid_stdlib_name) << A->getAsString(Args);
+      getDriver().Diag(diag::err_drv_invalid_stdlib_name)
+          << A->getAsString(Args);
+
+    cxxStdlibType = GetDefaultCXXStdlibType();
+  }
 
-  return GetDefaultCXXStdlibType();
+  return *cxxStdlibType;
 }
 
 /// Utility function to add a system include directory to CC1 arguments.
 /*static*/ void ToolChain::addSystemInclude(const ArgList &DriverArgs,
                                             ArgStringList &CC1Args,
                                             const Twine &Path) {
   CC1Args.push_back("-internal-isystem");
   CC1Args.push_back(DriverArgs.MakeArgString(Path));

clang/test/Driver/undefined-libs.cpp

+
+// Check that all the following options print a warning when given a non-existent
+// value. But only one warning.
+
+// RUN: not %clangxx -stdlib=nostdlib %s 2>&1 | FileCheck --check-prefix=STDLIB %s
+// STDLIB: error: invalid library name in argument '-stdlib=nostdlib'
+// STDLIB-EMPTY:
+//
+// RUN: not %clangxx -rtlib=nortlib %s 2>&1 | FileCheck --check-prefix=RTLIB %s
+// RTLIB: error: invalid runtime library name in argument '-rtlib=nortlib'
+// RTLIB-EMPTY:
+//
+// RUN: not %clangxx -unwindlib=nounwindlib %s 2>&1 | FileCheck --check-prefix=UNWINDLIB %s
+// UNWINDLIB: error: invalid unwind library name in argument '-unwindlib=nounwindlib'
+// UNWINDLIB-EMPTY: