[MSVC] Recognize "static_assert" keyword in C mode

[rnk]

Summary

MSVC treats static_assert as a keyword in C mode and they implement in
the C++11 way. It does not support _Static_assert in C mode either.

This is a non-conforming extension (the keyword is outside the
implementer's namespace), so it is placed under -fms-compatibility
instead of -fms-extensions like most MSVC-specific keyword extensions.

Fixes PR26672

Patch by Andrey Bokhanko!

[rnk]

I think we should do this because MSVC doesn't make _Static_assert available to C code. David says that, according to the C standard, assert.h is supposed to #define static_assert _Static_assert. MSVC doesn't do that because they provide static_assert directly as a keyword. Pretty soon we'll start seeing portable C projects including assert.h to use plain static_assert, and if we don't do something, that code will compile with MSVC but not clang-cl.

I think we should do this under fms-compatibility, not fms-extensions, because it is technically not a conforming extension. =/ There isn't a bit for that currently, but we should add one. Maybe call it KEYMSCOMPAT?

[rsmith]

Do they treat this as a keyword or as a predefined macro? (Does #ifdef think static_assert is defined?)

[andreybokhanko]

Patch updated in response to Reid's proposal (to introduce KEYMSCOMPAT).

(To clarify, I agree with Aaron that we should wait for MS' response first. Though I frankly don't expect them to accept recognition of "static_assert" as a bug, so wanted to have a patch ready.)

Yours,

Andrey

Software Engineer
Intel Compiler Team

[rnk]

lgtm

[andreybokhanko]

It seems the discussion (http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160229/thread.html#151860) concluded that we don't want this fix to be committed until we see some compelling code that relies on "static_assert" availability in C language without <assert.h> being included.

Thus, please consider this review request to be abandoned.

Yours,
Andrey

[thakis]

I just ran into this and I'm a bit confused about the discussion here. This snippet (in a .c file)

#include <assert.h>

static_assert(4 == 4 , "");

builds in all compilers except clang-cl. How does not supporting this make sense?

Instead of this patch we could have an assert.h wrapper in lib/Headers that defines static_assert to _Static_assert in ms mode for C files, and I suppose that's a cleaner fix. But I hope it's not controversial that we should try and support standard C programs?

[rnk]

In D17444#1392823, @thakis wrote:

Instead of this patch we could have an assert.h wrapper in lib/Headers that defines static_assert to _Static_assert in ms mode for C files, and I suppose that's a cleaner fix. But I hope it's not controversial that we should try and support standard C programs?

I'd prefer it if we didn't shadow more MSVC CRT headers like assert.h. Users have a long history of holding the compiler wrong, losing these interpositions, and having things not work. If we could just work out the box, users will have less problems, we'll have fewer bug reports, etc, etc. I still think we should follow MSVC and add this is a plain old C++ extension to C under -fms-extensions.

[thakis]

In D17444#1393339, @rnk wrote:

In D17444#1392823, @thakis wrote:

Instead of this patch we could have an assert.h wrapper in lib/Headers that defines static_assert to _Static_assert in ms mode for C files, and I suppose that's a cleaner fix. But I hope it's not controversial that we should try and support standard C programs?

I'd prefer it if we didn't shadow more MSVC CRT headers like assert.h. Users have a long history of holding the compiler wrong, losing these interpositions, and having things not work. If we could just work out the box, users will have less problems, we'll have fewer bug reports, etc, etc. I still think we should follow MSVC and add this is a plain old C++ extension to C under -fms-extensions.

Given that we already have interposition headers and users already have to hold the compiler right, does it matter if we have one more interposing header?

[rnk]

In D17444#1393509, @thakis wrote:

Given that we already have interposition headers and users already have to hold the compiler right, does it matter if we have one more interposing header?

I'd say it's one more bit of technical debt that we'll have to figure out how to undo later. I'm OK doing it if @rsmith still feels that we shouldn't do this in the compiler.

Also, I suppose this is really actually an -fms-compatibility thing not -fms-extensions because it takes an identifier from outside the implementer's namespace.

[aaron.ballman]

Also, I suppose this is really actually an -fms-compatibility thing not -fms-extensions because it takes an identifier from outside the implementer's namespace.

I just verified, it really does take the identifier and it does not treat static_assert as a macro (even when you include assert.h). I suppose we probably should treat this as a keyword like MSVC seems to be doing, rather than interposing on assert.h, because the interposition would give subtly different resulting behavior.

[thakis]

I don't have an opinion if this patch here or the assert wrapper is better, but I feel somewhat strongly that we should land one or the other so that the standard C program

#include <assert.h>

static_assert(4 == 4 , "");

builds in clang-cl like it does in every other compiler.

Sounds like rnk and rsmith are the champions for each approach, so maybe the two of you could have a meeting of the minds somewhere and Decide What To Do Here?

[rnk]

I chatted with @rsmith offline and the solution we came up with was to do both of the following:

• make static_assert a keyword under -fms-compatibility (it is a technically non-conforming extension)
• ask @STL_MSFT if #define static_assert _Static_assert can be added to the VC assert.h header. This has the benefit of fixing the issue for users with a new VC and an old clang.
• when a fixed assert.h is released, remove this nonconforming keyword from clang, or make it conditional on a new enough _MSC_VER

However, local testing has shown that Visual C++ doesn't recognize _Static_assert, so the second part of this isn't so simple. The problem is that we want users to be able to compile the following conforming C11 code with -fno-ms-compatibility:

#include <assert.h>
static_assert(true, "foo");

The other solution is to have VC assert.h say:

#ifdef __clang__
#define static_assert _Static_assert
#endif

... but maybe it would be better to teach the Visual C++ compiler to recognize the C11 _Static_assert keyword in the first place.

[rnk]

• rebase

[rnk]

Comandeering and updating description...

[STL_MSFT]

@rnk I've forwarded this to the compiler front-end and Universal CRT teams.

[rnk]

In D17444#1397179, @STL_MSFT wrote:

@rnk I've forwarded this to the compiler front-end and Universal CRT teams.

Thanks!

[thakis]

Awesome, thanks.

clang/test/Lexer/keywords_test.cpp

57 ↗

(On Diff #186741)

Should this comment (or a comment elsewhere) mention the plan that we want asked for the change to msvc's assert.h and that we want to stop defining this if _MSC_VER is new enough to have that change?

[aaron.ballman]

LGTM!

include/clang/Basic/TokenKinds.def

 //   KEYCXX   - This is a C++ keyword, or a C++-specific keyword in the
 //              implementation namespace
 //   KEYNOCXX - This is a keyword in every non-C++ dialect.
 //   KEYCXX11 - This is a C++ keyword introduced to C++ in C++11
 //   KEYCONCEPTS - This is a keyword if the C++ extensions for concepts
 //                 are enabled.
 //   KEYGNU   - This is a keyword if GNU extensions are enabled
 //   KEYMS    - This is a keyword if Microsoft extensions are enabled
+//   KEYMSCOMPAT - This is a keyword if Microsoft compatibility mode is enabled
 //   KEYNOMS18 - This is a keyword that must never be enabled under
 //               MSVC <= v18.
 //   KEYOPENCL  - This is a keyword in OpenCL
 //   KEYNOOPENCL  - This is a keyword that is not supported in OpenCL
 //   KEYALTIVEC - This is a keyword in AltiVec
 //   KEYZVECTOR - This is a keyword for the System z vector extensions,
 //                which are heavily based on AltiVec
 //   KEYBORLAND - This is a keyword if Borland extensions are enabled

 CXX11_KEYWORD(alignas               , 0)
 CXX11_KEYWORD(alignof               , 0)
 CXX11_KEYWORD(char16_t              , KEYNOMS18)
 CXX11_KEYWORD(char32_t              , KEYNOMS18)
 CXX11_KEYWORD(constexpr             , 0)
 CXX11_KEYWORD(decltype              , 0)
 CXX11_KEYWORD(noexcept              , 0)
 CXX11_KEYWORD(nullptr               , 0)
-CXX11_KEYWORD(static_assert         , 0)
+CXX11_KEYWORD(static_assert         , KEYMSCOMPAT)
 CXX11_KEYWORD(thread_local          , 0)
 
 // C++ concepts TS keywords
 CONCEPTS_KEYWORD(concept)
 CONCEPTS_KEYWORD(requires)
 
 // C++ coroutines TS keywords
 KEYWORD(co_await                    , KEYCOROUTINES)

lib/Basic/IdentifierTable.cpp

enum {
     KEYNOMS18 = 0x01000,
     KEYNOOPENCL = 0x02000,
     WCHARSUPPORT = 0x04000,
     HALFSUPPORT = 0x08000,
     KEYCONCEPTS = 0x10000,
     KEYOBJC2    = 0x20000,
     KEYZVECTOR  = 0x40000,
     KEYCOROUTINES = 0x80000,
+    KEYMSCOMPAT = 0x100000,
     KEYALL = (0xfffff & ~KEYNOMS18 &
               ~KEYNOOPENCL) // KEYNOMS18 and KEYNOOPENCL are used to exclude.
   };
 
   /// \brief How a keyword is treated in the selected standard.
   enum KeywordStatus {
     KS_Disabled,    // Disabled
     KS_Extension,   // Is an extension
     KS_Enabled,     // Enabled
     KS_Future       // Is a keyword in future standard
   };
 }
 
 /// \brief Translates flags as specified in TokenKinds.def into keyword status
 /// in the given language standard.
 static KeywordStatus getKeywordStatus(const LangOptions &LangOpts,
                                       unsigned Flags) {
   if (Flags == KEYALL) return KS_Enabled;
   if (LangOpts.CPlusPlus && (Flags & KEYCXX)) return KS_Enabled;
   if (LangOpts.CPlusPlus11 && (Flags & KEYCXX11)) return KS_Enabled;
   if (LangOpts.C99 && (Flags & KEYC99)) return KS_Enabled;
   if (LangOpts.GNUKeywords && (Flags & KEYGNU)) return KS_Extension;
   if (LangOpts.MicrosoftExt && (Flags & KEYMS)) return KS_Extension;
+  if (LangOpts.MSVCCompat && (Flags & KEYMSCOMPAT)) return KS_Enabled;
   if (LangOpts.Borland && (Flags & KEYBORLAND)) return KS_Extension;
   if (LangOpts.Bool && (Flags & BOOLSUPPORT)) return KS_Enabled;
   if (LangOpts.Half && (Flags & HALFSUPPORT)) return KS_Enabled;
   if (LangOpts.WChar && (Flags & WCHARSUPPORT)) return KS_Enabled;
   if (LangOpts.AltiVec && (Flags & KEYALTIVEC)) return KS_Enabled;
   if (LangOpts.OpenCL && (Flags & KEYOPENCL)) return KS_Enabled;
   if (!LangOpts.CPlusPlus && (Flags & KEYNOCXX)) return KS_Enabled;
   if (LangOpts.C11 && (Flags & KEYC11)) return KS_Enabled;

test/Lexer/keywords_test.cpp

 // RUN: %clang_cc1 -std=c++03 -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++11 -DCXX11 -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++14 -fconcepts-ts -DCXX11 -DCONCEPTS -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++03 -fdeclspec -DDECLSPEC -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++03 -fms-extensions -DDECLSPEC -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++03 -fborland-extensions -DDECLSPEC -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++03 -fms-extensions -fno-declspec -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++03 -fborland-extensions -fno-declspec -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++03 -fno-declspec -fdeclspec -DDECLSPEC -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++03 -fdeclspec -fno-declspec -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++03 -fms-extensions -fno-declspec -fdeclspec -DDECLSPEC -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++03 -fms-extensions -fdeclspec -fno-declspec -fsyntax-only %s
-// RUN: %clang -std=c++03 -target i686-windows-msvc -DDECLSPEC -fsyntax-only %s
+// RUN: %clang -std=c++03 -target i686-windows-msvc -DMS -DDECLSPEC -fsyntax-only %s
 // RUN: %clang -std=c++03 -target x86_64-scei-ps4 -DDECLSPEC -fsyntax-only %s
-// RUN: %clang -std=c++03 -target i686-windows-msvc -fno-declspec -fsyntax-only %s
+// RUN: %clang -std=c++03 -target i686-windows-msvc -DMS -fno-declspec -fsyntax-only %s
 // RUN: %clang -std=c++03 -target x86_64-scei-ps4 -fno-declspec -fsyntax-only %s
 
 #define IS_KEYWORD(NAME) _Static_assert(!__is_identifier(NAME), #NAME)
 #define NOT_KEYWORD(NAME) _Static_assert(__is_identifier(NAME), #NAME)
 #define IS_TYPE(NAME) void is_##NAME##_type() { int f(NAME); }
 
 #ifdef CONCEPTS
 #define CONCEPTS_KEYWORD(NAME)  IS_KEYWORD(NAME)

 CXX11_KEYWORD(alignof);
 CXX11_KEYWORD(alignas);
 CXX11_KEYWORD(char16_t);
 CXX11_TYPE(char16_t);
 CXX11_KEYWORD(char32_t);
 CXX11_TYPE(char32_t);
 CXX11_KEYWORD(constexpr);
 CXX11_KEYWORD(noexcept);
+#ifndef MS
 CXX11_KEYWORD(static_assert);
+#else
+// MS compiler recognizes static_assert in all modes. So should we.
+IS_KEYWORD(static_assert);
+#endif
 CXX11_KEYWORD(thread_local);
 
 // Concepts TS keywords
 CONCEPTS_KEYWORD(concept);
 CONCEPTS_KEYWORD(requires);
 
 // __declspec extension
 DECLSPEC_KEYWORD(__declspec);
 
 // Clang extension
 IS_KEYWORD(__char16_t);
 IS_TYPE(__char16_t);
 IS_KEYWORD(__char32_t);
 IS_TYPE(__char32_t);