Page MenuHomePhabricator

[Support] Avoid normalization in sys::getDefaultTargetTriple
ClosedPublic

Authored by phosek on May 15 2018, 3:32 PM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

phosek created this revision.May 15 2018, 3:32 PM
rnk added a comment.May 16 2018, 11:28 AM

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

rnk added a comment.May 17 2018, 11:16 AM

It broke two tests, I still need to investigate.

rnk added a comment.May 17 2018, 3:03 PM

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 updated this revision to Diff 147431.May 17 2018, 7:56 PM

Applied, thanks so much for looking into it.

rnk accepted this revision.May 18 2018, 10:45 AM

lgtm

This revision is now accepted and ready to land.May 18 2018, 10:45 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.May 20 2018, 3:58 PM

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

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

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.

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

Reverted in r332822 / r332823.

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.

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

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.