[PATCH] D131730: __has_trivial_copy should map to __is_trivially_copyable

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

In D131730#3718430 , @royjacobson 
wrote:

> Could you add name+email for the commit message?

Zachary Henkel
za...@microsoft.com
Github Username: ZacharyHenkel


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131730

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131730: __has_trivial_copy should map to __is_trivially_copyable

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

In D131730#3718921 , @erichkeane 
wrote:

> Did @royjacobson 's patch make it to the 15.0 branch?  If so, Roy: After this 
> is committed, can you file to get this merged into the 15.x branch?  This 
> seems trivial enough (HAH!) of a fix that it is zero risk, and also keeps it 
> from being all that embarrassing here.

Yes this is definitely in 15.  During RC testing the diagnostic fired.  The 
test build where I followed the suggestion and replaced __has_trivial_copy with 
__is_trivially_constructible didn't go very well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131730

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131730: __has_trivial_copy should map to __is_trivially_copyable

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added reviewers: royjacobson, aaron.ballman, cor3ntin, erichkeane.
Herald added a project: All.
zahen requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Found during clang 15 RC1 testing due to the new diagnostic added by 
@royjacobson since clang 14.  Uncertain if this fix meets the bar to also be 
applied to the release branch.

If accepted, I'll need someone with commit access to submit on my behalf.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131730

Files:
  clang/docs/LanguageExtensions.rst
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/SemaCXX/deprecated-builtins.cpp


Index: clang/test/SemaCXX/deprecated-builtins.cpp
===
--- clang/test/SemaCXX/deprecated-builtins.cpp
+++ clang/test/SemaCXX/deprecated-builtins.cpp
@@ -11,7 +11,7 @@
 a = __has_nothrow_constructor(A);  // expected-warning-re 
{{__has_nothrow_constructor {{.*}} use __is_nothrow_constructible}}
 a = __has_trivial_assign(A);  // expected-warning-re 
{{__has_trivial_assign {{.*}} use __is_trivially_assignable}}
 a = __has_trivial_move_assign(A);  // expected-warning-re 
{{__has_trivial_move_assign {{.*}} use __is_trivially_assignable}}
-a = __has_trivial_copy(A);  // expected-warning-re {{__has_trivial_copy 
{{.*}} use __is_trivially_constructible}}
+a = __has_trivial_copy(A);  // expected-warning-re {{__has_trivial_copy 
{{.*}} use __is_trivially_copyable}}
 a = __has_trivial_constructor(A);  // expected-warning-re 
{{__has_trivial_constructor {{.*}} use __is_trivially_constructible}}
 a = __has_trivial_move_constructor(A);  // expected-warning-re 
{{__has_trivial_move_constructor {{.*}} use __is_trivially_constructible}}
 a = __has_trivial_destructor(A);  // expected-warning-re 
{{__has_trivial_destructor {{.*}} use __is_trivially_destructible}}
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -5412,6 +5412,8 @@
   Replacement = BTT_IsTriviallyAssignable;
   break;
 case UTT_HasTrivialCopy:
+  Replacement = UTT_IsTriviallyCopyable;
+  break;
 case UTT_HasTrivialDefaultConstructor:
 case UTT_HasTrivialMoveConstructor:
   Replacement = TT_IsTriviallyConstructible;
Index: clang/docs/LanguageExtensions.rst
===
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -1373,7 +1373,7 @@
 * ``__has_trivial_move_assign`` (GNU, Microsoft):
   Deprecated, use ``__is_trivially_assignable`` instead.
 * ``__has_trivial_copy`` (GNU, Microsoft):
-  Deprecated, use ``__is_trivially_constructible`` instead.
+  Deprecated, use ``__is_trivially_copyable`` instead.
 * ``__has_trivial_constructor`` (GNU, Microsoft):
   Deprecated, use ``__is_trivially_constructible`` instead.
 * ``__has_trivial_move_constructor`` (GNU, Microsoft):


Index: clang/test/SemaCXX/deprecated-builtins.cpp
===
--- clang/test/SemaCXX/deprecated-builtins.cpp
+++ clang/test/SemaCXX/deprecated-builtins.cpp
@@ -11,7 +11,7 @@
 a = __has_nothrow_constructor(A);  // expected-warning-re {{__has_nothrow_constructor {{.*}} use __is_nothrow_constructible}}
 a = __has_trivial_assign(A);  // expected-warning-re {{__has_trivial_assign {{.*}} use __is_trivially_assignable}}
 a = __has_trivial_move_assign(A);  // expected-warning-re {{__has_trivial_move_assign {{.*}} use __is_trivially_assignable}}
-a = __has_trivial_copy(A);  // expected-warning-re {{__has_trivial_copy {{.*}} use __is_trivially_constructible}}
+a = __has_trivial_copy(A);  // expected-warning-re {{__has_trivial_copy {{.*}} use __is_trivially_copyable}}
 a = __has_trivial_constructor(A);  // expected-warning-re {{__has_trivial_constructor {{.*}} use __is_trivially_constructible}}
 a = __has_trivial_move_constructor(A);  // expected-warning-re {{__has_trivial_move_constructor {{.*}} use __is_trivially_constructible}}
 a = __has_trivial_destructor(A);  // expected-warning-re {{__has_trivial_destructor {{.*}} use __is_trivially_destructible}}
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -5412,6 +5412,8 @@
   Replacement = BTT_IsTriviallyAssignable;
   break;
 case UTT_HasTrivialCopy:
+  Replacement = UTT_IsTriviallyCopyable;
+  break;
 case UTT_HasTrivialDefaultConstructor:
 case UTT_HasTrivialMoveConstructor:
   Replacement = TT_IsTriviallyConstructible;
Index: clang/docs/LanguageExtensions.rst
===
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensio

[PATCH] D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

In D101775#2791938 , @dexonsmith 
wrote:

> @zahen , what author information should I use for the Git commit?

Zachary Henkel, za...@microsoft.com (GitHub username ZacharyHenkel)

Thanks Duncan!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101775

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

@dexonsmith anything else required before this can land?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101775

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

> If no one has ideas in the next few days, I'll "accept" and commit for you. 
> (Feel free to ping me; there's a good chance I'll lose track of this 
> otherwise; usual ping rate is 1x/week, but happy for you to ping me sooner on 
> this...)

@dexonsmith Ping as requested.  Unfortunately I haven't come up with any way to 
get this tested in a cross-platform manner.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101775

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101776: Work around an unfortunate macro in the Windows SDK

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

In D101776#2736003 , @aaron.ballman 
wrote:

>> Once accepted I'll need someone to commit the change on my behalf.
>
> Thanks for mentioning this up front! What email address and name would you 
> like to have used for attribution on the commit?

Zachary Henkel, za...@microsoft.com (GitHub username ZacharyHenkel)


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

https://reviews.llvm.org/D101776

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101776: Work around an unfortunate macro in the Windows SDK

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 342715.
zahen added a comment.

Updated the macro based on Aaron's suggestion.  clang-formatted the function.


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

https://reviews.llvm.org/D101776

Files:
  clang/include/clang/Analysis/CFG.h


Index: clang/include/clang/Analysis/CFG.h
===
--- clang/include/clang/Analysis/CFG.h
+++ clang/include/clang/Analysis/CFG.h
@@ -1389,13 +1389,12 @@
   // Member templates useful for various batch operations over CFGs.
   
//======//
 
-  template 
-  void VisitBlockStmts(CALLBACK& O) const {
+  template  void VisitBlockStmts(Callback &O) const {
 for (const_iterator I = begin(), E = end(); I != E; ++I)
   for (CFGBlock::const_iterator BI = (*I)->begin(), BE = (*I)->end();
BI != BE; ++BI) {
 if (Optional stmt = BI->getAs())
-  O(const_cast(stmt->getStmt()));
+  O(const_cast(stmt->getStmt()));
   }
   }
 


Index: clang/include/clang/Analysis/CFG.h
===
--- clang/include/clang/Analysis/CFG.h
+++ clang/include/clang/Analysis/CFG.h
@@ -1389,13 +1389,12 @@
   // Member templates useful for various batch operations over CFGs.
   //======//
 
-  template 
-  void VisitBlockStmts(CALLBACK& O) const {
+  template  void VisitBlockStmts(Callback &O) const {
 for (const_iterator I = begin(), E = end(); I != E; ++I)
   for (CFGBlock::const_iterator BI = (*I)->begin(), BE = (*I)->end();
BI != BE; ++BI) {
 if (Optional stmt = BI->getAs())
-  O(const_cast(stmt->getStmt()));
+  O(const_cast(stmt->getStmt()));
   }
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101776: Work around an unfortunate macro in the Windows SDK

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Rename LLVM's single use of CALLBACK as a template parameter to work around the 
Windows SDK `#define CALLBACK __stdcall`

I'm sympathetic to the Michael Bolton argument but these are the only 2 lines 
affected in all of LLVM.

Once accepted I'll need someone to commit the change on my behalf.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101776

Files:
  clang/include/clang/Analysis/CFG.h


Index: clang/include/clang/Analysis/CFG.h
===
--- clang/include/clang/Analysis/CFG.h
+++ clang/include/clang/Analysis/CFG.h
@@ -1389,8 +1389,8 @@
   // Member templates useful for various batch operations over CFGs.
   
//======//
 
-  template 
-  void VisitBlockStmts(CALLBACK& O) const {
+  template 
+  void VisitBlockStmts(CALLBACKFN& O) const {
 for (const_iterator I = begin(), E = end(); I != E; ++I)
   for (CFGBlock::const_iterator BI = (*I)->begin(), BE = (*I)->end();
BI != BE; ++BI) {


Index: clang/include/clang/Analysis/CFG.h
===
--- clang/include/clang/Analysis/CFG.h
+++ clang/include/clang/Analysis/CFG.h
@@ -1389,8 +1389,8 @@
   // Member templates useful for various batch operations over CFGs.
   //======//
 
-  template 
-  void VisitBlockStmts(CALLBACK& O) const {
+  template 
+  void VisitBlockStmts(CALLBACKFN& O) const {
 for (const_iterator I = begin(), E = end(); I != E; ++I)
   for (CFGBlock::const_iterator BI = (*I)->begin(), BE = (*I)->end();
BI != BE; ++BI) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

Thanks for taking a look right away Duncan!  The same concern about a lack of 
tests was raised when I first added `-fno-temp-file` and I'm afraid inspiration 
hasn't struck me in the meantime.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101775

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added reviewers: dexonsmith, erik.pilkington.
zahen requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When creating a PCH file the use of a temp file will be dictated by the 
presence or absence of the -fno-temp-file flag.  Creating a module file will 
always use a temp file via the new ForceUseTemporary flag.

This design was worked out over email with @dexonsmith.

Once accepted I'll need someone to commit this change on my behalf.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101775

Files:
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendActions.cpp


Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -218,7 +218,8 @@
   // Because this is exposed via libclang we must disable RemoveFileOnSignal.
   return CI.createDefaultOutputFile(/*Binary=*/true, InFile, /*Extension=*/"",
 /*RemoveFileOnSignal=*/false,
-/*CreateMissingDirectories=*/true);
+/*CreateMissingDirectories=*/true,
+/*ForceUseTemporary=*/true);
 }
 
 bool GenerateModuleInterfaceAction::BeginSourceFileAction(
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -736,11 +736,9 @@
   }
 }
 
-std::unique_ptr
-CompilerInstance::createDefaultOutputFile(bool Binary, StringRef InFile,
-  StringRef Extension,
-  bool RemoveFileOnSignal,
-  bool CreateMissingDirectories) {
+std::unique_ptr CompilerInstance::createDefaultOutputFile(
+bool Binary, StringRef InFile, StringRef Extension, bool 
RemoveFileOnSignal,
+bool CreateMissingDirectories, bool ForceUseTemporary) {
   StringRef OutputPath = getFrontendOpts().OutputFile;
   Optional> PathStorage;
   if (OutputPath.empty()) {
@@ -753,9 +751,8 @@
 }
   }
 
-  // Force a temporary file if RemoveFileOnSignal was disabled.
   return createOutputFile(OutputPath, Binary, RemoveFileOnSignal,
-  getFrontendOpts().UseTemporary || 
!RemoveFileOnSignal,
+  getFrontendOpts().UseTemporary || ForceUseTemporary,
   CreateMissingDirectories);
 }
 
Index: clang/include/clang/Frontend/CompilerInstance.h
===
--- clang/include/clang/Frontend/CompilerInstance.h
+++ clang/include/clang/Frontend/CompilerInstance.h
@@ -698,15 +698,13 @@
   /// The files created by this are usually removed on signal, and, depending
   /// on FrontendOptions, may also use a temporary file (that is, the data is
   /// written to a temporary file which will atomically replace the target
-  /// output on success). If a client (like libclang) needs to disable
-  /// RemoveFileOnSignal, temporary files will be forced on.
+  /// output on success).
   ///
   /// \return - Null on error.
-  std::unique_ptr
-  createDefaultOutputFile(bool Binary = true, StringRef BaseInput = "",
-  StringRef Extension = "",
-  bool RemoveFileOnSignal = true,
-  bool CreateMissingDirectories = false);
+  std::unique_ptr createDefaultOutputFile(
+  bool Binary = true, StringRef BaseInput = "", StringRef Extension = "",
+  bool RemoveFileOnSignal = true, bool CreateMissingDirectories = false,
+  bool ForceUseTemporary = false);
 
   /// Create a new output file, optionally deriving the output path name, and
   /// add it to the list of tracked output files.


Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -218,7 +218,8 @@
   // Because this is exposed via libclang we must disable RemoveFileOnSignal.
   return CI.createDefaultOutputFile(/*Binary=*/true, InFile, /*Extension=*/"",
 /*RemoveFileOnSignal=*/false,
-/*CreateMissingDirectories=*/true);
+/*CreateMissingDirectories=*/true,
+/*ForceUseTemporary=*/true);
 }
 
 bool GenerateModuleInterfaceAction::BeginSourceFileAction(
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -736,11 +736,9 @@
   }
 }
 
-std::un

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

This patch doesn't need a test case outside of the one that @rnk requested to 
make sure that the flag flows from the clang-cl driver appropriately.  
`pch-instantiate-templates` as authored doesn't match cl.exe behavior and being 
unable to turn it off will prevent our adoption of clang 11.  This is a release 
blocking issue at least for 11.1 if not 11.0 (I leave it to @hans to make the 
decision).  I suspect there are other users that will run into the same problem 
we have.

Of course we'll continue to work on a reduced repro to understand where the 
feature and msvc diverge but that will be a follow up patch or new bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88680

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86622: Fix failing tests after VCTOOLSDIR change

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

Sorry again about the break!  As with the initial patch, I'll need you to land 
this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86622: Fix failing tests after VCTOOLSDIR change

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added a reviewer: hans.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
zahen requested review of this revision.

Switch from hardcoded x64 arch to a regex in the target triple


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86622

Files:
  clang/test/Driver/cl-options.c


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -684,7 +684,7 @@
 
 // Validate that the default triple is used when run an empty tools dir is 
specified
 // RUN: %clang_cl -vctoolsdir "" -### -- %s 2>&1 | FileCheck %s --check-prefix 
VCTOOLSDIR
-// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
+// VCTOOLSDIR: "-triple" "{{[a-zA-Z0-9_-]*}}-pc-windows-msvc19.11.0"
 
 // Validate that built-in include paths are based on the supplied path
 // RUN: %clang_cl -vctoolsdir "/fake" -### -- %s 2>&1 | FileCheck %s 
--check-prefix FAKEDIR


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -684,7 +684,7 @@
 
 // Validate that the default triple is used when run an empty tools dir is specified
 // RUN: %clang_cl -vctoolsdir "" -### -- %s 2>&1 | FileCheck %s --check-prefix VCTOOLSDIR
-// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
+// VCTOOLSDIR: "-triple" "{{[a-zA-Z0-9_-]*}}-pc-windows-msvc19.11.0"
 
 // Validate that built-in include paths are based on the supplied path
 // RUN: %clang_cl -vctoolsdir "/fake" -### -- %s 2>&1 | FileCheck %s --check-prefix FAKEDIR
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

Thanks @hans!  I'll need you (or someone else with permission) to land the 
change on my behalf.


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

https://reviews.llvm.org/D85998

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

The real reason we don't see it internally is because we use -c for all 
compilation.


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

https://reviews.llvm.org/D85998

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen marked an inline comment as done.
zahen added a comment.

In D85998#2236401 , @hans wrote:

> I'm still curious why it seems it's not looking for link.exe in the /fake dir 
> though.

clang\lib\Driver\ToolChains\MSVC.cpp:311

FindVisualStudioExecutable 
FilePath = /fake\bin\Hostx64\x64\link.exe 
The test llvm::sys::fs::can_execute(FilePath) fails and the function returns 
"link.exe".  This isn't seen in our environment because //absent path// probes 
are always allowed, it's only //actual file reads// that need to be tracked.


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

https://reviews.llvm.org/D85998

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen marked 3 inline comments as done.
zahen added inline comments.



Comment at: clang/test/Driver/cl-options.c:686
+// vctoolsdir is handled by the driver; just check that we don't error. Pass 
-c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+

hans wrote:
> zahen wrote:
> > zahen wrote:
> > > hans wrote:
> > > > zahen wrote:
> > > > > hans wrote:
> > > > > > aganea wrote:
> > > > > > > Check that we're not detecting a local installation, and that we 
> > > > > > > fallback to the default triple 19.11, ie.
> > > > > > > ```
> > > > > > > // RUN: %clang_cl ... -vctoolsdir "" -### | FileCheck %s 
> > > > > > > --check-prefix VCTOOLSDIR
> > > > > > > // VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
> > > > > > > ```
> > > > > > Maybe add a comment explaining the purpose of the test.
> > > > > > 
> > > > > > And would it be possible to have a test where -vctoolsdir is set to 
> > > > > > something, to see that it's picked up correctly?
> > > > > What's the expectation on the test boxes?  I can author such a test 
> > > > > but it would be very brittle unless we have a spec for how VS should 
> > > > > be installed.
> > > > I'd suggest passing -vctoolsdir "/foo" and check that some /foo dir is 
> > > > getting passed as a system include dir to cc1, and that the link.exe 
> > > > invocation is under /foo.
> > > > 
> > > > I don't think it would be brittle.
> > > Clever idea! I'll do it that way and key off the "ignored directory" 
> > > warning that's emitted.
> > Did further digging and the path test would depend on other environment 
> > variables.  If the %INCLUDE% variable is set, as it is by vcvars.bat, those 
> > paths are adopted and VCToolChainPath is not used.  I'm hesitant to change 
> > that logic without stronger motivation.
> > 
> > Other ideas for a test that would work run both from a vc command prompt 
> > and a vanilla command prompt?  I don't see anything else in the full 
> > verbose invocation to key off of:
> > 
> > 
> > ```
> > D:\repos\llvm\llvm-project\build\Debug\bin>clang-cl -vctoolsdir "/fake" -v 
> > main.cpp
> > clang version 12.0.0 (https://github.com/llvm/llvm-project 
> > 537f5483fe4e09c39ffd7ac837c00b50dd13d676)
> > Target: x86_64-pc-windows-msvc
> > Thread model: posix
> > InstalledDir: D:\repos\llvm\llvm-project\build\Debug\bin
> >  "D:\\repos\\llvm\\llvm-project\\build\\Debug\\bin\\clang-cl.exe" -cc1 
> > -triple x86_64-pc-windows-msvc19.11.0 -emit-obj -mrelax-all 
> > -mincremental-linker-compatible --mrelax-relocations -disable-free 
> > -main-file-name main.cpp -mrelocation-model pic -pic-level 2 
> > -mframe-pointer=none -relaxed-aliasing -fmath-errno -fno-rounding-math 
> > -mconstructor-aliases -munwind-tables -target-cpu x86-64 -mllvm 
> > -x86-asm-syntax=intel -D_MT -flto-visibility-public-std 
> > --dependent-lib=libcmt --dependent-lib=oldnames -stack-protector 2 
> > -fms-volatile -fdiagnostics-format msvc -v -resource-dir 
> > "D:\\repos\\llvm\\llvm-project\\build\\Debug\\lib\\clang\\12.0.0" 
> > -internal-isystem 
> > "D:\\repos\\llvm\\llvm-project\\build\\Debug\\lib\\clang\\12.0.0\\include" 
> > -internal-isystem "C:\\Program Files (x86)\\Microsoft Visual 
> > Studio\\2019\\Enterprise\\VC\\Tools\\MSVC\\14.27.29110\\ATLMFC\\include" 
> > -internal-isystem "C:\\Program Files (x86)\\Microsoft Visual 
> > Studio\\2019\\Enterprise\\VC\\Tools\\MSVC\\14.27.29110\\include" 
> > -internal-isystem "C:\\Program Files (x86)\\Windows 
> > Kits\\NETFXSDK\\4.8\\include\\um" -internal-isystem "C:\\Program Files 
> > (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\ucrt" -internal-isystem 
> > "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\shared" 
> > -internal-isystem "C:\\Program Files (x86)\\Windows 
> > Kits\\10\\include\\10.0.18362.0\\um" -internal-isystem "C:\\Program Files 
> > (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\winrt" -internal-isystem 
> > "C:\\Program Files (x86)\\Windows 
> > Kits\\10\\include\\10.0.18362.0\\cppwinrt" -fdeprecated-macro 
> > -fdebug-compilation-dir "D:\\repos\\llvm\\llvm-project\\build\\Debug\\bin" 
> > -ferror-limit 19 -fmessage-length=121 -fno-use-cxa-atexit -fms-extensions 
> > -fms-compatibility -fms-compatibility-version=19.11 -std=c++14 
> > -fdelayed-template-parsing -fcolor-diagnostics -faddrsig -o 
> > "C:\\Users\\zahen\\AppData\\Local\\Temp\\main-8f329a.obj" -x c++ main.cpp
> > clang -cc1 version 12.0.0 based upon LLVM 12.0.0git default target 
> > x86_64-pc-windows-msvc
> > #include "..." search starts here:
> > #include <...> search starts here:
> >  D:\repos\llvm\llvm-project\build\Debug\lib\clang\12.0.0\include
> >  C:\Program Files (x86)\Microsoft Visual 
> > Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\ATLMFC\include
> >  C:\Program Files (x86)\Microsoft Visual 
> > Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\include
> >  C:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\include\um
> >  C:\Progra

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 287671.
zahen added a comment.
Herald added a subscriber: ormris.

It felt too invasive to swap the precedence of using vctoolsdir vs %INCLUDE% 
for all clang-cl users so I compromised by skipping the %INCLUDE% check when 
vctoolsdir was passed on the command line.  This way only the users of the new 
flag will see different behavior.

With that change the suggested test for -internal-isystem passes in both 
vanilla and vc command prompts.


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

https://reviews.llvm.org/D85998

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/test/Driver/cl-options.c

Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -682,4 +682,13 @@
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// Validate that the default triple is used when run an empty tools dir is specified
+// RUN: %clang_cl -vctoolsdir "" -### -- %s 2>&1 | FileCheck %s --check-prefix VCTOOLSDIR
+// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
+
+// Validate that built-in include paths are based on the supplied path
+// RUN: %clang_cl -vctoolsdir "/fake" -### -- %s 2>&1 | FileCheck %s --check-prefix FAKEDIR
+// FAKEDIR: "-internal-isystem" "/fake\\include"
+// FAKEDIR: "-internal-isystem" "/fake\\atlmfc\\include"
+
 void f() { }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -66,6 +66,20 @@
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
 std::string &value, std::string *phValue);
 
+// Check command line arguments to try and find a toolchain.
+static bool
+findVCToolChainViaCommandLine(const ArgList &Args, std::string &Path,
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Don't validate the input; trust the value supplied by the user.
+  // The primary motivation is to prevent unnecessary file and registry access.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+return true;
+  }
+  return false;
+}
+
 // Check various environment variables to try and find a toolchain.
 static bool findVCToolChainViaEnvironment(std::string &Path,
   MSVCToolChain::ToolsetLayout &VSLayout) {
@@ -747,11 +761,12 @@
   if (getDriver().getInstalledDir() != getDriver().Dir)
 getProgramPaths().push_back(getDriver().Dir);
 
-  // Check the environment first, since that's probably the user telling us
-  // what they want to use.
-  // Failing that, just try to find the newest Visual Studio version we can
-  // and use its default VC toolchain.
-  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
+  // Check the command line first, that's the user explicitly telling us what to
+  // use. Check the environment next, in case we're being invoked from a VS
+  // command prompt. Failing that, just try to find the newest Visual Studio
+  // version we can and use its default VC toolchain.
+  findVCToolChainViaCommandLine(Args, VCToolChainPath, VSLayout) ||
+  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
   findVCToolChainViaSetupConfig(VCToolChainPath, VSLayout) ||
   findVCToolChainViaRegistry(VCToolChainPath, VSLayout);
 }
@@ -1263,15 +1278,18 @@
 return;
 
   // Honor %INCLUDE%. It should know essential search paths with vcvarsall.bat.
-  if (llvm::Optional cl_include_dir =
-  llvm::sys::Process::GetEnv("INCLUDE")) {
-SmallVector Dirs;
-StringRef(*cl_include_dir)
-.split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
-for (StringRef Dir : Dirs)
-  addSystemInclude(DriverArgs, CC1Args, Dir);
-if (!Dirs.empty())
-  return;
+  // Skip if the user expressly set a vctoolsdir
+  if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+if (llvm::Optional cl_include_dir =
+llvm::sys::Process::GetEnv("INCLUDE")) {
+  SmallVector Dirs;
+  StringRef(*cl_include_dir)
+  .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+  for (StringRef Dir : Dirs)
+addSystemInclude(DriverArgs, CC1Args, Dir);
+  if (!Dirs.empty())
+return;
+}
   }
 
   // When built with access to the proper Windows APIs, try to actually find
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -4737,6 +4737,8 @@
 def _SLASH_Tp : CLCompileJoinedOrSeparate<"Tp">,
   HelpText<"Treat  as C++ source file">, MetaVarName<"">;
 def _SLASH_T

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen added inline comments.



Comment at: clang/test/Driver/cl-options.c:686
+// vctoolsdir is handled by the driver; just check that we don't error. Pass 
-c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+

zahen wrote:
> hans wrote:
> > zahen wrote:
> > > hans wrote:
> > > > aganea wrote:
> > > > > Check that we're not detecting a local installation, and that we 
> > > > > fallback to the default triple 19.11, ie.
> > > > > ```
> > > > > // RUN: %clang_cl ... -vctoolsdir "" -### | FileCheck %s 
> > > > > --check-prefix VCTOOLSDIR
> > > > > // VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
> > > > > ```
> > > > Maybe add a comment explaining the purpose of the test.
> > > > 
> > > > And would it be possible to have a test where -vctoolsdir is set to 
> > > > something, to see that it's picked up correctly?
> > > What's the expectation on the test boxes?  I can author such a test but 
> > > it would be very brittle unless we have a spec for how VS should be 
> > > installed.
> > I'd suggest passing -vctoolsdir "/foo" and check that some /foo dir is 
> > getting passed as a system include dir to cc1, and that the link.exe 
> > invocation is under /foo.
> > 
> > I don't think it would be brittle.
> Clever idea! I'll do it that way and key off the "ignored directory" warning 
> that's emitted.
Did further digging and the path test would depend on other environment 
variables.  If the %INCLUDE% variable is set, as it is by vcvars.bat, those 
paths are adopted and VCToolChainPath is not used.  I'm hesitant to change that 
logic without stronger motivation.

Other ideas for a test that would work run both from a vc command prompt and a 
vanilla command prompt?  I don't see anything else in the full verbose 
invocation to key off of:


```
D:\repos\llvm\llvm-project\build\Debug\bin>clang-cl -vctoolsdir "/fake" -v 
main.cpp
clang version 12.0.0 (https://github.com/llvm/llvm-project 
537f5483fe4e09c39ffd7ac837c00b50dd13d676)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: D:\repos\llvm\llvm-project\build\Debug\bin
 "D:\\repos\\llvm\\llvm-project\\build\\Debug\\bin\\clang-cl.exe" -cc1 -triple 
x86_64-pc-windows-msvc19.11.0 -emit-obj -mrelax-all 
-mincremental-linker-compatible --mrelax-relocations -disable-free 
-main-file-name main.cpp -mrelocation-model pic -pic-level 2 
-mframe-pointer=none -relaxed-aliasing -fmath-errno -fno-rounding-math 
-mconstructor-aliases -munwind-tables -target-cpu x86-64 -mllvm 
-x86-asm-syntax=intel -D_MT -flto-visibility-public-std --dependent-lib=libcmt 
--dependent-lib=oldnames -stack-protector 2 -fms-volatile -fdiagnostics-format 
msvc -v -resource-dir 
"D:\\repos\\llvm\\llvm-project\\build\\Debug\\lib\\clang\\12.0.0" 
-internal-isystem 
"D:\\repos\\llvm\\llvm-project\\build\\Debug\\lib\\clang\\12.0.0\\include" 
-internal-isystem "C:\\Program Files (x86)\\Microsoft Visual 
Studio\\2019\\Enterprise\\VC\\Tools\\MSVC\\14.27.29110\\ATLMFC\\include" 
-internal-isystem "C:\\Program Files (x86)\\Microsoft Visual 
Studio\\2019\\Enterprise\\VC\\Tools\\MSVC\\14.27.29110\\include" 
-internal-isystem "C:\\Program Files (x86)\\Windows 
Kits\\NETFXSDK\\4.8\\include\\um" -internal-isystem "C:\\Program Files 
(x86)\\Windows Kits\\10\\include\\10.0.18362.0\\ucrt" -internal-isystem 
"C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\shared" 
-internal-isystem "C:\\Program Files (x86)\\Windows 
Kits\\10\\include\\10.0.18362.0\\um" -internal-isystem "C:\\Program Files 
(x86)\\Windows Kits\\10\\include\\10.0.18362.0\\winrt" -internal-isystem 
"C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\cppwinrt" 
-fdeprecated-macro -fdebug-compilation-dir 
"D:\\repos\\llvm\\llvm-project\\build\\Debug\\bin" -ferror-limit 19 
-fmessage-length=121 -fno-use-cxa-atexit -fms-extensions -fms-compatibility 
-fms-compatibility-version=19.11 -std=c++14 -fdelayed-template-parsing 
-fcolor-diagnostics -faddrsig -o 
"C:\\Users\\zahen\\AppData\\Local\\Temp\\main-8f329a.obj" -x c++ main.cpp
clang -cc1 version 12.0.0 based upon LLVM 12.0.0git default target 
x86_64-pc-windows-msvc
#include "..." search starts here:
#include <...> search starts here:
 D:\repos\llvm\llvm-project\build\Debug\lib\clang\12.0.0\include
 C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\ATLMFC\include
 C:\Program Files (x86)\Microsoft Visual 
Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\include
 C:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\include\um
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\ucrt
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\shared
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\um
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\winrt
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\cppwinrt
End of search list.
 "link.exe" -out:main.exe -nologo 
"C:\\Users\\zahen\\AppData\

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen added inline comments.



Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:73
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Don't validate the input; trust the value supplied by the user.
+  // The primary motivation is to prevent unnecessary file and registry access.

hans wrote:
> zahen wrote:
> > hans wrote:
> > > zahen wrote:
> > > > aganea wrote:
> > > > > hans wrote:
> > > > > > What will the error look like if the user sets this but gets it 
> > > > > > wrong? I'm sympathetic to not wanting to validate it, but if it's 
> > > > > > wrong it would be nice if the error isn't too cryptic.
> > > > > @hans The flag proposed here behaves like `env VCToolsInstallDir="" 
> > > > > clang-cl ...`, and in that case there's no validation. Do you think 
> > > > > the path to `-vctoolsdir` should be validated and throw an error 
> > > > > otherwise? I find it nice to completly disable the toolchain 
> > > > > auto-detection if passing in a empty dir, without needing an 
> > > > > additional flag, ie `clang-cl -vctoolsdir ""`.
> > > > ```
> > > > D:\>clang-cl -vctoolsdir fake main.cpp
> > > > clang-cl: error: unable to execute command: program not executable
> > > > clang-cl: error: linker command failed with exit code 1 (use -v to see 
> > > > invocation)
> > > > ```
> > > I realize this is not because of your patch, but it would be nice if this 
> > > error could be more clear. Ideally it would at least print the path to 
> > > what's failing to execute. Would you be willing to add that?
> > It's printed when using -v as directed. I don't feel strongly against 
> > adding the command line in all failure cases, but did want to point that 
> > out.  Still think it's worth it?
> No, I'm not asking for it to be validated, but if e.g. execution of link.exe 
> fails, it would be nice if the error message at least printed the path it was 
> trying to execute so the user has a chance to see what's wrong.
and the invocation with a bogus directory is as follows.  No explicit path 
provided
```
"link.exe" ...
```


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

https://reviews.llvm.org/D85998

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen added inline comments.



Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:73
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Don't validate the input; trust the value supplied by the user.
+  // The primary motivation is to prevent unnecessary file and registry access.

hans wrote:
> zahen wrote:
> > aganea wrote:
> > > hans wrote:
> > > > What will the error look like if the user sets this but gets it wrong? 
> > > > I'm sympathetic to not wanting to validate it, but if it's wrong it 
> > > > would be nice if the error isn't too cryptic.
> > > @hans The flag proposed here behaves like `env VCToolsInstallDir="" 
> > > clang-cl ...`, and in that case there's no validation. Do you think the 
> > > path to `-vctoolsdir` should be validated and throw an error otherwise? I 
> > > find it nice to completly disable the toolchain auto-detection if passing 
> > > in a empty dir, without needing an additional flag, ie `clang-cl 
> > > -vctoolsdir ""`.
> > ```
> > D:\>clang-cl -vctoolsdir fake main.cpp
> > clang-cl: error: unable to execute command: program not executable
> > clang-cl: error: linker command failed with exit code 1 (use -v to see 
> > invocation)
> > ```
> I realize this is not because of your patch, but it would be nice if this 
> error could be more clear. Ideally it would at least print the path to what's 
> failing to execute. Would you be willing to add that?
It's printed when using -v as directed. I don't feel strongly against adding 
the command line in all failure cases, but did want to point that out.  Still 
think it's worth it?



Comment at: clang/test/Driver/cl-options.c:686
+// vctoolsdir is handled by the driver; just check that we don't error. Pass 
-c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+

hans wrote:
> zahen wrote:
> > hans wrote:
> > > aganea wrote:
> > > > Check that we're not detecting a local installation, and that we 
> > > > fallback to the default triple 19.11, ie.
> > > > ```
> > > > // RUN: %clang_cl ... -vctoolsdir "" -### | FileCheck %s --check-prefix 
> > > > VCTOOLSDIR
> > > > // VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
> > > > ```
> > > Maybe add a comment explaining the purpose of the test.
> > > 
> > > And would it be possible to have a test where -vctoolsdir is set to 
> > > something, to see that it's picked up correctly?
> > What's the expectation on the test boxes?  I can author such a test but it 
> > would be very brittle unless we have a spec for how VS should be installed.
> I'd suggest passing -vctoolsdir "/foo" and check that some /foo dir is 
> getting passed as a system include dir to cc1, and that the link.exe 
> invocation is under /foo.
> 
> I don't think it would be brittle.
Clever idea! I'll do it that way and key off the "ignored directory" warning 
that's emitted.


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

https://reviews.llvm.org/D85998

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen marked an inline comment as done.
zahen added a comment.

@hans the explicit path must be declared, all exes and dlls.  The unexpected 
probes for link.exe when invoking clang-cl.exe when it isn't actually going to 
be invoked are what we want to avoid.




Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:73
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Don't validate the input; trust the value supplied by the user.
+  // The primary motivation is to prevent unnecessary file and registry access.

hans wrote:
> What will the error look like if the user sets this but gets it wrong? I'm 
> sympathetic to not wanting to validate it, but if it's wrong it would be nice 
> if the error isn't too cryptic.
```
D:\>clang-cl -vctoolsdir fake main.cpp
clang-cl: error: unable to execute command: program not executable
clang-cl: error: linker command failed with exit code 1 (use -v to see 
invocation)
```



Comment at: clang/test/Driver/cl-options.c:686
+// vctoolsdir is handled by the driver; just check that we don't error. Pass 
-c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+

hans wrote:
> aganea wrote:
> > Check that we're not detecting a local installation, and that we fallback 
> > to the default triple 19.11, ie.
> > ```
> > // RUN: %clang_cl ... -vctoolsdir "" -### | FileCheck %s --check-prefix 
> > VCTOOLSDIR
> > // VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
> > ```
> Maybe add a comment explaining the purpose of the test.
> 
> And would it be possible to have a test where -vctoolsdir is set to 
> something, to see that it's picked up correctly?
What's the expectation on the test boxes?  I can author such a test but it 
would be very brittle unless we have a spec for how VS should be installed.


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

https://reviews.llvm.org/D85998

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 287443.
zahen added a comment.

Good call @aganea on these comments. Updated


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

https://reviews.llvm.org/D85998

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/test/Driver/cl-options.c


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -682,4 +682,7 @@
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// RUN: %clang_cl -vctoolsdir "" -### -- %s 2>&1 | FileCheck %s --check-prefix 
VCTOOLSDIR
+// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
+
 void f() { }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -66,6 +66,20 @@
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
 std::string &value, std::string *phValue);
 
+// Check command line arguments to try and find a toolchain.
+static bool
+findVCToolChainViaCommandLine(const ArgList &Args, std::string &Path,
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Don't validate the input; trust the value supplied by the user.
+  // The primary motivation is to prevent unnecessary file and registry access.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+return true;
+  }
+  return false;
+}
+
 // Check various environment variables to try and find a toolchain.
 static bool findVCToolChainViaEnvironment(std::string &Path,
   MSVCToolChain::ToolsetLayout 
&VSLayout) {
@@ -747,11 +761,12 @@
   if (getDriver().getInstalledDir() != getDriver().Dir)
 getProgramPaths().push_back(getDriver().Dir);
 
-  // Check the environment first, since that's probably the user telling us
-  // what they want to use.
-  // Failing that, just try to find the newest Visual Studio version we can
-  // and use its default VC toolchain.
-  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
+  // Check the command line first, that's the user explicitly telling us what 
to
+  // use. Check the environment next, in case we're being invoked from a VS
+  // command prompt. Failing that, just try to find the newest Visual Studio
+  // version we can and use its default VC toolchain.
+  findVCToolChainViaCommandLine(Args, VCToolChainPath, VSLayout) ||
+  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
   findVCToolChainViaSetupConfig(VCToolChainPath, VSLayout) ||
   findVCToolChainViaRegistry(VCToolChainPath, VSLayout);
 }
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -4737,6 +4737,8 @@
 def _SLASH_Tp : CLCompileJoinedOrSeparate<"Tp">,
   HelpText<"Treat  as C++ source file">, MetaVarName<"">;
 def _SLASH_TP : CLCompileFlag<"TP">, HelpText<"Treat all source files as C++">;
+def _SLASH_vctoolsdir : CLJoinedOrSeparate<"vctoolsdir">,
+  HelpText<"Path to the VCToolChain">, MetaVarName<"">;
 def _SLASH_volatile_iso : Option<["/", "-"], "volatile:iso", KIND_FLAG>,
   Group<_SLASH_volatile_Group>, Flags<[CLOption, DriverOption]>,
   HelpText<"Volatile loads and stores have standard semantics">;


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -682,4 +682,7 @@
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// RUN: %clang_cl -vctoolsdir "" -### -- %s 2>&1 | FileCheck %s --check-prefix VCTOOLSDIR
+// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
+
 void f() { }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -66,6 +66,20 @@
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
 std::string &value, std::string *phValue);
 
+// Check command line arguments to try and find a toolchain.
+static bool
+findVCToolChainViaCommandLine(const ArgList &Args, std::string &Path,
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Don't validate the input; trust the value supplied by the user.
+  // The primary motivation is to prevent unnecessary file and registry access.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+  

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 287425.
zahen edited reviewers, added: aganea; removed: thakis.
zahen added a comment.

Updates as requested


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

https://reviews.llvm.org/D85998

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/test/Driver/cl-options.c


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -682,4 +682,7 @@
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// RUN: %clang_cl -vctoolsdir "" -### -- %s 2>&1 | FileCheck %s --check-prefix 
VCTOOLSDIR
+// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
+
 void f() { }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -66,6 +66,20 @@
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
 std::string &value, std::string *phValue);
 
+// Check command line arguments to try and find a toolchain.
+static bool
+findVCToolChainViaCommandLine(const ArgList &Args, std::string &Path,
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Don't validate the input; trust the value supplied by the user.
+  // The primary motivation is to prevent unnecessary file and registry access.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+return true;
+  }
+  return false;
+}
+
 // Check various environment variables to try and find a toolchain.
 static bool findVCToolChainViaEnvironment(std::string &Path,
   MSVCToolChain::ToolsetLayout 
&VSLayout) {
@@ -751,7 +765,8 @@
   // what they want to use.
   // Failing that, just try to find the newest Visual Studio version we can
   // and use its default VC toolchain.
-  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
+  findVCToolChainViaCommandLine(Args, VCToolChainPath, VSLayout) ||
+  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
   findVCToolChainViaSetupConfig(VCToolChainPath, VSLayout) ||
   findVCToolChainViaRegistry(VCToolChainPath, VSLayout);
 }
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -4737,6 +4737,8 @@
 def _SLASH_Tp : CLCompileJoinedOrSeparate<"Tp">,
   HelpText<"Treat  as C++ source file">, MetaVarName<"">;
 def _SLASH_TP : CLCompileFlag<"TP">, HelpText<"Treat all source files as C++">;
+def _SLASH_vctoolsdir : CLJoinedOrSeparate<"vctoolsdir">,
+  HelpText<"Path to the VCToolChain">, MetaVarName<"">;
 def _SLASH_volatile_iso : Option<["/", "-"], "volatile:iso", KIND_FLAG>,
   Group<_SLASH_volatile_Group>, Flags<[CLOption, DriverOption]>,
   HelpText<"Volatile loads and stores have standard semantics">;


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -682,4 +682,7 @@
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// RUN: %clang_cl -vctoolsdir "" -### -- %s 2>&1 | FileCheck %s --check-prefix VCTOOLSDIR
+// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
+
 void f() { }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -66,6 +66,20 @@
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
 std::string &value, std::string *phValue);
 
+// Check command line arguments to try and find a toolchain.
+static bool
+findVCToolChainViaCommandLine(const ArgList &Args, std::string &Path,
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Don't validate the input; trust the value supplied by the user.
+  // The primary motivation is to prevent unnecessary file and registry access.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+return true;
+  }
+  return false;
+}
+
 // Check various environment variables to try and find a toolchain.
 static bool findVCToolChainViaEnvironment(std::string &Path,
   MSVCToolChain::ToolsetLayout &VSLayout) {
@@ -751,7 +765,8 @@
   // what they want to use.
   // Failing that, just try to find the newest Visual Studio version we can
   // and use its default VC toolchain.
-  findVCToolChainViaEnvir

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

We use BuildXL.  Thanks for the comments.  I'll make the requested changes and 
get a new rev posted.


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

https://reviews.llvm.org/D85998

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

The build system strives to be deterministic so all file probes need to be 
accounted for; environment variables are also problematic.  Our builds 
deliberately don't run from a Visual Studio command prompt.  In addition, we'd 
like to avoid coupling clang tools that don't perform codegen to link.exe.  We 
do use -fmsc-version= during the build.


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

https://reviews.llvm.org/D85998

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

Once accepted, someone else will need to submit on my behalf.




Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:74
+   if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;

Deliberately not validating the input.  The primary motivation is to prevent 
unnecessary file and registry access.


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

https://reviews.llvm.org/D85998

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 285803.
zahen added a comment.

Fix formatting


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

https://reviews.llvm.org/D85998

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/test/Driver/cl-options.c


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -682,4 +682,7 @@
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// vctoolsdir is handled by the driver; just check that we don't error. Pass 
-c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+
 void f() { }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -66,6 +66,19 @@
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
 std::string &value, std::string *phValue);
 
+// Check command line arguments to try and find a toolchain.
+static bool
+findVCToolChainViaCommandLine(const ArgList &Args, std::string &Path,
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Trust the value supplied by the user.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+return true;
+  }
+  return false;
+}
+
 // Check various environment variables to try and find a toolchain.
 static bool findVCToolChainViaEnvironment(std::string &Path,
   MSVCToolChain::ToolsetLayout 
&VSLayout) {
@@ -751,7 +764,8 @@
   // what they want to use.
   // Failing that, just try to find the newest Visual Studio version we can
   // and use its default VC toolchain.
-  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
+  findVCToolChainViaCommandLine(Args, VCToolChainPath, VSLayout) ||
+  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
   findVCToolChainViaSetupConfig(VCToolChainPath, VSLayout) ||
   findVCToolChainViaRegistry(VCToolChainPath, VSLayout);
 }
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -4737,6 +4737,8 @@
 def _SLASH_Tp : CLCompileJoinedOrSeparate<"Tp">,
   HelpText<"Treat  as C++ source file">, MetaVarName<"">;
 def _SLASH_TP : CLCompileFlag<"TP">, HelpText<"Treat all source files as C++">;
+def _SLASH_vctoolsdir : CLJoinedOrSeparate<"vctoolsdir">,
+  HelpText<"Path to the VCToolChain">, MetaVarName<"">;
 def _SLASH_volatile_iso : Option<["/", "-"], "volatile:iso", KIND_FLAG>,
   Group<_SLASH_volatile_Group>, Flags<[CLOption, DriverOption]>,
   HelpText<"Volatile loads and stores have standard semantics">;


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -682,4 +682,7 @@
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// vctoolsdir is handled by the driver; just check that we don't error. Pass -c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+
 void f() { }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -66,6 +66,19 @@
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
 std::string &value, std::string *phValue);
 
+// Check command line arguments to try and find a toolchain.
+static bool
+findVCToolChainViaCommandLine(const ArgList &Args, std::string &Path,
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Trust the value supplied by the user.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+return true;
+  }
+  return false;
+}
+
 // Check various environment variables to try and find a toolchain.
 static bool findVCToolChainViaEnvironment(std::string &Path,
   MSVCToolChain::ToolsetLayout &VSLayout) {
@@ -751,7 +764,8 @@
   // what they want to use.
   // Failing that, just try to find the newest Visual Studio version we can
   // and use its default VC toolchain.
-  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
+  findVCToolChainViaCommandLine(Args, VCToolChainPath, VSLayout) ||
+  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
   findVCToolChainViaSetupConfig(VCToolChainPath, VSLay

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added reviewers: rnk, hans, thakis.
Herald added subscribers: cfe-commits, dang.
Herald added a project: clang.
zahen requested review of this revision.

Add an option to directly specify where the msvc toolchain lives for clang-cl 
and avoid unwanted file and registry probes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85998

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/test/Driver/cl-options.c


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -682,4 +682,7 @@
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// vctoolsdir is handled by the driver; just check that we don't error. Pass 
-c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+
 void f() { }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -66,6 +66,18 @@
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
 std::string &value, std::string *phValue);
 
+// Check command line arguments to try and find a toolchain.
+static bool findVCToolChainViaCommandLine(const ArgList &Args, std::string 
&Path,
+  MSVCToolChain::ToolsetLayout 
&VSLayout) {
+  // Trust the value supplied by the user.
+   if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+return true;
+  }
+  return false;
+}
+
 // Check various environment variables to try and find a toolchain.
 static bool findVCToolChainViaEnvironment(std::string &Path,
   MSVCToolChain::ToolsetLayout 
&VSLayout) {
@@ -751,7 +763,8 @@
   // what they want to use.
   // Failing that, just try to find the newest Visual Studio version we can
   // and use its default VC toolchain.
-  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
+findVCToolChainViaCommandLine(Args, VCToolChainPath, VSLayout) ||
+  findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
   findVCToolChainViaSetupConfig(VCToolChainPath, VSLayout) ||
   findVCToolChainViaRegistry(VCToolChainPath, VSLayout);
 }
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -4737,6 +4737,8 @@
 def _SLASH_Tp : CLCompileJoinedOrSeparate<"Tp">,
   HelpText<"Treat  as C++ source file">, MetaVarName<"">;
 def _SLASH_TP : CLCompileFlag<"TP">, HelpText<"Treat all source files as C++">;
+def _SLASH_vctoolsdir : CLJoinedOrSeparate<"vctoolsdir">,
+  HelpText<"Path to the VCToolChain">, MetaVarName<"">;
 def _SLASH_volatile_iso : Option<["/", "-"], "volatile:iso", KIND_FLAG>,
   Group<_SLASH_volatile_Group>, Flags<[CLOption, DriverOption]>,
   HelpText<"Volatile loads and stores have standard semantics">;


Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -682,4 +682,7 @@
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// vctoolsdir is handled by the driver; just check that we don't error. Pass -c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+
 void f() { }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -66,6 +66,18 @@
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
 std::string &value, std::string *phValue);
 
+// Check command line arguments to try and find a toolchain.
+static bool findVCToolChainViaCommandLine(const ArgList &Args, std::string &Path,
+  MSVCToolChain::ToolsetLayout &VSLayout) {
+  // Trust the value supplied by the user.
+   if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+return true;
+  }
+  return false;
+}
+
 // Check various environment variables to try and find a toolchain.
 static bool findVCToolChainViaEnvironment(std::string &Path,
   MSVCToolChain::ToolsetLayout &VSLayout) {
@@ -751,7 +763,8 @@
   // what they want to use.
   // Failing that, just try to find the newest Visual Studio version we can
   // and use its default VC toolchain.
-  findVCToolChainViaEnviro

[PATCH] D72405: Allow /D flags absent during PCH creation under msvc-compat

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

Wild guess is that `2> %t.err` should be removed from the `-verify` lines? That 
change passes in the single env I have access to.  @rnk any idea what might be 
going on?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72405



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72405: Allow /D flags absent during PCH creation under msvc-compat

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 237795.
zahen added a comment.

Incorporate review feedback.

I had no luck converting the CHECK-FOO & CHECK-NOFOO tests to use `-verify` 
because the errors were reported against "(frontend)"

  error: 'error' diagnostics seen but not expected:
(frontend): definition of macro 'FOO' differs between the precompiled 
header ('1') and the command line ('blah')


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

https://reviews.llvm.org/D72405

Files:
  clang/lib/Lex/PPDirectives.cpp
  clang/test/PCH/fuzzy-pch-msvc.c


Index: clang/test/PCH/fuzzy-pch-msvc.c
===
--- /dev/null
+++ clang/test/PCH/fuzzy-pch-msvc.c
@@ -0,0 +1,32 @@
+// Test -D and -U interaction with a PCH when running clang-cl.
+
+// RUN: %clang_cl -DFOO -Yc %S/variables.h
+
+// RUN: not %clang_cl -c -DFOO=blah -DBAR=int -Wno-microsoft-include 
-Yuvariables.h %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-FOO %s < %t.err
+
+// RUN: not %clang_cl -c -UFOO -DBAR=int -Wno-microsoft-include -Yuvariables.h 
%s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-NOFOO %s < %t.err 
+
+// RUN: %clang_cl -c -DBAR=int -Wno-microsoft-include -Yuvariables.h -Xclang 
-verify %s 
+#include "variables.h"
+
+BAR bar = 17;
+
+#ifndef FOO
+#  error FOO was not defined
+#endif
+
+#if FOO != 1
+#  error FOO has the wrong definition
+#endif
+
+#ifndef BAR
+#  error BAR was not defined 
+#endif
+
+
+// CHECK-FOO: definition of macro 'FOO' differs between the precompiled header 
('1') and the command line ('blah')
+// CHECK-NOFOO: macro 'FOO' was defined in the precompiled header but undef'd 
on the command line
+
+// expected-warning@1 {{definition of macro 'BAR' does not match definition in 
precompiled header}}
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2727,7 +2727,9 @@
  /*Syntactic=*/LangOpts.MicrosoftExt))
   Diag(MI->getDefinitionLoc(), diag::warn_pp_macro_def_mismatch_with_pch)
   << MacroNameTok.getIdentifierInfo();
-return;
+// Issue the diagnostic but allow the change if msvc extensions are enabled
+if (!LangOpts.MicrosoftExt)
+  return;
   }
 
   // Finally, if this identifier already had a macro defined for it, verify 
that


Index: clang/test/PCH/fuzzy-pch-msvc.c
===
--- /dev/null
+++ clang/test/PCH/fuzzy-pch-msvc.c
@@ -0,0 +1,32 @@
+// Test -D and -U interaction with a PCH when running clang-cl.
+
+// RUN: %clang_cl -DFOO -Yc %S/variables.h
+
+// RUN: not %clang_cl -c -DFOO=blah -DBAR=int -Wno-microsoft-include -Yuvariables.h %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-FOO %s < %t.err
+
+// RUN: not %clang_cl -c -UFOO -DBAR=int -Wno-microsoft-include -Yuvariables.h %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-NOFOO %s < %t.err 
+
+// RUN: %clang_cl -c -DBAR=int -Wno-microsoft-include -Yuvariables.h -Xclang -verify %s 
+#include "variables.h"
+
+BAR bar = 17;
+
+#ifndef FOO
+#  error FOO was not defined
+#endif
+
+#if FOO != 1
+#  error FOO has the wrong definition
+#endif
+
+#ifndef BAR
+#  error BAR was not defined 
+#endif
+
+
+// CHECK-FOO: definition of macro 'FOO' differs between the precompiled header ('1') and the command line ('blah')
+// CHECK-NOFOO: macro 'FOO' was defined in the precompiled header but undef'd on the command line
+
+// expected-warning@1 {{definition of macro 'BAR' does not match definition in precompiled header}}
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2727,7 +2727,9 @@
  /*Syntactic=*/LangOpts.MicrosoftExt))
   Diag(MI->getDefinitionLoc(), diag::warn_pp_macro_def_mismatch_with_pch)
   << MacroNameTok.getIdentifierInfo();
-return;
+// Issue the diagnostic but allow the change if msvc extensions are enabled
+if (!LangOpts.MicrosoftExt)
+  return;
   }
 
   // Finally, if this identifier already had a macro defined for it, verify that
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72405: Allow /D flags absent during PCH creation under msvc-compat

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

My change keeps the diagnostic so consumers can opt into the same enforcement 
that exists today.  Furthermore, the existing fuzzy-pch.c tests show that new 
-D flags are allowed under a "clangier" PCH structure.  None of the existing 
tests error on:

  BAR bar = 17;

when `-DBar=int` is only part of the the test file's command line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72405



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72405: Allow /D flags absent during PCH creation under msvc-compat

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added reviewers: rnk, thakis, hans.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Before this patch adding a new /D flag when compiling a source file that 
consumed a PCH with clang-cl would issue a diagnostic and then fail.  With the 
patch, the diagnostic is still issued but the definition is accepted.  This 
matches the msvc behavior.  The fuzzy-pch-msvc.c is a clone of the existing 
fuzzy-pch.c tests with some msvc specific rework.

msvc diagnostic:

  warning C4605: '/DBAR=int' specified on current command line, but was not 
specified when precompiled header was built

Output of the CHECK-BAR test prior to the code change:

  (1,9): warning: definition of macro 'BAR' does not match definition 
in precompiled header [-Wclang-cl-pch]
  #define BAR int
  ^
  D:\repos\llvm\llvm-project\clang\test\PCH\fuzzy-pch-msvc.c(12,1): error: 
unknown type name 'BAR'
  BAR bar = 17;
  ^
  D:\repos\llvm\llvm-project\clang\test\PCH\fuzzy-pch-msvc.c(23,4): error: BAR 
was not defined
  #  error BAR was not defined
 ^
  1 warning and 2 errors generated.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72405

Files:
  clang/lib/Lex/PPDirectives.cpp
  clang/test/PCH/fuzzy-pch-msvc.c


Index: clang/test/PCH/fuzzy-pch-msvc.c
===
--- /dev/null
+++ clang/test/PCH/fuzzy-pch-msvc.c
@@ -0,0 +1,32 @@
+// Test -D and -U interaction with a PCH when running clang-cl.
+
+// RUN: %clang_cl -DFOO -Yc %S/variables.h
+// RUN: %clang_cl -c -DBAR=int -Wno-microsoft-include -Yuvariables.h %s
+// RUN: %clang_cl -c -DFOO -DBAR=int -Wno-microsoft-include -Yuvariables.h %s
+// RUN: not %clang_cl -c -DFOO=blah -DBAR=int -Wno-microsoft-include 
-Yuvariables.h %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-FOO %s < %t.err
+// RUN: not %clang_cl -c -UFOO -DBAR=int -Wno-microsoft-include -Yuvariables.h 
%s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-NOFOO %s < %t.err
+
+// RUN: not %clang_cl -c -DBAR=int -WX -Wno-microsoft-include -Yuvariables.h 
%s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-BAR %s < %t.err
+
+#include "variables.h"
+
+BAR bar = 17;
+
+#ifndef FOO
+#  error FOO was not defined
+#endif
+
+#if FOO != 1
+#  error FOO has the wrong definition
+#endif
+
+#ifndef BAR
+#  error BAR was not defined
+#endif
+
+// CHECK-FOO: definition of macro 'FOO' differs between the precompiled header 
('1') and the command line ('blah')
+// CHECK-NOFOO: macro 'FOO' was defined in the precompiled header but undef'd 
on the command line
+// CHECK-BAR: error: definition of macro 'BAR' does not match definition in 
precompiled header [-Werror,-Wclang-cl-pch]
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2727,7 +2727,9 @@
  /*Syntactic=*/LangOpts.MicrosoftExt))
   Diag(MI->getDefinitionLoc(), diag::warn_pp_macro_def_mismatch_with_pch)
   << MacroNameTok.getIdentifierInfo();
-return;
+// Issue the diagnostic but allow the change under msvc compatability mode
+if (!getLangOpts().MSVCCompat)
+  return;
   }
 
   // Finally, if this identifier already had a macro defined for it, verify 
that


Index: clang/test/PCH/fuzzy-pch-msvc.c
===
--- /dev/null
+++ clang/test/PCH/fuzzy-pch-msvc.c
@@ -0,0 +1,32 @@
+// Test -D and -U interaction with a PCH when running clang-cl.
+
+// RUN: %clang_cl -DFOO -Yc %S/variables.h
+// RUN: %clang_cl -c -DBAR=int -Wno-microsoft-include -Yuvariables.h %s
+// RUN: %clang_cl -c -DFOO -DBAR=int -Wno-microsoft-include -Yuvariables.h %s
+// RUN: not %clang_cl -c -DFOO=blah -DBAR=int -Wno-microsoft-include -Yuvariables.h %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-FOO %s < %t.err
+// RUN: not %clang_cl -c -UFOO -DBAR=int -Wno-microsoft-include -Yuvariables.h %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-NOFOO %s < %t.err
+
+// RUN: not %clang_cl -c -DBAR=int -WX -Wno-microsoft-include -Yuvariables.h %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-BAR %s < %t.err
+
+#include "variables.h"
+
+BAR bar = 17;
+
+#ifndef FOO
+#  error FOO was not defined
+#endif
+
+#if FOO != 1
+#  error FOO has the wrong definition
+#endif
+
+#ifndef BAR
+#  error BAR was not defined
+#endif
+
+// CHECK-FOO: definition of macro 'FOO' differs between the precompiled header ('1') and the command line ('blah')
+// CHECK-NOFOO: macro 'FOO' was defined in the precompiled header but undef'd on the command line
+// CHECK-BAR: error: definition of macro 'BAR' does not match definition in precompiled header [-Werror,-Wclang-cl-pch]
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2727,7 +2727,9 @@

[PATCH] D70615: Add an -fno-temp-file flag for compilation

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 234563.
zahen added a comment.

Fixed MemorySanitizer: use-of-uninitialized-value warning


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

https://reviews.llvm.org/D70615

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/test/Driver/clang_f_opts.c


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -571,3 +571,6 @@
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-GOOD-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-BAD: hasn't been enabled
+
+// RUN: %clang -### -S -fno-temp-file %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-TEMP-FILE %s
+// CHECK-NO-TEMP-FILE: "-fno-temp-file"
\ No newline at end of file
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -140,7 +140,7 @@
   std::unique_ptr OS =
   CI.createOutputFile(CI.getFrontendOpts().OutputFile, /*Binary=*/true,
   /*RemoveFileOnSignal=*/false, InFile,
-  /*Extension=*/"", /*UseTemporary=*/true);
+  /*Extension=*/"", CI.getFrontendOpts().UseTemporary);
   if (!OS)
 return nullptr;
 
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1886,6 +1886,7 @@
   Opts.ModulesEmbedFiles = Args.getAllArgValues(OPT_fmodules_embed_file_EQ);
   Opts.ModulesEmbedAllFiles = Args.hasArg(OPT_fmodules_embed_all_files);
   Opts.IncludeTimestamps = !Args.hasArg(OPT_fno_pch_timestamp);
+  Opts.UseTemporary = !Args.hasArg(OPT_fno_temp_file);
 
   Opts.CodeCompleteOpts.IncludeMacros
 = Args.hasArg(OPT_code_completion_macros);
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -672,7 +672,7 @@
   StringRef Extension) {
   return createOutputFile(getFrontendOpts().OutputFile, Binary,
   /*RemoveFileOnSignal=*/true, InFile, Extension,
-  /*UseTemporary=*/true);
+  getFrontendOpts().UseTemporary);
 }
 
 std::unique_ptr CompilerInstance::createNullOutputFile() {
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4688,6 +4688,7 @@
   Args.AddLastArg(CmdArgs, options::OPT_ftime_trace_granularity_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_ftrapv);
   Args.AddLastArg(CmdArgs, options::OPT_malign_double);
+  Args.AddLastArg(CmdArgs, options::OPT_fno_temp_file);
 
   if (Arg *A = Args.getLastArg(options::OPT_ftrapv_handler_EQ)) {
 CmdArgs.push_back("-ftrapv-handler");
Index: clang/include/clang/Frontend/FrontendOptions.h
===
--- clang/include/clang/Frontend/FrontendOptions.h
+++ clang/include/clang/Frontend/FrontendOptions.h
@@ -294,6 +294,9 @@
   /// Whether timestamps should be written to the produced PCH file.
   unsigned IncludeTimestamps : 1;
 
+  /// Should a temporary file be used during compilation.
+  unsigned UseTemporary : 1;
+
   CodeCompleteOptions CodeCompleteOpts;
 
   /// Specifies the output format of the AST.
@@ -442,7 +445,7 @@
 UseGlobalModuleIndex(true), GenerateGlobalModuleIndex(true),
 ASTDumpDecls(false), ASTDumpLookups(false),
 BuildingImplicitModule(false), ModulesEmbedAllFiles(false),
-IncludeTimestamps(true), TimeTraceGranularity(500) {}
+IncludeTimestamps(true), UseTemporary(true), TimeTraceGranularity(500) 
{}
 
   /// getInputKindForExtension - Return the appropriate input kind for a file
   /// extension. For example, "c" would return Language::C.
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1559,6 +1559,9 @@
 def fno_strict_vtable_pointers: Flag<["-"], "fno-strict-vtable-pointers">,
   Group;
 def fno_strict_overflow : Flag<["-"], "fno-strict-overflow">, Group;
+def fno_temp_file : Flag<["-"], "fno-temp-file">, Group,
+  Flags<[CC1Option, CoreOption]>, HelpText<
+  "Directly create compilation output files. This may lead to incorrect 
incremental builds if the compil

[PATCH] D70615: Add an -fno-temp-file flag for compilation

[Zachary Henkel via Phabricator via cfe-commits]

zahen reopened this revision.
zahen added a comment.
This revision is now accepted and ready to land.

Sorry about that I'll add a corrected patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70615



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71439: Allow redeclaration of __declspec(uuid)

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

@rnk Can you please submit on my behalf.  I don't have commit access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71439



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70615: Add an -fno-temp-file flag for compilation

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

Any additional changes required?  If not could someone please submit on my 
behalf?  @rnk, @hans, @thakis ?


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

https://reviews.llvm.org/D70615



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71439: Allow redeclaration of __declspec(uuid)

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added reviewers: rnk, thakis, hans, zturner.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

msvc allows a subsequent declaration of a uuid attribute on a struct/class.  
Mirror this behavior in clang-cl.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71439

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaCXX/ms-uuid.cpp


Index: clang/test/SemaCXX/ms-uuid.cpp
===
--- clang/test/SemaCXX/ms-uuid.cpp
+++ clang/test/SemaCXX/ms-uuid.cpp
@@ -106,3 +106,9 @@
 }
 
 }
+
+// Test class/struct redeclaration where the subsequent
+// declaration has a uuid attribute
+struct X{};
+
+struct __declspec(uuid("----")) X;
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5394,9 +5394,11 @@
   if (const auto *UA = D->getAttr()) {
 if (UA->getGuid().equals_lower(Uuid))
   return nullptr;
-Diag(UA->getLocation(), diag::err_mismatched_uuid);
-Diag(CI.getLoc(), diag::note_previous_uuid);
-D->dropAttr();
+if (!UA->getGuid().empty()) {
+  Diag(UA->getLocation(), diag::err_mismatched_uuid);
+  Diag(CI.getLoc(), diag::note_previous_uuid);
+  D->dropAttr();
+}
   }
 
   return ::new (Context) UuidAttr(Context, CI, Uuid);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -2678,6 +2678,10 @@
   // C's _Noreturn is allowed to be added to a function after it is 
defined.
   ++I;
   continue;
+} else if (isa(NewAttribute)) {
+  // msvc will allow a subsequent definition to add an uuid to a class
+  ++I;
+  continue;
 } else if (const AlignedAttr *AA = dyn_cast(NewAttribute)) {
   if (AA->isAlignas()) {
 // C++11 [dcl.align]p6:


Index: clang/test/SemaCXX/ms-uuid.cpp
===
--- clang/test/SemaCXX/ms-uuid.cpp
+++ clang/test/SemaCXX/ms-uuid.cpp
@@ -106,3 +106,9 @@
 }
 
 }
+
+// Test class/struct redeclaration where the subsequent
+// declaration has a uuid attribute
+struct X{};
+
+struct __declspec(uuid("----")) X;
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5394,9 +5394,11 @@
   if (const auto *UA = D->getAttr()) {
 if (UA->getGuid().equals_lower(Uuid))
   return nullptr;
-Diag(UA->getLocation(), diag::err_mismatched_uuid);
-Diag(CI.getLoc(), diag::note_previous_uuid);
-D->dropAttr();
+if (!UA->getGuid().empty()) {
+  Diag(UA->getLocation(), diag::err_mismatched_uuid);
+  Diag(CI.getLoc(), diag::note_previous_uuid);
+  D->dropAttr();
+}
   }
 
   return ::new (Context) UuidAttr(Context, CI, Uuid);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -2678,6 +2678,10 @@
   // C's _Noreturn is allowed to be added to a function after it is defined.
   ++I;
   continue;
+} else if (isa(NewAttribute)) {
+  // msvc will allow a subsequent definition to add an uuid to a class
+  ++I;
+  continue;
 } else if (const AlignedAttr *AA = dyn_cast(NewAttribute)) {
   if (AA->isAlignas()) {
 // C++11 [dcl.align]p6:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70615: Add an -fno-temp-file flag for compilation

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

Thanks Hans!  I'll need someone to do the actual submission since I don't have 
commit rights.


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

https://reviews.llvm.org/D70615



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70615: Add an -fno-temp-file flag for compilation

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

In our build system every file access in an "output directory" needs to be 
accounted for.  Until this patch, the random temporary file name has forced us 
to rely on workarounds that hurt build throughput.

I've uploaded example ProcMon dumps of clang-cl.exe file accesses to help 
illustrate what the patch is trying to achieve.  In the RTM log, clang-cl 
creates a file ConsoleApplication1-dc91d1ce.obj.tmp directly in the output 
directory, writes the contents and ends by calling SetRenameInformationFile to 
"create" the final ConsoleApplication1.obj.  In the patched log the file 
ConsoleApplication1.obj is created and written to directly.

F11003490: clang9.0-RTM.CSV 
F11003493: clang9.0-patched.CSV 


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

https://reviews.llvm.org/D70615



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70615: Add an -fno-temp-file flag for compilation

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

In D70615#1770550 , @hans wrote:

> Seems fine to me. I'm not sure how to test the actual "don't write to temp 
> file" functionality, but at least there could be a test to check that the 
> flag gets forwarded to cc1.


Added a best guess on the flag forwarding test case


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

https://reviews.llvm.org/D70615



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70615: Add an -fno-temp-file flag for compilation

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 232442.
zahen added a comment.

Fixed patch


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

https://reviews.llvm.org/D70615

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/test/Driver/clang_f_opts.c


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -571,3 +571,6 @@
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-GOOD-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-BAD: hasn't been enabled
+
+// RUN: %clang -### -S -fno-temp-file %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-TEMP-FILE %s
+// CHECK-NO-TEMP-FILE: "-fno-temp-file"
\ No newline at end of file
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -140,7 +140,7 @@
   std::unique_ptr OS =
   CI.createOutputFile(CI.getFrontendOpts().OutputFile, /*Binary=*/true,
   /*RemoveFileOnSignal=*/false, InFile,
-  /*Extension=*/"", /*UseTemporary=*/true);
+  /*Extension=*/"", CI.getFrontendOpts().UseTemporary);
   if (!OS)
 return nullptr;
 
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1886,6 +1886,7 @@
   Opts.ModulesEmbedFiles = Args.getAllArgValues(OPT_fmodules_embed_file_EQ);
   Opts.ModulesEmbedAllFiles = Args.hasArg(OPT_fmodules_embed_all_files);
   Opts.IncludeTimestamps = !Args.hasArg(OPT_fno_pch_timestamp);
+  Opts.UseTemporary = !Args.hasArg(OPT_fno_temp_file);
 
   Opts.CodeCompleteOpts.IncludeMacros
 = Args.hasArg(OPT_code_completion_macros);
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -672,7 +672,7 @@
   StringRef Extension) {
   return createOutputFile(getFrontendOpts().OutputFile, Binary,
   /*RemoveFileOnSignal=*/true, InFile, Extension,
-  /*UseTemporary=*/true);
+  getFrontendOpts().UseTemporary);
 }
 
 std::unique_ptr CompilerInstance::createNullOutputFile() {
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4688,6 +4688,7 @@
   Args.AddLastArg(CmdArgs, options::OPT_ftime_trace_granularity_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_ftrapv);
   Args.AddLastArg(CmdArgs, options::OPT_malign_double);
+  Args.AddLastArg(CmdArgs, options::OPT_fno_temp_file);
 
   if (Arg *A = Args.getLastArg(options::OPT_ftrapv_handler_EQ)) {
 CmdArgs.push_back("-ftrapv-handler");
Index: clang/include/clang/Frontend/FrontendOptions.h
===
--- clang/include/clang/Frontend/FrontendOptions.h
+++ clang/include/clang/Frontend/FrontendOptions.h
@@ -294,6 +294,9 @@
   /// Whether timestamps should be written to the produced PCH file.
   unsigned IncludeTimestamps : 1;
 
+  /// Should a temporary file be used during compilation.
+  unsigned UseTemporary : 1;
+
   CodeCompleteOptions CodeCompleteOpts;
 
   /// Specifies the output format of the AST.
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1559,6 +1559,9 @@
 def fno_strict_vtable_pointers: Flag<["-"], "fno-strict-vtable-pointers">,
   Group;
 def fno_strict_overflow : Flag<["-"], "fno-strict-overflow">, Group;
+def fno_temp_file : Flag<["-"], "fno-temp-file">, Group,
+  Flags<[CC1Option, CoreOption]>, HelpText<
+  "Directly create compilation output files. This may lead to incorrect 
incremental builds if the compiler crashes">;
 def fno_threadsafe_statics : Flag<["-"], "fno-threadsafe-statics">, 
Group,
   Flags<[CC1Option]>, HelpText<"Do not emit code to make initialization of 
local statics thread safe">;
 def fno_use_cxa_atexit : Flag<["-"], "fno-use-cxa-atexit">, Group, 
Flags<[CC1Option]>,


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -571,3 +571,6 @@
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
 // CH

[PATCH] D70615: Add an -fno-temp-file flag for compilation

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 232441.
zahen added a comment.

Updated with review feedback and formatting fixes


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

https://reviews.llvm.org/D70615

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/clang_f_opts.c


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -571,3 +571,6 @@
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-GOOD-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-BAD: hasn't been enabled
+
+// RUN: %clang -### -S -fno-temp-file %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-TEMP-FILE %s
+// CHECK-NO-TEMP-FILE: "-fno-temp-file"
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1886,7 +1886,7 @@
   Opts.ModulesEmbedFiles = Args.getAllArgValues(OPT_fmodules_embed_file_EQ);
   Opts.ModulesEmbedAllFiles = Args.hasArg(OPT_fmodules_embed_all_files);
   Opts.IncludeTimestamps = !Args.hasArg(OPT_fno_pch_timestamp);
-  Opts.UseTemporary = !Args.hasArg(OPT_fno_temp_file); 
+  Opts.UseTemporary = !Args.hasArg(OPT_fno_temp_file);
 
   Opts.CodeCompleteOpts.IncludeMacros
 = Args.hasArg(OPT_code_completion_macros);
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -672,7 +672,7 @@
   StringRef Extension) {
   return createOutputFile(getFrontendOpts().OutputFile, Binary,
   /*RemoveFileOnSignal=*/true, InFile, Extension,
-  getFrontendOpts().UseTemporary );
+  getFrontendOpts().UseTemporary);
 }
 
 std::unique_ptr CompilerInstance::createNullOutputFile() {
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1560,7 +1560,8 @@
   Group;
 def fno_strict_overflow : Flag<["-"], "fno-strict-overflow">, Group;
 def fno_temp_file : Flag<["-"], "fno-temp-file">, Group,
-  Flags<[CC1Option, CoreOption]>, HelpText<"Do not create a temp file during 
compilation">;
+  Flags<[CC1Option, CoreOption]>, HelpText<
+  "Directly create compilation output files. This may lead to incorrect 
incremental builds if the compiler crashes">;
 def fno_threadsafe_statics : Flag<["-"], "fno-threadsafe-statics">, 
Group,
   Flags<[CC1Option]>, HelpText<"Do not emit code to make initialization of 
local statics thread safe">;
 def fno_use_cxa_atexit : Flag<["-"], "fno-use-cxa-atexit">, Group, 
Flags<[CC1Option]>,


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -571,3 +571,6 @@
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-GOOD-NOT: hasn't been enabled
 // CHECK-TRIVIAL-ZERO-BAD: hasn't been enabled
+
+// RUN: %clang -### -S -fno-temp-file %s 2>&1 | FileCheck -check-prefix=CHECK-NO-TEMP-FILE %s
+// CHECK-NO-TEMP-FILE: "-fno-temp-file"
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1886,7 +1886,7 @@
   Opts.ModulesEmbedFiles = Args.getAllArgValues(OPT_fmodules_embed_file_EQ);
   Opts.ModulesEmbedAllFiles = Args.hasArg(OPT_fmodules_embed_all_files);
   Opts.IncludeTimestamps = !Args.hasArg(OPT_fno_pch_timestamp);
-  Opts.UseTemporary = !Args.hasArg(OPT_fno_temp_file); 
+  Opts.UseTemporary = !Args.hasArg(OPT_fno_temp_file);
 
   Opts.CodeCompleteOpts.IncludeMacros
 = Args.hasArg(OPT_code_completion_macros);
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -672,7 +672,7 @@
   StringRef Extension) {
   return createOutputFile(getFrontendOpts().OutputFile, Binary,
   /*RemoveFileOnSignal=*/true, InFile, Extension,
-  getFrontendOpts().UseTemporary );
+  getFrontendOpts().UseTemporary);
 }
 
 std::unique_ptr CompilerInstance::createNullOutputFile() {
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Optio

[PATCH] D70615: Add an -fno-temp-file flag for compilation

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added reviewers: rsmith, rnk, zturner.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Our build system does not handle randomly named files created during the build 
well.  We'd prefer to write compilation output directly without creating a 
temporary file.  Function parameters already existed to control this behavior 
but were not exposed all the way out to the command line.

I'm open to suggestions what kinds of tests could be added alongside the 
change.  Anecdotally, this code been running in internal production builds for 
months.

I do not have commit access so I will need someone else to submit the patch 
once it's accepted.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70615

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/FrontendActions.cpp


Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -140,7 +140,7 @@
   std::unique_ptr OS =
   CI.createOutputFile(CI.getFrontendOpts().OutputFile, /*Binary=*/true,
   /*RemoveFileOnSignal=*/false, InFile,
-  /*Extension=*/"", /*UseTemporary=*/true);
+  /*Extension=*/"", CI.getFrontendOpts().UseTemporary);
   if (!OS)
 return nullptr;
 
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1886,6 +1886,7 @@
   Opts.ModulesEmbedFiles = Args.getAllArgValues(OPT_fmodules_embed_file_EQ);
   Opts.ModulesEmbedAllFiles = Args.hasArg(OPT_fmodules_embed_all_files);
   Opts.IncludeTimestamps = !Args.hasArg(OPT_fno_pch_timestamp);
+  Opts.UseTemporary = !Args.hasArg(OPT_fno_temp_file); 
 
   Opts.CodeCompleteOpts.IncludeMacros
 = Args.hasArg(OPT_code_completion_macros);
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -672,7 +672,7 @@
   StringRef Extension) {
   return createOutputFile(getFrontendOpts().OutputFile, Binary,
   /*RemoveFileOnSignal=*/true, InFile, Extension,
-  /*UseTemporary=*/true);
+  getFrontendOpts().UseTemporary );
 }
 
 std::unique_ptr CompilerInstance::createNullOutputFile() {
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4688,6 +4688,7 @@
   Args.AddLastArg(CmdArgs, options::OPT_ftime_trace_granularity_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_ftrapv);
   Args.AddLastArg(CmdArgs, options::OPT_malign_double);
+  Args.AddLastArg(CmdArgs, options::OPT_fno_temp_file);
 
   if (Arg *A = Args.getLastArg(options::OPT_ftrapv_handler_EQ)) {
 CmdArgs.push_back("-ftrapv-handler");
Index: clang/include/clang/Frontend/FrontendOptions.h
===
--- clang/include/clang/Frontend/FrontendOptions.h
+++ clang/include/clang/Frontend/FrontendOptions.h
@@ -294,6 +294,9 @@
   /// Whether timestamps should be written to the produced PCH file.
   unsigned IncludeTimestamps : 1;
 
+  /// Should a temporary file be used during compilation.
+  unsigned UseTemporary : 1;
+
   CodeCompleteOptions CodeCompleteOpts;
 
   /// Specifies the output format of the AST.
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1559,6 +1559,8 @@
 def fno_strict_vtable_pointers: Flag<["-"], "fno-strict-vtable-pointers">,
   Group;
 def fno_strict_overflow : Flag<["-"], "fno-strict-overflow">, Group;
+def fno_temp_file : Flag<["-"], "fno-temp-file">, Group,
+  Flags<[CC1Option, CoreOption]>, HelpText<"Do not create a temp file during 
compilation">;
 def fno_threadsafe_statics : Flag<["-"], "fno-threadsafe-statics">, 
Group,
   Flags<[CC1Option]>, HelpText<"Do not emit code to make initialization of 
local statics thread safe">;
 def fno_use_cxa_atexit : Flag<["-"], "fno-use-cxa-atexit">, Group, 
Flags<[CC1Option]>,


Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -140,7 +140,7 @@
   std::unique_ptr OS =
   CI.createOutputFile(CI.getFrontendOpts()

[PATCH] D67590: Properly ignore mismatched exception specifiers in MSVC Compat mode.

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

I just checked a trivial mismatch example in clang 8.0 
(https://godbolt.org/z/wiSAp6) and I missed how the compatibility mode 
operates.  I withdraw my suggestion since the patch keeps consistency with the 
existing behavior.  Thanks for the `-Werror=microsoft-exception-spec` 
suggestion that's exactly what our project wants.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67590



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67590: Properly ignore mismatched exception specifiers in MSVC Compat mode.

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

Is there any interest in supporting the cl.exe flag `/permissive-`?  I 
considered a hard error on mismatched exception specifier in clang-cl a 
feature, not a bug.  If msvc compat mode respected that flag this could 
continue to be an error with that flag set (and upgraded strictness in other 
cases).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67590



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62752: Move VarBypassDector.h to include/clang/CodeGen

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added a reviewer: chandlerc.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Allow the VarBypassDetector class to be consumed by external tools by exposing 
its header from a standard include path.

If accepted, I will need a user with check-in permission to apply the patch.


Repository:
  rC Clang

https://reviews.llvm.org/D62752

Files:
  clang/include/clang/CodeGen/VarBypassDetector.h
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/VarBypassDetector.cpp
  clang/lib/CodeGen/VarBypassDetector.h


Index: clang/lib/CodeGen/VarBypassDetector.cpp
===
--- clang/lib/CodeGen/VarBypassDetector.cpp
+++ clang/lib/CodeGen/VarBypassDetector.cpp
@@ -6,7 +6,7 @@
 //
 
//===--===//
 
-#include "VarBypassDetector.h"
+#include "clang/CodeGen/VarBypassDetector.h"
 
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -20,7 +20,6 @@
 #include "CodeGenModule.h"
 #include "CodeGenPGO.h"
 #include "EHScopeStack.h"
-#include "VarBypassDetector.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/CurrentSourceLocExprScope.h"
 #include "clang/AST/ExprCXX.h"
@@ -32,6 +31,7 @@
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/OpenMPKinds.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/CodeGen/VarBypassDetector.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/MapVector.h"
Index: clang/include/clang/CodeGen/VarBypassDetector.h
===
--- clang/include/clang/CodeGen/VarBypassDetector.h
+++ clang/include/clang/CodeGen/VarBypassDetector.h
@@ -11,8 +11,8 @@
 //
 
//===--===//
 
-#ifndef LLVM_CLANG_LIB_CODEGEN_VARBYPASSDETECTOR_H
-#define LLVM_CLANG_LIB_CODEGEN_VARBYPASSDETECTOR_H
+#ifndef LLVM_CLANG_CODEGEN_VARBYPASSDETECTOR_H
+#define LLVM_CLANG_CODEGEN_VARBYPASSDETECTOR_H
 
 #include "clang/AST/Decl.h"
 #include "llvm/ADT/DenseMap.h"


Index: clang/lib/CodeGen/VarBypassDetector.cpp
===
--- clang/lib/CodeGen/VarBypassDetector.cpp
+++ clang/lib/CodeGen/VarBypassDetector.cpp
@@ -6,7 +6,7 @@
 //
 //===--===//
 
-#include "VarBypassDetector.h"
+#include "clang/CodeGen/VarBypassDetector.h"
 
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -20,7 +20,6 @@
 #include "CodeGenModule.h"
 #include "CodeGenPGO.h"
 #include "EHScopeStack.h"
-#include "VarBypassDetector.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/CurrentSourceLocExprScope.h"
 #include "clang/AST/ExprCXX.h"
@@ -32,6 +31,7 @@
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/OpenMPKinds.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/CodeGen/VarBypassDetector.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/MapVector.h"
Index: clang/include/clang/CodeGen/VarBypassDetector.h
===
--- clang/include/clang/CodeGen/VarBypassDetector.h
+++ clang/include/clang/CodeGen/VarBypassDetector.h
@@ -11,8 +11,8 @@
 //
 //===--===//
 
-#ifndef LLVM_CLANG_LIB_CODEGEN_VARBYPASSDETECTOR_H
-#define LLVM_CLANG_LIB_CODEGEN_VARBYPASSDETECTOR_H
+#ifndef LLVM_CLANG_CODEGEN_VARBYPASSDETECTOR_H
+#define LLVM_CLANG_CODEGEN_VARBYPASSDETECTOR_H
 
 #include "clang/AST/Decl.h"
 #include "llvm/ADT/DenseMap.h"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57189: Fix compatibility with the msvc AI compiler option

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added reviewers: rnk, zturner.
Herald added a subscriber: cfe-commits.

The unsupported /AI option accepts directory paths in a manner similar to the 
supported /I option and allows spaces between the flag and the directory.  An 
example snippet from our production compile that broke on clang-cl.exe because 
it read the path as an input file.

  /AI %WindowsSdkDir%\References\%WindowsSDKVersion% /I 
%WindowsSdkDir%\Include\%WindowsSDKVersion%\winrt


Repository:
  rC Clang

https://reviews.llvm.org/D57189

Files:
  include/clang/Driver/CLCompatOptions.td


Index: include/clang/Driver/CLCompatOptions.td
===
--- include/clang/Driver/CLCompatOptions.td
+++ include/clang/Driver/CLCompatOptions.td
@@ -394,7 +394,7 @@
 
 // Unsupported:
 
-def _SLASH_AI : CLJoined<"AI">;
+def _SLASH_AI : CLJoinedOrSeparate<"AI">;
 def _SLASH_Bt : CLFlag<"Bt">;
 def _SLASH_Bt_plus : CLFlag<"Bt+">;
 def _SLASH_clr : CLJoined<"clr">;


Index: include/clang/Driver/CLCompatOptions.td
===
--- include/clang/Driver/CLCompatOptions.td
+++ include/clang/Driver/CLCompatOptions.td
@@ -394,7 +394,7 @@
 
 // Unsupported:
 
-def _SLASH_AI : CLJoined<"AI">;
+def _SLASH_AI : CLJoinedOrSeparate<"AI">;
 def _SLASH_Bt : CLFlag<"Bt">;
 def _SLASH_Bt_plus : CLFlag<"Bt+">;
 def _SLASH_clr : CLJoined<"clr">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55685: Update Microsoft name mangling scheme for exception specifiers in the type system

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

I don't have check-in permission, so I'd appreciate if someone could handle the 
actual commit.


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

https://reviews.llvm.org/D55685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55685: Update Microsoft name mangling scheme for exception specifiers in the type system

[Zachary Henkel via Phabricator via cfe-commits]

zahen updated this revision to Diff 178490.
zahen added a comment.

Added support for msvc minor version as requested.  Tied the updated mangling 
scheme to C++17 & compatibility mode > 1912 (15.5).  Added additional tests.


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

https://reviews.llvm.org/D55685

Files:
  include/clang/Basic/LangOptions.h
  lib/AST/MicrosoftMangle.cpp
  lib/Driver/ToolChains/MSVC.cpp
  test/CodeGenCXX/mangle-ms-exception-spec.cpp

Index: test/CodeGenCXX/mangle-ms-exception-spec.cpp
===
--- test/CodeGenCXX/mangle-ms-exception-spec.cpp
+++ test/CodeGenCXX/mangle-ms-exception-spec.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -std=c++11 -fms-extensions -emit-llvm %s -o - -triple=x86_64-pc-win32 -Wno-noexcept-type -fms-compatibility-version=19.12 | FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
+// RUN: %clang_cc1 -std=c++17 -fms-extensions -emit-llvm %s -o - -triple=x86_64-pc-win32 | FileCheck %s --check-prefix=CHECK --check-prefix=NOCOMPAT
+// RUN: %clang_cc1 -std=c++17 -fms-extensions -emit-llvm %s -o - -triple=x86_64-pc-win32 -fms-compatibility-version=19.12 | FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
+
+// Prove that mangling only changed for noexcept types under /std:C++17, not all noexcept functions
+// CHECK-DAG: @"?nochange@@YAXXZ"
+void nochange() noexcept {}
+
+// CXX11-DAG: @"?a@@YAXP6AHXZ@Z"
+// NOCOMPAT-DAG: @"?a@@YAXP6AHXZ@Z"
+// CXX17-DAG: @"?a@@YAXP6AHX_E@Z"
+void a(int() noexcept) {}
+// CHECK-DAG: @"?b@@YAXP6AHXZ@Z"
+void b(int() noexcept(false)) {}
+// CXX11-DAG: @"?c@@YAXP6AHXZ@Z"
+// NOCOMPAT-DAG: @"?c@@YAXP6AHXZ@Z"
+// CXX17-DAG: @"?c@@YAXP6AHX_E@Z"
+void c(int() noexcept(true)) {}
+// CHECK-DAG: @"?d@@YAXP6AHXZ@Z"
+void d(int()) {}
+
+template 
+class e;
+template 
+class e {
+  // CXX11-DAG: @"?ee@?$e@$$A6AXXZ@@EEAAXXZ"
+  // NOCOMPAT-DAG: @"?ee@?$e@$$A6AXXZ@@EEAAXXZ"
+  // CXX17-DAG: @"?ee@?$e@$$A6AXX_E@@EEAAXXZ"
+  virtual T ee(U &&...) noexcept {};
+};
+
+e e1;
+
+template 
+class f;
+template 
+class f {
+  // CHECK-DAG: @"?ff@?$f@$$A6AXXZ@@EEAAXXZ"
+  virtual T ff(U &&...) noexcept {};
+};
+
+f f1;
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -1286,7 +1286,7 @@
   if (MSVT.empty() &&
   Args.hasFlag(options::OPT_fms_extensions, options::OPT_fno_ms_extensions,
IsWindowsMSVC)) {
-// -fms-compatibility-version=19.11 is default, aka 2017
+// -fms-compatibility-version=19.11 is default, aka 2017, 15.3
 MSVT = VersionTuple(19, 11);
   }
   return MSVT;
Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -315,7 +315,8 @@
   QualifierMangleMode QMM = QMM_Mangle);
   void mangleFunctionType(const FunctionType *T,
   const FunctionDecl *D = nullptr,
-  bool ForceThisQuals = false);
+  bool ForceThisQuals = false,
+  bool MangleExceptionSpec = true);
   void mangleNestedName(const NamedDecl *ND);
 
 private:
@@ -512,7 +513,7 @@
 
 mangleFunctionClass(FD);
 
-mangleFunctionType(FT, FD);
+mangleFunctionType(FT, FD, false, false);
   } else {
 Out << '9';
   }
@@ -2061,7 +2062,8 @@
 
 void MicrosoftCXXNameMangler::mangleFunctionType(const FunctionType *T,
  const FunctionDecl *D,
- bool ForceThisQuals) {
+ bool ForceThisQuals,
+ bool MangleExceptionSpec) {
   //  ::=  
   //   
   const FunctionProtoType *Proto = dyn_cast(T);
@@ -2194,7 +2196,12 @@
   Out << '@';
   }
 
-  mangleThrowSpecification(Proto);
+  if (MangleExceptionSpec && getASTContext().getLangOpts().CPlusPlus17 &&
+  getASTContext().getLangOpts().isCompatibleWithMSVC(
+  LangOptions::MSVC2017_5))
+mangleThrowSpecification(Proto);
+  else
+Out << 'Z';
 }
 
 void MicrosoftCXXNameMangler::mangleFunctionClass(const FunctionDecl *FD) {
@@ -2299,15 +2306,15 @@
 void MicrosoftCXXNameMangler::mangleCallingConvention(const FunctionType *T) {
   mangleCallingConvention(T->getCallConv());
 }
+
 void MicrosoftCXXNameMangler::mangleThrowSpecification(
 const FunctionProtoType *FT) {
-  //  ::= Z # throw(...) (default)
-  //  ::= @ # throw() or __declspec/__attribute__((nothrow))
-  //  ::= +
-  // NOTE: Since the Microsoft compiler ignores throw specifications, they are
-  // all actually mangled as 'Z'. (They're ignored because their associated
-  // functionality isn't implemented, and probably never will be.

[PATCH] D55685: Update Microsoft name mangling scheme for exception specifiers in the type system

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

In D55685#1331810 , @zturner wrote:

> BTW, as far as updating the demangler, as long as it doesn't crash or 
> generate an error on a valid `_E` mangling, that should be sufficient (with a 
> test).  If you want bonus points you can make it print out `noexcept` when 
> you see the `_E`, but I won't require it as it's a bit of extra work.  If you 
> decide not to do that, I'll just file a bug for it so that we don't forget.


I have the full set of demangler updates ready to go and will upload shortly 
(Monday at the latest due to travel)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55685: Update Microsoft name mangling scheme for exception specifiers in the type system

[Zachary Henkel via Phabricator via cfe-commits]

zahen added a comment.

In D55685#1330717 , @zturner wrote:

> Also we still need to put this behind `-fms-compatibility-version`.  Finally, 
> it would be nice if you could also update the demangler 
> (`llvm/lib/Demangle/MicrosoftDemangle.cpp`)


This was introduced in 15.5 (1912).  What's the preferred way to represent that 
in clang code?




Comment at: lib/AST/MicrosoftMangle.cpp:2311-2314
+  if (FT->canThrow())
+Out << 'Z';
+  else
+Out << "_E";

zturner wrote:
> I knew that the mangling changed whenever a pointer to a `noexcept` function 
> is passed as an argument, and we don't yet handle that, but I'm surprised to 
> hear that they changed an existing mangling, since it's a hard ABI break.
> 
> Do you know the major and minor version numbers that this changed in?  I'd 
> like to test it out for starters, but also since this is an ABI break we 
> would need to put it behind `-fms-compatibility-version` and only mangle 
> using the new scheme when the compatibility version is sufficiently high.
It's only when a function is used as a type.  My original rathole was trying to 
enumerate all of the places where that could be, but instead I settled on 
"everywhere but the initial definition".  It's why false is passed in the 4th 
parameter on line 516.

I've confirmed this changed in 15.5 so I'll use that as the compat version.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55685: Update Microsoft name mangling scheme for exception specifiers in the type system

[Zachary Henkel via Phabricator via cfe-commits]

zahen created this revision.
zahen added reviewers: zturner, rnk.
Herald added a subscriber: cfe-commits.

The msvc exception specifier for noexcept function types has changed from the 
prior default of "Z" to "_E" if the function cannot throw when compiling with 
/std:C++17.


Repository:
  rC Clang

https://reviews.llvm.org/D55685

Files:
  lib/AST/MicrosoftMangle.cpp
  test/CodeGenCXX/mangle-ms-exception-spec.cpp

Index: test/CodeGenCXX/mangle-ms-exception-spec.cpp
===
--- test/CodeGenCXX/mangle-ms-exception-spec.cpp
+++ test/CodeGenCXX/mangle-ms-exception-spec.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -std=c++11 -fms-extensions -emit-llvm %s -o - -triple=x86_64-pc-win32 -Wno-noexcept-type | FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
+// RUN: %clang_cc1 -std=c++17 -fms-extensions -emit-llvm %s -o - -triple=x86_64-pc-win32 | FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
+
+// CXX11-DAG: @"?a@@YAXP6AHXZ@Z"
+// CXX17-DAG: @"?a@@YAXP6AHX_E@Z"
+void a(int() noexcept) {}
+// CHECK-DAG: @"?b@@YAXP6AHXZ@Z"
+void b(int() noexcept(false)) {}
+// CXX11-DAG: @"?c@@YAXP6AHXZ@Z"
+// CXX17-DAG: @"?c@@YAXP6AHX_E@Z"
+void c(int() noexcept(true)) {}
+// CHECK-DAG: @"?d@@YAXP6AHXZ@Z"
+void d(int()) {}
+
+template 
+class e;
+template 
+class e {
+  // CXX11-DAG: @"?ee@?$e@$$A6AXXZ@@EEAAXXZ"
+  // CXX17-DAG: @"?ee@?$e@$$A6AXX_E@@EEAAXXZ"
+  virtual T ee(U &&...) noexcept {};
+};
+
+e e1;
+
+template 
+class f;
+template 
+class f {
+  // CHECK-DAG: @"?ff@?$f@$$A6AXXZ@@EEAAXXZ"
+  virtual T ff(U &&...) noexcept {};
+};
+
+f f1;
Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -315,7 +315,8 @@
   QualifierMangleMode QMM = QMM_Mangle);
   void mangleFunctionType(const FunctionType *T,
   const FunctionDecl *D = nullptr,
-  bool ForceThisQuals = false);
+  bool ForceThisQuals = false,
+  bool MangleExceptionSpec = true);
   void mangleNestedName(const NamedDecl *ND);
 
 private:
@@ -512,7 +513,7 @@
 
 mangleFunctionClass(FD);
 
-mangleFunctionType(FT, FD);
+mangleFunctionType(FT, FD, false, false);
   } else {
 Out << '9';
   }
@@ -2061,7 +2062,8 @@
 
 void MicrosoftCXXNameMangler::mangleFunctionType(const FunctionType *T,
  const FunctionDecl *D,
- bool ForceThisQuals) {
+ bool ForceThisQuals,
+ bool MangleExceptionSpec) {
   //  ::=  
   //   
   const FunctionProtoType *Proto = dyn_cast(T);
@@ -2194,7 +2196,10 @@
   Out << '@';
   }
 
-  mangleThrowSpecification(Proto);
+if (getASTContext().getLangOpts().CPlusPlus17 && MangleExceptionSpec)
+  mangleThrowSpecification(Proto);
+else
+  Out << 'Z';
 }
 
 void MicrosoftCXXNameMangler::mangleFunctionClass(const FunctionDecl *FD) {
@@ -2301,13 +2306,12 @@
 }
 void MicrosoftCXXNameMangler::mangleThrowSpecification(
 const FunctionProtoType *FT) {
-  //  ::= Z # throw(...) (default)
-  //  ::= @ # throw() or __declspec/__attribute__((nothrow))
-  //  ::= +
-  // NOTE: Since the Microsoft compiler ignores throw specifications, they are
-  // all actually mangled as 'Z'. (They're ignored because their associated
-  // functionality isn't implemented, and probably never will be.)
-  Out << 'Z';
+  //  ::= Z # (default)
+  //  ::= _E # noexcept
+  if (FT->canThrow())
+Out << 'Z';
+  else
+Out << "_E";
 }
 
 void MicrosoftCXXNameMangler::mangleType(const UnresolvedUsingType *T,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits