I would hope that in this situation our version auto-detection picks up _MSC_VER from cl.exe on PATH, but I guess that doesn't fire because of our VS installation path validation.

We want to raise the default anyway: https://bugs.llvm.org/show_bug.cgi?id=34243

Let's just do that.

Let's see if r315107 makes it better.

Since Hans is out, should we revert his dllexport change for  issue 772461 ?

We can, but that was supposed to be a fix for something else in Chrome. Maybe that will be easier to work around, though.

We have a local workaround for that (https://chromium-review.googlesource.com/c/701456/). If you know how to fix, that'd be better I suppose. Or we could wait with rolling until Hans is back.

Progress, compiler_rt builds now. However, at least some of the built binaries seem to not run:

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2Fwin_upload_clang%2F239%2F%2B%2Frecipes%2Fsteps%2Fpackage_clang%2F0%2Fstdout

[32/464] Generating Sanitizer-x86_64-Test.exe
FAILED: projects/compiler-rt/lib/sanitizer_common/tests/Sanitizer-x86_64-Test.exe 
cmd.exe /C "cd /D E:\b\build\slave\win_upload_clang\build\src\third_party\llvm-bootstrap\projects\compiler-rt\lib\sanitizer_common\tests && E:\b\build\slave\win_upload_clang\build\src\third_party\llvm-bootstrap\.\bin\clang.exe SANITIZER_TEST_OBJECTS.sanitizer_allocator_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_atomic_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_bitvector_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_bvgraph_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_common_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_deadlock_detector_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_flags_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_format_interceptor_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_ioctl_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_libc_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_linux_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_list_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_mutex_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_nolibc_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_posix_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_printf_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_procmaps_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_quarantine_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stackdepot_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stacktrace_printer_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stacktrace_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stoptheworld_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_suppressions_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_symbolizer_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_test_main.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_thread_registry_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.gtest-all.cc.x86_64.o E:/b/build/slave/win_upload_clang/build/src/third_party/llvm-bootstrap/projects/compiler-rt/lib/sanitizer_common/tests/RTSanitizerCommon.test.x86_64.lib -o E:/b/build/slave/win_upload_clang/build/src/third_party/llvm-bootstrap/projects/compiler-rt/lib/sanitizer_common/tests/./Sanitizer-x86_64-Test.exe -g -Wl,/STACK:0xC00000"
clang.exe: error: unable to execute command: program not executable

...wait, that's clang.exe that can't be executed? That's worse than what we had in comment 0 then :-/

...but there's a build warning a bit further up:

[15/464] Generating SANITIZER_TEST_OBJECTS.sanitizer_bitvector_test.cc.x86_64.o
E:/b/build/slave/win_upload_clang/build/src/third_party/llvm/projects/compiler-rt/lib/sanitizer_common/tests/sanitizer_bitvector_test.cc:72:29: warning: format specifies type 'unsigned long' but the argument has type 'std::_Tree_const_iterator<std::_Tree_val<std::_Tree_simple_types<unsigned long long> > >::value_type' (aka 'unsigned long long') [-Wformat]
    fprintf(stderr, "%lu ", *it);
                     ~~~    ^~~
                     %llu
1 warning generated.

So clang.exe generally seems to work, at least for a little while. So the error means that clang fails to execute the linker (?)

Yeah, it's failing to invoke the linker.

If I run it in a x64 vs2017 command window, it works.

But if I do it like the build does:

:\src\chromium\src\third_party\llvm-bootstrap>\src\chromium\src\third_party\depot_tools\win_toolchain\vs_files\9bc7ccbf9f4bd50d4a3bd185e8ca94ff1618de0b\win_sdk\bin\SetEnv.cmd /x64

C:\src\chromium\src\third_party\llvm-bootstrap>where link
C:\src\chromium\src\third_party\depot_tools\win_toolchain\vs_files\9bc7ccbf9f4bd50d4a3bd185e8ca94ff1618de0b\VC\Tools\MSVC\14.11.25503\bin\HostX64\x64\link.exe

C:\src\chromium\src\third_party\llvm-bootstrap>cmd.exe /C "cd /D C:\src\chromium\src\third_party\llvm-bootstrap\projects\compiler-rt\lib\sanitizer_common\tests && C:\src\chromium\src\third_party\llvm-bootstrap\.\bin\clang.exe SANITIZER_TEST_OBJECTS.sanitizer_allocator_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_atomic_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_bitvector_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_bvgraph_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_common_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_deadlock_detector_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_flags_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_format_interceptor_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_ioctl_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_libc_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_linux_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_list_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_mutex_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_nolibc_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_posix_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_printf_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_procmaps_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_quarantine_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stackdepot_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stacktrace_printer_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stacktrace_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stoptheworld_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_suppressions_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_symbolizer_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_test_main.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_thread_registry_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.gtest-all.cc.x86_64.o C:/src/chromium/src/third_party/llvm-bootstrap/projects/compiler-rt/lib/sanitizer_common/tests/RTSanitizerCommon.test.x86_64.lib -o C:/src/chromium/src/third_party/llvm-bootstrap/projects/compiler-rt/lib/sanitizer_common/tests/./Sanitizer-x86_64-Test.exe -g -Wl,/STACK:0xC00000"
clang.exe: error: unable to execute command: program not executable
clang.exe: error: linker command failed with exit code 1 (use -v to see invocation)

-v output:

clang version 6.0.0 (trunk 315332)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: C:\src\chromium\src\third_party\llvm-bootstrap\bin
 "link.exe" -out:C:/src/chromium/src/third_party/llvm-bootstrap/projects/compiler-rt/lib/sanitizer_common/tests/./Sanitizer-x86_64-Test.exe -defaultlib:libcmt -nologo -debug SANITIZER_TEST_OBJECTS.sanitizer_allocator_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_atomic_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_bitvector_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_bvgraph_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_common_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_deadlock_detector_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_flags_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_format_interceptor_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_ioctl_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_libc_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_linux_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_list_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_mutex_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_nolibc_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_posix_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_printf_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_procmaps_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_quarantine_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stackdepot_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stacktrace_printer_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stacktrace_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_stoptheworld_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_suppressions_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_symbolizer_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_test_main.cc.x86_64.o SANITIZER_TEST_OBJECTS.sanitizer_thread_registry_test.cc.x86_64.o SANITIZER_TEST_OBJECTS.gtest-all.cc.x86_64.o C:/src/chromium/src/third_party/llvm-bootstrap/projects/compiler-rt/lib/sanitizer_common/tests/RTSanitizerCommon.test.x86_64.lib /STACK:0xC00000
clang.exe: error: unable to execute command: program not executable
clang.exe: error: linker command failed with exit code 1 (use -v to see invocation)


But just running link.exe manually seems to work fine:

C:\src\chromium\src\third_party\llvm-bootstrap>link.exe /?
Microsoft (R) Incremental Linker Version 14.11.25507.1
Copyright (C) Microsoft Corporation.  All rights reserved.

 usage: LINK [options] [files] [@commandfile]
...




I think I've seen that "program not executable" error somewhere before though. Maybe clang is doing something special when it tries to execute it, and SetEnv changed slightly so that it no longer works.

Or maybe it's Clang's "detect vs2017 location" that's somehow broken.

> InstalledDir: C:\src\chromium\src\third_party\llvm-bootstrap\bin
> "link.exe" -out:C:/src/chromium/src/third_party/llvm-bootstrap/projects/compil

This looks wrong. It's supposed to use the full path to link.exe.

Yeah, I think this is a problem with Clang's findVCToolChainViaEnvironment():


// Check various environment variables to try and find a toolchain.
static bool findVCToolChainViaEnvironment(std::string &Path,
                                          MSVCToolChain::ToolsetLayout &VSLayout) {
  // These variables are typically set by vcvarsall.bat
  // when launching a developer command prompt.
  if (llvm::Optional<std::string> VCToolsInstallDir =
          llvm::sys::Process::GetEnv("VCToolsInstallDir")) {
    // This is only set by newer Visual Studios, and it leads straight to
    // the toolchain directory.
    Path = std::move(*VCToolsInstallDir);
    VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
    return true;
  }
  if (llvm::Optional<std::string> VCInstallDir =
          llvm::sys::Process::GetEnv("VCINSTALLDIR")) {
    // If the previous variable isn't set but this one is, then we've found
    // an older Visual Studio. This variable is set by newer Visual Studios too,
    // so this check has to appear second.
    // In older Visual Studios, the VC directory is the toolchain.
    Path = std::move(*VCInstallDir);
    VSLayout = MSVCToolChain::ToolsetLayout::OlderVS; 
    return true;
  }


Our SetEnv.cmd doesn't set VCToolsInstallDir, so we end up in the VCINSTALLDIR branch, which I think makes us assume pre-2017 dir layout.

We could either set that, or improve clang's logic.

I now vaguely remember that we had a similar problem earlier: https://codereview.chromium.org/1604423002

The Right Fix is probably set VCToolsInstallDir in our script, since that identifies 2017. Bruce, does that sound right to you? Is it doable to push new 2017 packages with that var set?

We could do something like this on the Clang which will make it fall back to the PATH search. It feels a bit icky, but I think it'd do the trick:


diff --git a/lib/Driver/ToolChains/MSVC.cpp b/lib/Driver/ToolChains/MSVC.cpp
index ae41ee9e22..6f9c7dccef 100644
--- a/lib/Driver/ToolChains/MSVC.cpp
+++ b/lib/Driver/ToolChains/MSVC.cpp
@@ -92,10 +92,16 @@ static bool findVCToolChainViaEnvironment(std::string &Path,
     // If the previous variable isn't set but this one is, then we've found
     // an older Visual Studio. This variable is set by newer Visual Studios too,
     // so this check has to appear second.
-    // In older Visual Studios, the VC directory is the toolchain.
-    Path = std::move(*VCInstallDir);
-    VSLayout = MSVCToolChain::ToolsetLayout::OlderVS;
-    return true;
+    // In older Visual Studios, the VC directory is the toolchain, so we expect
+    // to find bin/cl.exe here.
+    llvm::SmallString<256> ExeTestPath = VCInstallDir;
+    llvm::sys::path::append(ExeTestPath, "bin");
+    llvm::sys::path::append(ExeTestPath, "cl.exe");
+    if (llvm::sys::fs::exists(ExeTestPath)) {
+      Path = std::move(*VCInstallDir);
+      VSLayout = MSVCToolChain::ToolsetLayout::OlderVS;
+      return true;
+    }
   }
 
   // We couldn't find any VC environment variables. Let's walk through PATH and

I think it's better if we try to make the chrome MSVC env setup more similar to the "real" env setup, instead of teaching clang about our weird one-off msvc configuration.

https://chromium-review.googlesource.com/#/c/chromium/tools/depot_tools/+/709697  might do this.

Bruce: What's the best way to test changes to that script locally?

Assuming the change works, can we build a new toolchain zip ourselves somehow? How?

You can make local changes to that script and they won't be destroyed as long as you don't run "gclient runhooks". In order to get them noticed by a particular output directory you probably have to do a gn clean.

As for building a new toolchain it is probably best for me to do that. Roughly speaking the process is to spin up a VM, install VS, install the debuggers package, and then run depot_tools\win_toolchain\package_from_installed.py. I try to list the steps in every CL I land that changes the hash in order to make it reproducible.

Ideally I would want to make a package that is identical to the current one except for this one change - I might create a new script that can recreate a package with just a spot change. That would avoid changing the compiler which has probably received a few spot updates since I last packaged it.

VS 2017 15.4 (which contains at least two code-gen fixes) just landed so that's actually a good opportunity to package up a new compiler and pull in this fix at the same time. I'll create the package - using the WIP patch - but I won't land the change to the new hash until after the branch point. Good plan?

This blocks clang rolls, so we need something this week. (Wip patch is also untested.)

Is there some way we could push the existing package with just that change ourselves? Having a bus factor of 1 for this process seems suboptimal :-)

I'm sure other people could do the process (scottmg@ used to), I'm just the one most familiar with it.

I'll create a package ASAP. However it will probably use Update 4 which means we'll need goma to add that compiler - I filed b/67641181 for that.

If I can create a package with the previous compiler version I'll do that.

I'm currently trying to package Clang with VS rolled back to 2015 (https://chromium-review.googlesource.com/c/chromium/src/+/710218). If that works out, we're not immediately blocked by this, but it will still be nice to get it fixed of course.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/d4b5b1e68920294c40dc13efb73cf7cafdd3eb36

commit d4b5b1e68920294c40dc13efb73cf7cafdd3eb36
Author: Nico Weber <thakis@chromium.org>
Date: Tue Oct 10 22:08:27 2017

Attempt to set VCToolsInstallDir in SetEnv script.

clang-cl relies on this env var being set for 2017 to find the linker.

Bug:  772123 
Change-Id: Id03896504d38dc706b2bd96a2c3834c6cb9db9fe
TBR=brucedawson
Reviewed-on: https://chromium-review.googlesource.com/709697
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>

[modify] https://crrev.com/d4b5b1e68920294c40dc13efb73cf7cafdd3eb36/win_toolchain/package_from_installed.py

I've created a modified packaging script that will repackage an existing package directory. That makes it trivial to do modifications like this - just change setenv.cmd in place and then re-package that directory. No installation of old VS versions needed, no VM needed.

I assume that an updated packaging of our current VS 2017 is still relevant?

If we could get a 2017 with the change in comment 23, that'd be great. Currently for clang rolls we need to upload a change with https://chromium-review.googlesource.com/c/chromium/src/+/710218/2/build/vs_toolchain.py , send "build package" try jobs, and then undo that change. It'd be great if it just worked with 2017 instead.

crrev.com/c/713441 is running try jobs now and should fix this.

I'll upload a separate CL for the updated packaging script that I used to do this - I should have added this feature a long time ago. Oh well.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5aed7b7bbfb9311aa80b293c8d9e21f099fff776

commit 5aed7b7bbfb9311aa80b293c8d9e21f099fff776
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Wed Oct 11 22:52:33 2017

Repackaged VS 2017 with updated SetEnv.cmd

Clang sanitizer tests don't build with MSVC2017 SDK due to a missing
environment variable. The packaging script has been updated to provide
that in the future but this is just a repackaging of the previous
package with a spot-fix, using a new --repackage option to
win_toolchain/package_from_installed.py.

To test this I did a "gclient runhooks" to pull down the new package
and then did a directory compare. This verified that the only file
that is changed is SetEnv.cmd and the only change there is to add:

set VCToolsInstallDir=%~dp0..\..\VC\Tools\MSVC\14.11.25503\

So, this should give identical results to the previous package except
in the case where VCToolsInstallDir is needed.

Bug:  772123 
Change-Id: I70644d24e306825abbb4245802c7c3234c4b54dc
Reviewed-on: https://chromium-review.googlesource.com/713441
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508151}
[modify] https://crrev.com/5aed7b7bbfb9311aa80b293c8d9e21f099fff776/build/vs_toolchain.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/aacb2ad3c143f67b8f945469b1aca13db0a9f082

commit aacb2ad3c143f67b8f945469b1aca13db0a9f082
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Wed Oct 11 23:27:24 2017

Add --repackage option to VS packaging script

Sometimes it is necessary to recreate a VS toolchain package with a tiny
change such as editing or removing a single file. Recreating the VS
environment can be time consuming or even impossible (the VS installer
does not allow going to arbitrary versions) so this switch allows
repackaging an existing unpacked directory. The typical usage would be
to make the modifications to one of the toolchain directories in
depot_tools\win_toolchain\vs_files and then repackage it using:

    python package_from_installed.py --repackage third_party\depot_tools\win_toolchain\vs_files\9bc7ccbf9f4bd50d4a3bd185e8ca94ff1618de0b

Bug:  772123 
Change-Id: I77b928f695e5f07e33f68dd37711c8761a3c7a22
Reviewed-on: https://chromium-review.googlesource.com/713562
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>

[modify] https://crrev.com/aacb2ad3c143f67b8f945469b1aca13db0a9f082/win_toolchain/package_from_installed.py