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 ↗

(On Diff #168607)

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.

lib/Sema/SemaDecl.cpp

if (New->getFriendObjectKind() &&
       Old->getOwningModuleForLinkage() != New->getOwningModuleForLinkage()) {
     New->setLocalOwningModule(Old->getOwningModule());
     makeMergedDefinitionVisible(New);
     return false;
   }
 
   Module *NewM = New->getOwningModule();
   Module *OldM = Old->getOwningModule();
   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;
   if (NewIsModuleInterface || OldIsModuleInterface) {
     // C++ Modules TS [basic.def.odr] 6.2/6.7 [sic]:
     //   if a declaration of D [...] appears in the purview of a module, all
     //   other such declarations shall appear in the purview of the same module

 /// \param IsTemplateId Whether the name is a (simple-)template-id, and thus
 /// we're declaring an explicit / partial specialization / instantiation.
 ///
 /// \returns true if we cannot safely recover from this error, false otherwise.
 bool Sema::diagnoseQualifiedDeclaration(CXXScopeSpec &SS, DeclContext *DC,
                                         DeclarationName Name,
                                         SourceLocation Loc, bool IsTemplateId) {
   DeclContext *Cur = CurContext;
-  while (Cur->isTransparentContext() || isa<CapturedDecl>(Cur))
+  while (isa<LinkageSpecDecl>(Cur) || isa<CapturedDecl>(Cur))
     Cur = Cur->getParent();
 
   // If the user provided a superfluous scope specifier that refers back to the
   // class in which the entity is already declared, diagnose and ignore it.
   //
   // class X {
   //   void X::f();
   // };