[MS] PR26234: Allow typedef redefinition of equally qualified, sized and aligned types in C

[• d.zobnin.bugzilla]

Summary

Allow typedef redefinition with different types in C if the types are equally qualified, sized and aligned. MSVC allows such redefinition, emits warning C4142: "benign redefinition of type" and propagates the type from the first typedef declaration in the entire redeclaration chain.

[majnemer]

MSVC permits:

typedef int foo;
typedef float foo;

This is crazy and most certainly not 'benign'. I think that we should insist that the underlying types not just be the same size and alignment but the same kind of scalar type. If two types are integral or enumerator types, they should have the same signedness.

I believe this would be enough to get things like vortex working without harming developer productivity.

include/clang/Basic/DiagnosticSemaKinds.td

4267–4269

I think giving a warning that 'benign' sends the message that the code is OK and that the warning should be disabled. I think we should say something like "typedef redefinition is ignored due to conflicting underlying type".

lib/AST/ASTContext.cpp

6733–6734

Shouldn't this be MSVCCompat, not MicrosoftExt?

lib/Sema/SemaDecl.cpp

1867–1868

Please use auto here.

[rnk]

include/clang/AST/ASTContext.h

1772–1783

Please update the comments to reflect that this only applies to typedef types.

include/clang/Basic/DiagnosticSemaKinds.td

4267–4269

We aren't ignoring it because the underyling types differ, we're ignoring it because they are the same size and signedness and we're in MSVC quirks mode. Maybe "ignoring conflicting integer typedef redefinition%diff{...} as a Microsoft extension; types have the same width and signedness"? Is that too much text?

lib/AST/ASTContext.cpp

6733–6734

+1

[• d.zobnin.bugzilla]

Thanks for the review!

Updated the patch according to the comments: allowed only integer, enum and pointer types which have the same qualifiers, signedness, width and alignment. Changed the diagnostic text.

Please take a look.

[• d.zobnin.bugzilla]

include/clang/AST/ASTContext.h

1772–1783

By the way, MSVC allows to redefine not anly typedefs, but variables as well (under the same conditions as typedefs):

int x;
long x;

Typedef redefinition seems to be just an example of MSVC's permissive behavior. The same for return types, etc.

So, do you think we need to compatible with MSVC, for example, also considering only integer types?

include/clang/Basic/DiagnosticSemaKinds.td

4267–4269

Done. I also tried to move the phrase "types have the same ..." to a note, but I thought the only warning was better.

[rnk]

include/clang/AST/ASTContext.h

1772–1783

So, do you think we need to compatible with MSVC, for example, also considering only integer types?

I'd rather not for now. I can imagine that there's lots of code out there with crazy mixed up typedefs that it's useful to accept, but it's a lot less common to re-prototype someone else's functions incorrectly.

lib/Sema/SemaDecl.cpp

1898–1902

Hm, this seems like an AST fidelity issue that Richard would care about. The TSI should reflect the type the user wrote at this position. Maybe you could wrap an AdjustedType node around the original type and install that instead.

[thakis]

Out of interest, what is this needed for? System headers?

[• d.zobnin.bugzilla]

@thakis, it is needed to be able to compile SPEC2000: 255.vortex.

include/clang/AST/ASTContext.h

public:
 
   /// \brief Return true if the given vector types are of the same unqualified
   /// type or if they are equivalent to the same GCC vector type.
   ///
   /// \note This ignores whether they are target-specific (AltiVec or Neon)
   /// types.
   bool areCompatibleVectorTypes(QualType FirstVec, QualType SecondVec);
 
+  /// Return true is the given typedef types are compatible in C from MSVC's
+  /// point of view.
+  //
+  // Conditions:
+  //   1. Both typedef types are either integer, enumeral or pointers;
+  //   2. Both typedef types have the same qualifiers and signedness;
+  //   3. Both typedef types have the same size and alignment;
+  //   4. If pointers:
+  //     4.1. Levels of pointers are equal;
+  //     4.2. Pointee types are MSVC-compatible OR
+  //     4.3. One type points to void and another points to char.
+  bool areMSCompatibleTypedefTypesInC(QualType OldType, QualType NewType);
+
   /// \brief Return true if this is an \c NSObject object with its \c NSObject
   /// attribute set.
   static bool isObjCNSObjectType(QualType Ty) {
     return Ty->isObjCNSObjectType();
   }
 
   //===--------------------------------------------------------------------===//
   //                         Type Sizing and Analysis

include/clang/Basic/DiagnosticSemaKinds.td

def err_redefinition_different_kind : Error<
   "redefinition of %0 as different kind of symbol">;
 def err_redefinition_different_namespace_alias : Error<
   "redefinition of %0 as an alias for a different namespace">;
 def note_previous_namespace_alias : Note<
   "previously defined as an alias for %0">;
 def warn_forward_class_redefinition : Warning<
   "redefinition of forward class %0 of a typedef name of an object type is ignored">,
   InGroup<DiagGroup<"objc-forward-class-redefinition">>;
+def warn_int_typedef_redefinition_ignored : ExtWarn<
+  "ignoring conflicting integer typedef redefinition%diff{ ($ vs $)|}0,1 "
+  "as a Microsoft extension; types have the same width and signedness">,
+  InGroup<Microsoft>;
 def err_redefinition_different_typedef : Error<
   "%select{typedef|type alias|type alias template}0 "
   "redefinition with different types%diff{ ($ vs $)|}1,2">;
 def err_tag_reference_non_tag : Error<
   "elaborated type refers to %select{a non-tag type|a typedef|a type alias|a template|a type alias template}0">;
 def err_tag_reference_conflict : Error<
   "implicit declaration introduced by elaborated type conflicts with "
   "%select{a declaration|a typedef|a type alias|a template}0 of the same name">;

lib/AST/ASTContext.cpp

if (First->getNumElements() == Second->getNumElements() &&
       First->getVectorKind() != VectorType::AltiVecBool &&
       Second->getVectorKind() != VectorType::AltiVecPixel &&
       Second->getVectorKind() != VectorType::AltiVecBool)
     return true;
 
   return false;
 }
 
+bool ASTContext::areMSCompatibleTypedefTypesInC(QualType OldType,
+                                                QualType NewType) {
+  assert(getLangOpts().MSVCCompat &&
+         "This routine must be called in Microsoft mode only!");
+  assert(!getLangOpts().CPlusPlus && !getLangOpts().ObjC1 &&
+         !getLangOpts().ObjC2 && "This routine must be called in C mode only!");
+
+  QualType OldCan = getCanonicalType(OldType);
+  QualType NewCan = getCanonicalType(NewType);
+
+  if (OldCan.getCVRQualifiers() != NewCan.getCVRQualifiers())
+    return false;
+
+  if (OldCan->isPointerType() && NewCan->isPointerType()) {
+    QualType OldPointee = OldType->getPointeeType();
+    QualType NewPointee = NewType->getPointeeType();
+    // void * and char * are interchangeable.
+    if ((OldPointee->isCharType() && NewPointee->isVoidType()) ||
+        (OldPointee->isVoidType() && NewPointee->isCharType()))
+      return true;
+    return areMSCompatibleTypedefTypesInC(OldPointee, NewPointee);
+  }
+
+  bool EquallySignedInts = (OldCan->isSignedIntegerOrEnumerationType() &&
+                            NewCan->isSignedIntegerOrEnumerationType()) ||
+                           (OldCan->isUnsignedIntegerOrEnumerationType() &&
+                            NewCan->isUnsignedIntegerOrEnumerationType());
+  if (!EquallySignedInts)
+    return false;
+
+  auto OldTypeInfo = getTypeInfo(OldCan);
+  auto NewTypeInfo = getTypeInfo(NewCan);
+  return (OldTypeInfo.Width == NewTypeInfo.Width &&
+          OldTypeInfo.Align == NewTypeInfo.Align);
+}
+
 //===----------------------------------------------------------------------===//
 // ObjCQualifiedIdTypesAreCompatible - Compatibility testing for qualified id's.
 //===----------------------------------------------------------------------===//
 
 /// ProtocolCompatibleWithProtocol - return 'true' if 'lProto' is in the
 /// inheritance hierarchy of 'rProto'.
 bool
 ASTContext::ProtocolCompatibleWithProtocol(ObjCProtocolDecl *lProto,

lib/Sema/SemaDecl.cpp

while (Filter.hasNext()) {
     }
 
     Filter.erase();
   }
 
   Filter.done();
 }
 
+static bool areMSCompatibleTypedefs(TypedefNameDecl *Old, TypedefNameDecl *New,
+                                    ASTContext &Context) {
+#ifndef NDEBUG
+  const LangOptions &Opts = Context.getLangOpts();
+  assert(Opts.MSVCCompat &&
+         "This routine must be called in Microsoft mode only!");
+  assert(!Opts.CPlusPlus && !Opts.ObjC1 && !Opts.ObjC2 &&
+         "This routine must be called in C mode only!");
+  assert(Old && New && "Expected valid typedef declarations!");
+#endif
+
+  // If both are locally-scoped, emit an error.
+  if (!Old->getDeclContext()->isFileContext() &&
+      !New->getDeclContext()->isFileContext())
+    return false;
+
+  return Context.areMSCompatibleTypedefTypesInC(Old->getUnderlyingType(),
+                                                New->getUnderlyingType());
+}
+
 bool Sema::isIncompatibleTypedef(TypeDecl *Old, TypedefNameDecl *New) {
   QualType OldType;
-  if (TypedefNameDecl *OldTypedef = dyn_cast<TypedefNameDecl>(Old))
+  auto *OldTypedef = dyn_cast<TypedefNameDecl>(Old);
+  if (OldTypedef)
     OldType = OldTypedef->getUnderlyingType();
   else
     OldType = Context.getTypeDeclType(Old);
   QualType NewType = New->getUnderlyingType();
 
   if (NewType->isVariablyModifiedType()) {
     // Must not redefine a typedef with a variably-modified type.
     int Kind = isa<TypeAliasDecl>(Old) ? 1 : 0;
     Diag(New->getLocation(), diag::err_redefinition_variably_modified_typedef)
       << Kind << NewType;
     if (Old->getLocation().isValid())
       Diag(Old->getLocation(), diag::note_previous_definition);
     New->setInvalidDecl();
     return true;    
   }
-  
   if (OldType != NewType &&
       !OldType->isDependentType() &&
       !NewType->isDependentType() &&
-      !Context.hasSameType(OldType, NewType)) { 
+      !Context.hasSameType(OldType, NewType)) {
+    const LangOptions &Opts = Context.getLangOpts();
+    bool AllowedAsMicrosoftExtInC =
+        Opts.MSVCCompat && !Opts.CPlusPlus && !Opts.ObjC1 && !Opts.ObjC2 &&
+        OldTypedef && areMSCompatibleTypedefs(OldTypedef, New, Context);
+
+    SourceLocation OldLocation = Old->getLocation();
+    if (AllowedAsMicrosoftExtInC) {
+      Diag(New->getLocation(), diag::warn_int_typedef_redefinition_ignored)
+          << NewType << OldType;
+      if (OldTypedef->isModed())
+        New->setModedTypeSourceInfo(OldTypedef->getTypeSourceInfo(),
+                                    OldTypedef->getUnderlyingType());
+      else
+        New->setTypeSourceInfo(OldTypedef->getTypeSourceInfo());
+      OldLocation = OldTypedef->getFirstDecl()->getLocation();
+    } else {
       int Kind = isa<TypeAliasDecl>(Old) ? 1 : 0;
       Diag(New->getLocation(), diag::err_redefinition_different_typedef)
           << Kind << NewType << OldType;
-    if (Old->getLocation().isValid())
-      Diag(Old->getLocation(), diag::note_previous_definition);
       New->setInvalidDecl();
-    return true;
+    }
+    if (OldLocation.isValid())
+      Diag(OldLocation, diag::note_previous_definition);
+    return !AllowedAsMicrosoftExtInC;
   }
   return false;
 }
 
 /// MergeTypedefNameDecl - We just parsed a typedef 'New' which has the
 /// same name and scope as a previous declaration 'Old'.  Figure out
 /// how to resolve this situation, merging decls or emitting
 /// diagnostics as appropriate. If there was an error, set New to be invalid.

test/Sema/ms-benign-typedef-redef.c

+// RUN: %clang_cc1 -triple i386-pc-win32 -fms-compatibility -fsyntax-only -verify %s
+
+// Check char types.
+typedef unsigned char       UC1;
+typedef unsigned __int8     UC1;
+
+typedef unsigned char       UC2;  // expected-note{{previous}}
+typedef signed char         UC2;  // expected-error{{redefinition}}
+
+typedef char                SC1;  // expected-note{{previous}}
+typedef signed char         SC1;  // expected-warning{{ignoring conflicting integer typedef redefinition}}
+typedef __int8              SC1;
+// Check short types.
+typedef unsigned short      US1;  // expected-note{{previous}}
+typedef __wchar_t           US1;  // expected-warning{{ignoring conflicting integer typedef redefinition}}
+typedef unsigned __int16    US1;
+
+typedef unsigned short      US2;  // expected-note{{previous}}
+typedef short               US2;  // expected-error{{redefinition}}
+
+typedef short               SS1;
+typedef __int16             SS1;
+// Check int types.
+typedef unsigned int        UI1;  // expected-note{{previous}}
+typedef unsigned long       UI1;  // expected-warning{{ignoring conflicting integer typedef redefinition}}
+typedef unsigned __int32    UI1;
+
+typedef unsigned int        UI2;  // expected-note{{previous}}
+typedef long                UI2;  // expected-error{{redefinition}}
+// Check long long types.
+typedef unsigned long long  ULL;
+typedef unsigned __int64    ULL;
+
+typedef long long           LL1;
+typedef __int64             LL1;
+
+typedef unsigned long long  LL2;  // expected-note{{previous}}
+typedef __int64             LL2;  // expected-error{{redefinition}}
+// Integer vs floating type - error.
+typedef enum { A }          IF1;  // expected-note{{previous}}
+typedef float               IF1;  // expected-error{{redefinition}}
+
+typedef int                 IF2;  // expected-note{{previous}}
+typedef float               IF2;  // expected-error{{redefinition}}
+
+typedef long long           IF3;  // expected-note{{previous}}
+typedef double              IF3;  // expected-error{{redefinition}}
+// Enum vs int, enum vs enum - OK.
+typedef enum { B }          IE1;  // expected-note{{previous}}
+typedef int                 IE1;  // expected-warning{{ignoring conflicting integer typedef redefinition}}
+
+typedef enum { C }          IE2;  // expected-note{{previous}}
+typedef enum { D, E }       IE2;  // expected-warning{{ignoring conflicting integer typedef redefinition}}
+// Enum vs pointer - error.
+typedef enum { F }          IE3;  // expected-note{{previous}}
+typedef int               * IE3;  // expected-error{{redefinition}}
+// Check pointer types.
+// Poiters with different levels - error.
+typedef int              ** DL1;  // expected-note{{previous}}
+typedef long              * DL1;  // expected-error{{redefinition}}
+// void * vs char * - OK.
+typedef char              * PT1;  // expected-note{{previous}}
+typedef void              * PT1;  // expected-warning{{ignoring conflicting integer typedef redefinition}}
+
+typedef unsigned char       UChar;
+typedef UChar             * PT2;  // expected-note{{previous}}
+typedef void              * PT2;  // expected-warning{{ignoring conflicting integer typedef redefinition}}
+
+typedef char             ** PT3;  // expected-note{{previous}}
+typedef void             ** PT3;  // expected-warning{{ignoring conflicting integer typedef redefinition}}
+// void * vs other pointer - error.
+typedef int               * PT4;  // expected-note{{previous}}
+typedef void              * PT4;  // expected-error{{redefinition}}
+// Check pointee types.
+typedef int               * PT5;  // expected-note{{previous}}
+typedef long              * PT5;  // expected-warning{{ignoring conflicting integer typedef redefinition}}
+
+typedef unsigned int      * PT6;  // expected-note{{previous}}
+typedef long              * PT6;  // expected-error{{redefinition}}
+
+typedef int               * PT7;  // expected-note{{previous}}
+typedef float             * PT7;  // expected-error{{redefinition}}
+// Different qualifiers - error.
+typedef short               DQ1;  // expected-note{{previous}}
+typedef const short         DQ1;  // expected-error{{redefinition}}
+
+typedef int   * const       DQ2;  // expected-note{{previous}}
+typedef long  *             DQ2;  // expected-error{{redefinition}}
+
+typedef int               * DQ3;  // expected-note{{previous}}
+typedef volatile int      * DQ3;  // expected-error{{redefinition}}
+// Other than (enum | integer | pointer) type - error.
+typedef struct S { int x; } OT1;  // expected-note{{previous}}
+typedef int                 OT1;  // expected-error{{redefinition}}
+
+typedef float               OT2;  // expected-note{{previous}}
+typedef double              OT2;  // expected-error{{redefinition}}
+
+typedef int                 DS1;  // Different scopes - OK.
+
+int main() {
+  typedef long              DS1;  // Different scopes - OK.
+  {
+    typedef long            DS2;  // Different scopes - OK.
+  }
+  typedef int               DS2;  // Different scopes - OK.
+  // Both are in the same non-file scope - error.
+  typedef int               LS1;  // expected-note{{previous}}
+  typedef long              LS1;  // expected-error{{redefinition}}
+}