Page MenuHomePhabricator

Function definition may have uninstantiated body
ClosedPublic

Authored by sepavloff on Feb 20 2017, 8:15 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

sepavloff created this revision.Feb 20 2017, 8:15 AM
sepavloff updated this revision to Diff 89206.Feb 21 2017, 7:03 AM

Implement isDefined through isThisDeclarationADefinition.

sepavloff updated this revision to Diff 92340.Mar 20 2017, 9:31 AM

Small simplicifation

sepavloff updated this revision to Diff 99698.May 21 2017, 10:55 AM

Made the patch a bit more compact. NFC.

rsmith edited edge metadata.May 22 2017, 1:26 PM

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 marked 2 inline comments as done.May 27 2017, 8:51 AM

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 this revision to Diff 100534.May 27 2017, 8:54 AM

Updated patch

  • Reduce number of added functions,
  • Fixed some comments,
  • Function isOdrDefined now checks uninstantiated bodies only for friend functions.
sepavloff updated this revision to Diff 101260.Jun 2 2017, 12:23 PM

Do not call getCanonicalDecl for deleted functions

rsmith added inline comments.Jun 2 2017, 1:59 PM
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 this revision to Diff 101362.Jun 4 2017, 12:01 PM

Updated patch according to review notes

sepavloff updated this revision to Diff 106192.Jul 12 2017, 6:18 AM

Rebased patch

sepavloff updated this revision to Diff 115534.Sep 16 2017, 5:48 AM

Rebased patch.

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
yaron.keren resigned from this revision.Feb 28 2018, 10:55 AM
rsmith accepted this revision.Feb 28 2018, 11:19 AM
rsmith added inline comments.
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.

This revision is now accepted and ready to land.Feb 28 2018, 11:19 AM
This revision was automatically updated to reflect the committed changes.
sepavloff marked 3 inline comments as done.Feb 28 2018, 11:11 PM

Thank you!

lib/Sema/SemaDecl.cpp
11995–12006 ↗(On Diff #115534)

Thank you for the reference.