This is an archive of the discontinued LLVM Phabricator instance.

[Module] Merge function prototype with a non-prototype function declaration
AbandonedPublic

Authored by ahatanak on Sep 27 2016, 9:58 PM.

Details

Summary

This patch fixes a crash that occurs when a non-prototype function is declared before a header containing a prototype of the same function is included. This caused Sema::LookupName to find both function declarations, which eventually caused clang to crash in Sema::AddOverloadCandidate.

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 72741.Sep 27 2016, 9:58 PM
ahatanak retitled this revision from to [Module] Merge function prototype with a non-prototype function declaration.
ahatanak updated this object.
ahatanak added reviewers: doug.gregor, rsmith.
ahatanak added a subscriber: cfe-commits.
rsmith edited edge metadata.Oct 7 2016, 3:02 PM

I'm concerned that name lookup will find an inappropriate declaration after this merging. If we have two visible declarations of a function in the same scope, and one has a prototype but the other does not, the user should be able to rely on the prototype being used. isPreferredLookupResult in SemaLookup.cpp picks the most recent declaration in an attempt to handle this, but that won't do the right thing here, so you should also teach it to prefer a prototyped function over an unprototyped one.

You should be able to test for this case by trying to pass a pointer to the wrong type to func1. (You may need a different testcase that provides the prototype in the .c file and the declaration without prototype in the header in order to hit this, depending on which result name lookup ends up preferring.)

lib/Serialization/ASTReaderDecl.cpp
2715

Instead of your change, can we change this to use typesAreCompatible instead of hasSameType?

It looks like this patch doesn't do what I thought would do when the unprototyped function declaration is in the header and the prototyped function declaration is in the source code. The DeclRefExpr created for the call to func1("abc", 12) points to the unprototyped function "func1()" instead of "func1(const char *, int)".

Sorry for the delay in replying.

lib/Serialization/ASTReaderDecl.cpp
2715

Using typesAreCompatible instead of hasSameType fixes the crash in the test case I added.

However, if I declare the prototype in merge-non-prototype-fn.c and the non-prototype function in header2.h, the call to func1 in foo1 picks the non-prototype function. It seems to me that we should do something similar to what Sema::MergeFunctionDecl does, which is to copy the ParamDecls of the prototype to the non-prototype function, but I'm not sure how to go about it. Should we duplicate the logic in ASTReaderDecl.cpp? Or perhaps we should change the code that does the lookup for func1 to pick the prototype when we are handling modules (which seems to be what you are suggesting)?

ahatanak updated this revision to Diff 92742.Mar 22 2017, 5:09 PM

Update patch.

  1. In isSameEntity, call typesAreCompatible instead of hasSameType so that non-prototype function declarations are merged with function prototypes.
  2. Define a function (mergeCompatibleFunctionDecls) that changes the type of non-prototype functions declarations and adds parameters.
  3. Make hasSameOverloadableAttrs check the presence of noreturn function attributes. Without this change, test/Modules/redecl-merge.m would fail after making the changes in 1.
  4. Add a test case that declares a non-prototype declaration in the header and function prototype in the .c file.
ahatanak abandoned this revision.Apr 3 2018, 12:10 PM

This appears to have been fixed.