Page MenuHomePhabricator

Function with unparsed body is a definition
ClosedPublic

Authored by sepavloff on Feb 25 2017, 12:24 AM.

Details

Summary

While a function body is being parsed, the function declaration is not considered
as a definition because it does not have a body yet. In some cases it leads to
incorrect interpretation, the case is presented in
https://bugs.llvm.org/show_bug.cgi?id=14785:

    template<typename T> struct Somewhat {
      void internal() const {}
      friend void operator+(int const &, Somewhat<T> const &) {}
    };
void operator+(int const &, Somewhat<char> const &x) { x.internal(); }

When statement x.internal() in the body of global operator+ is parsed, the type
of x must be completed, so the instantiation of Somewhat<char> is started. It
instantiates the declaration of operator+ defined inline, and makes a check for
redefinition. The check does not detect another definition because the declaration
of operator+ is still not defining as does not have a body yet.

To solves this problem the function isThisDeclarationADefinition considers
a function declaration as a definition if it has flag WillHaveBody set.

This change fixes PR14785.

Diff Detail

Repository
rL LLVM

Event Timeline

sepavloff created this revision.Feb 25 2017, 12:24 AM
jlebar edited edge metadata.Feb 27 2017, 2:35 PM

This doesn't look wrong to me, but I feel utterly unqualified to review this.

sepavloff updated this revision to Diff 101517.Jun 6 2017, 12:13 AM

Rebased patch

rsmith edited edge metadata.Jun 6 2017, 1:15 PM

If you want to take this path, you should also add ActOnStartOfFunctionBody calls to the other parts of the parser that parse function definitions: Parser::ParseLateTemplatedFuncDef, Parser::ParseLexedObjCMethodDefs, and Parser::ParseLexedMethodDefs. You should also rename ActOnFinishFunctionBody to something like ActOnFinishFunctionDef to avoid confusion, because it pairs with ActOnStartOfFunctionDef not the new ActOnStartFunctionBody.

However, I wonder if there isn't a simpler way to handle this, rather than adding another function that is called almost every time that ActOnStartOfFunctionDef is called: the Declarator object passed to ActOnStartOfFunctionDef already carries a FunctionDefinitionKind which tells you whether the function definition has a body. You could extend the non-Declarator overload to also take a FunctionDefinitionKind and then update the callers to pass in the right value. Then in ActOnStartOfFunctionDef, only set WillHaveBody if the kind is FDK_Definition.

Thank you for explanation. Probably making new semantic action is an overkill.

The intent was to postpone setting flag WillHaveBody as late as possible, to the moment when it becomes clear that the function indeed will have a body. Otherwise parsing of deleted functions is broken. Also in the case of invalid input the resulting function declaration become definition while previously they were not, it also changes existing behavior. The new semantic action set the flag enough late after the call to ActOnStartOfFunctionBody, when this treatment is already made.

The new solution uses different path. As previously, ActOnStartOfFunctionBody sets WillHaveBody but this flag is reset when it becomes clear that body will be available. Such technique is already uses for defaulted functions, so only deleted require the flag reset. WillHaveBody is also reset when parsing of function is finished. This transient flag is not needed after the function parsing is done. If ActOnFinishFunctionBody is always called after ActOnStartOfFunctionBody, erroneous functions are not leaked as definition.

sepavloff updated this revision to Diff 101993.Jun 8 2017, 10:23 PM

Changed implementation

sepavloff edited the summary of this revision. (Show Details)Jun 8 2017, 10:24 PM
rsmith accepted this revision.Jun 12 2017, 1:27 PM

LGTM as-is, but a couple of questions for possible further improvements.

lib/Sema/SemaDecl.cpp
12221 ↗(On Diff #101993)

Would it make sense to leave this true for a defaulted function whose body has not yet been synthesized?

lib/Sema/SemaTemplateInstantiateDecl.cpp
1812–1819 ↗(On Diff #101993)

Can we replace this...

1841–1853 ↗(On Diff #101993)

... and this with

SemaRef.CheckForFunctionRedefinition(Function);
Function->setWillHaveBody(true);

?

This revision is now accepted and ready to land.Jun 12 2017, 1:27 PM
This revision was automatically updated to reflect the committed changes.

Thank you!
Committed as is, improvements will be made as separate patches.

Initial commit (r305379) was reverted (r305381) because it broke self builds. The reason was not related to the WillHaveBody flag but was due to the change:

if (isFriend) {
  Function->setObjectOfFriendDecl();
  if (FunctionTemplate)
    FunctionTemplate->setObjectOfFriendDecl();
}

Attempt to execute FunctionTemplate->setObjectOfFriendDecl() caused assertion violation because the declaration had IDNS_NonMemberOperator in its IdentifierNamespace. There is nothing wrong with this flag, setObjectOfFriendDecl must be updated accordingly. This is however a problem of friend function templates, it will be addressed in other patch. For now the call to setObjectOfFriendDecl is made as previously, with the exception that it is called for Function prior to call to CheckFunctionDeclaration, as it is necessary for proper diagnostics.