[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

2813–2830

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

2813–2830

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

void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
   FD->HasImplicitReturnZero = Record.readInt();
   FD->IsConstexpr = Record.readInt();
   FD->UsesSEHTry = Record.readInt();
   FD->HasSkippedBody = Record.readInt();
   FD->IsLateTemplateParsed = Record.readInt();
   FD->setCachedLinkage(Linkage(Record.readInt()));
   FD->EndRangeLoc = ReadSourceLocation();
 
+  bool IsFunctionNoProtoType = FD->getType()->getAs<FunctionNoProtoType>();
+
   switch ((FunctionDecl::TemplatedKind)Record.readInt()) {
   case FunctionDecl::TK_NonTemplate:
     mergeRedeclarable(FD, Redecl);
     break;
   case FunctionDecl::TK_FunctionTemplate:
     // Merged when we merge the template.
     FD->setDescribedFunctionTemplate(ReadDeclAs<FunctionTemplateDecl>());
     break;
case FunctionDecl::TK_DependentFunctionTemplateSpecialization: {
     // These are not merged; we don't need to merge redeclarations of dependent
     // template friends.
     break;
   }
   }
 
   // Read in the parameters.
   unsigned NumParams = Record.readInt();
+
+  // Return here if FD is a non-prototype function declaration and has been
+  // merged with a function prototype.
+  if (IsFunctionNoProtoType && FD->getType()->getAs<FunctionProtoType>())
+    return;
+
   SmallVector<ParmVarDecl *, 16> Params;
   Params.reserve(NumParams);
   for (unsigned I = 0; I != NumParams; ++I)
     Params.push_back(ReadDeclAs<ParmVarDecl>());
   FD->setParams(Reader.getContext(), Params);
 }
 
 void ASTDeclReader::VisitObjCMethodDecl(ObjCMethodDecl *MD) {
void ASTDeclReader::mergeTemplatePattern(RedeclarableTemplateDecl *D,
   if (auto *DVar = dyn_cast<VarDecl>(DPattern))
     return mergeRedeclarable(DVar, cast<VarDecl>(ExistingPattern), Result);
   if (auto *DAlias = dyn_cast<TypeAliasDecl>(DPattern))
     return mergeRedeclarable(DAlias, cast<TypedefNameDecl>(ExistingPattern),
                              Result);
   llvm_unreachable("merged an unknown kind of redeclarable template");
 }
 
+static void mergeCompatibleFunctionDecls(FunctionDecl *OldFD,
+                                         FunctionDecl *NewFD,
+                                         ASTContext &Context) {
+  const auto *OldFuncType = OldFD->getType()->getAs<FunctionType>();
+  const auto *NewFuncType = NewFD->getType()->getAs<FunctionType>();
+  const FunctionProtoType *OldProto = dyn_cast<FunctionProtoType>(OldFuncType);
+
+  if (!OldProto || !isa<FunctionNoProtoType>(NewFuncType))
+    return;
+
+  SmallVector<QualType, 4> ParamTypes(OldProto->param_types());
+  QualType NewType = Context.getFunctionType(
+      NewFuncType->getReturnType(), ParamTypes, OldProto->getExtProtoInfo());
+  NewFD->setType(NewType);
+  NewFD->setHasInheritedPrototype();
+
+  SmallVector<ParmVarDecl *, 4> Params;
+  for (const auto &ParamType : OldProto->param_types()) {
+    auto *Param = ParmVarDecl::Create(Context, NewFD, SourceLocation(),
+                                      SourceLocation(), nullptr, ParamType,
+                                      nullptr, SC_None, nullptr);
+    Param->setScopeInfo(0, Params.size());
+    Param->setImplicit();
+    Params.push_back(Param);
+  }
+
+  NewFD->setParams(Params);
+}
+
 /// \brief Attempts to merge the given declaration (D) with another declaration
 /// of the same entity.
 template<typename T>
 void ASTDeclReader::mergeRedeclarable(Redeclarable<T> *DBase, T *Existing,
                                       RedeclarableResult &Redecl,
                                       DeclID TemplatePatternID) {
   T *D = static_cast<T*>(DBase);
   T *ExistingCanon = Existing->getCanonicalDecl();
   T *DCanon = D->getCanonicalDecl();
   if (ExistingCanon != DCanon) {
     assert(DCanon->getGlobalID() == Redecl.getFirstID() &&
            "already merged this declaration");
 
     // Have our redeclaration link point back at the canonical declaration
     // of the existing declaration, so that this declaration has the
     // appropriate canonical declaration.
     D->RedeclLink = Redeclarable<T>::PreviousDeclLink(ExistingCanon);
     D->First = ExistingCanon;
     ExistingCanon->Used |= D->Used;
     D->Used = false;
 
+    if (!Reader.getContext().getLangOpts().CPlusPlus) {
+      auto *OldFD = dyn_cast<FunctionDecl>(Existing);
+      auto *FD = dyn_cast<FunctionDecl>(D);
+      if (OldFD && FD)
+        mergeCompatibleFunctionDecls(OldFD, FD, Reader.getContext());
+    }
+
     // When we merge a namespace, update its pointer to the first namespace.
     // We cannot have loaded any redeclarations of this declaration yet, so
     // there's nothing else that needs to be updated.
     if (auto *Namespace = dyn_cast<NamespaceDecl>(D))
       Namespace->AnonOrFirstNamespaceAndInline.setPointer(
           assert_cast<NamespaceDecl*>(ExistingCanon));
 
     // When we merge a template, merge its pattern.
static bool isSameTemplateParameterList(const TemplateParameterList *X,
 
   return true;
 }
 
 /// Determine whether the attributes we can overload on are identical for A and
 /// B. Will ignore any overloadable attrs represented in the type of A and B.
 static bool hasSameOverloadableAttrs(const FunctionDecl *A,
                                      const FunctionDecl *B) {
+  if (A->getType()->getAs<FunctionType>()->getNoReturnAttr() !=
+      B->getType()->getAs<FunctionType>()->getNoReturnAttr())
+    return false;
+
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
   SmallVector<const EnableIfAttr *, 4> AEnableIfs;
   // Since this is an equality check, we can ignore that enable_if attrs show up
   // in reverse order.
   for (const auto *EIA : A->specific_attrs<EnableIfAttr>())
     AEnableIfs.push_back(EIA);
if (FunctionDecl *FuncX = dyn_cast<FunctionDecl>(X)) {
     FunctionDecl *FuncY = cast<FunctionDecl>(Y);
     if (CXXConstructorDecl *CtorX = dyn_cast<CXXConstructorDecl>(X)) {
       CXXConstructorDecl *CtorY = cast<CXXConstructorDecl>(Y);
       if (CtorX->getInheritedConstructor() &&
           !isSameEntity(CtorX->getInheritedConstructor().getConstructor(),
                         CtorY->getInheritedConstructor().getConstructor()))
         return false;
     }
     ASTContext &C = FuncX->getASTContext();
-    if (!C.hasSameType(FuncX->getType(), FuncY->getType())) {
+    if (!C.typesAreCompatible(FuncX->getType(), FuncY->getType())) {
       // We can get functions with different types on the redecl chain in C++17
       // if they have differing exception specifications and at least one of
       // the excpetion specs is unresolved.
       // FIXME: Do we need to check for C++14 deduced return types here too?
       auto *XFPT = FuncX->getType()->getAs<FunctionProtoType>();
       auto *YFPT = FuncY->getType()->getAs<FunctionProtoType>();
       if (C.getLangOpts().CPlusPlus1z && XFPT && YFPT &&
           (isUnresolvedExceptionSpec(XFPT->getExceptionSpecType()) ||
            isUnresolvedExceptionSpec(YFPT->getExceptionSpecType())) &&
           C.hasSameFunctionTypeIgnoringExceptionSpec(FuncX->getType(),
                                                      FuncY->getType()))
         return true;
       return false;
     }
     return FuncX->getLinkageInternal() == FuncY->getLinkageInternal() &&
            hasSameOverloadableAttrs(FuncX, FuncY);
   }
 
   // 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);
+char *func2();
+
+#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
+
+char *func1();
+char *func2(const char*, int);
+
+#include "header2.h"
+
+void foo1(void) {
+  func1("abc", 12);
+  func2(1, 2, 3, 4); // expected-error {{too many arguments to function call, expected 2, have 4}}
+                     // expected-note@header2.h:5 {{'func2' declared here}}
+}