[PATCH] D64793: [Driver] Properly use values-X[ca].o, values-xpg[46].o on Solaris

[Rainer Orth via Phabricator via cfe-commits]

ro marked 5 inline comments as done.
ro added a comment.

In D64793#1604877 <https://reviews.llvm.org/D64793#1604877>, @jyknight wrote:

> > I fear it is necessary: at least it matches documented behaviour of both the Sun/Oracle Studio compilers and gcc.
>
> I will defer to your opinion here. But -- one last attempt at dissuading you. :)
>
> Is this really doing something _important_, or is it just legacy cruft which can be safely ignored by now? With your "logb" example, it seems to me that it is probably preferable to always use the new correct "xpg6" implementation, and just ignore the legacy/incorrect version. Similarly, the example given in https://gcc.gnu.org/PR40411 of freopen -- again, seems like it'd be better to just use the new xpg6 behavior, always.


For new code, you're certainly right, but existing C90 code that relies on pre-C99 behaviour should receive correct results IMO.
Otherwise, you could just as well remove C90 support in clang and argue that C99 (or even C11) is better ;-)

>> The -std= options usually get passed to the linking step because CFLAGS is added to the options as well
> 
> With gnu make they are not (unless it's doing a single-step compiling+linking step). Other tools like CMake also don't pass standards versions to linking. This makes sense, because a program can contain code compiled with multiple standards versions, and multiple languages. Thus, I'd expect most users to just get the default xpg6 and Xa objects, even if they do specify -std=c90 for compilation.

I don't really buy this: there are several cases that do need passing the same (or companion) options to both the compiler and linker.
Think of building 32-bit code with a 64-bit-default compiler where you need to pass -m32 to both.  Similarly for -pthread both
during compilation (to define `_REENTRANT`) and linking (to link with `-lpthread`).  There are tons of other cases where something 
like this is mandatory.

Depending on the build environment, the exact method to achieve this will differ (like adding -m32 to $(CC)), but it most certainly
can be done.

It's true that mixing different standards version in the same executable is problematic to say the best, but that's a quirk of that Solaris
method we can do nothing about: developers just need to be aware of the limitation.



================
Comment at: lib/Driver/ToolChains/Solaris.cpp:16
 #include "clang/Driver/Options.h"
+#include "clang/Frontend/LangStandard.h"
 #include "llvm/Option/ArgList.h"
----------------
rnk wrote:
> ro wrote:
> > jyknight wrote:
> > > I'm not sure that's an acceptable dependency -- I think Driver probably is not supposed to depend on Frontend. If so, I guess LangStandard should move to clang/Basic/. Which also means moving InputKind from clang/include/clang/Frontend/FrontendOptions.h.
> > > 
> > > (Maybe someone else can weigh in on this question?)
> > I wondered about this myself, including frontend code in the
> > driver might be considered a layering violation.  I certainly need
> > guidance here what is and isn't acceptable here.
> I see there are no other includes of Frontend from Driver, so I think LangStandards* does need to move to Basic. The only piece of InputKind that's needed is the language enum. I'm surprised there isn't already one somewhere else, but if there isn't, I think it would be reasonable to define the input kind languages in LangStandard.h and use them from FrontendOptions.
I've now implemented that move as [[https://reviews.llvm.org/D65562]].
I'll submit a revised version of this patch shortly.


================
Comment at: lib/Frontend/LangStandards.cpp:31-37
   Kind K = llvm::StringSwitch<Kind>(Name)
 #define LANGSTANDARD(id, name, lang, desc, features) \
     .Case(name, lang_##id)
+#define LANGSTANDARD_ALIAS(id, alias) \
+    .Case(alias, lang_##id)
 #include "clang/Frontend/LangStandards.def"
     .Default(lang_unspecified);
----------------
rnk wrote:
> I see that this code pattern is repeated in two other places, lib/Tooling/InterpolatingCompilationDatabase.cpp and lib/Frontend/CompilerInvocation.cpp. I think it would be good to factor out a string-to-kind helper and use it in the three places.
Also included in the [[ https://reviews.llvm.org/D6556 | move patch]].


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64793/new/

https://reviews.llvm.org/D64793