[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman added a comment.

> I believe that /Ze has more in common with `-fms-compatibility` than 
> `-fms-extensions`, but I could be wrong. Also, they may just be completely 
> different things at this point. `/permissive` is closer in spirit to 
> `-fms-compatibility`.

Better documentation at some point in the future would be helpful.

> In any case, if the has_builtin check works, I'd rather leave the macros 
> alone. There could be unintended consequences, and I'm not fully convinced 
> this is the right change.

Sounds good! That should also fix the aux triple issue with the `__cpuidex` 
patch as well (but not the underlying issue). Thank you for your insight and 
patience!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

> Also assuming that /Ze is similar to our -fms-extensions which I believe is 
> the case, but not completely sure

I believe that /Ze has more in common with `-fms-compatibility` than 
`-fms-extensions`, but I could be wrong. Also, they may just be completely 
different things at this point. `/permissive` is closer in spirit to 
`-fms-compatibility`.

In any case, if the has_builtin check works, I'd rather leave the macros alone. 
There could be unintended consequences, and I'm not fully convinced this is the 
right change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman added a comment.

In D157334#4596328 , @rnk wrote:

> I don't think this is the right thing to do, I think I recall saying as much 
> on some other review. The MSVC docs 
> 
>  say that `/Za`/`/Ze` control `_MSC_EXTENSION`, and as I understand it, `/Za` 
> is more like our `-fms-compatibility` flag, so it makes sense to control this 
> macro with `-fms-compatibility`.

From what I can understand from this page 
,
 `/Ze` enables the MSVC extensions (which is set by default) and `/Za` disables 
the MSVC extensions. From what I can gather reading the Clang options docs 
, 
`-fms-compatibility` is intended to be a superset of `-fms-extensions`. 
Assuming those two things, I think it's logical to set `_MSC_EXTENSIONS` with 
`-fms-extensions` rather than `-fms-compatibility`. (Also assuming that `/Ze` 
is similar to our `-fms-extensions` which I believe is the case, but not 
completely sure).

> Even though the name of the macro and the flag correspond, they aren't 
> covering the same thing.
>
> Let's try to focus on the use case: We need a feature flag or macro to 
> communicate that `-fms-extensions` is enabled on mingw. `_MSC_VER` is out 
> because we're doing mingw. Is there something else we could check for like 
> `__has_declspec_attribute` or `__has_builtin`?

`__has_builtin` works (no idea why I didn't think of this, thank you very much 
for the suggestion) for that case.

Like you mentioned in the https://reviews.llvm.org/D150646, exposing the 
builtins with `-fms-extensions` doesn't make sense, and that should probably be 
fixed at some point. That should probably be left to another patch though, 
along with other issues related to when builtins get exposed (like the 
previously mentioned aux triple issue).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I don't think this is the right thing to do, I think I recall saying as much on 
some other review. The MSVC docs 

 say that `/Za`/`/Ze` control `_MSC_EXTENSION`, and as I understand it, `/Za` 
is more like our `-fms-compatibility` flag, so it makes sense to control this 
macro with `-fms-compatibility`.

Even though the name of the macro and the flag correspond, they aren't covering 
the same thing.

Let's try to focus on the use case: We need a feature flag or macro to 
communicate that `-fms-extensions` is enabled on mingw. `_MSC_VER` is out 
because we're doing mingw. Is there something else we could check for like 
`__has_declspec_attribute` or `__has_builtin`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman updated this revision to Diff 550529.
aidengrossman added a comment.

Hoist _MSC_EXTENSIONS macro logic even further and add release note on chages.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Basic/Targets/OSTargets.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/test/Preprocessor/predefined-win-macros.c


Index: clang/test/Preprocessor/predefined-win-macros.c
===
--- clang/test/Preprocessor/predefined-win-macros.c
+++ clang/test/Preprocessor/predefined-win-macros.c
@@ -120,6 +120,11 @@
 // CHECK-AMD64-MINGW: #define _WIN32 1
 // CHECK-AMD64-MINGW: #define _WIN64 1
 
+// RUN: %clang_cc1 -triple x86_64-windows-gnu %s -E -dM -o - -fms-extensions \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-AMD64-MINGW-MSE
+
+// CHECK-AMD64-MINGW-MSE: #define _MSC_EXTENSIONS 1
+
 // RUN: %clang_cc1 -triple aarch64-windows-gnu %s -E -dM -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-ARM64-MINGW
 
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -926,11 +926,19 @@
 Builder.defineMacro("__private_extern__", "extern");
 
   if (LangOpts.MicrosoftExt) {
+Builder.defineMacro("_MSC_EXTENSIONS");
+
 if (LangOpts.WChar) {
   // wchar_t supported as a keyword.
   Builder.defineMacro("_WCHAR_T_DEFINED");
   Builder.defineMacro("_NATIVE_WCHAR_T_DEFINED");
 }
+
+if (LangOpts.CPlusPlus11) {
+  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
+  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
+  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
+}
   }
 
   // Macros to help identify the narrow and wide character sets
Index: clang/lib/Basic/Targets/OSTargets.cpp
===
--- clang/lib/Basic/Targets/OSTargets.cpp
+++ clang/lib/Basic/Targets/OSTargets.cpp
@@ -226,16 +226,6 @@
 }
   }
 
-  if (Opts.MicrosoftExt) {
-Builder.defineMacro("_MSC_EXTENSIONS");
-
-if (Opts.CPlusPlus11) {
-  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
-  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
-  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
-}
-  }
-
   if (!Opts.MSVolatile)
 Builder.defineMacro("_ISO_VOLATILE");
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -110,6 +110,9 @@
 
 Modified Compiler Flags
 ---
+- ``-fms-extensions`` now defines the ``_MSC_EXTENSIONS`` macro regardless of
+  whether or not the ``-fms-compatibility`` flag is set and regardless of the
+  target triple.
 
 Removed Compiler Flags
 -


Index: clang/test/Preprocessor/predefined-win-macros.c
===
--- clang/test/Preprocessor/predefined-win-macros.c
+++ clang/test/Preprocessor/predefined-win-macros.c
@@ -120,6 +120,11 @@
 // CHECK-AMD64-MINGW: #define _WIN32 1
 // CHECK-AMD64-MINGW: #define _WIN64 1
 
+// RUN: %clang_cc1 -triple x86_64-windows-gnu %s -E -dM -o - -fms-extensions \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-AMD64-MINGW-MSE
+
+// CHECK-AMD64-MINGW-MSE: #define _MSC_EXTENSIONS 1
+
 // RUN: %clang_cc1 -triple aarch64-windows-gnu %s -E -dM -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-ARM64-MINGW
 
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -926,11 +926,19 @@
 Builder.defineMacro("__private_extern__", "extern");
 
   if (LangOpts.MicrosoftExt) {
+Builder.defineMacro("_MSC_EXTENSIONS");
+
 if (LangOpts.WChar) {
   // wchar_t supported as a keyword.
   Builder.defineMacro("_WCHAR_T_DEFINED");
   Builder.defineMacro("_NATIVE_WCHAR_T_DEFINED");
 }
+
+if (LangOpts.CPlusPlus11) {
+  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
+  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
+  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
+}
   }
 
   // Macros to help identify the narrow and wide character sets
Index: clang/lib/Basic/Targets/OSTargets.cpp
===
--- clang/lib/Basic/Targets/OSTargets.cpp
+++ clang/lib/Basic/Targets/OSTargets.cpp
@@ -226,16 +226,6 @@
 }
   }
 
-  if (Opts.MicrosoftExt) {
-Builder.defineMacro("_MSC_EXTENSIONS");
-
-if (Opts.CPlusPlus11) {
-  

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'm comfortable with the changes but we should add a release note so users know 
about the change in behavior. I also wonder if we should take this opportunity 
to add some user-facing documentation around how the target triple, 
`-fms-compatibility`, and `-fms-extensions` interact (what features do they 
enable, what macros do they define, how do they change ABI, etc)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D157334#4573902 , @aaron.ballman 
wrote:

> I'm curious to hear what others think about this (especially @rnk and @hans).

Seems reasonable to me.

(Looks like the define was originally added here: 
https://github.com/llvm/llvm-project/commit/4992ca4b17d82743c304f4c1b2da020f237d6b18)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D157334#4574197 , @aidengrossman 
wrote:

> It's interesting that GCC doesn't set the `_MSC_EXTENSIONS` macro with 
> `-fms-extensions`. It should if it wants to match the behavior of MSVC.

FWIW, I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110977 to see what 
the GCC devs think and hopefully we can proactively avoid diverging for too 
long here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman added a comment.

It's something set on by default in MSVC 
(https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170).
 It's interesting that GCC doesn't set the `_MSC_EXTENSIONS` macro with 
`-fms-extensions`. It should if it wants to match the behavior of MSVC. It 
might cause an issue somewhere, but I think it'll probably be exceedingly rare 
and I think would be resulting from incorrect user code (assuming all the 
extensions enabled by MSVC with the flag they use are present in clang). I 
would've thought not having the macro defined would cause more issues, but 
given this part of the code hasn't been touched in six years (and that was a 
refactoring that was presumably NFC), it doesn't seem like people rely on this 
macro too much. Thanks for running the tests though! Having concrete data to 
back up hypotheses is always great to have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

I think this makes sense in principle - however it does diverge from GCC. GCC 
also supports `-fms-extensions` (but I'm not sure if the set of extensions that 
are enabled by the option is an exact match between the two cases), but GCC 
doesn't expose any extra define in that mode. I don't foresee that it'll cause 
any concrete harm though...

Which toolchain is the original source for the name `_MSC_EXTENSIONS` - is it 
something that MSVC itself sets (always, or optionally?), or something that 
Clang itself has made up?

I tried including this patch in a nightly build run (building llvm-mingw and a 
handful of projects with it, including VLC as a large testcase), and it didn't 
trip up anything, so it seems mostly safe in that aspect at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman added a comment.

Since `-fms-extensions` can be enabled on Linux as well, we should probably 
hoist this further since this patch only accounts for the windows case as I 
just hoist the conditional to be in `addWindowsDefines`. I'll work on getting 
another patch version up in a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'm curious to hear what others think about this (especially @rnk and @hans). I 
think this is the correct approach -- `-fms-extensions` is separate from the 
Windows target; for example, users can enable `__declspec` attributes this way: 
https://godbolt.org/z/GEa3oqWPb So I think that it makes sense to define 
`_MSC_EXTENSIONS` whenever `-fms-extensions` is enabled. But I'm not certain if 
others feel the same way or not or if this approach will cause problems anyone 
can think of.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman updated this revision to Diff 548473.
aidengrossman added a comment.

Reformat using arc as manually uploading patch messes up formatting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

Files:
  clang/lib/Basic/Targets/OSTargets.cpp
  clang/test/Preprocessor/predefined-win-macros.c


Index: clang/test/Preprocessor/predefined-win-macros.c
===
--- clang/test/Preprocessor/predefined-win-macros.c
+++ clang/test/Preprocessor/predefined-win-macros.c
@@ -120,6 +120,11 @@
 // CHECK-AMD64-MINGW: #define _WIN32 1
 // CHECK-AMD64-MINGW: #define _WIN64 1
 
+// RUN: %clang_cc1 -triple x86_64-windows-gnu %s -E -dM -o - -fms-extensions \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-AMD64-MINGW-MSE
+
+// CHECK-AMD64-MINGW-MSE: #define _MSC_EXTENSIONS 1
+
 // RUN: %clang_cc1 -triple aarch64-windows-gnu %s -E -dM -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-ARM64-MINGW
 
Index: clang/lib/Basic/Targets/OSTargets.cpp
===
--- clang/lib/Basic/Targets/OSTargets.cpp
+++ clang/lib/Basic/Targets/OSTargets.cpp
@@ -226,16 +226,6 @@
 }
   }
 
-  if (Opts.MicrosoftExt) {
-Builder.defineMacro("_MSC_EXTENSIONS");
-
-if (Opts.CPlusPlus11) {
-  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
-  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
-  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
-}
-  }
-
   if (!Opts.MSVolatile)
 Builder.defineMacro("_ISO_VOLATILE");
 
@@ -264,6 +254,16 @@
   else if (Triple.isKnownWindowsMSVCEnvironment() ||
(Triple.isWindowsItaniumEnvironment() && Opts.MSVCCompat))
 addVisualCDefines(Opts, Builder);
+
+  if (Opts.MicrosoftExt) {
+Builder.defineMacro("_MSC_EXTENSIONS");
+
+if (Opts.CPlusPlus11) {
+  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
+  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
+  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
+}
+  }
 }
 
 } // namespace targets


Index: clang/test/Preprocessor/predefined-win-macros.c
===
--- clang/test/Preprocessor/predefined-win-macros.c
+++ clang/test/Preprocessor/predefined-win-macros.c
@@ -120,6 +120,11 @@
 // CHECK-AMD64-MINGW: #define _WIN32 1
 // CHECK-AMD64-MINGW: #define _WIN64 1
 
+// RUN: %clang_cc1 -triple x86_64-windows-gnu %s -E -dM -o - -fms-extensions \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-AMD64-MINGW-MSE
+
+// CHECK-AMD64-MINGW-MSE: #define _MSC_EXTENSIONS 1
+
 // RUN: %clang_cc1 -triple aarch64-windows-gnu %s -E -dM -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-ARM64-MINGW
 
Index: clang/lib/Basic/Targets/OSTargets.cpp
===
--- clang/lib/Basic/Targets/OSTargets.cpp
+++ clang/lib/Basic/Targets/OSTargets.cpp
@@ -226,16 +226,6 @@
 }
   }
 
-  if (Opts.MicrosoftExt) {
-Builder.defineMacro("_MSC_EXTENSIONS");
-
-if (Opts.CPlusPlus11) {
-  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
-  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
-  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
-}
-  }
-
   if (!Opts.MSVolatile)
 Builder.defineMacro("_ISO_VOLATILE");
 
@@ -264,6 +254,16 @@
   else if (Triple.isKnownWindowsMSVCEnvironment() ||
(Triple.isWindowsItaniumEnvironment() && Opts.MSVCCompat))
 addVisualCDefines(Opts, Builder);
+
+  if (Opts.MicrosoftExt) {
+Builder.defineMacro("_MSC_EXTENSIONS");
+
+if (Opts.CPlusPlus11) {
+  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
+  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
+  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
+}
+  }
 }
 
 } // namespace targets
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman added reviewers: aaron.ballman, hans, mstorsjo, rnk, glandium.
aidengrossman added a comment.

This should fix the issues seen in https://reviews.llvm.org/D150646 regarding 
`-fms-extensions` being set on MinGW and the builtins being defined but no 
`_MSC_EXTENSIONS` macro being set which caused a redefinition of the 
`__cpuidex` function. I'm not sure if this is 100% correct, but at least 
something needs to be done when `-fms-extensions` is passed and the target 
triple includes `-gnu`. Also not sure what happened with the patch formatting 
in Phabricator. The git diff is much cleaner.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman created this revision.
Herald added subscribers: pengfei, mstorsjo.
Herald added a project: All.
aidengrossman requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch makes sure _MSC_EXTENSIONS is defined if -fms-extensions is set by 
hosting the logic for setting the -fms-extensions related macros out of some 
conditional checks related to the target triple. Otherwise we would end up in 
situations where Windows builtins added by -fms-extensions would be defined but 
_MSC_EXTENSIONS wouldn't be defined like on MinGW with the triple 
x86_64-windows-gnu.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157334

Files:
  clang/lib/Basic/Targets/OSTargets.cpp
  clang/test/Preprocessor/predefined-win-macros.c


Index: clang/test/Preprocessor/predefined-win-macros.c
===
--- clang/test/Preprocessor/predefined-win-macros.c
+++ clang/test/Preprocessor/predefined-win-macros.c
@@ -120,9 +120,14 @@
 // CHECK-AMD64-MINGW: #define _WIN32 1
 // CHECK-AMD64-MINGW: #define _WIN64 1

+// RUN: %clang_cc1 -triple x86_64-windows-gnu %s -E -dM -o - -fms-extensions \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-AMD64-MINGW-MSE
+
+// CHECK-AMD64-MINGW-MSE: #define _MSC_EXTENSIONS 1
+
 // RUN: %clang_cc1 -triple aarch64-windows-gnu %s -E -dM -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-ARM64-MINGW

 // CHECK-ARM64-MINGW-NOT: #define _M_ARM64 1
 // CHECK-ARM64-MINGW: #define WIN32 1
 // CHECK-ARM64-MINGW: #define WIN64 1
Index: clang/lib/Basic/Targets/OSTargets.cpp
===
--- clang/lib/Basic/Targets/OSTargets.cpp
+++ clang/lib/Basic/Targets/OSTargets.cpp
@@ -226,25 +226,15 @@
 }
   }

-  if (Opts.MicrosoftExt) {
-Builder.defineMacro("_MSC_EXTENSIONS");
-
-if (Opts.CPlusPlus11) {
-  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
-  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
-  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
-}
-  }
-
   if (!Opts.MSVolatile)
 Builder.defineMacro("_ISO_VOLATILE");

   if (Opts.Kernel)
 Builder.defineMacro("_KERNEL_MODE");

   Builder.defineMacro("_INTEGRAL_MAX_BITS", "64");
   Builder.defineMacro("__STDC_NO_THREADS__");

   // Starting with VS 2022 17.1, MSVC predefines the below macro to inform
   // users of the execution character set defined at compile time.
   // The value given is the Windows Code Page Identifier:
@@ -264,7 +254,17 @@
   else if (Triple.isKnownWindowsMSVCEnvironment() ||
(Triple.isWindowsItaniumEnvironment() && Opts.MSVCCompat))
 addVisualCDefines(Opts, Builder);
+
+  if (Opts.MicrosoftExt) {
+Builder.defineMacro("_MSC_EXTENSIONS");
+
+if (Opts.CPlusPlus11) {
+  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
+  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
+  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
+}
+  }
 }

 } // namespace targets
 } // namespace clang


Index: clang/test/Preprocessor/predefined-win-macros.c
===
--- clang/test/Preprocessor/predefined-win-macros.c
+++ clang/test/Preprocessor/predefined-win-macros.c
@@ -120,9 +120,14 @@
 // CHECK-AMD64-MINGW: #define _WIN32 1
 // CHECK-AMD64-MINGW: #define _WIN64 1

+// RUN: %clang_cc1 -triple x86_64-windows-gnu %s -E -dM -o - -fms-extensions \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-AMD64-MINGW-MSE
+
+// CHECK-AMD64-MINGW-MSE: #define _MSC_EXTENSIONS 1
+
 // RUN: %clang_cc1 -triple aarch64-windows-gnu %s -E -dM -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-ARM64-MINGW

 // CHECK-ARM64-MINGW-NOT: #define _M_ARM64 1
 // CHECK-ARM64-MINGW: #define WIN32 1
 // CHECK-ARM64-MINGW: #define WIN64 1
Index: clang/lib/Basic/Targets/OSTargets.cpp
===
--- clang/lib/Basic/Targets/OSTargets.cpp
+++ clang/lib/Basic/Targets/OSTargets.cpp
@@ -226,25 +226,15 @@
 }
   }

-  if (Opts.MicrosoftExt) {
-Builder.defineMacro("_MSC_EXTENSIONS");
-
-if (Opts.CPlusPlus11) {
-  Builder.defineMacro("_RVALUE_REFERENCES_V2_SUPPORTED");
-  Builder.defineMacro("_RVALUE_REFERENCES_SUPPORTED");
-  Builder.defineMacro("_NATIVE_NULLPTR_SUPPORTED");
-}
-  }
-
   if (!Opts.MSVolatile)
 Builder.defineMacro("_ISO_VOLATILE");

   if (Opts.Kernel)
 Builder.defineMacro("_KERNEL_MODE");

   Builder.defineMacro("_INTEGRAL_MAX_BITS", "64");
   Builder.defineMacro("__STDC_NO_THREADS__");

   // Starting with VS 2022 17.1, MSVC predefines the below macro to inform
   // users of the execution character set defined at compile time.
   // The value given is the Windows Code Page Identifier:
@@ -264,7 +254,17 @@
   else if (Triple.isKnownWindowsMSVCEnvironment() ||