This is an archive of the discontinued LLVM Phabricator instance.

Support setting default value for -rtlib at build time
ClosedPublic

Authored by zlei on Jul 21 2016, 8:02 PM.

Details

Summary

This patch introduces a new cmake variable: CLANG_DEFAULT_RTLIB, thru
which we can specify a default value for -rtlib (libgcc or
compiler-rt) at build time, just like how we set the default C++
stdlib thru CLANG_DEFAULT_CXX_STDLIB.

With these two options, we can configure clang to build binaries on
Linux that have no runtime dependence on any gcc libs (libstdc++ or
libgcc_s).

Diff Detail

Repository
rL LLVM

Event Timeline

zlei updated this revision to Diff 65016.Jul 21 2016, 8:02 PM
zlei retitled this revision from to Support setting default value for -rtlib at build time.
zlei updated this object.
zlei added reviewers: cfe-commits, ddunbar, Hahnfeld.
Hahnfeld requested changes to this revision.Jul 25 2016, 1:30 AM
Hahnfeld edited edge metadata.

In general the idea looks good and takes this point off my personal todo list :-)

With the changes applied and configured with CLANG_DEFAULT_RTLIB=compiler-rt some tests fail so you may have to adapt the idea of -rtlib=platform for the tests.

Just a minor comment: Please include the full context in your diff as explained in the docs: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
This makes reviewing easier because one can browse the surrounding code ;-)

lib/Driver/ToolChain.cpp
530 ↗(On Diff #65016)

What I dislike here is that it falls back to the platform default if the specified argument value is invalid regardless of what was configured. This may be surprising for the user as he gets a different behaviour when specifiying -rtlib=bla than completely omitting.

I just added a comment explaining how this works in GetCXXStdlibType. Maybe you can adapt its idea?

This revision now requires changes to proceed.Jul 25 2016, 1:30 AM
beanz added a subscriber: beanz.Jul 25 2016, 2:05 PM

One small comment on the CMake code.

CMakeLists.txt
210 ↗(On Diff #65016)

You'll want this to be:

set(CLANG_DEFAULT_RTLIB "" CACHE STRING "Default runtime library to use (libgcc or compiler-rt)" FORCE)

Cached variables can only be overwritten by a FORCE.

beanz added a reviewer: beanz.Jul 25 2016, 2:06 PM
zlei added a comment.Jul 25 2016, 9:19 PM

With the changes applied and configured with CLANG_DEFAULT_RTLIB=compiler-rt some tests fail so you may have to adapt the idea of -rtlib=platform for the tests.

Got it. And I also need to fix those failing tests by adding -rtlib=platform to them, right?

CMakeLists.txt
210 ↗(On Diff #65016)

Sorry, I'm not very familiar with cmake. But why isn't CLANG_DEFAULT_CXX_STDLIB declared with FORCE? Should I fix it as well?

lib/Driver/ToolChain.cpp
530 ↗(On Diff #65016)

If you give an invalid value to -rtlib, clang should just abort and give you an error message, instead of falling back to the platform default. I see that's also how -stdlib behaves.

Or maybe I misunderstood your point?

In D22663#495728, @zlei wrote:

With the changes applied and configured with CLANG_DEFAULT_RTLIB=compiler-rt some tests fail so you may have to adapt the idea of -rtlib=platform for the tests.

Got it. And I also need to fix those failing tests by adding -rtlib=platform to them, right?

Right, I think the tests should pass regardless of what default is selected (which currently is no completely true for CLANG_DEFAULT_CXX_STDLIB because it makes test/Driver/darwin-stdlib.cpp fail)

CMakeLists.txt
210 ↗(On Diff #65016)

That's a matter of what we want: without FORCE the value will only be reset for that single execution of CMake and not be updated in the cache!

This is what I think is correct: The user should be repeatedly reminded that he isn't getting what he originally requested...

lib/Driver/ToolChain.cpp
530 ↗(On Diff #65016)

Ehm yes, you are completely right.

For -rtlib=platform it should then be enough to write if (A && LibName != "platform") to not emit the error in this case. We could later use the same approach to simplify handling of -stdlib...

zlei updated this revision to Diff 65497.Jul 26 2016, 4:47 AM
zlei edited edge metadata.

Support a new option -rtlib=platform for internal use, and fix tests that previously failed when configured with CLANG_DEFAULT_RTLIB=compiler-rt.

zlei added inline comments.Jul 26 2016, 4:57 AM
CMakeLists.txt
210 ↗(On Diff #65497)

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.

Hahnfeld added inline comments.Jul 26 2016, 5:10 AM
CMakeLists.txt
210 ↗(On Diff #65497)

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...

zlei added inline comments.Jul 26 2016, 6:33 AM
CMakeLists.txt
210 ↗(On Diff #65497)

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 :)

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
 //
CMakeLists.txt
210 ↗(On Diff #65497)

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 ;-)

beanz added inline comments.Jul 26 2016, 8:33 AM
CMakeLists.txt
210 ↗(On Diff #65497)

The first set call shouldn't be forced because you want to allow users to manually specify the value. If you were to set FORCE on it it would always overwrite any value specified on the command line.

The second set is debatable one way or the other. The CMake will function correctly with or without the set being forced, however as @Hahnfeld commented the invalid value will remain in the CMake cache. That means that every time CMake is re-run the warning will get printed, and the value will be overridden within the scope of that execution.

I think it is better to FORCE the second set so that the cache gets overwritten with a valid value and the warning only gets logged the first time CMake is run. That said, I don't really have a strong opinion here, so do whatever you think is right.

zlei updated this revision to Diff 65643.Jul 26 2016, 6:41 PM
zlei edited edge metadata.

Fix a bug on Darwin that crashes clang when CLANG_DEFAULT_RTLIB=libgcc

zlei added a comment.Jul 26 2016, 6:44 PM

@Hahnfeld Your patch applied. Thanks.

Hahnfeld accepted this revision.Jul 26 2016, 11:39 PM
Hahnfeld edited edge metadata.

LGTM, thanks for this work!

Can you commit the patch yourself or should I do that for you?

This revision is now accepted and ready to land.Jul 26 2016, 11:39 PM
zlei added a comment.Jul 27 2016, 1:18 AM

@Hahnfeld Could you please commit it? I don't have the access.

Thanks :)

This revision was automatically updated to reflect the committed changes.