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

1789 ↗

(On Diff #99698)

it -> and

1829–1830 ↗

(On Diff #99698)

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.

1842 ↗

(On Diff #99698)

is the sense -> in the sense

1848 ↗

(On Diff #99698)

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

2538 ↗

(On Diff #99698)

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

11882 ↗

(On Diff #99698)

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

1829–1830 ↗

(On Diff #99698)

Fixed also some comments according to this note.

1848 ↗

(On Diff #99698)

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

2538 ↗

(On Diff #99698)

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

lib/Sema/SemaDecl.cpp

11882 ↗

(On Diff #99698)

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

1868–1871 ↗

(On Diff #101260)

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

lib/AST/Decl.cpp

2557 ↗

(On Diff #101260)

redeclaration -> redefinition

lib/Sema/SemaDecl.cpp

11882 ↗

(On Diff #99698)

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

1840 ↗

(On Diff #115534)

Please remove trailing blank comment lines here and below.

lib/Sema/SemaDecl.cpp

11986 ↗

(On Diff #115534)

&& 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.

11995–12006 ↗

(On Diff #115534)

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

11995–12006 ↗

(On Diff #115534)

Thank you for the reference.

cfe/trunk/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).
+  // Function definitions.
+  //
+  // 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 and does not allow other definition of this function.
+  //   - 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).
+
+  /// 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);
   }
 
   /// Returns whether the function has a trivial body that does not require any
   /// specific codegen.
   bool hasTrivialBody() const;
 
-  /// 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 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);
   }
 
   /// \brief Get the definition for this declaration.
public:
   ///
   /// 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 || HasSkippedBody ||
            IsLateTemplateParsed || WillHaveBody || hasDefiningAttr();
   }
 
-  /// Returns whether this specific declaration of the function has a body -
-  /// that is, if it is a non-deleted definition.
+  /// Returns whether this specific declaration of the function has a body.
   bool doesThisDeclarationHaveABody() const {
     return Body || IsLateTemplateParsed;
   }
 
   void setBody(Stmt *B);
   void setLazyBody(uint64_t Offset) { Body = Offset; }
 
   /// Whether this function is variadic.

cfe/trunk/lib/Sema/SemaDecl.cpp

static bool ShouldWarnAboutMissingPrototype(const FunctionDecl *FD,
   return MissingPrototype;
 }
 
 void
 Sema::CheckForFunctionRedefinition(FunctionDecl *FD,
                                    const FunctionDecl *EffectiveDefinition,
                                    SkipBodyInfo *SkipBody) {
   const FunctionDecl *Definition = EffectiveDefinition;
+  if (!Definition && !FD->isDefined(Definition) && !FD->isCXXClassMember()) {
+    // If this is a friend function defined in a class template, it does not
+    // have a body until it is used, nevertheless it is a definition, see
+    // [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.
+    //
+    // The following code must produce redefinition error:
+    //
+    //     template<typename T> struct C20 { friend void func_20() {} };
+    //     C20<int> c20i;
+    //     void func_20() {}
+    //
+    for (auto I : FD->redecls()) {
+      if (I != FD && !I->isInvalidDecl() &&
+          I->getFriendObjectKind() != Decl::FOK_None) {
+        if (FunctionDecl *Original = I->getInstantiatedFromMemberFunction()) {
+          if (Original->isThisDeclarationADefinition()) {
+            Definition = I;
+            break;
+          }
+        }
+      }
+    }
+  }
   if (!Definition)
-    if (!FD->isDefined(Definition))
-      return;
 
   if (canRedefineFunction(Definition, getLangOpts()))
     return;
 
   // Don't emit an error when this is redefinition of a typo-corrected
   // definition.
   if (TypoCorrectedFunctionDefinitions.count(Definition))
     return;

cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (isFriend) {
 
     // C++11 [temp.friend]p4 (DR329):
     //   When a function is defined in a friend function declaration in a class
     //   template, the function is instantiated when the function is odr-used.
     //   The same restrictions on multiple declarations and definitions that
     //   apply to non-template function declarations and definitions also apply
     //   to these implicit definitions.
     if (D->isThisDeclarationADefinition()) {
-      // Check for a function body.
-      const FunctionDecl *Definition = nullptr;
-      if (Function->isDefined(Definition) &&
-          Definition->getTemplateSpecializationKind() == TSK_Undeclared) {
-        SemaRef.Diag(Function->getLocation(), diag::err_redefinition)
-            << Function->getDeclName();
-        SemaRef.Diag(Definition->getLocation(), diag::note_previous_definition);
-      }
-      // Check for redefinitions due to other instantiations of this or
-      // a similar friend function.
-      else for (auto R : Function->redecls()) {
+      SemaRef.CheckForFunctionRedefinition(Function);
+      if (!Function->isInvalidDecl()) {
+        for (auto R : Function->redecls()) {
           if (R == Function)
             continue;
 
           // If some prior declaration of this function has been used, we need
           // to instantiate its definition.
           if (!QueuedInstantiation && R->isUsed(false)) {
             if (MemberSpecializationInfo *MSInfo =
-                  Function->getMemberSpecializationInfo()) {
               if (MSInfo->getPointOfInstantiation().isInvalid()) {
                 SourceLocation Loc = R->getLocation(); // FIXME
                 MSInfo->setPointOfInstantiation(Loc);
                 SemaRef.PendingLocalImplicitInstantiations.push_back(
-                                               std::make_pair(Function, Loc));
                 QueuedInstantiation = true;
               }
             }
           }
-
-        // If some prior declaration of this function was a friend with an
-        // uninstantiated definition, reject it.
-        if (R->getFriendObjectKind()) {
-          if (const FunctionDecl *RPattern =
-                  R->getTemplateInstantiationPattern()) {
-            if (RPattern->isDefined(RPattern)) {
-              SemaRef.Diag(Function->getLocation(), diag::err_redefinition)
-                << Function->getDeclName();
-              SemaRef.Diag(R->getLocation(), diag::note_previous_definition);
-              break;
-            }
-          }
         }
       }
     }
 
     // Check the template parameter list against the previous declaration. The
     // goal here is to pick up default arguments added since the friend was
     // declared; we know the template parameter lists match, since otherwise
     // we would not have picked this template as the previous declaration.

cfe/trunk/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>