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

[Jonas Hahnfeld via cfe-commits]

Hahnfeld added a comment.

In https://reviews.llvm.org/D22663#495728, @zlei wrote:

> In https://reviews.llvm.org/D22663#494460, @Hahnfeld 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)


================
Comment at: CMakeLists.txt:210
@@ +209,3 @@
+  message(WARNING "Resetting default rtlib to use platform default")
+  set(CLANG_DEFAULT_RTLIB "")
+endif()
----------------
zlei wrote:
> beanz wrote:
> > 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`.
> 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?
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...

================
Comment at: lib/Driver/ToolChain.cpp:530
@@ -538,1 +529,3 @@
+  const Arg* A = Args.getLastArg(options::OPT_rtlib_EQ);
+  StringRef LibName = A ? A->getValue() : CLANG_DEFAULT_RTLIB;
 
----------------
zlei wrote:
> Hahnfeld wrote:
> > 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?
> 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?
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`...


https://reviews.llvm.org/D22663