[MS Compat] Allow _Atomic(Type) and 'struct _Atomic' to coexist

[majnemer]

Summary

MSVC 2013 ships, as part of its STL implementation, a class named
'_Atomic'. This is unfortunate because this keyword is in conflict with
the C11 keyword with the same name. Our solution was to disable this
keyword when targeting MSVC 2013 and reenable it for 2015.

However, this makes it impossible for clang's headers to make use of
_Atomic. This is problematic in the case of libc++ as it makes heavy
use of this keyword.

Let the keywordness of _Atomic float under certain circumstances:
the body of a class named _Atomic, or a class with a base specifier
named _Atomic, will not have the keyword variant of _Atomic for the
duration of the class body. This is sufficient to allow us to correctly
handle _Atomic in the STL while permitting us to use _Atomic as a
keyword everywhere else.

[rsmith]

lib/Parse/ParseDeclCXX.cpp

1310–1325 ↗

(On Diff #29807)

This seems a lot more general than you need: can you just make the set function revert to identifier and the destructor revert to tok::kw__Atomic?

1311 ↗

(On Diff #29807)

Maybe use a constructor with a bool Enabled rather than a setter?

1327–1339 ↗

(On Diff #29807)

I think this can be simplified to:

if (getLangOpts().MSVCCompat && Tok.is(tok::kw__Atomic) && TagType == DeclSpec::TST_struct)

1334 ↗

(On Diff #29807)

Please add a comment explaining why you're doing this. We conventionally use a comment containing "HACK" for workarounds for standard library issues. (Though we're missing that in the comment above, in the call to TryKeywordIdentFallback.)

1892–1897 ↗

(On Diff #29807)

You don't appear to have any testcases for this.

test/SemaCXX/MicrosoftCompatibility.cpp

12–13 ↗

(On Diff #29807)

Please add a testcase that uses _Atomic as a keyword after it's defined as a type. Please also add a testcase showing the uses of the _Atomic type that we need to be compatible with (as a base class, maybe?).

[rnk]

This seems like a better solution. I assume you manually tested with MSVC's <atomic>.

test/SemaCXX/MicrosoftCompatibility.cpp

13 ↗

(On Diff #29807)

Did you want to add a test for the base specifier case?

[majnemer]

lib/Parse/ParseDeclCXX.cpp

1310–1325 ↗

(On Diff #29807)

If I unconditionally revert to kw__Atomic, I will not correctly handle cases where ParseClassSpecifier is called recursively. If this happens, the _Atomic identifier will have getTokenID() already set to tok::identifier and we should let the caller's RAII object revert it back to kw__Atomic instead.

1334 ↗

(On Diff #29807)

Sure, done.

1892–1897 ↗

(On Diff #29807)

Fixed.

test/SemaCXX/MicrosoftCompatibility.cpp

12–13 ↗

(On Diff #29807)

Done.

13 ↗

(On Diff #29807)

Done.

[majnemer]

• Address review comments

[rsmith]

lib/Parse/ParseDeclCXX.cpp

1310–1325 ↗

(On Diff #29836)

Oh, I see what you're doing; this also restores the change to the IdentifierInfo's kind when parsing the base specifier. I'm not really happy with that approach. Saving and restoring the kind of the _Atomic identifier while parsing struct _Atomic seems OK; doing it when parsing every single struct does not seem targeted enough.

Instead, I suggest the following:

• Only change the type of _Atomic within the definition of struct _Atomic
• When parsing a base specifier and you see _Atomic, change just that one token to an identifier
• When parsing a declaration and you see _Atomic, check whether (a) you're in a typedef, and (b) you've not got a type yet, and (c) the token after the next token is a semicolon, and if so, convert that _Atomic token to an identifier

1896–1898 ↗

(On Diff #29836)

This can be simplified in the same way as the previous check.

1899 ↗

(On Diff #29836)

This could be null. But you can just grab the identifier info from Tok (both here and above).

[majnemer]

lib/Parse/ParseDeclCXX.cpp

1310–1325 ↗

(On Diff #29836)

I can implement your suggestion but I don't think it will work without some tweaks. What I was accomplishing with my aggressive handling of _Atomic was getting typedef _Atomic<_Ty, sizeof (_Ty)> _My_base; handled "correctly".

The next token after _Atomic in that typedef is not a semicolon, it is a tok::less.

If we are going with this implementation technique, I don't think I even need Ident__Atomic anymore.

1896–1898 ↗

(On Diff #29836)

Sure.

1899 ↗

(On Diff #29836)

Sure.

[majnemer]

• Address review comments
• Address Richard's latest comments.

[majnemer]

Ping.

[rsmith]

LGTM

lib/Parse/ParseDeclCXX.cpp

1896 ↗

(On Diff #29855)

GetLookAheadToken(1) is more commonly written as NextToken().

cfe/trunk/include/clang/Basic/IdentifierTable.h

bool IsFutureCompatKeyword  : 1; // True if identifier is a keyword in a
                                    // newer Standard or proposed Standard.
   bool IsPoisoned             : 1; // True if identifier is poisoned.
   bool IsCPPOperatorKeyword   : 1; // True if ident is a C++ operator keyword.
   bool NeedsHandleIdentifier  : 1; // See "RecomputeNeedsHandleIdentifier".
   bool IsFromAST              : 1; // True if identifier was loaded (at least 
                                    // partially) from an AST file.
   bool ChangedAfterLoad       : 1; // True if identifier has changed from the
                                    // definition loaded from an AST file.
-  bool RevertedTokenID        : 1; // True if RevertTokenIDToIdentifier was
+  bool RevertedTokenID        : 1; // True if revertTokenIDToIdentifier was
                                    // called.
   bool OutOfDate              : 1; // True if there may be additional
                                    // information about this identifier
                                    // stored externally.
   bool IsModulesImport        : 1; // True if this is the 'import' contextual
                                    // keyword.
   // 30 bit left in 64-bit word.
 
bool hadMacroDefinition() const {
     return HadMacro;
   }
 
   /// If this is a source-language token (e.g. 'for'), this API
   /// can be used to cause the lexer to map identifiers to source-language
   /// tokens.
   tok::TokenKind getTokenID() const { return (tok::TokenKind)TokenID; }
 
-  /// \brief True if RevertTokenIDToIdentifier() was called.
+  /// \brief True if revertTokenIDToIdentifier() was called.
   bool hasRevertedTokenIDToIdentifier() const { return RevertedTokenID; }
 
   /// \brief Revert TokenID to tok::identifier; used for GNU libstdc++ 4.2
   /// compatibility.
   ///
   /// TokenID is normally read-only but there are 2 instances where we revert it
   /// to tok::identifier for libstdc++ 4.2. Keep track of when this happens
   /// using this method so we can inform serialization about it.
   void revertTokenIDToIdentifier() {
     assert(TokenID != tok::identifier && "Already at tok::identifier");
     TokenID = tok::identifier;
     RevertedTokenID = true;
   }
+  void revertIdentifierToTokenID(tok::TokenKind TK) {
+    assert(TokenID == tok::identifier && "Should be at tok::identifier");
+    TokenID = TK;
+    RevertedTokenID = false;
+  }
 
   /// \brief Return the preprocessor keyword ID for this identifier.
   ///
   /// For example, "define" will return tok::pp_define.
   tok::PPKeywordKind getPPKeywordID() const;
 
   /// \brief Return the Objective-C keyword ID for the this identifier.
   ///

cfe/trunk/include/clang/Basic/TokenKinds.def

 KEYWORD(typedef                     , KEYALL)
 KEYWORD(union                       , KEYALL)
 KEYWORD(unsigned                    , KEYALL)
 KEYWORD(void                        , KEYALL)
 KEYWORD(volatile                    , KEYALL)
 KEYWORD(while                       , KEYALL)
 KEYWORD(_Alignas                    , KEYALL)
 KEYWORD(_Alignof                    , KEYALL)
-KEYWORD(_Atomic                     , KEYALL|KEYNOMS18|KEYNOOPENCL)
+KEYWORD(_Atomic                     , KEYALL|KEYNOOPENCL)
 KEYWORD(_Bool                       , KEYNOCXX)
 KEYWORD(_Complex                    , KEYALL)
 KEYWORD(_Generic                    , KEYALL)
 KEYWORD(_Imaginary                  , KEYALL)
 KEYWORD(_Noreturn                   , KEYALL)
 KEYWORD(_Static_assert              , KEYALL)
 KEYWORD(_Thread_local               , KEYALL)
 KEYWORD(__func__                    , KEYALL)

cfe/trunk/lib/Parse/ParseDecl.cpp

void Parser::ParseDeclarationSpecifiers(DeclSpec &DS,
   // We use Sema's policy to get bool macros right.
   const PrintingPolicy &Policy = Actions.getPrintingPolicy();
   while (1) {
     bool isInvalid = false;
     bool isStorageClass = false;
     const char *PrevSpec = nullptr;
     unsigned DiagID = 0;
 
+    // HACK: MSVC doesn't consider _Atomic to be a keyword and its STL
+    // implementation for VS2013 uses _Atomic as an identifier for one of the
+    // classes in <atomic>.
+    //
+    // A typedef declaration containing _Atomic<...> is among the places where
+    // the class is used.  If we are currently parsing such a declaration, treat
+    // the token as an identifier.
+    if (getLangOpts().MSVCCompat && Tok.is(tok::kw__Atomic) &&
+        DS.getStorageClassSpec() == clang::DeclSpec::SCS_typedef &&
+        !DS.hasTypeSpecifier() && GetLookAheadToken(1).is(tok::less))
+      Tok.setKind(tok::identifier);
+
     SourceLocation Loc = Tok.getLocation();
 
     switch (Tok.getKind()) {
     default:
     DoneWithDeclSpec:
       if (!AttrsLastTime)
         ProhibitAttributes(attrs);
       else {

cfe/trunk/lib/Parse/ParseDeclCXX.cpp

if (TagType == DeclSpec::TST_struct &&
                   tok::kw___is_volatile))
     // GNU libstdc++ 4.2 and libc++ use certain intrinsic names as the
     // name of struct templates, but some are keywords in GCC >= 4.3
     // and Clang. Therefore, when we see the token sequence "struct
     // X", make X into a normal identifier rather than a keyword, to
     // allow libstdc++ 4.2 and libc++ to work properly.
     TryKeywordIdentFallback(true);
 
+  struct PreserveAtomicIdentifierInfoRAII {
+    PreserveAtomicIdentifierInfoRAII(Token &Tok, bool Enabled)
+        : AtomicII(nullptr) {
+      if (!Enabled)
+        return;
+      assert(Tok.is(tok::kw__Atomic));
+      AtomicII = Tok.getIdentifierInfo();
+      AtomicII->revertTokenIDToIdentifier();
+      Tok.setKind(tok::identifier);
+    }
+    ~PreserveAtomicIdentifierInfoRAII() {
+      if (!AtomicII)
+        return;
+      AtomicII->revertIdentifierToTokenID(tok::kw__Atomic);
+    }
+    IdentifierInfo *AtomicII;
+  };
+
+  // HACK: MSVC doesn't consider _Atomic to be a keyword and its STL
+  // implementation for VS2013 uses _Atomic as an identifier for one of the
+  // classes in <atomic>.  When we are parsing 'struct _Atomic', don't consider
+  // '_Atomic' to be a keyword.  We are careful to undo this so that clang can
+  // use '_Atomic' in its own header files.
+  bool ShouldChangeAtomicToIdentifier = getLangOpts().MSVCCompat &&
+                                        Tok.is(tok::kw__Atomic) &&
+                                        TagType == DeclSpec::TST_struct;
+  PreserveAtomicIdentifierInfoRAII AtomicTokenGuard(
+      Tok, ShouldChangeAtomicToIdentifier);
+
   // Parse the (optional) nested-name-specifier.
   CXXScopeSpec &SS = DS.getTypeSpecScope();
   if (getLangOpts().CPlusPlus) {
     // "FOO : BAR" is not a potential typo for "FOO::BAR".  In this context it
     // is a base-specifier-list.
     ColonProtectionRAIIObject X(*this);
 
     CXXScopeSpec Spec;
if (Tok.is(tok::kw_virtual))  {
     }
 
     IsVirtual = true;
   }
 
   CheckMisplacedCXX11Attribute(Attributes, StartLoc);
 
   // Parse the class-name.
+
+  // HACK: MSVC doesn't consider _Atomic to be a keyword and its STL
+  // implementation for VS2013 uses _Atomic as an identifier for one of the
+  // classes in <atomic>.  Treat '_Atomic' to be an identifier when we are
+  // parsing the class-name for a base specifier.
+  if (getLangOpts().MSVCCompat && Tok.is(tok::kw__Atomic) &&
+      NextToken().is(tok::less))
+    Tok.setKind(tok::identifier);
+
   SourceLocation EndLocation;
   SourceLocation BaseLoc;
   TypeResult BaseType = ParseBaseTypeSpecifier(BaseLoc, EndLocation);
   if (BaseType.isInvalid())
     return true;
 
   // Parse the optional ellipsis (for a pack expansion). The ellipsis is 
   // actually part of the base-specifier-list grammar productions, but we

cfe/trunk/test/SemaCXX/MicrosoftCompatibility.cpp

 // RUN: %clang_cc1 %s -triple i686-pc-win32 -fsyntax-only -std=c++11 -Wmicrosoft -verify -fms-compatibility -fexceptions -fcxx-exceptions -fms-compatibility-version=19.00
 // RUN: %clang_cc1 %s -triple i686-pc-win32 -fsyntax-only -std=c++11 -Wmicrosoft -verify -fms-compatibility -fexceptions -fcxx-exceptions -fms-compatibility-version=18.00
 
 #if defined(_HAS_CHAR16_T_LANGUAGE_SUPPORT) && _HAS_CHAR16_T_LANGUAGE_SUPPORT
 char16_t x;
 char32_t y;
 #else
 typedef unsigned short char16_t;
 typedef unsigned int char32_t;
 #endif
 
-#if _MSC_VER >= 1900
 _Atomic(int) z;
-#else
-struct _Atomic {};
-#endif
+template <typename T>
+struct _Atomic {
+  _Atomic() {}
+  ~_Atomic() {}
+};
+template <typename T>
+struct atomic : _Atomic<T> {
+  typedef _Atomic<T> TheBase;
+  TheBase field;
+};
+_Atomic(int) alpha;
 
 typename decltype(3) a; // expected-warning {{expected a qualified name after 'typename'}}
 
 namespace ms_conversion_rules {
 
 void f(float a);
 void f(int a);