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

3598–3603

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

3617

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

3617

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

[akuegel]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3617

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

3617

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

                                    TemplateTy &SuggestedTemplate,
                                    TemplateNameKind &SuggestedKind);
 
+  bool DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
+                                      NamedDecl *Instantiation,
+                                      bool InstantiatedFromMember,
+                                      const NamedDecl *Pattern,
+                                      const NamedDecl *PatternDef,
+                                      TemplateSpecializationKind TSK,
+                                      bool Complain = true);
+
   void DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl);
   TemplateDecl *AdjustDeclIfTemplate(Decl *&Decl);
 

lib/Sema/SemaTemplate.cpp

       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,
+                                          const 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(const_cast<NamedDecl*>(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

lib/Sema/SemaTemplateInstantiate.cpp

   }
 }
 
-/// 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

                        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;

                            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;

lib/Sema/SemaTemplateInstantiateDecl.cpp

 
   // 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;
 

   }
   assert(PatternDecl && "template definition is not a template");
 
+  // FIXME: We need to track the instantiation stack in order to know which
+  // definitions should be visible within this instantiation.
+  if (DiagnoseUninstantiableTemplate(PointOfInstantiation, Function,
+                                Function->getInstantiatedFromMemberFunction(),
+                                     PatternDecl, PatternDecl, TSK,
+                                     /*Complain*/DefinitionRequired))
+     return;
+
+
+
   // Postpone late parsed template instantiations.
   if (PatternDecl->isLateTemplateParsed() &&
       !LateTemplateParser) {

     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.
-
+  // FIXME: Check if we could sink these diagnostics in
+  // DiagnoseUninstantiableTemplate.
   if (!Pattern && !PatternDecl->isDefaulted()) {
     if (DefinitionRequired) {
       if (Function->getPrimaryTemplate())

         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;

   //   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;

   PrettyDeclStackTraceEntry CrashInfo(*this, Function, SourceLocation(),
                                       "instantiating function definition");
 
+  // 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());
 

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 *}
+}