[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

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

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

1327–1339

I think this can be simplified to:

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

1334

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

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

test/SemaCXX/MicrosoftCompatibility.cpp

12–13

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

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

[majnemer]

lib/Parse/ParseDeclCXX.cpp

1310–1325

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

Sure, done.

1892–1897

Fixed.

test/SemaCXX/MicrosoftCompatibility.cpp

12–13

Done.

13

Done.

[majnemer]

• Address review comments

[rsmith]

lib/Parse/ParseDeclCXX.cpp

1310–1325

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

1892–1894

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

1895

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

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.

1892–1894

Sure.

1895

Sure.

[majnemer]

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

[majnemer]

Ping.

[rsmith]

LGTM

lib/Parse/ParseDeclCXX.cpp

1898

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

include/clang/Basic/IdentifierTable.h

public:
   /// 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.
   ///

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)

include/clang/Parse/Parser.h

class Parser : public CodeCompletionHandler {
   
   /// \brief Identifier for "message".
   IdentifierInfo *Ident_message;
 
   /// C++0x contextual keywords.
   mutable IdentifierInfo *Ident_final;
   mutable IdentifierInfo *Ident_override;
 
+  /// \brief Identifier for "_Atomic".
+  IdentifierInfo *Ident__Atomic;
+
   // C++ type trait keywords that can be reverted to identifiers and still be
   // used as type traits.
   llvm::SmallDenseMap<IdentifierInfo *, tok::TokenKind> RevertibleTypeTraits;
 
   std::unique_ptr<PragmaHandler> AlignHandler;
   std::unique_ptr<PragmaHandler> GCCVisibilityHandler;
   std::unique_ptr<PragmaHandler> OptionsHandler;
   std::unique_ptr<PragmaHandler> PackHandler;

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 PreserveIdentifierInfoRAII {
+    void set(IdentifierInfo *ToPreserve) {
+      II = ToPreserve;
+      TK = II->getTokenID();
+    }
+    ~PreserveIdentifierInfoRAII() {
+      if (II) {
+        if (TK == tok::identifier && II->getTokenID() != tok::identifier)
+          II->RevertTokenIDToIdentifier();
+        else if (TK != tok::identifier && II->getTokenID() == tok::identifier)
+          II->RevertIdentifierToTokenID(TK);
+      }
+    }
+    IdentifierInfo *II = nullptr;
+    tok::TokenKind TK;
+  };
+
+  PreserveIdentifierInfoRAII AtomicTokenGuard;
+  if (getLangOpts().MSVCCompat) {
+    if (!Ident__Atomic)
+      Ident__Atomic = &PP.getIdentifierTable().get("_Atomic");
+    AtomicTokenGuard.set(Ident__Atomic);
+  }
+
+  if (getLangOpts().MSVCCompat && TagType == DeclSpec::TST_struct &&
+      Tok.isNot(tok::identifier) && !Tok.isAnnotation() &&
+      Tok.getIdentifierInfo() && Tok.is(tok::kw__Atomic)) {
+    Ident__Atomic->RevertTokenIDToIdentifier();
+    Tok.setKind(tok::identifier);
+  }
+
   // 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.
+  if (getLangOpts().MSVCCompat && Tok.isNot(tok::identifier) &&
+      !Tok.isAnnotation() && Tok.getIdentifierInfo() &&
+      Tok.is(tok::kw__Atomic)) {
+    Ident__Atomic->RevertTokenIDToIdentifier();
+    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
   // parse it here for convenience.

lib/Parse/Parser.cpp

if(getLangOpts().Borland) {
     PP.SetPoisonReason(Ident__exception_info,diag::err_seh___except_filter);
     PP.SetPoisonReason(Ident___exception_info,diag::err_seh___except_filter);
     PP.SetPoisonReason(Ident_GetExceptionInfo,diag::err_seh___except_filter);
     PP.SetPoisonReason(Ident__abnormal_termination,diag::err_seh___finally_block);
     PP.SetPoisonReason(Ident___abnormal_termination,diag::err_seh___finally_block);
     PP.SetPoisonReason(Ident_AbnormalTermination,diag::err_seh___finally_block);
   }
 
+  Ident__Atomic = nullptr;
+
   Actions.Initialize();
 
   // Prime the lexer look-ahead.
   ConsumeToken();
 }
 
 void Parser::LateTemplateParserCleanupCallback(void *P) {
   // While this RAII helper doesn't bracket any actual work, the destructor will

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
 
 typename decltype(3) a; // expected-warning {{expected a qualified name after 'typename'}}
 
 namespace ms_conversion_rules {
 
 void f(float a);
 void f(int a);