[PATCH] Improve Windows toolchain support for non-standard environments.
Aaron Ballman
aaron.ballman at gmail.com
Fri Oct 17 12:48:52 PDT 2014
On Fri, Oct 17, 2014 at 3:06 PM, Zachary Turner <zturner at google.com> wrote:
> Fix an issue with a stack smash.
>
> There is still an outstanding issue remaining after this patch, which is that link cannot locate libraries. I think we can address this in a followup patch by adding some library paths to the link line similar to how we already do for system include paths that we pass to cl.
Thank you for working on this! Some comments below.
>
> http://reviews.llvm.org/D5845
>
> Files:
> lib/Driver/ToolChains.h
> lib/Driver/Tools.cpp
> lib/Driver/WindowsToolChain.cpp
> Index: lib/Driver/ToolChains.h
> ===================================================================
> --- lib/Driver/ToolChains.h
> +++ lib/Driver/ToolChains.h
> @@ -746,6 +746,11 @@
> llvm::opt::ArgStringList &CC1Args) const override;
>
> protected:
> + void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList &DriverArgs,
> + llvm::opt::ArgStringList &CC1Args,
> + const std::string &folder,
> + const char *subfolder) const;
> +
> Tool *buildLinker() const override;
> Tool *buildAssembler() const override;
> };
> Index: lib/Driver/Tools.cpp
> ===================================================================
> --- lib/Driver/Tools.cpp
> +++ lib/Driver/Tools.cpp
> @@ -7803,6 +7803,41 @@
> CmdArgs.push_back(Args.MakeArgString(LibSanitizer));
> }
>
> +// Try to find FallbackName on PATH that is not identical to ClangProgramPath.
> +// If one cannot be found, return FallbackName.
> +// We do this special search to prevent clang-cl from falling back onto itself
> +// if it's available as cl.exe on the path.
> +static std::string FindFallback(const char *FallbackName,
> + const char *ClangProgramPath) {
> + // TODO(zturner): Maybe instead of checking the PATH this should use
> + // |toolchains::Windows::getVisualStudioDir()|\bin. This would make clang
> + // smarter in the case of running clang without first running vcvarsall, and
> + // also make behavior more consistent so that, for example, we don't find
> + // system includes and libraries from one VS installation, and use cl and link
> + // from another installation.
> + llvm::Optional<std::string> OptPath = llvm::sys::Process::GetEnv("PATH");
> + if (!OptPath.hasValue())
> + return FallbackName;
> +
> + const char EnvPathSeparatorStr[] = {llvm::sys::EnvPathSeparator, '\0'};
> + SmallVector<StringRef, 8> PathSegments;
> + llvm::SplitString(OptPath.getValue(), PathSegments, EnvPathSeparatorStr);
> +
> + for (size_t i = 0, e = PathSegments.size(); i != e; ++i) {
Range-based for loop?
> + StringRef PathSegment = PathSegments[i];
> + if (PathSegment.empty())
> + continue;
> +
> + SmallString<128> FilePath(PathSegment);
> + llvm::sys::path::append(FilePath, FallbackName);
> + if (llvm::sys::fs::can_execute(Twine(FilePath)) &&
> + !llvm::sys::fs::equivalent(Twine(FilePath), ClangProgramPath))
> + return FilePath.str();
> + }
> +
> + return FallbackName;
> +}
> +
> void visualstudio::Link::ConstructJob(Compilation &C, const JobAction &JA,
> const InputInfo &Output,
> const InputInfoList &Inputs,
> @@ -7888,8 +7923,19 @@
> A.renderAsInput(Args, CmdArgs);
> }
>
> - const char *Exec =
> - Args.MakeArgString(getToolChain().GetProgramPath("link.exe"));
> + // Make sure we're using link.exe and cl.exe from the same Visual Studio.
> + // Otherwise its possible
> + // that they may come from different locations, for example if GnuWin32 is in
> + // the path.
The formatting of this comment is a bit wonky.
> + llvm::SmallString<128> smartPath(
> + FindFallback("cl.exe", C.getDriver().getClangProgramPath()));
> + llvm::sys::path::remove_filename(smartPath);
> + llvm::sys::path::append(smartPath, "link.exe");
> + const char *Exec = nullptr;
> + if (llvm::sys::fs::can_execute(smartPath.c_str()))
> + Exec = Args.MakeArgString(smartPath);
This has me slightly concerned about TOCTOU issues. Just because it's
executable now doesn't mean it will be by the time we attempt to
actually execute it. If we find something named link.exe that's not
executable, I suspect we should simply error out when the execution
fails, instead of testing that early.
> + else
> + Exec = Args.MakeArgString(getToolChain().GetProgramPath("link.exe"));
> C.addCommand(llvm::make_unique<Command>(JA, *this, Exec, CmdArgs));
> }
>
> @@ -7901,35 +7947,6 @@
> C.addCommand(GetCommand(C, JA, Output, Inputs, Args, LinkingOutput));
> }
>
> -// Try to find FallbackName on PATH that is not identical to ClangProgramPath.
> -// If one cannot be found, return FallbackName.
> -// We do this special search to prevent clang-cl from falling back onto itself
> -// if it's available as cl.exe on the path.
> -static std::string FindFallback(const char *FallbackName,
> - const char *ClangProgramPath) {
> - llvm::Optional<std::string> OptPath = llvm::sys::Process::GetEnv("PATH");
> - if (!OptPath.hasValue())
> - return FallbackName;
> -
> - const char EnvPathSeparatorStr[] = {llvm::sys::EnvPathSeparator, '\0'};
> - SmallVector<StringRef, 8> PathSegments;
> - llvm::SplitString(OptPath.getValue(), PathSegments, EnvPathSeparatorStr);
> -
> - for (size_t i = 0, e = PathSegments.size(); i != e; ++i) {
> - StringRef PathSegment = PathSegments[i];
> - if (PathSegment.empty())
> - continue;
> -
> - SmallString<128> FilePath(PathSegment);
> - llvm::sys::path::append(FilePath, FallbackName);
> - if (llvm::sys::fs::can_execute(Twine(FilePath)) &&
> - !llvm::sys::fs::equivalent(Twine(FilePath), ClangProgramPath))
> - return FilePath.str();
> - }
> -
> - return FallbackName;
> -}
> -
> std::unique_ptr<Command> visualstudio::Compile::GetCommand(
> Compilation &C, const JobAction &JA, const InputInfo &Output,
> const InputInfoList &Inputs, const ArgList &Args,
> Index: lib/Driver/WindowsToolChain.cpp
> ===================================================================
> --- lib/Driver/WindowsToolChain.cpp
> +++ lib/Driver/WindowsToolChain.cpp
> @@ -77,61 +77,59 @@
> return getArch() == llvm::Triple::x86_64;
> }
>
> +#ifdef USE_WIN32
> +static bool readFullStringValue(HKEY hkey, const char *valueName,
> + std::string &value) {
We should be preferring the W versions of these APIs instead of the A
versions, especially since this is being used to pull out file paths.
> + DWORD result = 0;
> + DWORD valueSize = 0;
> + // First just query for the required size.
> + result = RegQueryValueEx(hkey, valueName, NULL, NULL, NULL, &valueSize);
We should be caring about the key type as well, since we only want
string values.
> + if (result != ERROR_SUCCESS)
> + return false;
> + std::vector<BYTE> buffer(valueSize);
> + result = RegQueryValueEx(hkey, valueName, NULL, NULL, &buffer[0], &valueSize);
> + if (result == ERROR_SUCCESS)
> + value.assign(reinterpret_cast<const char *>(buffer.data()));
> + return result;
> +}
> +#endif
> +
> /// \brief Read registry string.
> /// This also supports a means to look for high-versioned keys by use
> /// of a $VERSION placeholder in the key path.
> /// $VERSION in the key path is a placeholder for the version number,
> /// causing the highest value path to be searched for and used.
> -/// I.e. "HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\VisualStudio\\$VERSION".
> +/// I.e. "SOFTWARE\\Microsoft\\VisualStudio\\$VERSION".
> /// There can be additional characters in the component. Only the numberic
I know this isn't your typo, but can you fix "numberic"? ;-)
> -/// characters are compared.
> +/// characters are compared. This function only searches HKLM.
> static bool getSystemRegistryString(const char *keyPath, const char *valueName,
> - char *value, size_t maxLength) {
> + std::string &value, std::string *phValue) {
> #ifndef USE_WIN32
> return false;
> #else
> - HKEY hRootKey = NULL;
> + HKEY hRootKey = HKEY_LOCAL_MACHINE;
> HKEY hKey = NULL;
> - const char* subKey = NULL;
> - DWORD valueType;
> - DWORD valueSize = maxLength - 1;
> + DWORD valueSize = 0;
> long lResult;
> bool returnValue = false;
>
> - if (strncmp(keyPath, "HKEY_CLASSES_ROOT\\", 18) == 0) {
> - hRootKey = HKEY_CLASSES_ROOT;
> - subKey = keyPath + 18;
> - } else if (strncmp(keyPath, "HKEY_USERS\\", 11) == 0) {
> - hRootKey = HKEY_USERS;
> - subKey = keyPath + 11;
> - } else if (strncmp(keyPath, "HKEY_LOCAL_MACHINE\\", 19) == 0) {
> - hRootKey = HKEY_LOCAL_MACHINE;
> - subKey = keyPath + 19;
> - } else if (strncmp(keyPath, "HKEY_CURRENT_USER\\", 18) == 0) {
> - hRootKey = HKEY_CURRENT_USER;
> - subKey = keyPath + 18;
> - } else {
> - return false;
> - }
> -
> - const char *placeHolder = strstr(subKey, "$VERSION");
> - char bestName[256];
> - bestName[0] = '\0';
> + const char *placeHolder = strstr(keyPath, "$VERSION");
> + std::string bestName;
> // If we have a $VERSION placeholder, do the highest-version search.
> if (placeHolder) {
> const char *keyEnd = placeHolder - 1;
> const char *nextKey = placeHolder;
> // Find end of previous key.
> - while ((keyEnd > subKey) && (*keyEnd != '\\'))
> + while ((keyEnd > keyPath) && (*keyEnd != '\\'))
> keyEnd--;
> // Find end of key containing $VERSION.
> while (*nextKey && (*nextKey != '\\'))
> nextKey++;
> - size_t partialKeyLength = keyEnd - subKey;
> + size_t partialKeyLength = keyEnd - keyPath;
> char partialKey[256];
> if (partialKeyLength > sizeof(partialKey))
> partialKeyLength = sizeof(partialKey);
> - strncpy(partialKey, subKey, partialKeyLength);
> + strncpy(partialKey, keyPath, partialKeyLength);
> partialKey[partialKeyLength] = '\0';
> HKEY hTopKey = NULL;
> lResult = RegOpenKeyEx(hRootKey, partialKey, 0, KEY_READ | KEY_WOW64_32KEY,
> @@ -158,18 +156,18 @@
> if (dvalue > bestValue) {
> // Test that InstallDir is indeed there before keeping this index.
> // Open the chosen key path remainder.
> - strcpy(bestName, keyName);
> + bestName = keyName;
> // Append rest of key.
> - strncat(bestName, nextKey, sizeof(bestName) - 1);
> - bestName[sizeof(bestName) - 1] = '\0';
> - lResult = RegOpenKeyEx(hTopKey, bestName, 0,
> + bestName.append(nextKey);
> + lResult = RegOpenKeyEx(hTopKey, bestName.c_str(), 0,
> KEY_READ | KEY_WOW64_32KEY, &hKey);
> if (lResult == ERROR_SUCCESS) {
> - lResult = RegQueryValueEx(hKey, valueName, NULL, &valueType,
> - (LPBYTE)value, &valueSize);
> + lResult = readFullStringValue(hKey, valueName, value);
> if (lResult == ERROR_SUCCESS) {
> bestIndex = (int)index;
> bestValue = dvalue;
> + if (phValue)
> + *phValue = bestName;
> returnValue = true;
> }
> RegCloseKey(hKey);
> @@ -180,83 +178,74 @@
> RegCloseKey(hTopKey);
> }
> } else {
> - lResult = RegOpenKeyEx(hRootKey, subKey, 0, KEY_READ | KEY_WOW64_32KEY,
> - &hKey);
> + lResult =
> + RegOpenKeyEx(hRootKey, keyPath, 0, KEY_READ | KEY_WOW64_32KEY, &hKey);
> if (lResult == ERROR_SUCCESS) {
> - lResult = RegQueryValueEx(hKey, valueName, NULL, &valueType,
> - (LPBYTE)value, &valueSize);
> + lResult = readFullStringValue(hKey, valueName, value);
> if (lResult == ERROR_SUCCESS)
> returnValue = true;
> + if (phValue)
> + phValue->clear();
> RegCloseKey(hKey);
> }
> }
> return returnValue;
> #endif // USE_WIN32
> }
>
> /// \brief Get Windows SDK installation directory.
> -static bool getWindowsSDKDir(std::string &path) {
> - char windowsSDKInstallDir[256];
> +static bool getWindowsSDKDir(std::string &path, int &major, int &minor) {
Same worries here about Unicode paths as elsewhere. Note, I don't
think you have to fix that with this patch, but if we're in there,
this should be fixed (because those paths are user-configurable).
> + std::string sdkVersion;
> // Try the Windows registry.
> bool hasSDKDir = getSystemRegistryString(
> - "HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Microsoft SDKs\\Windows\\$VERSION",
> - "InstallationFolder",
> - windowsSDKInstallDir,
> - sizeof(windowsSDKInstallDir) - 1);
> - // If we have both vc80 and vc90, pick version we were compiled with.
> - if (hasSDKDir && windowsSDKInstallDir[0]) {
> - path = windowsSDKInstallDir;
> - return true;
> - }
> - return false;
> + "SOFTWARE\\Microsoft\\Microsoft SDKs\\Windows\\$VERSION",
> + "InstallationFolder", path, &sdkVersion);
> + if (!sdkVersion.empty())
> + ::sscanf(sdkVersion.c_str(), "v%d.%d", &major, &minor);
> + return hasSDKDir && !path.empty();
> }
>
> // Get Visual Studio installation directory.
> static bool getVisualStudioDir(std::string &path) {
> // First check the environment variables that vsvars32.bat sets.
> - const char* vcinstalldir = getenv("VCINSTALLDIR");
> + const char *vcinstalldir = getenv("VCINSTALLDIR");
> if (vcinstalldir) {
> - char *p = const_cast<char *>(strstr(vcinstalldir, "\\VC"));
> - if (p)
> - *p = '\0';
> path = vcinstalldir;
> + path = path.substr(0, path.find("\\VC"));
> return true;
> }
>
> - char vsIDEInstallDir[256];
> - char vsExpressIDEInstallDir[256];
> + std::string vsIDEInstallDir;
> + std::string vsExpressIDEInstallDir;
> // Then try the windows registry.
> - bool hasVCDir = getSystemRegistryString(
> - "HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\VisualStudio\\$VERSION",
> - "InstallDir", vsIDEInstallDir, sizeof(vsIDEInstallDir) - 1);
> - bool hasVCExpressDir = getSystemRegistryString(
> - "HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\VCExpress\\$VERSION",
> - "InstallDir", vsExpressIDEInstallDir, sizeof(vsExpressIDEInstallDir) - 1);
> - // If we have both vc80 and vc90, pick version we were compiled with.
> - if (hasVCDir && vsIDEInstallDir[0]) {
> - char *p = (char*)strstr(vsIDEInstallDir, "\\Common7\\IDE");
> - if (p)
> - *p = '\0';
> - path = vsIDEInstallDir;
> + bool hasVCDir =
> + getSystemRegistryString("SOFTWARE\\Microsoft\\VisualStudio\\$VERSION",
> + "InstallDir", vsIDEInstallDir, nullptr);
> + if (hasVCDir && !vsIDEInstallDir.empty()) {
> + path = vsIDEInstallDir.substr(0, vsIDEInstallDir.find("\\Common7\\IDE"));
> return true;
> }
>
> - if (hasVCExpressDir && vsExpressIDEInstallDir[0]) {
> - char *p = (char*)strstr(vsExpressIDEInstallDir, "\\Common7\\IDE");
> - if (p)
> - *p = '\0';
> - path = vsExpressIDEInstallDir;
> + bool hasVCExpressDir =
> + getSystemRegistryString("SOFTWARE\\Microsoft\\VCExpress\\$VERSION",
> + "InstallDir", vsExpressIDEInstallDir, nullptr);
> + if (hasVCExpressDir && !vsExpressIDEInstallDir.empty()) {
> + path = vsExpressIDEInstallDir.substr(
> + 0, vsIDEInstallDir.find("\\Common7\\IDE"));
> return true;
> }
>
> // Try the environment.
> + const char *vs120comntools = getenv("VS120COMNTOOLS");
> const char *vs100comntools = getenv("VS100COMNTOOLS");
> const char *vs90comntools = getenv("VS90COMNTOOLS");
> const char *vs80comntools = getenv("VS80COMNTOOLS");
>
> const char *vscomntools = nullptr;
>
> // Find any version we can
> + if (vs120comntools)
> + vscomntools = vs120comntools;
> if (vs100comntools)
This should become an else if instead of just an if.
> vscomntools = vs100comntools;
> else if (vs90comntools)
> @@ -272,6 +261,15 @@
> return false;
> }
>
> +void Windows::AddSystemIncludeWithSubfolder(const ArgList &DriverArgs,
> + ArgStringList &CC1Args,
> + const std::string &folder,
> + const char *subfolder) const {
> + llvm::SmallString<128> path(folder);
> + llvm::sys::path::append(path, subfolder);
> + addSystemInclude(DriverArgs, CC1Args, path.str());
> +}
> +
> void Windows::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
> ArgStringList &CC1Args) const {
> if (DriverArgs.hasArg(options::OPT_nostdinc))
> @@ -298,23 +296,28 @@
> }
>
> std::string VSDir;
> - std::string WindowsSDKDir;
>
> // When built with access to the proper Windows APIs, try to actually find
> // the correct include paths first.
> if (getVisualStudioDir(VSDir)) {
> - SmallString<128> P;
> - P = VSDir;
> - llvm::sys::path::append(P, "VC\\include");
> - addSystemInclude(DriverArgs, CC1Args, P.str());
> - if (getWindowsSDKDir(WindowsSDKDir)) {
> - P = WindowsSDKDir;
> - llvm::sys::path::append(P, "include");
> - addSystemInclude(DriverArgs, CC1Args, P.str());
> + AddSystemIncludeWithSubfolder(DriverArgs, CC1Args, VSDir, "VC\\include");
> +
> + std::string WindowsSDKDir;
> + int major, minor;
> + if (getWindowsSDKDir(WindowsSDKDir, major, minor)) {
> + if (major >= 8) {
> + AddSystemIncludeWithSubfolder(DriverArgs, CC1Args, WindowsSDKDir,
> + "include\\shared");
> + AddSystemIncludeWithSubfolder(DriverArgs, CC1Args, WindowsSDKDir,
> + "include\\um");
> + AddSystemIncludeWithSubfolder(DriverArgs, CC1Args, WindowsSDKDir,
> + "include\\winrt");
> + } else {
> + AddSystemIncludeWithSubfolder(DriverArgs, CC1Args, WindowsSDKDir,
> + "include");
> + }
> } else {
> - P = VSDir;
> - llvm::sys::path::append(P, "VC\\PlatformSDK\\Include");
> - addSystemInclude(DriverArgs, CC1Args, P.str());
> + addSystemInclude(DriverArgs, CC1Args, VSDir);
> }
> return;
> }
>
~Aaron
More information about the cfe-commits
mailing list