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

[aidengrossman]

Summary

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.

[aidengrossman]

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.

[aidengrossman]

Reformat using arc as manually uploading patch messes up formatting.

[aaron.ballman]

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.

[aidengrossman]

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.

[mstorsjo]

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.

[aidengrossman]

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.

[aaron.ballman]

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.

[hans]

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)

[aaron.ballman]

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)?

[aidengrossman]

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

[rnk]

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?

[aidengrossman]

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).

[rnk]

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.

[aidengrossman]

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!

clang/docs/ReleaseNotes.rst

 New Compiler Flags
 ------------------
 
 Deprecated Compiler Flags
 -------------------------
 
 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
 -------------------------
 
 Attribute Changes in Clang
 --------------------------
 
 Improvements to Clang's diagnostics

clang/lib/Basic/Targets/OSTargets.cpp

 //===--- OSTargets.cpp - Implement OS target feature support --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (Opts.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
         Builder.defineMacro("_MSVC_LANG", "202002L");
       else if (Opts.CPlusPlus17)
         Builder.defineMacro("_MSVC_LANG", "201703L");
       else if (Opts.CPlusPlus14)
         Builder.defineMacro("_MSVC_LANG", "201402L");
     }
   }
 
-  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__");

clang/lib/Frontend/InitPreprocessor.cpp

 //===--- InitPreprocessor.cpp - PP initialization code. ---------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
#undef TOSTR2
 
   if (LangOpts.Deprecated)
     Builder.defineMacro("__DEPRECATED");
 
   if (!LangOpts.MSVCCompat && LangOpts.CPlusPlus)
     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
   // FIXME: clang currently ignores -fexec-charset=. If this changes,
   // then this may need to be updated.
   Builder.defineMacro("__clang_literal_encoding__", "\"UTF-8\"");
   if (TI.getTypeWidth(TI.getWCharType()) >= 32) {
     // FIXME: 32-bit wchar_t signals UTF-32. This may change

clang/test/Preprocessor/predefined-win-macros.c

 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-AMD64-MINGW
 
 // CHECK-AMD64-MINGW: #define WIN32 1
 // CHECK-AMD64-MINGW: #define WIN64 1
 // CHECK-AMD64-MINGW: #define WINNT 1
 // 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
 // CHECK-ARM64-MINGW: #define WINNT 1
 // CHECK-ARM64-MINGW: #define _WIN32 1
 // CHECK-ARM64-MINGW: #define _WIN64 1
 // CHECK-ARM64-MINGW: #define __GCC_ASM_FLAG_OUTPUTS__ 1
 // CHECK-ARM64-MINGW: #define __aarch64__ 1