Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,8 @@ Improvements to Clang's diagnostics

- Improve the diagnostics for shadows template parameter to report correct location (#GH129060).

- Improve the ``-Wundefined-func-template`` warning when a function template is not instantiated due to being unreachable in modules.

Improvements to Clang's time-trace
----------------------------------

Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -5688,6 +5688,8 @@ def warn_func_template_missing : Warning<"instantiation of function %q0 "
InGroup<UndefinedFuncTemplate>, DefaultIgnore;
def note_forward_template_decl : Note<
"forward declaration of template entity is here">;
def note_unreachable_template_decl
: Note<"unreachable declaration of template entity is here">;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be talking about "visibility" for Clang header modules and reachability for C++20 modules?
I am a little fuzzy on the terminology, but I think there was some distinction between the two we are making for the users.

@ChuanqiXu9, could you advise on the correct wording here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, "unreachable" is a formal definition for C++20 modules. But it looks like the method to judge unreachable decl in this patch is the same for C++20 modules. So I feel it might be fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! If you are okay with this, let's keep as is.

Just a follow-up question for a better understanding: I somehow thought we would only check for reachability (as opposed to visibility) only since C++20.
Or should we expect Clang to sometimes complain about reachability in C++17 and below with -fmodules enabled? (The added test uses C++20, but I believe it can also be C++17; but I'm not 100% sure)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we expect Clang to sometimes complain about reachability in C++17 and below with -fmodules enabled?

I think yes. Clang already does this — in Sema::DiagnoseUninstantiableTemplate, when a reachable function definition is missing, Clang may emit a "missing-import" diagnostic.

However, for this particular testcase, the issue is only triggered with the C++20 standard module feature

But it looks like the method to judge unreachable decl in this patch is the same for C++20 modules.

Yes, this aligns with my reading of the code. The same code is shared between header modules and C++20 modules.

def note_inst_declaration_hint : Note<"add an explicit instantiation "
"declaration to suppress this warning if %q0 is explicitly instantiated in "
"another translation unit">;
Expand Down
12 changes: 5 additions & 7 deletions clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -11245,13 +11245,11 @@ class Sema final : public SemaBase {

/// 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 DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
NamedDecl *Instantiation,
bool InstantiatedFromMember,
const NamedDecl *Pattern,
const NamedDecl *PatternDef,
TemplateSpecializationKind TSK,
bool Complain = true);
bool DiagnoseUninstantiableTemplate(
SourceLocation PointOfInstantiation, NamedDecl *Instantiation,
bool InstantiatedFromMember, const NamedDecl *Pattern,
const NamedDecl *PatternDef, TemplateSpecializationKind TSK,
bool Complain = true, bool *Unreachable = nullptr);

/// DiagnoseTemplateParameterShadow - Produce a diagnostic complaining
/// that the template parameter 'PrevDecl' is being shadowed by a new
Expand Down
4 changes: 3 additions & 1 deletion clang/lib/Sema/SemaTemplate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ bool Sema::DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
const NamedDecl *Pattern,
const NamedDecl *PatternDef,
TemplateSpecializationKind TSK,
bool Complain /*= true*/) {
bool Complain, bool *Unreachable) {
assert(isa<TagDecl>(Instantiation) || isa<FunctionDecl>(Instantiation) ||
isa<VarDecl>(Instantiation));

Expand All @@ -778,6 +778,8 @@ bool Sema::DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
if (!hasReachableDefinition(const_cast<NamedDecl *>(PatternDef),
&SuggestedDef,
/*OnlyNeedComplete*/ false)) {
if (Unreachable)
*Unreachable = true;
// If we're allowed to diagnose this and recover, do so.
bool Recover = Complain && !isSFINAEContext();
if (Complain)
Expand Down
28 changes: 19 additions & 9 deletions clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5386,12 +5386,15 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
PatternDef = nullptr;
}

// True is the template definition is unreachable, otherwise false.
bool Unreachable = false;
// 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, PatternDef, TSK,
/*Complain*/DefinitionRequired)) {
if (DiagnoseUninstantiableTemplate(
PointOfInstantiation, Function,
Function->getInstantiatedFromMemberFunction(), PatternDecl,
PatternDef, TSK,
/*Complain*/ DefinitionRequired, &Unreachable)) {
if (DefinitionRequired)
Function->setInvalidDecl();
else if (TSK == TSK_ExplicitInstantiationDefinition ||
Expand All @@ -5416,11 +5419,18 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
if (AtEndOfTU && !getDiagnostics().hasErrorOccurred() &&
!getSourceManager().isInSystemHeader(PatternDecl->getBeginLoc())) {
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;
<< Function;
if (Unreachable) {
// FIXME: would be nice to mention which module the function template
// comes from.
Diag(PatternDecl->getLocation(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hokein I think we could also mention the point of instantiation here too.
There are two kinds of problems we can't distinguish:

  • it is used intentionally and users want to make it reachable/visible (so imported and declared modules would be relevant),
  • unreachable declaration is used unintentionally and users want to remove the use (in which case instantiation point is relevant).

So I think we want both.

If we pick that path, I think we could probably simplify the code a little too:

          Diag(PatternDecl->getLocation(), 
               Unreachable ? diag::note_unreachable_template_decl
                           : diag::note_forward_template_decl);
          if (getLangOpts().CPlusPlus11)
            Diag(PointOfInstantiation, diag::note_inst_declaration_hint)
                << Function;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note_inst_declaration_hint is about a fix hint which suggests to add an explicit instantiation declaration to suppress this warning, it is not feasible to the unreachable-template-decl case.

diag::note_unreachable_template_decl);
} else {
Diag(PatternDecl->getLocation(), diag::note_forward_template_decl);
if (getLangOpts().CPlusPlus11)
Diag(PointOfInstantiation, diag::note_inst_declaration_hint)
<< Function;
}
}
}

Expand Down
39 changes: 39 additions & 0 deletions clang/test/Modules/diag-undefined-template.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// RUN: rm -rf %t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: could we get the split-file-based test for this?
It's pretty hard to read those tests that happen to depend on many files otherwise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// RUN: split-file %s %t
// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -I%t \
// RUN: -Wundefined-func-template \
// RUN: -fimplicit-module-maps %t/main.cpp 2>&1 | grep "unreachable declaration of template entity is here"

// Note that the diagnostics are triggered when building the 'hoge' module, which is imported from the main file.
// The "-verify" flag doesn't work in this case. Instead, we grep the expected text to verify the test.

//--- shared_ptr2.h
#pragma once

template<class T>
void x() { }

//--- hoge.h
#pragma once

#include "shared_ptr2.h"

inline void f() {
x<int>();
}

//--- module.modulemap
module hoge {
header "hoge.h"
}

module shared_ptr2 {
header "shared_ptr2.h"
}

//--- main.cpp
#include "hoge.h"

int main() {
f();
}