[Support] Avoid normalization in sys::getDefaultTargetTriple

[phosek]

Summary

The return value of sys::getDefaultTargetTriple, which is derived from
-DLLVM_DEFAULT_TRIPLE, is used to construct tool names, default target,
and in the future also to control the search path directly; as such it
should be used textually, without interpretation by LLVM.

Normalization of this value may lead to unexpected results, for example
if we configure LLVM with -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-linux-gnu,
normalization will transform that value to x86_64--linux-gnu. Driver will
use that value to search for tools prefixed with x86_64--linux-gnu- which
may be confusing. This is also inconsistent with the behavior of the
--target flag which is taken as-is without any normalization and overrides
the value of LLVM_DEFAULT_TARGET_TRIPLE.

Users of sys::getDefaultTargetTriple already perform their own
normalization as needed, so this change shouldn't impact existing logic.

[delcypher]

I'm not familiar with this code but if I'm reading the patch correctly, then the commit message omits the fact that it changes the windows target triple from having a win32 suffix to windows-msvc suffix. I'm not sure what the implications are here but the fact you've done this is probably worth mentioning in the commit message.

[phosek]

In D47153#1106780, @delcypher wrote:

I'm not familiar with this code but if I'm reading the patch correctly, then the commit message omits the fact that it changes he windows target triple from having a win32 suffix to windows-msvc suffix. I'm not sure what the implications are here but the fact you've done this is probably worth mentioning in the commit message.

Done. I've also tested this change on Windows and this seems to be passing now so should be ready for re-land.

[phosek]

In D47153#1107469, @phosek wrote:

Done. I've also tested this change on Windows and this seems to be passing now so should be ready for re-land.

Spoke too early, the following tests are still failing:

Clang :: Analysis/cxx-crashes.cpp
Clang :: CodeGen/2007-06-18-SextAttrAggregate.c
Clang :: CodeGen/2008-12-23-AsmIntPointerTie.c
Clang :: CodeGen/statements.c
Clang :: CodeGenCXX/2005-01-03-StaticInitializers.cpp
Clang :: CodeGenCXX/reinterpret-cast.cpp
Clang :: CodeGenCXX/vtable-debug-info.cpp
Clang :: Driver/coverage_no_integrated_as.c
Clang :: Driver/inhibit-downstream-commands.c
Clang :: Driver/linker-opts.c
Clang :: Driver/no-integrated-as.s
Clang :: Driver/nostdincxx.cpp
Clang :: Sema/array-init.c
Clang :: Sema/compound-literal.c
Clang :: SemaCXX/cstyle-cast.cpp
Clang :: SemaCXX/functional-cast.cpp
Clang :: SemaObjC/nonnull.m
Clang :: SemaObjCXX/pointer-to-objc-pointer-conv.mm

[delcypher]

In D47153#1107469, @phosek wrote:

In D47153#1106780, @delcypher wrote:

I'm not familiar with this code but if I'm reading the patch correctly, then the commit message omits the fact that it changes he windows target triple from having a win32 suffix to windows-msvc suffix. I'm not sure what the implications are here but the fact you've done this is probably worth mentioning in the commit message.

Done. I've also tested this change on Windows and this seems to be passing now so should be ready for re-land.

Thanks. This code really isn't my area though so you should wait for a LGTM from one of your reviewers.

llvm/utils/lit/lit/llvm/config.py

253 ↗

(On Diff #148136)

This comment probably needs updating. s/win32/windows/

[phosek]

llvm/utils/lit/lit/llvm/config.py

252 ↗

(On Diff #148240)

@rnk I assume this should only be the case when the triple is *-windows-msvc, not *-windows-gnu, but what if user omits the environment? Shall we just bail out or pick some default?

[rnk]

I think we can probably split this patch up to reduce the risk of breaking things. I think if you revert the config-ix.cmake change and just move triple normalization from LLVM to clang, everything should keep working. LLVM tools like llc might start to break in interesting ways, though, since they won't do the normalization that clang is doing. If check-llvm passes, though, I'm happy.

The config-ix.cmake and %itanium_abi_triple can be a follow-up.

[phosek]

In D47153#1111541, @rnk wrote:

I think we can probably split this patch up to reduce the risk of breaking things. I think if you revert the config-ix.cmake change and just move triple normalization from LLVM to clang, everything should keep working. LLVM tools like llc might start to break in interesting ways, though, since they won't do the normalization that clang is doing. If check-llvm passes, though, I'm happy.

The config-ix.cmake and %itanium_abi_triple can be a follow-up.

All the tests seem to be passing now, but the patch is rather large now. I tried splitting up the the triple normalization and everything seems to be still passing so I'm fine landing those two parts separately.

[rnk]

lgtm

clang/lib/Frontend/CompilerInvocation.cpp

if (EABIVersion == llvm::EABI::Unknown)
                                                 << Value;
     else
       Opts.EABIVersion = EABIVersion;
   }
   Opts.CPU = Args.getLastArgValue(OPT_target_cpu);
   Opts.FPMath = Args.getLastArgValue(OPT_mfpmath);
   Opts.FeaturesAsWritten = Args.getAllArgValues(OPT_target_feature);
   Opts.LinkerVersion = Args.getLastArgValue(OPT_target_linker_version);
-  Opts.Triple = llvm::Triple::normalize(Args.getLastArgValue(OPT_triple));
+  Opts.Triple = Args.getLastArgValue(OPT_triple);
   // Use the default target triple if unspecified.
   if (Opts.Triple.empty())
     Opts.Triple = llvm::sys::getDefaultTargetTriple();
+  Opts.Triple = llvm::Triple::normalize(Opts.Triple);
   Opts.OpenCLExtensionsAsWritten = Args.getAllArgValues(OPT_cl_ext_EQ);
   Opts.ForceEnableInt128 = Args.hasArg(OPT_fforce_enable_int128);
   Opts.NVPTXUseShortPointers = Args.hasFlag(
       options::OPT_fcuda_short_ptr, options::OPT_fno_cuda_short_ptr, false);
 }
 
 bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
                                         const char *const *ArgBegin,

llvm/lib/Support/Unix/Host.inc

std::string sys::getDefaultTargetTriple() {
 
   // Override the default target with an environment variable named by
   // LLVM_TARGET_TRIPLE_ENV.
 #if defined(LLVM_TARGET_TRIPLE_ENV)
   if (const char *EnvTriple = std::getenv(LLVM_TARGET_TRIPLE_ENV))
     TargetTripleString = EnvTriple;
 #endif
 
-  return Triple::normalize(TargetTripleString);
+  return TargetTripleString;
 }

llvm/lib/Support/Windows/Host.inc

std::string sys::getDefaultTargetTriple() {
   const char *Triple = LLVM_DEFAULT_TARGET_TRIPLE;
 
   // Override the default target with an environment variable named by LLVM_TARGET_TRIPLE_ENV.
 #if defined(LLVM_TARGET_TRIPLE_ENV)
   if (const char *EnvTriple = std::getenv(LLVM_TARGET_TRIPLE_ENV))
     Triple = EnvTriple;
 #endif
 
-  return Triple::normalize(Triple);
+  return Triple;
 }