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

[ahatanak]

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.

[ahatanak]

ping

[rsmith]

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?

[ahatanak]

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)".

[ahatanak]

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]

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]

Rebase.

[ahatanak]

ping

[ahatanak]

This appears to have been fixed.

lib/Serialization/ASTReaderDecl.cpp

if (TagDecl *TagX = dyn_cast<TagDecl>(X)) {
     return (TagX->getTagKind() == TagY->getTagKind()) ||
       ((TagX->getTagKind() == TTK_Struct || TagX->getTagKind() == TTK_Class ||
         TagX->getTagKind() == TTK_Interface) &&
        (TagY->getTagKind() == TTK_Struct || TagY->getTagKind() == TTK_Class ||
         TagY->getTagKind() == TTK_Interface));
   }
 
   // Functions with the same type and linkage match.
-  // FIXME: This needs to cope with merging of prototyped/non-prototyped
-  // functions, etc.
   if (FunctionDecl *FuncX = dyn_cast<FunctionDecl>(X)) {
     FunctionDecl *FuncY = cast<FunctionDecl>(Y);
+    // Return true if either function is a non-prototyped declaration.
+    if (!FuncX->hasWrittenPrototype() || !FuncY->hasWrittenPrototype())
+      return true;
     if (CXXConstructorDecl *CtorX = dyn_cast<CXXConstructorDecl>(X)) {
       CXXConstructorDecl *CtorY = cast<CXXConstructorDecl>(Y);
       if (CtorX->getInheritedConstructor() &&
           !isSameEntity(CtorX->getInheritedConstructor().getConstructor(),
                         CtorY->getInheritedConstructor().getConstructor()))
         return false;
     }
     return (FuncX->getLinkageInternal() == FuncY->getLinkageInternal()) &&
       FuncX->getASTContext().hasSameType(FuncX->getType(), FuncY->getType());
   }
 
   // Variables with the same type and linkage match.
   if (VarDecl *VarX = dyn_cast<VarDecl>(X)) {
     VarDecl *VarY = cast<VarDecl>(Y);
     if (VarX->getLinkageInternal() == VarY->getLinkageInternal()) {
       ASTContext &C = VarX->getASTContext();
       if (C.hasSameType(VarX->getType(), VarY->getType()))

test/Modules/Inputs/merge-non-prototype-fn/header2.h

+#ifndef FUNC1
+#define FUNC1
+
+char *func1(const char *, int);
+
+#endif

test/Modules/Inputs/merge-non-prototype-fn/module.map

+module header2 {
+  header "header2.h"
+  export *
+}
+

test/Modules/merge-non-prototype-fn.c

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -I%S/Inputs/merge-non-prototype-fn -fmodules -fimplicit-module-maps -x c -fmodules-cache-path=%t -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+char *func1();
+
+#include "header2.h"
+
+void foo1(void) {
+  func1("abc", 12);
+}