Function definition may have uninstantiated body

[sepavloff]

Summary

Current implementation of FunctionDecl::isDefined does not take into
account declarations that do not have a body, but it can be instantiated
from a templated definition. This behavior creates problems when
processing friend functions defined in class templates. For instance,
the code:

template<typename T> struct X { friend void f() {} };
X<int> xi;
void f() {}

compiles successfully but must fail due to redefinition of f. The
declaration of the friend f is created when the containing template
X is instantiated, but it does not have a body as per 14.5.4p4
because f is not odr-used.

With this change the function Sema::CheckForFunctionRedefinition
considers functions with uninstantiated bodies as definitions.

[sepavloff]

Implement isDefined through isThisDeclarationADefinition.

[sepavloff]

Small simplicifation

[sepavloff]

Made the patch a bit more compact. NFC.

[rsmith]

Do we really need two different notions of "definition" and "odr definition" here? What goes wrong if we always treat the "instantiation of a friend function definition" case as being a definition?

include/clang/AST/Decl.h

1792

it -> and

1834–1835

This will not be true in the presence of modules: we will ensure that at most one declaration has a body, but multiple declarations can be instantiated from a defined member function of a class template, for instance. So I think the constraint is that you're not allowed to add *more* such definitions.

1847

is the sense -> in the sense

1853

I think this will do the wrong thing for an explicit specialization of a member of a class template:

template<typename T> struct A { void f() {}; };
template<> void A<int>::f();

Here, A<int>::f() is "instantiated from a member function" but it's not a definition, not even in the ODR sense. I think it would suffice to also check whether Original is a friend declaration here.

lib/AST/Decl.cpp

2537

It seems incorrect to me for isThisDeclarationADefinition() to return true on a redeclaration of a deleted function: only the first declaration should be considered to be a definition in that case. (This special case should be in isThisDeclarationADefinition(), not in isDefined().)

lib/Sema/SemaDecl.cpp

11699

This looks wrong to me: CheckForFunctionRedefinition is intended to be called *before* marking the function in question as a definition. If you call it afterwards, then the isOdrDefined call won't work properly because it will always check to see whether FD is a definition before checking any other declaration. So any case that reaches here with FD being a definition seems like a bug. I expect we're only getting here in that case due to the missing check for friend in isThisDeclarationAnOdrDefinition.

[sepavloff]

In D30170#761342, @rsmith wrote:

Do we really need two different notions of "definition" and "odr definition" here? What goes wrong if we always treat the "instantiation of a friend function definition" case as being a definition?

It requires 3 additional changes besides update of isDefined. All of them implement current behavior of the function and look like:

@@ -3749,7 +3750,8 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
                                          bool Recursive,
                                          bool DefinitionRequired,
                                          bool AtEndOfTU) {
-  if (Function->isInvalidDecl() || Function->isDefined())
+  const FunctionDecl *Def;
+  if (Function->isInvalidDecl() || (Function->isDefined(Def) && Def->isThisDeclarationADefinition()))
     return;

   // Never instantiate an explicit specialization except if it is a class scope

It make sense to replace this weird expression Function->isDefined(Def) && Def->isThisDeclarationADefinition() with a call to a new function, something like isActuallyDefined. So we anyway have to add a new function for checking function definition. In the case of IsOdrDefinition the new function is used in one place only, modified version of isDefined requires changes in unrelated code places.

Use of a special variant of isDefined dedicated to redefinition checks looks less invasive and more robust. If however there are reasons to use modified isDefined, it also can be implemented, there is nothing wrong with such solution.

include/clang/AST/Decl.h

1834–1835

Fixed also some comments according to this note.

1853

Although this function is not called in this case, its name must agree with its functionality, so changed implementation as you recommend.

lib/AST/Decl.cpp

2537

Indeed, if a declaration was selected to be a definition, it is strange if isThisDeclarationADefinition returns false for it.

lib/Sema/SemaDecl.cpp

11699

CheckForFunctionRedefinition is called after the declaration is put to redeclaration chain. At this point the declaration does not have a body, so problem may arise only if the provided declaration is an instantiation. It happens when the method is called from PerformPendingInstantiations -> InstantiateFunctionDefinition -> ActOnStartOfFunctionDef -> CheckForFunctionRedefinition. In this case function body is generated for declaration that is already in redeclaration chain. If the chain is in valid state, it does not have duplicate definitions and call to CheckForFunctionRedefinition is redundant and this check makes shortcut return.

[sepavloff]

Updated patch

• Reduce number of added functions,
• Fixed some comments,
• Function isOdrDefined now checks uninstantiated bodies only for friend functions.

[sepavloff]

Do not call getCanonicalDecl for deleted functions

[rsmith]

include/clang/AST/Decl.h

1900–1901

Can you check in this change and the corresponding changes to test/SemaCXX/cxx0x-cursory-default-delete.cpp separately?

lib/AST/Decl.cpp

2567

redeclaration -> redefinition

lib/Sema/SemaDecl.cpp

11699

Ultimately, I'd like to be able to remove the special-case handling for friend function redefinitions from TemplateDeclInstantiator::VisitFunctionDecl and simply call CheckForFunctionRedefinition from there (we just need to keep the part that checks for a prior declaration marked as 'used' and instantiates the definition if so). For that to work, this function has to be able to cope with the case where FD is an instantiated friend and there is another instantiated friend in the redeclaration chain. In that case, FD->isOdrDefined() is going to determine that FD itself is the definition and not find the other definition (because D->redecls() yields D first).

I think the best way to make that work would be to skip FD when looping over redeclarations and checking for an existing definition. At that point, isOdrDefined is really not a general-purpose mechanism for checking for a definition, and should probably simply be inlined into here.

[sepavloff]

Updated patch according to review notes

[sepavloff]

Rebased patch

[sepavloff]

Ping.

[sepavloff]

Rebased patch.

[sepavloff]

The issue is still observed in trunk. Other compilers process the tests correctly (checked using https://godbolt.org/). For instance, the code:

template<typename T> struct C20 {
friend void func_20() {} // expected-note{{previous definition is here}}
};
C20<int> c20i;
void func_20() {} // expected-error{{redefinition of 'func_20'}}

is rejected by gcc 7.3:

<source>: In function 'void func_20()':
<source>:5:6: error: redefinition of 'void func_20()'
 void func_20() {} // expected-error{{redefinition of 'func_20'}}
      ^~~~~~~
<source>:2:13: note: 'void func_20()' previously declared here
 friend void func_20() {} // expected-note{{previous definition is here}}
             ^~~~~~~
Compiler returned: 1

by ICC 18:

<source>(5): error: function "func_20" has already been defined
  void func_20() {} // expected-error{{redefinition of 'func_20'}}
       ^
compilation aborted for <source> (code 2)
Compiler returned: 2

and by MSVC 19 2017:

<source>(5): error C2084: function 'void C20<int>::func_20(void)' already has a body
<source>(2): note: see previous definition of 'func_20'
Microsoft (R) C/C++ Optimizing Compiler Version 19.10.25017 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.
Compiler returned: 2

[rsmith]

include/clang/AST/Decl.h

1803

Please remove trailing blank comment lines here and below.

lib/Sema/SemaDecl.cpp

11696

&& on the end of the previous line, please.

If the intent here is to detect non-member functions, using !FD->isCXXClassMember() or !isa<CXXMethodDecl>(FD) would be clearer.

11705–11716

We should include a comment here explaining why we need to do this (that is, why this doesn't just fall out from the normal isDefined check). You can just quote C++ [temp.inst]p2:

For the purpose of determining whether an instantiated redeclaration is valid according to [basic.def.odr] and [class.mem], a declaration that corresponds to a definition in the template is considered to be a definition.

[sepavloff]

Thank you!

lib/Sema/SemaDecl.cpp

11705–11716

Thank you for the reference.

include/clang/AST/Decl.h

public:
 
   void getNameForDiagnostic(raw_ostream &OS, const PrintingPolicy &Policy,
                             bool Qualified) const override;
 
   void setRangeEnd(SourceLocation E) { EndRangeLoc = E; }
 
   SourceRange getSourceRange() const override LLVM_READONLY;
 
-  /// \brief Returns true if the function has a body (definition). The
-  /// function body might be in any of the (re-)declarations of this
-  /// function. The variant that accepts a FunctionDecl pointer will
-  /// set that function declaration to the actual declaration
-  /// containing the body (if there is one).
+  /// \name Definition and body checks
+  ///
+  /// A function declaration may be:
+  /// - a non defining declaration,
+  /// - a definition. A function may be defined because:
+  ///   - it has a body, or will have it in the case of late parsing.
+  ///   - it has an uninstantiated body. The body does not exist because the
+  ///     function is not used yet, but the declaration is considered a
+  ///     definition from viewpoint of ODR checks.
+  ///   - it does not have a user specified body, but it does not allow
+  ///     redefinition, because it is deleted/defaulted or is defined through
+  ///     some other mechanism (alias, ifunc).
+  ///
+  /// Depending on whether the redeclaration chain contains a definition and of
+  /// what kind, the same classification applies to the function represented by
+  /// a set of redeclarations.
+  ///
+  /// \{
+
+  /// Returns true if the function has a body.
+  ///
+  /// The function body might be in any of the (re-)declarations of this
+  /// function. The variant that accepts a FunctionDecl pointer will set that
+  /// function declaration to the actual declaration containing the body (if
+  /// there is one).
+  ///
   bool hasBody(const FunctionDecl *&Definition) const;
 
   bool hasBody() const override {
     const FunctionDecl* Definition;
     return hasBody(Definition);
   }
 
-  /// hasTrivialBody - Returns whether the function has a trivial body that does
-  /// not require any specific codegen.
-  bool hasTrivialBody() const;
-
-  /// isDefined - Returns true if the function is defined at all, including
-  /// a deleted definition. Except for the behavior when the function is
-  /// deleted, behaves like hasBody.
+  /// Returns true if the function has a definition that does not need to be
+  /// instantiated.
+  ///
+  /// The only difference to isOdrDefined is that this function does not take
+  /// into account functions where body is not instantiated because it is not
+  /// used.
+  ///
+  /// The variant that accepts a FunctionDecl pointer will set that function
+  /// declaration to the declaration that is a definition (if there is one).
+  ///
   bool isDefined(const FunctionDecl *&Definition) const;
 
   virtual bool isDefined() const {
     const FunctionDecl* Definition;
     return isDefined(Definition);
   }
 
+  /// Returns true if the function is defined in the sense of ODR checks.
+  ///
+  bool isOdrDefined(const FunctionDecl *&Definition) const;
+  bool isOdrDefined() const {
+    const FunctionDecl* Definition;
+    return isOdrDefined(Definition);
+  }
+
+  /// Returns whether this specific declaration of the function is also a
+  /// definition that does not contain uninstantiated body.
+  ///
+  /// This does not determine whether the function has been defined (e.g., in a
+  /// previous definition); for that information, use isDefined.
+  ///
+  bool isThisDeclarationADefinition() const {
+    return IsDeleted || IsDefaulted || Body || IsLateTemplateParsed ||
+           hasDefiningAttr();
+  }
+
+  /// Returns whether this specific declaration of the function has a body.
+  ///
+  bool doesThisDeclarationHaveABody() const {
+    return Body || IsLateTemplateParsed;
+  }
+
+  /// Returns true if this declaration is a definition is the sense of ODR
+  /// checks.
+  ///
+  bool isThisDeclarationAnOdrDefinition() const {
+    if (isThisDeclarationADefinition())
+      return true;
+    if (FunctionDecl *Original = getInstantiatedFromMemberFunction())
+      return Original->isOdrDefined();
+    return false;
+  }
+
+  /// \}
+
+  /// hasTrivialBody - Returns whether the function has a trivial body that does
+  /// not require any specific codegen.
+  bool hasTrivialBody() const;
+
   /// \brief Get the definition for this declaration.
   FunctionDecl *getDefinition() {
     const FunctionDecl *Definition;
     if (isDefined(Definition))
       return const_cast<FunctionDecl *>(Definition);
     return nullptr;
   }
   const FunctionDecl *getDefinition() const {
     return const_cast<FunctionDecl *>(this)->getDefinition();
   }
 
   /// getBody - Retrieve the body (definition) of the function. The
   /// function body might be in any of the (re-)declarations of this
   /// function. The variant that accepts a FunctionDecl pointer will
   /// set that function declaration to the actual declaration
   /// containing the body (if there is one).
   /// NOTE: For checking if there is a body, use hasBody() instead, to avoid
   /// unnecessary AST de-serialization of the body.
   Stmt *getBody(const FunctionDecl *&Definition) const;
 
   Stmt *getBody() const override {
     const FunctionDecl* Definition;
     return getBody(Definition);
   }
 
-  /// isThisDeclarationADefinition - Returns whether this specific
-  /// declaration of the function is also a definition. This does not
-  /// determine whether the function has been defined (e.g., in a
-  /// previous definition); for that information, use isDefined. Note
-  /// that this returns false for a defaulted function unless that function
-  /// has been implicitly defined (possibly as deleted).
-  bool isThisDeclarationADefinition() const {
-    return IsDeleted || Body || IsLateTemplateParsed;
-  }
-
-  /// doesThisDeclarationHaveABody - Returns whether this specific
-  /// declaration of the function has a body - that is, if it is a non-
-  /// deleted definition.
-  bool doesThisDeclarationHaveABody() const {
-    return Body || IsLateTemplateParsed;
-  }
-
   void setBody(Stmt *B);
   void setLazyBody(uint64_t Offset) { Body = Offset; }
 
   /// Whether this function is variadic.
   bool isVariadic() const;
 
   /// Whether this function is marked as virtual explicitly.
   bool isVirtualAsWritten() const { return IsVirtualAsWritten; }
   void setVirtualAsWritten(bool V) { IsVirtualAsWritten = V; }

lib/AST/Decl.cpp

bool FunctionDecl::hasTrivialBody() const
 
   if (isa<CompoundStmt>(S) && cast<CompoundStmt>(S)->body_empty())
     return true;
   return false;
 }
 
 bool FunctionDecl::isDefined(const FunctionDecl *&Definition) const {
   for (auto I : redecls()) {
-    if (I->IsDeleted || I->IsDefaulted || I->Body || I->IsLateTemplateParsed ||
-        I->hasDefiningAttr()) {
+    if (I->isThisDeclarationADefinition()) {
+      Definition = I->IsDeleted ? I->getCanonicalDecl() : I;
+      return true;
+    }
+  }
+
+  return false;
+}
+
+bool FunctionDecl::isOdrDefined(const FunctionDecl *&Definition) const {
+  for (auto I : redecls()) {
+    if (I->isThisDeclarationAnOdrDefinition()) {
       Definition = I->IsDeleted ? I->getCanonicalDecl() : I;
       return true;
     }
   }
 
   return false;
 }
 
 Stmt *FunctionDecl::getBody(const FunctionDecl *&Definition) const {
   if (!hasBody(Definition))
     return nullptr;
 
   if (Definition->Body)
     return Definition->Body.get(getASTContext().getExternalSource());
 
   return nullptr;
 }
 
 void FunctionDecl::setBody(Stmt *B) {
   Body = B;
   if (B)
     EndRangeLoc = B->getLocEnd();
 }
 
 void FunctionDecl::setPure(bool P) {
   IsPure = P;
   if (P)
     if (auto *Parent = dyn_cast<CXXRecordDecl>(getDeclContext()))

lib/Sema/SemaDecl.cpp

 }
 
 void
 Sema::CheckForFunctionRedefinition(FunctionDecl *FD,
                                    const FunctionDecl *EffectiveDefinition,
                                    SkipBodyInfo *SkipBody) {
   const FunctionDecl *Definition = EffectiveDefinition;
   if (!Definition)
-    if (!FD->isDefined(Definition))
-      return;
+    if (!FD->isOdrDefined(Definition))
+      return;
+  assert(Definition);
+
+  if (FD == Definition)
 
   if (canRedefineFunction(Definition, getLangOpts()))
     return;
 
   // If we don't have a visible definition of the function, and it's inline or
   // a template, skip the new definition.
   if (SkipBody && !hasVisibleDefinition(Definition) &&
       (Definition->getFormalLinkage() == InternalLinkage ||
        Definition->isInlined() ||
        Definition->getDescribedFunctionTemplate() ||
        Definition->getNumTemplateParameterLists())) {
     SkipBody->ShouldSkip = true;
     if (auto *TD = Definition->getDescribedFunctionTemplate())
       makeMergedDefinitionVisible(TD, FD->getLocation());
     makeMergedDefinitionVisible(const_cast<FunctionDecl*>(Definition),
                                 FD->getLocation());
     return;
   }
 
   if (getLangOpts().GNUMode && Definition->isInlineSpecified() &&
       Definition->getStorageClass() == SC_Extern)
     Diag(FD->getLocation(), diag::err_redefinition_extern_inline)
         << FD->getDeclName() << getLangOpts().CPlusPlus;
   else

test/SemaCXX/cxx0x-cursory-default-delete.cpp

 struct bad_decls {
   bad_decls(volatile bad_decls&) = default; // expected-error {{may not be volatile}}
   bad_decls&& operator = (bad_decls) = default; // expected-error {{lvalue reference}} expected-error {{must return 'bad_decls &'}}
   bad_decls& operator = (volatile bad_decls&) = default; // expected-error {{may not be volatile}}
   bad_decls& operator = (const bad_decls&) const = default; // expected-error {{may not have 'const', 'constexpr' or 'volatile' qualifiers}}
 };
 
 struct DefaultDelete {
-  DefaultDelete() = default; // expected-note {{previous declaration is here}}
+  DefaultDelete() = default; // expected-note {{previous definition is here}}
   DefaultDelete() = delete; // expected-error {{constructor cannot be redeclared}}
 
-  ~DefaultDelete() = default; // expected-note {{previous declaration is here}}
+  ~DefaultDelete() = default; // expected-note {{previous definition is here}}
   ~DefaultDelete() = delete; // expected-error {{destructor cannot be redeclared}}
 
-  DefaultDelete &operator=(const DefaultDelete &) = default; // expected-note {{previous declaration is here}}
+  DefaultDelete &operator=(const DefaultDelete &) = default; // expected-note {{previous definition is here}}
   DefaultDelete &operator=(const DefaultDelete &) = delete; // expected-error {{class member cannot be redeclared}}
 };
 
 struct DeleteDefault {
   DeleteDefault() = delete; // expected-note {{previous definition is here}}
   DeleteDefault() = default; // expected-error {{constructor cannot be redeclared}}
 
   ~DeleteDefault() = delete; // expected-note {{previous definition is here}}

test/SemaCXX/friend2.cpp

template<typename T> class C11 {
   friend int func_11();  // expected-error{{functions that differ only in their return type cannot be overloaded}}
 };
 
 void func_12(int x);  // expected-note{{previous declaration is here}}
 template<typename T> class C12 {
   friend void func_12(int x = 0);  // expected-error{{friend declaration specifying a default argument must be the only declaration}}
 };
 
+// Friend function with uninstantiated body is still a definition.
+
+template<typename T> struct C20 {
+  friend void func_20() {} // expected-note{{previous definition is here}}
+};
+C20<int> c20i;
+void func_20() {} // expected-error{{redefinition of 'func_20'}}
+
+template<typename T> struct C21a {
+  friend void func_21() {} // expected-note{{previous definition is here}}
+};
+template<typename T> struct C21b {
+  friend void func_21() {} // expected-error{{redefinition of 'func_21'}}
+};
+C21a<int> c21ai;
+C21b<int> c21bi; // expected-note{{in instantiation of template class 'C21b<int>' requested here}}
+
+template<typename T> struct C22a {
+  friend void func_22() {} // expected-note{{previous definition is here}}
+};
+template<typename T> struct C22b {
+  friend void func_22();
+};
+C22a<int> c22ai;
+C22b<int> c22bi;
+void func_22() {} // expected-error{{redefinition of 'func_22'}}
+
+
 
 namespace pr22307 {
 
 struct t {
   friend int leak(t);
 };
 
 template<typename v>