[PATCH] D111952: [clang] [MinGW] Guess the right ix86 arch name spelling as sysroot

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/MinGW.h:63
 
+  static void FixTripleArch(const Driver &D, llvm::Triple &Triple,
+                            const llvm::opt::ArgList &Args);
----------------
MaskRay wrote:
> Use `functionName` for newer functions. (It's unfortunate that Clang has much inconsistency but we should do the right thing for new methods.)
Sure, will change it.


================
Comment at: clang/test/Driver/mingw-sysroot.cpp:49
+
+// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang/bin/x86_64-w64-mingw32-clang -target x86_64-w64-mingw32 -m32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG_I686 %s
+// CHECK_TESTROOT_CLANG_I686: "{{[^"]+}}/testroot-clang{{/|\\\\}}i686-w64-mingw32{{/|\\\\}}include"
----------------
MaskRay wrote:
> Changing `PATH` seems unreliable for some test environments.
> 
> Any alternative way to test this?
> 
> `--target=` is preferred over `-target ` for driver options. `-target ` is legacy but we probably need to keep indefinitely for its wide usage.
> 
> `linux-cross.cpp` has some nice tests which may inspire this file ? :)
Using `PATH` isn't very nice, no, but it's at least reliable enough to run on the current testing setups?

I don't see any good input for testing this in linux-cross.cpp - that file uses explicit `--sysroot=` parameters all over the place, while this test file is mostly about how to best implicitly deduce the sysroot when one isn't specified.

This file (both the old and the newly added tests) relies on checking what directories it finds in `<clang-executable>/../<triple>`, so it requires fake-installing the clang binary by symlinking into a test toolchain tree.

I guess it could be possible to execute that fake-installed clang binary by specifying the direct path to it, instead of adding it to `PATH` - but it's an important usecase to make sure that it works when the literal path isn't spelled out in `argv[0]` too.

I can change to use `--target=` in the added lines, but the rest of the file uses the other spelling anyway (and I don't think it's on topic to do such rewrites as part of this patch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111952