Add type_info predefined decl

[mwasplund]

Summary

Bug 39052 - [Modules TS] MSVC std library produces ambiguous type_info reference when including module with ms-compatibility

When compiling a modules-ts project with MSVC compatibility turned on the type_info class was getting defined multiple times. With this change I am adding the type_info class as a predefined decl to allow the reader/writer to resolve the duplicated declarations.

On top of this I am adding an extra check to ignore redeclaration checks on implicit types. This could be up for discussion since the spec does not specify if the implicit types should be in the global unit or if they can be overridden by the same type inside a module purview.

[rsmith]

Generally this looks good, but it needs an accompanying test.

include/clang/AST/ASTContext.h

1129–1130

Please indicate somewhere that this is the non-standard MSVC ::type_info type, not the standard std::type_info type.

lib/Sema/SemaDecl.cpp

1464

Somewhere up here we're calling getOwningModule() but should be calling getOwningModuleForLinkage(). (Please upload patches with full context as described at https://llvm.org/docs/Phabricator.html)

1467–1470

Rather than doing this, please change ASTContext::buildImplicitRecord to setModuleOwnershipKind(Decl::ModuleOwnershipKind::Unowned) on the created declaration.

[mwasplund]

In D52973#1259909, @rsmith wrote:

Generally this looks good, but it needs an accompanying test.

I was looking into that and trying to read up on the documentation for adding tests. I think I understand the crazy comment RUN test invocation, but got lost in the directory structure. Is there documentation on naming and placing regression tests? At first glance it seems arbitrary to me if the file is something like p6.cpp (which don't appear to be sequential) or my-test-name.cpp.

[rsmith]

In D52973#1259992, @mwasplund wrote:

In D52973#1259909, @rsmith wrote:

Generally this looks good, but it needs an accompanying test.

I was looking into that and trying to read up on the documentation for adding tests. I think I understand the crazy comment RUN test invocation, but got lost in the directory structure. Is there documentation on naming and placing regression tests? At first glance it seems arbitrary to me if the file is something like p6.cpp (which don't appear to be sequential) or my-test-name.cpp.

The tests under test/CXX are organized by section and paragraph of the C++ standard (we also have subdirectories for TSes, which are organized likewise, and for DRs, which are organized by number). The tests outside test/CXX are organized by the relevant part or feature of the compiler. Since this is a modules-specific failure but it's not related to a specific part of the Modules TS (in fact, it's an interaction between an MSVC-compatibility hack and our Modules TS implementation), the best home for it is probably a test in test/Modules, which you can name after the behavior you're testing (eg, ms-compat-typeinfo.cpp or similar). All inputs to the test other than the main test file should go in test/Modules/Inputs, in a subdirectory named after the stem of the test file (eg, test/Modules/Inputs/ms-compat-typeinfo).

[mwasplund]

lib/Sema/SemaDecl.cpp

1464

I am using the git mirror to work out of so I can commit incremental changes. However when I used the commands provided they were only showing my latest change, not the full diff from origin master. I will give it another try...

[mwasplund]

Can you see the full change? I am only seeing that latest commit after running: "git show HEAD -U999999 > mypatch.patch".

It was working fine with I ran "git diff master..mybranch > mybranch.diff" however that does not give the full context.

[mwasplund]

lib/Sema/SemaDecl.cpp

1467–1470

That doesn't seem to work. The check still fails since the owning module is now null. I can fix that by ignoring decls that have no owner on the old version, but I am also running into issues where the cached linkage no longer matches up.

[mwasplund]

I completely forgot this was open. I have been making incremental improvements to modules ts and this has gotten pulled into that change.

include/clang/AST/ASTContext.h

   /// The typedef for the predefined 'BOOL' type.
   mutable TypedefDecl *BOOLDecl = nullptr;
 
+  /// The class for the predifined 'type_info' type.
+  mutable RecordDecl *TypeInfoClassDecl = nullptr;
+
   // Typedefs which may be provided defining the structure of Objective-C
   // pseudo-builtins
   QualType ObjCIdRedefinitionType;

   /// Retrieve the declaration for the 128-bit unsigned integer type.
   TypedefDecl *getUInt128Decl() const;
 
+  /// Retrieve the declaration for the type_info class type.
+  RecordDecl *getTypeInfoClassDecl() const;
+
   //===--------------------------------------------------------------------===//
   //                           Type Constructors
   //===--------------------------------------------------------------------===//

include/clang/Serialization/ASTBitCodes.h

 
       /// The internal '__type_pack_element' template.
       PREDEF_DECL_TYPE_PACK_ELEMENT_ID = 16,
+
+      /// The internal 'type_info' class.
+      PREDEF_DECL_TYPE_INFO_CLASS_ID = 17,
     };
 
     /// The number of declaration IDs that are predefined.
     ///
     /// For more information about predefined declarations, see the
     /// \c PredefinedDeclIDs type and the PREDEF_DECL_*_ID constants.
-    const unsigned int NUM_PREDEF_DECL_IDS = 17;
+    const unsigned int NUM_PREDEF_DECL_IDS = 18;
 
     /// Record of updates for a declaration that was modified after
     /// being deserialized. This can occur within DECLTYPES_BLOCK_ID.

lib/AST/ASTContext.cpp

   return UInt128Decl;
 }
 
+RecordDecl *ASTContext::getTypeInfoClassDecl() const {
+  if (!TypeInfoClassDecl)
+    TypeInfoClassDecl = buildImplicitRecord("type_info", TTK_Class);
+  return TypeInfoClassDecl;
+}
+
 void ASTContext::InitBuiltinType(CanQualType &R, BuiltinType::Kind K) {
   auto *Ty = new (*this, TypeAlignment) BuiltinType(K);
   R = CanQualType::CreateUnsafe(QualType(Ty, 0));

lib/Sema/Sema.cpp

   if (getLangOpts().MSVCCompat) {
     if (getLangOpts().CPlusPlus &&
         IdResolver.begin(&Context.Idents.get("type_info")) == IdResolver.end())
-      PushOnScopeChains(Context.buildImplicitRecord("type_info", TTK_Class),
-                        TUScope);
+      PushOnScopeChains(Context.getTypeInfoClassDecl(), TUScope);
 
     addImplicitTypedef("size_t", Context.getSizeType());
   }

lib/Sema/SemaDecl.cpp

   if (NewM == OldM)
     return false;
 
+  // FIXME: The Modules TS does not specify how to handle inplicit types
+  // For now we will simply ignore the implicit global types
+  if (Old->isImplicit())
+    return false;
+
   // FIXME: Check proclaimed-ownership-declarations here too.
   bool NewIsModuleInterface = NewM && NewM->Kind == Module::ModuleInterfaceUnit;
   bool OldIsModuleInterface = OldM && OldM->Kind == Module::ModuleInterfaceUnit;

lib/Serialization/ASTReader.cpp

 
   case PREDEF_DECL_TYPE_PACK_ELEMENT_ID:
     return Context.getTypePackElementDecl();
+
+  case PREDEF_DECL_TYPE_INFO_CLASS_ID:
+    return Context.getTypeInfoClassDecl();
   }
   llvm_unreachable("PredefinedDeclIDs unknown enum value");
 }

lib/Serialization/ASTWriter.cpp

   RegisterPredefDecl(Context.CFConstantStringTagDecl,
                      PREDEF_DECL_CF_CONSTANT_STRING_TAG_ID);
   RegisterPredefDecl(Context.TypePackElementDecl,
-                     PREDEF_DECL_TYPE_PACK_ELEMENT_ID);
+                     PREDEF_DECL_TYPE_PACK_ELEMENT_ID);
+  RegisterPredefDecl(Context.TypeInfoClassDecl,
+                     PREDEF_DECL_TYPE_INFO_CLASS_ID);
 
   // Build a record containing all of the tentative definitions in this file, in
   // TentativeDefinitions order.  Generally, this record will be empty for