[modules] If we reach a definition of a class for which we already have a

non-visible definition, skip the new definition and make the old one visible
instead of trying to parse it again and failing horribly. C++'s ODR allows
us to assume that the two definitions are identical.

llvm-svn: 233250

clang/include/clang/AST/ASTMutationListener.h

@@ -24,6 +24,7 @@ namespace clang {
   class DeclContext;
   class FunctionDecl;
   class FunctionTemplateDecl;
+  class NamedDecl;
   class ObjCCategoryDecl;
   class ObjCContainerDecl;
   class ObjCInterfaceDecl;
@@ -113,6 +114,12 @@ public:
   /// \param D the declaration marked OpenMP threadprivate.
   virtual void DeclarationMarkedOpenMPThreadPrivate(const Decl *D) {}
 
+  /// \brief A definition has been made visible by being redefined locally.
+  ///
+  /// \param D The definition that was previously not visible.
+  virtual void RedefinedHiddenDefinition(const NamedDecl *D,
+                                         SourceLocation Loc) {}
+
   // NOTE: If new methods are added they should also be added to
   // MultiplexASTMutationListener.
 };

clang/include/clang/Parse/Parser.h

@@ -2284,6 +2284,10 @@ private:
                            AccessSpecifier AS, bool EnteringContext,
                            DeclSpecContext DSC, 
                            ParsedAttributesWithRange &Attributes);
+  void SkipCXXMemberSpecification(SourceLocation StartLoc,
+                                  SourceLocation AttrFixitLoc,
+                                  unsigned TagType,
+                                  Decl *TagDecl);
   void ParseCXXMemberSpecification(SourceLocation StartLoc,
                                    SourceLocation AttrFixitLoc,
                                    ParsedAttributesWithRange &Attrs,

clang/include/clang/Sema/Sema.h

@@ -1279,6 +1279,10 @@ private:
   bool RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
                            TypeDiagnoser &Diagnoser);
 public:
+  /// Determine if \p D has a visible definition. If not, suggest a declaration
+  /// that should be made visible to expose the definition.
+  bool hasVisibleDefinition(NamedDecl *D, NamedDecl **Suggested);
+
   bool RequireCompleteType(SourceLocation Loc, QualType T,
                            TypeDiagnoser &Diagnoser);
   bool RequireCompleteType(SourceLocation Loc, QualType T,
@@ -1725,7 +1729,7 @@ public:
                  bool &OwnedDecl, bool &IsDependent,
                  SourceLocation ScopedEnumKWLoc,
                  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
-                 bool IsTypeSpecifier);
+                 bool IsTypeSpecifier, bool *SkipBody = nullptr);
 
   Decl *ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
                                 unsigned TagSpec, SourceLocation TagLoc,
@@ -1790,6 +1794,11 @@ public:
   /// struct, or union).
   void ActOnTagStartDefinition(Scope *S, Decl *TagDecl);
 
+  /// \brief Invoked when we enter a tag definition that we're skipping.
+  void ActOnTagStartSkippedDefinition(Scope *S, Decl *TD) {
+    PushDeclContext(S, cast<DeclContext>(TD));
+  }
+
   Decl *ActOnObjCContainerStartDefinition(Decl *IDecl);
 
   /// ActOnStartCXXMemberDeclarations - Invoked when we have parsed a
@@ -1805,6 +1814,10 @@ public:
   void ActOnTagFinishDefinition(Scope *S, Decl *TagDecl,
                                 SourceLocation RBraceLoc);
 
+  void ActOnTagFinishSkippedDefinition() {
+    PopDeclContext();
+  }
+
   void ActOnObjCContainerFinishDefinition();
 
   /// \brief Invoked when we must temporarily exit the objective-c container

clang/include/clang/Serialization/ASTWriter.h

@@ -861,6 +861,8 @@ public:
                                     const ObjCCategoryDecl *ClassExt) override;
   void DeclarationMarkedUsed(const Decl *D) override;
   void DeclarationMarkedOpenMPThreadPrivate(const Decl *D) override;
+  void RedefinedHiddenDefinition(const NamedDecl *D,
+                                 SourceLocation Loc) override;
 };
 
 /// \brief AST and semantic-analysis consumer that generates a

clang/lib/Frontend/MultiplexConsumer.cpp

@@ -110,6 +110,8 @@ public:
                                     const ObjCCategoryDecl *ClassExt) override;
   void DeclarationMarkedUsed(const Decl *D) override;
   void DeclarationMarkedOpenMPThreadPrivate(const Decl *D) override;
+  void RedefinedHiddenDefinition(const NamedDecl *D,
+                                 SourceLocation Loc) override;
 
 private:
   std::vector<ASTMutationListener*> Listeners;
@@ -193,6 +195,11 @@ void MultiplexASTMutationListener::DeclarationMarkedOpenMPThreadPrivate(
   for (size_t i = 0, e = Listeners.size(); i != e; ++i)
     Listeners[i]->DeclarationMarkedOpenMPThreadPrivate(D);
 }
+void MultiplexASTMutationListener::RedefinedHiddenDefinition(
+    const NamedDecl *D, SourceLocation Loc) {
+  for (auto *L : Listeners)
+    L->RedefinedHiddenDefinition(D, Loc);
+}
 
 }  // end namespace clang
 

clang/lib/Parse/ParseDeclCXX.cpp

@@ -1550,6 +1550,7 @@ void Parser::ParseClassSpecifier(tok::TokenKind TagTokKind,
   TypeResult TypeResult = true; // invalid
 
   bool Owned = false;
+  bool SkipBody = false;
   if (TemplateId) {
     // Explicit specialization, class template partial specialization,
     // or explicit instantiation.
@@ -1695,7 +1696,8 @@ void Parser::ParseClassSpecifier(tok::TokenKind TagTokKind,
                                        TParams, Owned, IsDependent,
                                        SourceLocation(), false,
                                        clang::TypeResult(),
-                                       DSC == DSC_type_specifier);
+                                       DSC == DSC_type_specifier,
+                                       &SkipBody);
 
     // If ActOnTag said the type was dependent, try again with the
     // less common call.
@@ -1711,7 +1713,10 @@ void Parser::ParseClassSpecifier(tok::TokenKind TagTokKind,
     assert(Tok.is(tok::l_brace) ||
            (getLangOpts().CPlusPlus && Tok.is(tok::colon)) ||
            isCXX11FinalKeyword());
-    if (getLangOpts().CPlusPlus)
+    if (SkipBody)
+      SkipCXXMemberSpecification(StartLoc, AttrFixitLoc, TagType,
+                                 TagOrTempResult.get());
+    else if (getLangOpts().CPlusPlus)
       ParseCXXMemberSpecification(StartLoc, AttrFixitLoc, attrs, TagType,
                                   TagOrTempResult.get());
     else
@@ -2688,6 +2693,55 @@ ExprResult Parser::ParseCXXMemberInitializer(Decl *D, bool IsFunction,
   return ParseInitializer();
 }
 
+void Parser::SkipCXXMemberSpecification(SourceLocation RecordLoc,
+                                        SourceLocation AttrFixitLoc,
+                                        unsigned TagType, Decl *TagDecl) {
+  // Skip the optional 'final' keyword.
+  if (getLangOpts().CPlusPlus && Tok.is(tok::identifier)) {
+    assert(isCXX11FinalKeyword() && "not a class definition");
+    ConsumeToken();
+
+    // Diagnose any C++11 attributes after 'final' keyword.
+    // We deliberately discard these attributes.
+    ParsedAttributesWithRange Attrs(AttrFactory);
+    CheckMisplacedCXX11Attribute(Attrs, AttrFixitLoc);
+
+    // This can only happen if we had malformed misplaced attributes;
+    // we only get called if there is a colon or left-brace after the
+    // attributes.
+    if (Tok.isNot(tok::colon) && Tok.isNot(tok::l_brace))
+      return;
+  }
+
+  // Skip the base clauses. This requires actually parsing them, because
+  // otherwise we can't be sure where they end (a left brace may appear
+  // within a template argument).
+  if (Tok.is(tok::colon)) {
+    // Enter the scope of the class so that we can correctly parse its bases.
+    ParseScope ClassScope(this, Scope::ClassScope|Scope::DeclScope);
+    ParsingClassDefinition ParsingDef(*this, TagDecl, /*NonNestedClass*/ true,
+                                      TagType == DeclSpec::TST_interface);
+    Actions.ActOnTagStartSkippedDefinition(getCurScope(), TagDecl);
+
+    // Parse the bases but don't attach them to the class.
+    ParseBaseClause(nullptr);
+
+    Actions.ActOnTagFinishSkippedDefinition();
+
+    if (!Tok.is(tok::l_brace)) {
+      Diag(PP.getLocForEndOfToken(PrevTokLocation),
+           diag::err_expected_lbrace_after_base_specifiers);
+      return;
+    }
+  }
+
+  // Skip the body.
+  assert(Tok.is(tok::l_brace));
+  BalancedDelimiterTracker T(*this, tok::l_brace);
+  T.consumeOpen();
+  T.skipToEnd();
+}
+
 /// ParseCXXMemberSpecification - Parse the class definition.
 ///
 ///       member-specification:

clang/lib/Sema/SemaCXXScopeSpec.cpp

@@ -218,6 +218,7 @@ bool Sema::RequireCompleteDeclContext(CXXScopeSpec &SS,
   // Fixed enum types are complete, but they aren't valid as scopes
   // until we see a definition, so awkwardly pull out this special
   // case.
+  // FIXME: The definition might not be visible; complain if it is not.
   const EnumType *enumType = dyn_cast_or_null<EnumType>(tagType);
   if (!enumType || enumType->getDecl()->isCompleteDefinition())
     return false;

clang/lib/Sema/SemaDecl.cpp

@@ -16,6 +16,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTLambda.h"
+#include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/CommentDiagnostic.h"
@@ -11233,13 +11234,16 @@ static FixItHint createFriendTagNNSFixIt(Sema &SemaRef, NamedDecl *ND, Scope *S,
   return FixItHint::CreateInsertion(NameLoc, Insertion);
 }
 
-/// ActOnTag - This is invoked when we see 'struct foo' or 'struct {'.  In the
+/// \brief This is invoked when we see 'struct foo' or 'struct {'.  In the
 /// former case, Name will be non-null.  In the later case, Name will be null.
 /// TagSpec indicates what kind of tag this is. TUK indicates whether this is a
 /// reference/declaration/definition of a tag.
 ///
-/// IsTypeSpecifier is true if this is a type-specifier (or
+/// \param IsTypeSpecifier \c true if this is a type-specifier (or
 /// trailing-type-specifier) other than one in an alias-declaration.
+///
+/// \param SkipBody If non-null, will be set to true if the caller should skip
+/// the definition of this tag, and treat it as if it were a declaration.
 Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                      SourceLocation KWLoc, CXXScopeSpec &SS,
                      IdentifierInfo *Name, SourceLocation NameLoc,
@@ -11250,7 +11254,7 @@ Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                      SourceLocation ScopedEnumKWLoc,
                      bool ScopedEnumUsesClassTag,
                      TypeResult UnderlyingType,
-                     bool IsTypeSpecifier) {
+                     bool IsTypeSpecifier, bool *SkipBody) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
@@ -11676,7 +11680,7 @@ Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
 
           // Diagnose attempts to redefine a tag.
           if (TUK == TUK_Definition) {
-            if (TagDecl *Def = PrevTagDecl->getDefinition()) {
+            if (NamedDecl *Def = PrevTagDecl->getDefinition()) {
               // If we're defining a specialization and the previous definition
               // is from an implicit instantiation, don't emit an error
               // here; we'll catch this in the general case below.
@@ -11692,7 +11696,19 @@ Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                     TSK_ExplicitSpecialization;
               }
 
-              if (!IsExplicitSpecializationAfterInstantiation) {
+              if (SkipBody && getLangOpts().CPlusPlus &&
+                  !hasVisibleDefinition(Def, &Def)) {
+                // There is a definition of this tag, but it is not visible. We
+                // explicitly make use of C++'s one definition rule here, and
+                // assume that this definition is identical to the hidden one
+                // we already have. Make the existing definition visible and
+                // use it in place of this one.
+                *SkipBody = true;
+                if (auto *Listener = getASTMutationListener())
+                  Listener->RedefinedHiddenDefinition(Def, KWLoc);
+                Def->setHidden(false);
+                return Def;
+              } else if (!IsExplicitSpecializationAfterInstantiation) {
                 // A redeclaration in function prototype scope in C isn't
                 // visible elsewhere, so merely issue a warning.
                 if (!getLangOpts().CPlusPlus && S->containedInPrototypeScope())

clang/lib/Sema/SemaType.cpp

@@ -5126,9 +5126,9 @@ bool Sema::RequireCompleteType(SourceLocation Loc, QualType T,
 /// \param D The definition of the entity.
 /// \param Suggested Filled in with the declaration that should be made visible
 ///        in order to provide a definition of this entity.
-static bool hasVisibleDefinition(Sema &S, NamedDecl *D, NamedDecl **Suggested) {
+bool Sema::hasVisibleDefinition(NamedDecl *D, NamedDecl **Suggested) {
   // Easy case: if we don't have modules, all declarations are visible.
-  if (!S.getLangOpts().Modules)
+  if (!getLangOpts().Modules)
     return true;
 
   // If this definition was instantiated from a template, map back to the
@@ -5144,7 +5144,7 @@ static bool hasVisibleDefinition(Sema &S, NamedDecl *D, NamedDecl **Suggested) {
       // If the enum has a fixed underlying type, any declaration of it will do.
       *Suggested = nullptr;
       for (auto *Redecl : ED->redecls()) {
-        if (LookupResult::isVisible(S, Redecl))
+        if (LookupResult::isVisible(*this, Redecl))
           return true;
         if (Redecl->isThisDeclarationADefinition() ||
             (Redecl->isCanonicalDecl() && !*Suggested))
@@ -5159,7 +5159,7 @@ static bool hasVisibleDefinition(Sema &S, NamedDecl *D, NamedDecl **Suggested) {
   // FIXME: If we merged any other decl into D, and that declaration is visible,
   // then we should consider a definition to be visible.
   *Suggested = D;
-  return LookupResult::isVisible(S, D);
+  return LookupResult::isVisible(*this, D);
 }
 
 /// Locks in the inheritance model for the given class and all of its bases.
@@ -5210,7 +5210,7 @@ bool Sema::RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
     // If we know about the definition but it is not visible, complain.
     NamedDecl *SuggestedDef = nullptr;
     if (!Diagnoser.Suppressed && Def &&
-        !hasVisibleDefinition(*this, Def, &SuggestedDef)) {
+        !hasVisibleDefinition(Def, &SuggestedDef)) {
       // Suppress this error outside of a SFINAE context if we've already
       // emitted the error once for this type. There's no usefulness in
       // repeating the diagnostic.

clang/lib/Serialization/ASTCommon.h

@@ -35,7 +35,8 @@ enum DeclUpdateKind {
   UPD_DECL_MARKED_USED,
   UPD_MANGLING_NUMBER,
   UPD_STATIC_LOCAL_NUMBER,
-  UPD_DECL_MARKED_OPENMP_THREADPRIVATE
+  UPD_DECL_MARKED_OPENMP_THREADPRIVATE,
+  UPD_DECL_EXPORTED
 };
 
 TypeIdx TypeIdxFromBuiltin(const BuiltinType *BT);

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -3793,10 +3793,24 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile,
     case UPD_STATIC_LOCAL_NUMBER:
       Reader.Context.setStaticLocalNumber(cast<VarDecl>(D), Record[Idx++]);
       break;
+
     case UPD_DECL_MARKED_OPENMP_THREADPRIVATE:
       D->addAttr(OMPThreadPrivateDeclAttr::CreateImplicit(
           Reader.Context, ReadSourceRange(Record, Idx)));
       break;
+
+    case UPD_DECL_EXPORTED:
+      unsigned SubmoduleID = readSubmoduleID(Record, Idx);
+      Module *Owner = SubmoduleID ? Reader.getSubmodule(SubmoduleID) : nullptr;
+      if (Owner && Owner->NameVisibility != Module::AllVisible) {
+        // If Owner is made visible at some later point, make this declaration
+        // visible too.
+        Reader.HiddenNamesMap[Owner].HiddenDecls.push_back(D);
+      } else {
+        // The declaration is now visible.
+        D->Hidden = false;
+      }
+      break;
     }
   }
 }

clang/lib/Serialization/ASTWriter.cpp

@@ -4927,10 +4927,15 @@ void ASTWriter::WriteDeclUpdatesBlocks(RecordDataImpl &OffsetsRecord) {
       case UPD_STATIC_LOCAL_NUMBER:
         Record.push_back(Update.getNumber());
         break;
+
       case UPD_DECL_MARKED_OPENMP_THREADPRIVATE:
         AddSourceRange(D->getAttr<OMPThreadPrivateDeclAttr>()->getRange(),
                        Record);
         break;
+
+      case UPD_DECL_EXPORTED:
+        Record.push_back(inferSubmoduleIDFromLocation(Update.getLoc()));
+        break;
       }
     }
 
@@ -6074,3 +6079,11 @@ void ASTWriter::DeclarationMarkedOpenMPThreadPrivate(const Decl *D) {
 
   DeclUpdates[D].push_back(DeclUpdate(UPD_DECL_MARKED_OPENMP_THREADPRIVATE));
 }
+
+void ASTWriter::RedefinedHiddenDefinition(const NamedDecl *D,
+                                          SourceLocation Loc) {
+  assert(!WritingAST && "Already writing the AST!");
+  assert(D->isHidden() && "expected a hidden declaration");
+  assert(D->isFromASTFile() && "hidden decl not from AST file");
+  DeclUpdates[D].push_back(DeclUpdate(UPD_DECL_EXPORTED, Loc));
+}

clang/test/Modules/Inputs/submodules-merge-defs/defs.h

@@ -0,0 +1,6 @@
+struct A {};
+class B {
+  struct Inner1 {};
+  struct Inner2;
+};
+struct B::Inner2 : Inner1 {};

clang/test/Modules/Inputs/submodules-merge-defs/import-and-redefine.h

@@ -0,0 +1,5 @@
+// Trigger import of definitions, but don't make them visible.
+#include "empty.h"
+
+// Now parse the definitions again.
+#include "defs.h"

clang/test/Modules/Inputs/submodules-merge-defs/module.modulemap

@@ -0,0 +1,11 @@
+module "stuff" {
+  textual header "defs.h"
+  module "empty" { header "empty.h" }
+  module "use" { header "use-defs.h" }
+}
+
+module "redef" {
+  header "import-and-redefine.h"
+  // Do not re-export stuff.use
+  use "stuff"
+}

clang/test/Modules/Inputs/submodules-merge-defs/use-defs.h

@@ -0,0 +1 @@
+#include "defs.h"

clang/test/Modules/submodules-merge-defs.cpp

@@ -0,0 +1,13 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x c++ -fmodules-cache-path=%t -fmodules -I %S/Inputs/submodules-merge-defs %s -verify -fno-modules-error-recovery
+
+// Trigger import of definitions, but don't make them visible.
+#include "empty.h"
+
+A pre_a; // expected-error {{must be imported}} expected-error {{must use 'struct'}}
+// expected-note@defs.h:1 {{here}}
+
+// Make definitions from second module visible.
+#include "import-and-redefine.h"
+
+A post_a;