Emit -Wmicrosoft-enum-value warning instead of error in MS ABI

[rnk]

Summary

The first NFC change is to replace a getCXXABI().isMicrosoft() check
with getTriple().isWindowsMSVCEnvironment(). This code takes effect in
non-C++ compilations, so it doesn't make sense to check the C++ ABI. In
the MS ABI, enums are always considered to be "complete" because the
underlying type of an unfixed enum will always be 'int'. This behavior
was moved from -fms-compatibility to MS ABI back in r249656.

The second change is functional, and it downgrades an error to a warning
when the MS ABI is used rather than only under -fms-compatibility. The
reasoning is that it's unreasonable for the following code to reject the
following code for all MS ABI targets with -fno-ms-compatibility:

enum Foo { Foo_Val = 0xDEADBEEF };

This is valid code for any other target, but in the MS ABI, Foo_Val just
happens to be negative. With this change, clang emits a
-Wmicrosoft-enum-value warning on this code, but compiles it without
error.

Fixes PR38478

[hans]

lgtm with comment

The main functional change is that -fno-ms-compatibility now no longer
sends us down the hard error diagnostic code path for ObjC fixed enums.
Instead, complete-but-not-fixed MS enums go down the C99 codepath which
issues a -Wmicrosoft-enum-value warning about the enum value not being
representable as an 'int'. This was highlighted as the single largest
blocker to compiling windows.h with -fno-ms-compatibility, so this
should make that feasible.

Should there be a test exercising this part? The updated tests both have -fms-compatibility, so were already just warning.

[rnk]

• rewrite, abandon unification

[rnk]

In D67304#1663042, @hans wrote:

Should there be a test exercising this part? The updated tests both have -fms-compatibility, so were already just warning.

Good point, we aren't testing that this unfixed enum behavior is a part of the ABI, not -fms-compatibility.

After testing this some more, I think I need to back off on my misguided refactoring. I didn't notice that the C99 warning was an off-by-default pedantic warning, and it didn't insert the implicit cast in the case where the value was not representable in int.

[rnk]

Ptal, new patch

[hans]

lgtm

[thakis]

We have the old TODO of changing this warning to be emitted at enum use time (e.g. when Foo_Val is compared to 0) instead of declaration time. Maybe that's a better fix. Or is implementing that very involved?

[rnk]

In D67304#1664696, @thakis wrote:

We have the old TODO of changing this warning to be emitted at enum use time (e.g. when Foo_Val is compared to 0) instead of declaration time. Maybe that's a better fix. Or is implementing that very involved?

I guess it would depend on the logic of the point-of-use warning. We could make it very simple. We could:

• mark all enum types with coerced enumerators (maybe we already know this)
• warn on all ordering comparisons that use that enum type

If the user explicitly casts to any other integer type to do the comparison, we can expect them to know the rules.

I think it can't allow false negatives, though. -Wmicrosoft is kind of like our -pedantic mode for standard C. It basically says, if your code is -Wmicrosoft clean, we promise that it is not relying on Microsoft-specific behavior. The simplest way to do that is to soften the existing error to a warning, which is what this does.

cfe/trunk/lib/Sema/SemaDecl.cpp

if (UnderlyingType.isInvalid() || (!UnderlyingType.get() && ScopedEnum)) {
       if (CheckEnumUnderlyingType(TI))
         // Recover by falling back to int.
         EnumUnderlying = Context.IntTy.getTypePtr();
 
       if (DiagnoseUnexpandedParameterPack(TI->getTypeLoc().getBeginLoc(), TI,
                                           UPPC_FixedUnderlyingType))
         EnumUnderlying = Context.IntTy.getTypePtr();
 
-    } else if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+    } else if (Context.getTargetInfo().getTriple().isWindowsMSVCEnvironment()) {
       // For MSVC ABI compatibility, unfixed enums must use an underlying type
       // of 'int'. However, if this is an unfixed forward declaration, don't set
       // the underlying type unless the user enables -fms-compatibility. This
       // makes unfixed forward declared enums incomplete and is more conforming.
       if (TUK == TUK_Definition || getLangOpts().MSVCCompat)
         EnumUnderlying = Context.IntTy.getTypePtr();
     }
   }
EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
 
   if (Val)
     Val = DefaultLvalueConversion(Val).get();
 
   if (Val) {
     if (Enum->isDependentType() || Val->isTypeDependent())
       EltTy = Context.DependentTy;
     else {
-      if (getLangOpts().CPlusPlus11 && Enum->isFixed() &&
-          !getLangOpts().MSVCCompat) {
+      if (getLangOpts().CPlusPlus11 && Enum->isFixed()) {
         // C++11 [dcl.enum]p5: If the underlying type is fixed, [...] the
         // constant-expression in the enumerator-definition shall be a converted
         // constant expression of the underlying type.
         EltTy = Enum->getIntegerType();
         ExprResult Converted =
           CheckConvertedConstantExpression(Val, EltTy, EnumVal,
                                            CCEK_Enumerator);
         if (Converted.isInvalid())
           Val = nullptr;
         else
           Val = Converted.get();
       } else if (!Val->isValueDependent() &&
                  !(Val = VerifyIntegerConstantExpression(Val,
                                                          &EnumVal).get())) {
         // C99 6.7.2.2p2: Make sure we have an integer constant expression.
       } else {
         if (Enum->isComplete()) {
           EltTy = Enum->getIntegerType();
 
           // In Obj-C and Microsoft mode, require the enumeration value to be
           // representable in the underlying type of the enumeration. In C++11,
           // we perform a non-narrowing conversion as part of converted constant
           // expression checking.
           if (!isRepresentableIntegerValue(Context, EnumVal, EltTy)) {
-            if (getLangOpts().MSVCCompat) {
+            if (Context.getTargetInfo()
+                    .getTriple()
+                    .isWindowsMSVCEnvironment()) {
               Diag(IdLoc, diag::ext_enumerator_too_large) << EltTy;
-              Val = ImpCastExprToType(Val, EltTy, CK_IntegralCast).get();
-            } else
+            } else {
               Diag(IdLoc, diag::err_enumerator_too_large) << EltTy;
-          } else
-            Val = ImpCastExprToType(Val, EltTy,
-                                    EltTy->isBooleanType() ?
-                                    CK_IntegralToBoolean : CK_IntegralCast)
+            }
+          }
+
+          // Cast to the underlying type.
+                                  EltTy->isBooleanType() ? CK_IntegralToBoolean
+                                                         : CK_IntegralCast)
                     .get();
         } else if (getLangOpts().CPlusPlus) {
           // C++11 [dcl.enum]p5:
           //   If the underlying type is not fixed, the type of each enumerator
           //   is the type of its initializing value:
           //     - If an initializer is specified for an enumerator, the
           //       initializing value has the same type as the expression.
           EltTy = Val->getType();

cfe/trunk/test/Sema/MicrosoftCompatibility.c

-// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wmicrosoft -verify -fms-compatibility -triple i686-pc-win32
+// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wmicrosoft -verify -fms-compatibility -DMSVCCOMPAT -triple i686-pc-win32
+// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wmicrosoft -verify -fms-extensions -triple i686-pc-win32
 
+#ifdef MSVCCOMPAT
 enum ENUM1; // expected-warning {{forward references to 'enum' types are a Microsoft extension}}
 enum ENUM1 var1 = 3;
 enum ENUM1* var2 = 0;
+#else
+enum ENUM1; // expected-note {{forward declaration of}}
+enum ENUM1 var1 = 3; // expected-error {{variable has incomplete type 'enum ENUM1'}}
+enum ENUM1* var2 = 0;
+#endif
 
 
+// FIXME: The rest of this seems to be controlled by -fms-extensions. Move it.
 enum ENUM2 {
   ENUM2_a = (enum ENUM2) 4,
   ENUM2_b = 0x9FFFFFFF, // expected-warning {{enumerator value is not representable in the underlying type 'int'}}
   ENUM2_c = 0x100000000 // expected-warning {{enumerator value is not representable in the underlying type 'int'}}
 };
 
 __declspec(noreturn) void f6( void ) {
   return;  // expected-warning {{function 'f6' declared 'noreturn' should not return}}
 }
 
 __declspec(align(32768)) struct S1 { int a; } s;  /* expected-error {{requested alignment must be 8192 bytes or smaller}} */
 struct __declspec(aligned) S2 {}; /* expected-warning {{__declspec attribute 'aligned' is not supported}} */
 
 struct __declspec(appdomain) S3 {}; /* expected-warning {{__declspec attribute 'appdomain' is not supported}} */
 
 __declspec(__noreturn__) void f7(void); /* expected-warning {{__declspec attribute '__noreturn__' is not supported}} */
 
+#ifdef MSVCCOMPAT
 size_t x;
+#else
+size_t x; // expected-error {{unknown type name 'size_t'}}
+#endif