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

[ro]

Summary

Builtins-*-sunos :: compiler_rt_logbf_test.c currently FAILs on Solaris, both SPARC and
x86, 32 and 64-bit.

It turned out that this is due to different behaviour of logb depending on the C
standard compiled for, as documented on logb(3M):

RETURN VALUES
       Upon successful completion, these functions return the exponent of x.

       If x is subnormal:

           o      For SUSv3-conforming applications compiled with the c99 com-
                  piler  driver  (see standards(7)), the exponent of x as if x
                  were normalized is returned.

           o      Otherwise, if compiled with the cc compiler  driver,  -1022,
                  -126,  and  -16382  are  returned  for  logb(), logbf(), and
                  logbl(), respectively.

Studio c99 and gcc control this by linking with the appropriate version of values-xpg[46].o, but clang uses neither of those.

The following patch fixes this by following what gcc does, as corrected some time ago
in Fix use of Solaris values-Xc.o (PR target/40411), https://gcc.gnu.org/ml/gcc-patches/2018-01/msg02350.html and https://gcc.gnu.org/ml/gcc-patches/2018-01/msg02384.html.

It makes use of LangStandard::getLangStandardForName to parse the std=<std>
values. However, I found that the function currently doesn't handle the alias forms
(like c90 for c89). Given that it isn't currently used in the clang repo, I just added
that handling.

As a consequence, ClangDriverTests now also needs to be linked with libclangFrontend.

Tested on x86_64-pc-solaris2.11, sparcv9-sun-solaris2.11, and x86_64-pc-linux-gnu.
Ok for trunk (and a backport to the llvm 9 branch)?

[ro]

Ping? It's been a week now and this is a correctness issue on Solaris, not just a single failing test.

[jyknight]

Is this really necessary? Users don't typically pass -std= to the driver for linking anyways (what do you even pass if you've compiled both C and C++ code?) so this seems a rather odd way to control behavior.

How about instead just adding "values-xpg6.o" unconditionally, alongside the current unconditional "values-Xa.o", and just forget about the xpg4 and Xc modes?

lib/Driver/ToolChains/Solaris.cpp

17

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

[ro]

In D64793#1597550, @jyknight wrote:

Is this really necessary? Users don't typically pass -std= to the driver for linking anyways (what do you even pass if you've compiled both C and C++ code?) so this seems a rather odd way to control behavior.

I fear it is necessary: at least it matches documented behaviour of both the Sun/Oracle Studio compilers and gcc.

The -std= options usually get passed to the linking step because CFLAGS is added to the options as well. In the mixed-language case, you have to link
with the C++ compiler, and the !isGNUMode test deals with both languages alike.

How about instead just adding "values-xpg6.o" unconditionally, alongside the current unconditional "values-Xa.o", and just forget about the xpg4 and Xc modes?

If all else fails, that would have to be the last fallback. I'd rather do things correctly, though.

lib/Driver/ToolChains/Solaris.cpp

17

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.

[ro]

Ping^2? It's been two weeks now. Who can decide if the LangStandards code can be used from lib/Frontend as is or needs to be moved
elsewhere (lib/Basic?) for use by driver code?

[rnk]

In D64793#1597765, @ro wrote:

In D64793#1597550, @jyknight wrote:

How about instead just adding "values-xpg6.o" unconditionally, alongside the current unconditional "values-Xa.o", and just forget about the xpg4 and Xc modes?

If all else fails, that would have to be the last fallback. I'd rather do things correctly, though.

I think the cost of complexity in the solaris-specific part of the driver is low, so if the maintainer thinks it's worth having, we can certainly add it.

lib/Driver/ToolChains/Solaris.cpp

17

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.

lib/Frontend/LangStandards.cpp

31–37 ↗

(On Diff #210068)

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.

[jyknight]

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.

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.

[ro]

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

lib/Driver/ToolChains/Solaris.cpp

17

I've now implemented that move as https://reviews.llvm.org/D65562.
I'll submit a revised version of this patch shortly.

lib/Frontend/LangStandards.cpp

31–37 ↗

(On Diff #210068)

Also included in the move patch.

[ro]

Updated patch to make use of the move to
Basic.

Tested on top of that one on x86_64-pc-solaris2.11, sparcv9-sun-solaris2.11, and
x86_64-pc-linux-gnu.

[ro]

Account for enum class Language change in D65562.

Tested as before.

[rnk]

lgtm

If I interpret @jyknight correctly, having failed to dissuade you, he doesn't feel strongly enough about this to block it, he just wants to reduce legacy when feasible.

[ro]

In D64793#1613063, @rnk wrote:

lgtm

If I interpret @jyknight correctly, having failed to dissuade you, he doesn't feel strongly enough about this to block it, he just wants to reduce legacy when feasible.

Fully understood: I tend to do the same in the Solaris GCC port as well, getting rid of old cruft every once in a while (like deprecating/removing support for old OS versions). If we misunderstood, he can still object while I'm working to get the prerequisite lldb (and polly) patches in.

lib/Driver/ToolChains/Solaris.cpp

 //===--- Solaris.cpp - Solaris ToolChain Implementations --------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Solaris.h"
 #include "CommonArgs.h"
+#include "clang/Basic/LangStandard.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang::driver::toolchains;
 using namespace clang;
 using namespace llvm::opt;
void solaris::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   }
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles)) {
     if (!Args.hasArg(options::OPT_shared))
       CmdArgs.push_back(
           Args.MakeArgString(getToolChain().GetFilePath("crt1.o")));
 
     CmdArgs.push_back(Args.MakeArgString(getToolChain().GetFilePath("crti.o")));
+
+    const Arg *Std = Args.getLastArg(options::OPT_std_EQ, options::OPT_ansi);
+    bool HaveAnsi = false;
+    const LangStandard *LangStd = nullptr;
+    if (Std) {
+      HaveAnsi = Std->getOption().matches(options::OPT_ansi);
+      if (!HaveAnsi)
+        LangStd = LangStandard::getLangStandardForName(Std->getValue());
+    }
+
+    const char *values_X = "values-Xa.o";
+    // Use values-Xc.o for -ansi, -std=c*, -std=iso9899:199409.
+    if (HaveAnsi || (LangStd && !LangStd->isGNUMode()))
+      values_X = "values-Xc.o";
+    CmdArgs.push_back(Args.MakeArgString(getToolChain().GetFilePath(values_X)));
+
+    const char *values_xpg = "values-xpg6.o";
+    // Use values-xpg4.o for -std=c90, -std=gnu90, -std=iso9899:199409.
+    if (LangStd && LangStd->getLanguage() == clang::C && !LangStd->isC99())
+      values_xpg = "values-xpg4.o";
     CmdArgs.push_back(
-        Args.MakeArgString(getToolChain().GetFilePath("values-Xa.o")));
+        Args.MakeArgString(getToolChain().GetFilePath(values_xpg)));
     CmdArgs.push_back(
         Args.MakeArgString(getToolChain().GetFilePath("crtbegin.o")));
   }
 
   getToolChain().AddFilePathLibArgs(Args, CmdArgs);
 
   Args.AddAllArgs(CmdArgs, {options::OPT_L, options::OPT_T_Group,
                             options::OPT_e, options::OPT_r});

test/Driver/solaris-ld-values.c

+// General tests that the correct versions of values-*.o are used on
+// Solaris targets sane. Note that we use sysroot to make these tests
+// independent of the host system.
+
+// Check sparc-sun-solaris2.11, 32bit
+// RUN: %clang -no-canonical-prefixes -ansi %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-ANSI %s
+// CHECK-LD-SPARC32-ANSI: values-Xc.o
+// CHECK-LD-SPARC32-ANSI: values-xpg6.o
+
+// RUN: %clang -no-canonical-prefixes -std=c89 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-C89 %s
+// CHECK-LD-SPARC32-C89: values-Xc.o
+// CHECK-LD-SPARC32-C89: values-xpg4.o
+
+// RUN: %clang -no-canonical-prefixes -std=c90 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-C90 %s
+// CHECK-LD-SPARC32-C90: values-Xc.o
+// CHECK-LD-SPARC32-C90: values-xpg4.o
+
+// RUN: %clang -no-canonical-prefixes -std=iso9899:199409 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-C94 %s
+// CHECK-LD-SPARC32-C94: values-Xc.o
+// CHECK-LD-SPARC32-C94: values-xpg4.o
+
+// RUN: %clang -no-canonical-prefixes -std=c11 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-C11 %s
+// CHECK-LD-SPARC32-C11: values-Xc.o
+// CHECK-LD-SPARC32-C11: values-xpg6.o
+
+// RUN: %clang -no-canonical-prefixes -std=gnu89 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-GNU89 %s
+// CHECK-LD-SPARC32-GNU89: values-Xa.o
+// CHECK-LD-SPARC32-GNU89: values-xpg4.o
+
+// RUN: %clang -no-canonical-prefixes -std=gnu90 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-GNU90 %s
+// CHECK-LD-SPARC32-GNU90: values-Xa.o
+// CHECK-LD-SPARC32-GNU90: values-xpg4.o
+
+// RUN: %clang -no-canonical-prefixes -std=gnu11 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-GNU11 %s
+// CHECK-LD-SPARC32-GNU11: values-Xa.o
+// CHECK-LD-SPARC32-GNU11: values-xpg6.o
+
+// Check i386-pc-solaris2.11, 32bit
+// RUN: %clang -no-canonical-prefixes -ansi %s -### -o %t.o 2>&1 \
+// RUN:     --target=i386-pc-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_x86_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-X32-ANSI %s
+// CHECK-LD-X32-ANSI: values-Xc.o
+// CHECK-LD-X32-ANSI: values-xpg6.o

test/Driver/solaris-ld-values.cpp

+// General tests that the correct versions of values-*.o are used on
+// Solaris targets sane. Note that we use sysroot to make these tests
+// independent of the host system.
+
+// Check sparc-sun-solaris2.11, 32bit
+// RUN: %clang -no-canonical-prefixes -ansi %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-ANSI %s
+// CHECK-LD-SPARC32-ANSI: values-Xc.o
+// CHECK-LD-SPARC32-ANSI: values-xpg6.o
+
+// RUN: %clang -no-canonical-prefixes -std=c++98 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-CPP98 %s
+// CHECK-LD-SPARC32-CPP98: values-Xc.o
+// CHECK-LD-SPARC32-CPP98: values-xpg6.o
+
+// RUN: %clang -no-canonical-prefixes -std=c++11 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-CPP11 %s
+// CHECK-LD-SPARC32-CPP11: values-Xc.o
+// CHECK-LD-SPARC32-CPP11: values-xpg6.o
+
+// RUN: %clang -no-canonical-prefixes -std=gnu++98 %s -### -o %t.o 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32-GNUPP98 %s
+// CHECK-LD-SPARC32-GNUPP98: values-Xa.o
+// CHECK-LD-SPARC32-GNUPP98: values-xpg6.o
+
+// Check i386-pc-solaris2.11, 32bit
+// RUN: %clang -no-canonical-prefixes -ANSI %s -### -o %t.o 2>&1 \
+// RUN:     --target=i386-pc-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_x86_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-X32-ANSI %s
+// CHECK-LD-X32-ANSI: values-Xa.o
+// CHECK-LD-X32-ANSI: values-xpg6.o