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

[rnk]

I'm testing this on Windows and I'll stamp it when they pass.

[rnk]

It broke two tests, I still need to investigate.

[rnk]

Applying this diff to clang fixes the test failures:

$ git diff ../clang
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 05e5196e32d2..4824bdbba581 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -2916,10 +2916,11 @@ static void ParseTargetArgs(TargetOptions &Opts, ArgList &Args,
   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(

[phosek]

Applied, thanks so much for looking into it.

[rnk]

lgtm

[thakis]

This changes the default triple on Windows from x86_64-pc-win32 to x86_64-pc-windows-msvc. This breaks at least

[thakis]

(sorry, hit cmd-return accidentally, I will come in again…)

[thakis]

This changes the default triple on Windows from x86_64-pc-win32 to x86_64-pc-windows-msvc. This breaks at least lit's make_itanium_abi_triple() (http://llvm-cs.pcc.me.uk/utils/lit/lit/llvm/config.py#234)

def make_itanium_abi_triple(self, triple):
    m = re.match(r'(\w+)-(\w+)-(\w+)', triple)
    if not m:
        self.lit_config.fatal(
            "Could not turn '%s' into Itanium ABI triple" % triple)
    if m.group(3).lower() != 'win32':
        # All non-win32 triples use the Itanium ABI.
        return triple
    return m.group(1) + '-' + m.group(2) + '-mingw32'

which in turn breaks %itanium_abi_triple in lit tests, which then breaks some 257 tests on Windows.

(Also visible on http://lab.llvm.org:8011/builders/clang-x64-ninja-win7 , but that bot has been broken for a long time.)

So I'll revert this for now.

[thakis]

…hm looks like https://reviews.llvm.org/rL332750 touched files in both clang and llvm, which is probably why the diff looks super confusing on phab. I'll revert it in two pieces, I don't know how to commit changes to llvm and clang in one revision.

[thakis]

Reverted in r332822 / r332823.

[phosek]

In D46910#1105992, @thakis wrote:

This changes the default triple on Windows from x86_64-pc-win32 to x86_64-pc-windows-msvc. This breaks at least lit's make_itanium_abi_triple() (http://llvm-cs.pcc.me.uk/utils/lit/lit/llvm/config.py#234)

def make_itanium_abi_triple(self, triple):
    m = re.match(r'(\w+)-(\w+)-(\w+)', triple)
    if not m:
        self.lit_config.fatal(
            "Could not turn '%s' into Itanium ABI triple" % triple)
    if m.group(3).lower() != 'win32':
        # All non-win32 triples use the Itanium ABI.
        return triple
    return m.group(1) + '-' + m.group(2) + '-mingw32'

which in turn breaks %itanium_abi_triple in lit tests, which then breaks some 257 tests on Windows.

(Also visible on http://lab.llvm.org:8011/builders/clang-x64-ninja-win7 , but that bot has been broken for a long time.)

So I'll revert this for now.

Sorry about the breakage, I wasn't aware of that test failure and never got any email (probably because the bot has been red before). I'll try to fix make_itanium_abi_triple and test it on my Windows box tomorrow before resubmitting.

In D46910#1105993, @thakis wrote:

…hm looks like https://reviews.llvm.org/rL332750 touched files in both clang and llvm, which is probably why the diff looks super confusing on phab. I'll revert it in two pieces, I don't know how to commit changes to llvm and clang in one revision.

The change was made through monorepo and submitted via git llvm push which allows changing multiple projects in one commit.

[thakis]

Sorry about the breakage, I wasn't aware of that test failure and never got any email (probably because the bot has been red before). I'll try to fix make_itanium_abi_triple and test it on my Windows box tomorrow before resubmitting.

Thanks, sounds good. Reading the history of the review a bit, it sounds like Reid did try to test this on Windows. My guess is that he didn't create a completely new build directory, and that cmake's cache made it so that he didn't see all the changes that are in here. (Our bots always blow away the cmake cache on each build for this reason.) So make sure to create a fresh build dir for each local iteration of the patch when you test this.

The change was made through monorepo and submitted via git llvm push which allows changing multiple projects in one commit.

Is there some toggle necessary to make it so that all parts of the patch show up on phab? For this issue, phab only displays the clang bit, which confused me for a while.

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,