[PATCH] D22663: Support setting default value for -rtlib at build time

[Jonas Hahnfeld via cfe-commits]

Hahnfeld added a comment.

There is also the other way round: `CLANG_DEFAULT_RTLIB=libgcc` shows that Darwin doesn't support that and gets a `SIGSEGV` (because it doesn't expect it to be `libgcc` without an option to be set, hehe)

Based on some `GetCXXStdlibType` for toolchains that only support one `-stdlib`, I think we can just override `GetRuntimeLibType` here

  diff --git a/lib/Driver/ToolChains.cpp b/lib/Driver/ToolChains.cpp
  index f05261a..610285e 100644
  --- a/lib/Driver/ToolChains.cpp
  +++ b/lib/Driver/ToolChains.cpp
  @@ -403,18 +403,23 @@ void DarwinClang::AddLinkSanitizerLibArgs(const ArgList &Args,
         /*AddRPath*/ true);
   }
   
  +ToolChain::RuntimeLibType DarwinClang::GetRuntimeLibType(
  +    const ArgList &Args) const {
  +  if (Arg* A = Args.getLastArg(options::OPT_rtlib_EQ)) {
  +    StringRef Value = A->getValue();
  +    if (Value != "compiler-rt")
  +      getDriver().Diag(diag::err_drv_unsupported_rtlib_for_platform)
  +          << Value << "darwin";
  +  }
  +
  +  return ToolChain::RLT_CompilerRT;
  +}
  +
   void DarwinClang::AddLinkRuntimeLibArgs(const llvm::Triple &EffectiveTriple,
                                           const ArgList &Args,
                                           ArgStringList &CmdArgs) const {
  -  // Darwin only supports the compiler-rt based runtime libraries.
  -  switch (GetRuntimeLibType(Args)) {
  -  case ToolChain::RLT_CompilerRT:
  -    break;
  -  default:
  -    getDriver().Diag(diag::err_drv_unsupported_rtlib_for_platform)
  -        << Args.getLastArg(options::OPT_rtlib_EQ)->getValue() << "darwin";
  -    return;
  -  }
  +  // Call once to ensure diagnostic is printed if wrong value was specified
  +  GetRuntimeLibType(Args);
   
     // Darwin doesn't support real static executables, don't link any runtime
     // libraries with -static.
  diff --git a/lib/Driver/ToolChains.h b/lib/Driver/ToolChains.h
  index 25dae72..d36a799 100644
  --- a/lib/Driver/ToolChains.h
  +++ b/lib/Driver/ToolChains.h
  @@ -575,6 +575,8 @@ public:
     /// @name Apple ToolChain Implementation
     /// {
   
  +  RuntimeLibType GetRuntimeLibType(const llvm::opt::ArgList &Args) const override;
  +
     void AddLinkRuntimeLibArgs(const llvm::Triple &EffectiveTriple,
                                const llvm::opt::ArgList &Args,
                                llvm::opt::ArgStringList &CmdArgs) const override;
  diff --git a/test/Driver/mips-mti-linux.c b/test/Driver/mips-mti-linux.c
  index e3560e2..4835d79 100644
  --- a/test/Driver/mips-mti-linux.c
  +++ b/test/Driver/mips-mti-linux.c
  @@ -8,7 +8,7 @@
   
   // = Big-endian, mips32r2, hard float
   // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
  -// RUN:     --target=mips-mti-linux -mips32r2 -mhard-float \
  +// RUN:     --target=mips-mti-linux -mips32r2 -mhard-float -rtlib=platform \
   // RUN:     --sysroot=%S/Inputs/mips_mti_linux/sysroot \
   // RUN:   | FileCheck --check-prefix=CHECK-BE-HF-32R2 %s
   //
  @@ -26,7 +26,7 @@
   
   // = Little-endian, mips32r2, hard float
   // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
  -// RUN:     --target=mips-mti-linux -mips32r2 -EL -mhard-float \
  +// RUN:     --target=mips-mti-linux -mips32r2 -EL -mhard-float -rtlib=platform \
   // RUN:     --sysroot=%S/Inputs/mips_mti_linux/sysroot \
   // RUN:   | FileCheck --check-prefix=CHECK-LE-HF-32R2 %s
   //


================
Comment at: CMakeLists.txt:210
@@ -202,3 +209,3 @@
 
 set(CLANG_VENDOR ${PACKAGE_VENDOR} CACHE STRING
   "Vendor-specific text for showing with version information.")
----------------
zlei wrote:
> Hahnfeld wrote:
> > zlei wrote:
> > > I think the original code for resetting `CLANG_DEFAULT_CXX_STDLIB` doesn't work as expected, as beanz pointed out.
> > > 
> > > In this revision, I just disable invalid values for both `CLANG_DEFAULT_CXX_STDLIB` and `CLANG_DEFAULT_RTLIB`. Users will get a error message when assigning unsupported values to them.
> > I tested it this morning and it works as (at least I) expected: It will temporarily reset the value and warn the user that the parameter is not valid.
> > 
> > I'm against erroring out here because there actually is a sane default value...
> I don't have a strong opinion on this, but the problem is the original line `set(CLANG_DEFAULT_CXX_STDLIB "")` doesn't have actual effect (it seems to me).
> 
> I'm not sure what you mean by "temporarily reset the value". Last time I tested it, `-DCLANG_DEFAULT_CXX_STDLIB=blah` doesn't reset the value to an empty string (as expected?)
> 
> Anyway, I'm fine with either warning or erroring :)
It means that you will still see `CLANG_DEFAULT_CXX_STDLIB=blah` in `CMakeCache.txt` but it will be temporarily set to `""` when CMake builds the Makefiles.

Let's have @beanz decide on that: He is the master of CMake and I can only say that it is working as I wanted it to do ;-)


https://reviews.llvm.org/D22663