[PATCH] D49398: clang-cl: Postpone Wmsvc-not-found emission until link.exe gets used.
thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. r337290, thanks! https://reviews.llvm.org/D49398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49398: clang-cl: Postpone Wmsvc-not-found emission until link.exe gets used.
zturner added a comment. Lgtm. I also have a hard time saying which is best, we’re basically trying to help people who have already strayed from the path, so there’s probably no objectively correct answer https://reviews.llvm.org/D49398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D49398: clang-cl: Postpone Wmsvc-not-found emission until link.exe gets used.
Lgtm. I also have a hard time saying which is best, we’re basically trying
to help people who have already strayed from the path, so there’s probably
no objectively correct answer
On Tue, Jul 17, 2018 at 7:26 AM Nico Weber via Phabricator <
revi...@reviews.llvm.org> wrote:
> thakis updated this revision to Diff 155885.
> thakis added a comment.
>
> address comment
>
>
> https://reviews.llvm.org/D49398
>
> Files:
> lib/Driver/ToolChains/MSVC.cpp
> lib/Driver/ToolChains/MSVC.h
>
>
> Index: lib/Driver/ToolChains/MSVC.h
> ===
> --- lib/Driver/ToolChains/MSVC.h
> +++ lib/Driver/ToolChains/MSVC.h
> @@ -123,6 +123,8 @@
>
>void printVerboseInfo(raw_ostream ) const override;
>
> + bool FoundMSVCInstall() const { return !VCToolChainPath.empty(); }
> +
> protected:
>void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList ,
> llvm::opt::ArgStringList ,
> Index: lib/Driver/ToolChains/MSVC.cpp
> ===
> --- lib/Driver/ToolChains/MSVC.cpp
> +++ lib/Driver/ToolChains/MSVC.cpp
> @@ -469,6 +469,9 @@
> // their own link.exe which may come first.
> linkPath = FindVisualStudioExecutable(TC, "link.exe");
>
> +if (!TC.FoundMSVCInstall() && !llvm::sys::fs::can_execute(linkPath))
> + C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
> +
> #ifdef _WIN32
> // When cross-compiling with VS2017 or newer, link.exe expects to have
> // its containing bin directory at the top of PATH, followed by the
> @@ -684,8 +687,6 @@
> }
>
> Tool *MSVCToolChain::buildLinker() const {
> - if (VCToolChainPath.empty())
> -getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
>return new tools::visualstudio::Linker(*this);
> }
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49398: clang-cl: Postpone Wmsvc-not-found emission until link.exe gets used.
thakis updated this revision to Diff 155885.
thakis added a comment.
address comment
https://reviews.llvm.org/D49398
Files:
lib/Driver/ToolChains/MSVC.cpp
lib/Driver/ToolChains/MSVC.h
Index: lib/Driver/ToolChains/MSVC.h
===
--- lib/Driver/ToolChains/MSVC.h
+++ lib/Driver/ToolChains/MSVC.h
@@ -123,6 +123,8 @@
void printVerboseInfo(raw_ostream ) const override;
+ bool FoundMSVCInstall() const { return !VCToolChainPath.empty(); }
+
protected:
void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ,
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -469,6 +469,9 @@
// their own link.exe which may come first.
linkPath = FindVisualStudioExecutable(TC, "link.exe");
+if (!TC.FoundMSVCInstall() && !llvm::sys::fs::can_execute(linkPath))
+ C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+
#ifdef _WIN32
// When cross-compiling with VS2017 or newer, link.exe expects to have
// its containing bin directory at the top of PATH, followed by the
@@ -684,8 +687,6 @@
}
Tool *MSVCToolChain::buildLinker() const {
- if (VCToolChainPath.empty())
-getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
return new tools::visualstudio::Linker(*this);
}
Index: lib/Driver/ToolChains/MSVC.h
===
--- lib/Driver/ToolChains/MSVC.h
+++ lib/Driver/ToolChains/MSVC.h
@@ -123,6 +123,8 @@
void printVerboseInfo(raw_ostream ) const override;
+ bool FoundMSVCInstall() const { return !VCToolChainPath.empty(); }
+
protected:
void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ,
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -469,6 +469,9 @@
// their own link.exe which may come first.
linkPath = FindVisualStudioExecutable(TC, "link.exe");
+if (!TC.FoundMSVCInstall() && !llvm::sys::fs::can_execute(linkPath))
+ C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+
#ifdef _WIN32
// When cross-compiling with VS2017 or newer, link.exe expects to have
// its containing bin directory at the top of PATH, followed by the
@@ -684,8 +687,6 @@
}
Tool *MSVCToolChain::buildLinker() const {
- if (VCToolChainPath.empty())
-getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
return new tools::visualstudio::Linker(*this);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49398: clang-cl: Postpone Wmsvc-not-found emission until link.exe gets used.
thakis added inline comments.
Comment at: lib/Driver/ToolChains/MSVC.cpp:467-468
if (Linker.equals_lower("link")) {
+if (!TC.FoundMSVCInstall())
+ C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+
zturner wrote:
> It looks like it's possible for this warning to be emitted even when
> `FindVisualStudioExecutable` succeeds (after looking in the install location
> it checks `PATH`). Would it make more sense to put this check after the call
> to `FindVisualStudioExecutable`, but only if it fails?
If we find link.exe on PATH but LIB isn't set, will link.exe do the right
thing? Or is the warning still useful for that case?
I'll upload a patch that does what you suggest, but since I'm not sure what
exactly the warning is supposed to catch, I find it difficult to say which
version is better.
https://reviews.llvm.org/D49398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49398: clang-cl: Postpone Wmsvc-not-found emission until link.exe gets used.
zturner added inline comments.
Comment at: lib/Driver/ToolChains/MSVC.cpp:467-468
if (Linker.equals_lower("link")) {
+if (!TC.FoundMSVCInstall())
+ C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+
It looks like it's possible for this warning to be emitted even when
`FindVisualStudioExecutable` succeeds (after looking in the install location it
checks `PATH`). Would it make more sense to put this check after the call to
`FindVisualStudioExecutable`, but only if it fails?
https://reviews.llvm.org/D49398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49398: clang-cl: Postpone Wmsvc-not-found emission until link.exe gets used.
thakis created this revision.
thakis added a reviewer: zturner.
Wmsvc-not-found was added in r297851 to help diagnose why link.exe can't be
executed. However, it's emitted even when using -fuse-ld=lld, and in cross
builds there's no way to get rid of the warning other than disabling it.
Instead, emit it when we look up link.exe. That way, when passing -fuse-ld=lld
it will never be printed.
(We might want to eventually default to lld one day, at least when running on a
non-Win host, but that's for another day.)
Fixes PR38016.
https://reviews.llvm.org/D49398
Files:
lib/Driver/ToolChains/MSVC.cpp
lib/Driver/ToolChains/MSVC.h
Index: lib/Driver/ToolChains/MSVC.h
===
--- lib/Driver/ToolChains/MSVC.h
+++ lib/Driver/ToolChains/MSVC.h
@@ -123,6 +123,8 @@
void printVerboseInfo(raw_ostream ) const override;
+ bool FoundMSVCInstall() const { return !VCToolChainPath.empty(); }
+
protected:
void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ,
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -464,6 +464,9 @@
Linker = "lld-link";
if (Linker.equals_lower("link")) {
+if (!TC.FoundMSVCInstall())
+ C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+
// If we're using the MSVC linker, it's not sufficient to just use link
// from the program PATH, because other environments like GnuWin32 install
// their own link.exe which may come first.
@@ -684,8 +687,6 @@
}
Tool *MSVCToolChain::buildLinker() const {
- if (VCToolChainPath.empty())
-getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
return new tools::visualstudio::Linker(*this);
}
Index: lib/Driver/ToolChains/MSVC.h
===
--- lib/Driver/ToolChains/MSVC.h
+++ lib/Driver/ToolChains/MSVC.h
@@ -123,6 +123,8 @@
void printVerboseInfo(raw_ostream ) const override;
+ bool FoundMSVCInstall() const { return !VCToolChainPath.empty(); }
+
protected:
void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ,
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -464,6 +464,9 @@
Linker = "lld-link";
if (Linker.equals_lower("link")) {
+if (!TC.FoundMSVCInstall())
+ C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+
// If we're using the MSVC linker, it's not sufficient to just use link
// from the program PATH, because other environments like GnuWin32 install
// their own link.exe which may come first.
@@ -684,8 +687,6 @@
}
Tool *MSVCToolChain::buildLinker() const {
- if (VCToolChainPath.empty())
-getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
return new tools::visualstudio::Linker(*this);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
