Make function local tags visible.

[v.g.vassilev]

Summary

Upon instantiating function decls, we should mark visible any tag decl in the function's body.

[rsmith]

lib/Sema/SemaLookup.cpp

1546–1548 ↗

(On Diff #67988)

This check should already do the right thing for the case of a function-scope declaration.

test/Modules/Inputs/PR28794/Subdir/LibBHeader.h

7–11

We should not be instantiating this template in the first place: the template definition should not be visible within pr28794.cpp because the LibBHeader.h submodule is never imported. I would guess the bug is that we're missing a visibility check for the template definition when we trigger the instantiation here.

[v.g.vassilev]

Do not instantiate function declarations which are not visible.

[v.g.vassilev]

Add forgotten change.

[rsmith]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3597–3599

Please update this FIXME to just "We need to track the instantiation stack in order to know which definitions should be visible within this instantiation."

3600–3605

I think this should be checked before we deal with late-parsed templates -- if the template definition isn't visible, an attempt to instantiate it shouldn't trigger it being parsed.

[rsmith]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3608–3609

If you can do that, it would be preferable. You'll presumably need to change the PatternDecl argument to PatternDecl->isThisDeclarationADefinition() || PatternDecl->isDefaulted() ? PatternDecl : nullptr or similar.

3670–3673

Yes, let's do this. It's not quite right, but the same is true for the InstantiateClass and InstantiateEnum cases, and it makes sense to handle all those cases the same way.

[v.g.vassilev]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3600–3605

If I am to sink the diags under the // TODO: this might change behavior. The diagnostics may not kick-in properly in case of late template parsing (see around line 3594).

[v.g.vassilev]

Add setHidden(false) and update fixme.

[rsmith]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3600–3605

As long as we treat a late-parsed template as being defined, it should be fine (and looking at FunctionDecl::isThisDeclarationADefinition, we do). It's not correct to parse a late-parsed template that's not visible, so we need to move this check to before we handle late-parsed templates for correctness.

[v.g.vassilev]

Move DiagnoseUninstantiableTemplate before the late template parsing.

[v.g.vassilev]

Sink some of the diagnostics in the DiagnoseUninstantiableTemplate.

[akuegel]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3646

Line 3619 to 3623 breaks the compilation of some of our targets. I am not sure if it is because of a mistake in our code or if there is something bad in this patch.
This comment seems to indicate that there is still something missing to make the check here correct?

[v.g.vassilev]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3646

@akuegel, would you be able to reduce a standalone example?

[akuegel]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3646

I tried to create a simple example, but those simple cases worked fine. So I am not sure what is going on in the more complex cases. I have asked Richard for help. Since Richard has access to that broken code, I hope he will figure it out.

[v.g.vassilev]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3646

Thanks! Otherwise, maybe creduce could help.

[rsmith]

Should be fixed in r279486, which also incorporates the updates to this change since it landed (sinking diagnostics into DiagnoseUninstantiableTemplate).

lib/Sema/SemaTemplate.cpp

494–495

} and else on the same line please. (Here and in a couple of places below.)

499–501

Sink this into the err_template_instantiate_undefined case below.

503–505

Sink this into the InstantiatedFromMember case below.

[v.g.vassilev]

Thanks! Shall I close this?

[v.g.vassilev]

Landed in r279164 and r279486.

include/clang/Sema/Sema.h

 
   bool DiagnoseUnknownTemplateName(const IdentifierInfo &II,
                                    SourceLocation IILoc,
                                    Scope *S,
                                    const CXXScopeSpec *SS,
                                    TemplateTy &SuggestedTemplate,
                                    TemplateNameKind &SuggestedKind);
 
+  bool DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
+                                      NamedDecl *Instantiation,
+                                      bool InstantiatedFromMember,
+                                      const NamedDecl *Pattern,
+                                      NamedDecl *PatternDef,
+                                      TemplateSpecializationKind TSK,
+                                      bool Complain = true);
+
   void DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl);
   TemplateDecl *AdjustDeclIfTemplate(Decl *&Decl);
 
   Decl *ActOnTypeParameter(Scope *S, bool Typename,
                            SourceLocation EllipsisLoc,
                            SourceLocation KeyLoc,
                            IdentifierInfo *ParamName,
                            SourceLocation ParamNameLoc,

lib/Sema/SemaTemplate.cpp

Sema::BuildDependentDeclRefExpr(const CXXScopeSpec &SS,
                                 SourceLocation TemplateKWLoc,
                                 const DeclarationNameInfo &NameInfo,
                                 const TemplateArgumentListInfo *TemplateArgs) {
   return DependentScopeDeclRefExpr::Create(
       Context, SS.getWithLocInContext(Context), TemplateKWLoc, NameInfo,
       TemplateArgs);
 }
 
+
+/// Determine whether we would be unable to instantiate this template (because
+/// it either has no definition, or is in the process of being instantiated).
+bool Sema::DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
+                                          NamedDecl *Instantiation,
+                                          bool InstantiatedFromMember,
+                                          const NamedDecl *Pattern,
+                                          NamedDecl *PatternDef,
+                                          TemplateSpecializationKind TSK,
+                                          bool Complain /*= true*/) {
+  assert(isa<TagDecl>(Instantiation) || isa<FunctionDecl>(Instantiation));
+
+  if (PatternDef && (isa<FunctionDecl>(PatternDef)
+                     || !cast<TagDecl>(PatternDef)->isBeingDefined())) {
+    NamedDecl *SuggestedDef = nullptr;
+    if (!hasVisibleDefinition(PatternDef, &SuggestedDef,
+                                /*OnlyNeedComplete*/false)) {
+      // If we're allowed to diagnose this and recover, do so.
+      bool Recover = Complain && !isSFINAEContext();
+      if (Complain)
+        diagnoseMissingImport(PointOfInstantiation, SuggestedDef,
+                              Sema::MissingImportKind::Definition, Recover);
+      return !Recover;
+    }
+    return false;
+  }
+
+
+  QualType InstantiationTy;
+  if (TagDecl *TD = dyn_cast<TagDecl>(Instantiation))
+    InstantiationTy = Context.getTypeDeclType(TD);
+  else
+    InstantiationTy = cast<FunctionDecl>(Instantiation)->getType();
+  if (!Complain || (PatternDef && PatternDef->isInvalidDecl())) {
+    // Say nothing
+  } else if (PatternDef) {
+    Diag(PointOfInstantiation,
+         diag::err_template_instantiate_within_definition)
+      << (TSK != TSK_ImplicitInstantiation)
+      << InstantiationTy;
+    // Not much point in noting the template declaration here, since
+    // we're lexically inside it.
+    Instantiation->setInvalidDecl();
+  } else if (InstantiatedFromMember) {
+    Diag(PointOfInstantiation,
+         diag::err_implicit_instantiate_member_undefined)
+      << InstantiationTy;
+    Diag(Pattern->getLocation(), diag::note_member_declared_at);
+  } else {
+    Diag(PointOfInstantiation, diag::err_template_instantiate_undefined)
+      << (TSK != TSK_ImplicitInstantiation)
+      << InstantiationTy;
+    Diag(Pattern->getLocation(), diag::note_template_decl_here);
+  }
+
+  // In general, Instantiation isn't marked invalid to get more than one
+  // error for multiple undefined instantiations. But the code that does
+  // explicit declaration -> explicit definition conversion can't handle
+  // invalid declarations, so mark as invalid in that case.
+  if (TSK == TSK_ExplicitInstantiationDeclaration)
+    Instantiation->setInvalidDecl();
+  return true;
+}
+
 /// DiagnoseTemplateParameterShadow - Produce a diagnostic complaining
 /// that the template parameter 'PrevDecl' is being shadowed by a new
 /// declaration at location Loc. Returns true to indicate that this is
 /// an error, and false otherwise.
 void Sema::DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl) {
   assert(PrevDecl->isTemplateParameter() && "Not a template parameter");
 
   // Microsoft Visual C++ permits template parameters to be shadowed.

lib/Sema/SemaTemplateInstantiate.cpp

 // Defined via #include from SemaTemplateInstantiateDecl.cpp
 namespace clang {
   namespace sema {
     Attr *instantiateTemplateAttribute(const Attr *At, ASTContext &C, Sema &S,
                             const MultiLevelTemplateArgumentList &TemplateArgs);
   }
 }
 
-/// Determine whether we would be unable to instantiate this template (because
-/// it either has no definition, or is in the process of being instantiated).
-static bool DiagnoseUninstantiableTemplate(Sema &S,
-                                           SourceLocation PointOfInstantiation,
-                                           TagDecl *Instantiation,
-                                           bool InstantiatedFromMember,
-                                           TagDecl *Pattern,
-                                           TagDecl *PatternDef,
-                                           TemplateSpecializationKind TSK,
-                                           bool Complain = true) {
-  if (PatternDef && !PatternDef->isBeingDefined()) {
-    NamedDecl *SuggestedDef = nullptr;
-    if (!S.hasVisibleDefinition(PatternDef, &SuggestedDef,
-                                /*OnlyNeedComplete*/false)) {
-      // If we're allowed to diagnose this and recover, do so.
-      bool Recover = Complain && !S.isSFINAEContext();
-      if (Complain)
-        S.diagnoseMissingImport(PointOfInstantiation, SuggestedDef,
-                                Sema::MissingImportKind::Definition, Recover);
-      return !Recover;
-    }
-    return false;
-  }
-
-  if (!Complain || (PatternDef && PatternDef->isInvalidDecl())) {
-    // Say nothing
-  } else if (PatternDef) {
-    assert(PatternDef->isBeingDefined());
-    S.Diag(PointOfInstantiation,
-           diag::err_template_instantiate_within_definition)
-      << (TSK != TSK_ImplicitInstantiation)
-      << S.Context.getTypeDeclType(Instantiation);
-    // Not much point in noting the template declaration here, since
-    // we're lexically inside it.
-    Instantiation->setInvalidDecl();
-  } else if (InstantiatedFromMember) {
-    S.Diag(PointOfInstantiation,
-           diag::err_implicit_instantiate_member_undefined)
-      << S.Context.getTypeDeclType(Instantiation);
-    S.Diag(Pattern->getLocation(), diag::note_member_declared_at);
-  } else {
-    S.Diag(PointOfInstantiation, diag::err_template_instantiate_undefined)
-      << (TSK != TSK_ImplicitInstantiation)
-      << S.Context.getTypeDeclType(Instantiation);
-    S.Diag(Pattern->getLocation(), diag::note_template_decl_here);
-  }
-
-  // In general, Instantiation isn't marked invalid to get more than one
-  // error for multiple undefined instantiations. But the code that does
-  // explicit declaration -> explicit definition conversion can't handle
-  // invalid declarations, so mark as invalid in that case.
-  if (TSK == TSK_ExplicitInstantiationDeclaration)
-    Instantiation->setInvalidDecl();
-  return true;
-}
-
 /// \brief Instantiate the definition of a class from a given pattern.
 ///
 /// \param PointOfInstantiation The point of instantiation within the
 /// source code.
 ///
 /// \param Instantiation is the declaration whose definition is being
 /// instantiated. This will be either a class template specialization
 /// or a member class of a class template specialization.

 bool
 Sema::InstantiateClass(SourceLocation PointOfInstantiation,
                        CXXRecordDecl *Instantiation, CXXRecordDecl *Pattern,
                        const MultiLevelTemplateArgumentList &TemplateArgs,
                        TemplateSpecializationKind TSK,
                        bool Complain) {
   CXXRecordDecl *PatternDef
     = cast_or_null<CXXRecordDecl>(Pattern->getDefinition());
-  if (DiagnoseUninstantiableTemplate(*this, PointOfInstantiation, Instantiation,
+  if (DiagnoseUninstantiableTemplate(PointOfInstantiation, Instantiation,
                                 Instantiation->getInstantiatedFromMemberClass(),
                                      Pattern, PatternDef, TSK, Complain))
     return true;
   Pattern = PatternDef;
 
   // \brief Record the point of instantiation.
   if (MemberSpecializationInfo *MSInfo 
         = Instantiation->getMemberSpecializationInfo()) {

 /// \param TSK The kind of implicit or explicit instantiation to perform.
 ///
 /// \return \c true if an error occurred, \c false otherwise.
 bool Sema::InstantiateEnum(SourceLocation PointOfInstantiation,
                            EnumDecl *Instantiation, EnumDecl *Pattern,
                            const MultiLevelTemplateArgumentList &TemplateArgs,
                            TemplateSpecializationKind TSK) {
   EnumDecl *PatternDef = Pattern->getDefinition();
-  if (DiagnoseUninstantiableTemplate(*this, PointOfInstantiation, Instantiation,
+  if (DiagnoseUninstantiableTemplate(PointOfInstantiation, Instantiation,
                                  Instantiation->getInstantiatedFromMemberEnum(),
                                      Pattern, PatternDef, TSK,/*Complain*/true))
     return true;
   Pattern = PatternDef;
 
   // Record the point of instantiation.
   if (MemberSpecializationInfo *MSInfo
         = Instantiation->getMemberSpecializationInfo()) {

lib/Sema/SemaTemplateInstantiateDecl.cpp

void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
                                          bool Recursive,
                                          bool DefinitionRequired,
                                          bool AtEndOfTU) {
   if (Function->isInvalidDecl() || Function->isDefined())
     return;
 
   // Never instantiate an explicit specialization except if it is a class scope
   // explicit specialization.
-  if (Function->getTemplateSpecializationKind() == TSK_ExplicitSpecialization &&
+  TemplateSpecializationKind TSK = Function->getTemplateSpecializationKind();
+  if (TSK == TSK_ExplicitSpecialization &&
       !Function->getClassScopeSpecializationPattern())
     return;
 
   // Find the function body that we'll be substituting.
   const FunctionDecl *PatternDecl = Function->getTemplateInstantiationPattern();
   assert(PatternDecl && "instantiating a non-template");
 
   Stmt *Pattern = PatternDecl->getBody(PatternDecl);
if (PatternDecl->isFromASTFile())
       ExternalSource->ReadLateParsedTemplates(LateParsedTemplateMap);
 
     LateParsedTemplate *LPT = LateParsedTemplateMap.lookup(PatternDecl);
     assert(LPT && "missing LateParsedTemplate");
     LateTemplateParser(OpaqueParser, *LPT);
     Pattern = PatternDecl->getBody(PatternDecl);
   }
 
   // FIXME: Check that the definition is visible before trying to instantiate
   // it. This requires us to track the instantiation stack in order to know
   // which definitions should be visible.
+  if (DiagnoseUninstantiableTemplate(PointOfInstantiation, Function,
+                                Function->getInstantiatedFromMemberFunction(),
+                                     PatternDecl,
+                                     const_cast<FunctionDecl*>(PatternDecl),
+                                     TSK, /*Complain*/DefinitionRequired))
+     return;
+
 
+  // TODO: Check if we could sink these diagnostics in
+  // DiagnoseUninstantiableTemplate.
   if (!Pattern && !PatternDecl->isDefaulted()) {
     if (DefinitionRequired) {
       if (Function->getPrimaryTemplate())
         Diag(PointOfInstantiation,
              diag::err_explicit_instantiation_undefined_func_template)
           << Function->getPrimaryTemplate();
       else
         Diag(PointOfInstantiation,
              diag::err_explicit_instantiation_undefined_member)
           << 1 << Function->getDeclName() << Function->getDeclContext();
 
       if (PatternDecl)
         Diag(PatternDecl->getLocation(),
              diag::note_explicit_instantiation_here);
       Function->setInvalidDecl();
-    } else if (Function->getTemplateSpecializationKind()
-                 == TSK_ExplicitInstantiationDefinition) {
+    } else if (TSK == TSK_ExplicitInstantiationDefinition) {
       assert(!Recursive);
       PendingInstantiations.push_back(
         std::make_pair(Function, PointOfInstantiation));
-    } else if (Function->getTemplateSpecializationKind()
-                 == TSK_ImplicitInstantiation) {
+    } else if (TSK == TSK_ImplicitInstantiation) {
       if (AtEndOfTU && !getDiagnostics().hasErrorOccurred()) {
         Diag(PointOfInstantiation, diag::warn_func_template_missing)
           << Function;
         Diag(PatternDecl->getLocation(), diag::note_forward_template_decl);
         if (getLangOpts().CPlusPlus11)
           Diag(PointOfInstantiation, diag::note_inst_declaration_hint)
             << Function;
       }
     }
 
     return;
   }
 
   // C++1y [temp.explicit]p10:
   //   Except for inline functions, declarations with types deduced from their
   //   initializer or return value, and class template specializations, other
   //   explicit instantiation declarations have the effect of suppressing the
   //   implicit instantiation of the entity to which they refer.
-  if (Function->getTemplateSpecializationKind() ==
-          TSK_ExplicitInstantiationDeclaration &&
+  if (TSK == TSK_ExplicitInstantiationDeclaration &&
       !PatternDecl->isInlined() &&
       !PatternDecl->getReturnType()->getContainedAutoType())
     return;
 
   if (PatternDecl->isInlined()) {
     // Function, and all later redeclarations of it (from imported modules,
     // for instance), are now implicitly inline.
     for (auto *D = Function->getMostRecentDecl(); /**/;
          D = D->getPreviousDecl()) {
       D->setImplicitlyInline();
       if (D == Function)
         break;
     }
   }
 
   InstantiatingTemplate Inst(*this, PointOfInstantiation, Function);
   if (Inst.isInvalid())
     return;
   PrettyDeclStackTraceEntry CrashInfo(*this, Function, SourceLocation(),
                                       "instantiating function definition");
 
+  // Q: Shall we add this here for consistency with InstantiateClass and InstantiateEnum.
+  // The instantiation is visible here, even if it was first declared in an
+  // unimported module.
+  //Function->setHidden(false);
+
   // Copy the inner loc start from the pattern.
   Function->setInnerLocStart(PatternDecl->getInnerLocStart());
 
   EnterExpressionEvaluationContext EvalContext(*this,
                                                Sema::PotentiallyEvaluated);
 
   // Introduce a new scope where local variable instantiations will be
   // recorded, unless we're actually a member function within a local

test/Modules/Inputs/PR28794/LibAHeader.h

+#ifndef LIB_A_HEADER
+#define LIB_A_HEADER
+
+typedef __SIZE_TYPE__ size_t;
+
+template <typename = int, size_t SlabSize = 4096, size_t = SlabSize>
+class BumpPtrAllocatorImpl;
+
+template <typename T, size_t SlabSize, size_t SizeThreshold>
+void * operator new(size_t, BumpPtrAllocatorImpl<T, SlabSize, SizeThreshold> &);
+
+#endif // LIB_A_HEADER

test/Modules/Inputs/PR28794/Subdir/Empty.h

+

test/Modules/Inputs/PR28794/Subdir/LibBHeader.h

+#ifndef LIB_B_HEADER
+#define LIB_B_HEADER
+
+#include "LibAHeader.h"
+
+template <typename T, size_t SlabSize, size_t SizeThreshold>
+void *operator new(size_t, BumpPtrAllocatorImpl<T, SlabSize, SizeThreshold> &) {
+  struct S {};
+  return (void*)0xdead;
+}
+
+#endif // LIB_B_HEADER

test/Modules/Inputs/PR28794/module.modulemap

+module M {
+  umbrella "Subdir" module * {export *}
+}

test/Modules/pr28794.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -I%S/Inputs/PR28794 -verify %s
+// RUN: %clang_cc1 -std=c++11 -fmodules -fmodule-map-file=%S/Inputs/PR28794/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR28794/ -verify %s
+
+#include "Subdir/Empty.h"
+#include "LibAHeader.h"
+
+BumpPtrAllocatorImpl<> &getPreprocessorAllocator();
+class B {
+  struct ModuleMacroInfo {
+    ModuleMacroInfo *getModuleInfo() {
+      return new (getPreprocessorAllocator()) ModuleMacroInfo();
+    }
+  };
+};
+
+// expected-no-diagnostics