[PATCH] D25001: [Module] Merge function prototype with a non-prototype function declaration

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak abandoned this revision.
ahatanak added a comment.

This appears to have been fixed.


https://reviews.llvm.org/D25001



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25001: [Module] Merge function prototype with a non-prototype function declaration

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

ping


https://reviews.llvm.org/D25001



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25001: [Module] Merge function prototype with a non-prototype function declaration

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 98578.
ahatanak added a comment.

Rebase.


https://reviews.llvm.org/D25001

Files:
  lib/Serialization/ASTReaderDecl.cpp
  test/Modules/Inputs/merge-non-prototype-fn/header2.h
  test/Modules/Inputs/merge-non-prototype-fn/module.map
  test/Modules/merge-non-prototype-fn.c

Index: test/Modules/merge-non-prototype-fn.c
===
--- /dev/null
+++ test/Modules/merge-non-prototype-fn.c
@@ -0,0 +1,13 @@
+// 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}}
+}
Index: test/Modules/Inputs/merge-non-prototype-fn/module.map
===
--- /dev/null
+++ test/Modules/Inputs/merge-non-prototype-fn/module.map
@@ -0,0 +1,5 @@
+module header2 {
+  header "header2.h"
+  export *
+}
+
Index: test/Modules/Inputs/merge-non-prototype-fn/header2.h
===
--- /dev/null
+++ test/Modules/Inputs/merge-non-prototype-fn/header2.h
@@ -0,0 +1,7 @@
+#ifndef FUNC1
+#define FUNC1
+
+char *func1(const char *, int);
+char *func2();
+
+#endif
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -769,6 +769,8 @@
   FD->setCachedLinkage(Linkage(Record.readInt()));
   FD->EndRangeLoc = ReadSourceLocation();
 
+  bool IsFunctionNoProtoType = FD->getType()->getAs();
+
   switch ((FunctionDecl::TemplatedKind)Record.readInt()) {
   case FunctionDecl::TK_NonTemplate:
 mergeRedeclarable(FD, Redecl);
@@ -876,6 +878,12 @@
 
   // 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())
+return;
+
   SmallVector Params;
   Params.reserve(NumParams);
   for (unsigned I = 0; I != NumParams; ++I)
@@ -2415,6 +2423,35 @@
   llvm_unreachable("merged an unknown kind of redeclarable template");
 }
 
+static void mergeCompatibleFunctionDecls(FunctionDecl *OldFD,
+ FunctionDecl *NewFD,
+ ASTContext ) {
+  const auto *OldFuncType = OldFD->getType()->getAs();
+  const auto *NewFuncType = NewFD->getType()->getAs();
+  const FunctionProtoType *OldProto = dyn_cast(OldFuncType);
+
+  if (!OldProto || !isa(NewFuncType))
+return;
+
+  SmallVector ParamTypes(OldProto->param_types());
+  QualType NewType = Context.getFunctionType(
+  NewFuncType->getReturnType(), ParamTypes, OldProto->getExtProtoInfo());
+  NewFD->setType(NewType);
+  NewFD->setHasInheritedPrototype();
+
+  SmallVector Params;
+  for (const auto  : 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
@@ -2436,6 +2473,13 @@
 ExistingCanon->Used |= D->Used;
 D->Used = false;
 
+if (!Reader.getContext().getLangOpts().CPlusPlus) {
+  auto *OldFD = dyn_cast(Existing);
+  auto *FD = dyn_cast(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.
@@ -2689,6 +2733,10 @@
 /// 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()->getNoReturnAttr() !=
+  B->getType()->getAs()->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.
 
@@ -2780,7 +2828,7 @@
 return false;
 }
 ASTContext  = FuncX->getASTContext();
-if (!C.hasSameType(FuncX->getType(), FuncY->getType())) {
+if (!C.typesAreCompatible(FuncX->getType(), FuncY->getType())) {
   // We can 

[PATCH] D25001: [Module] Merge function prototype with a non-prototype function declaration

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 92742.
ahatanak added a comment.

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.


https://reviews.llvm.org/D25001

Files:
  lib/Serialization/ASTReaderDecl.cpp
  test/Modules/Inputs/merge-non-prototype-fn/header2.h
  test/Modules/Inputs/merge-non-prototype-fn/module.map
  test/Modules/merge-non-prototype-fn.c

Index: test/Modules/merge-non-prototype-fn.c
===
--- /dev/null
+++ test/Modules/merge-non-prototype-fn.c
@@ -0,0 +1,13 @@
+// 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}}
+}
Index: test/Modules/Inputs/merge-non-prototype-fn/module.map
===
--- /dev/null
+++ test/Modules/Inputs/merge-non-prototype-fn/module.map
@@ -0,0 +1,5 @@
+module header2 {
+  header "header2.h"
+  export *
+}
+
Index: test/Modules/Inputs/merge-non-prototype-fn/header2.h
===
--- /dev/null
+++ test/Modules/Inputs/merge-non-prototype-fn/header2.h
@@ -0,0 +1,7 @@
+#ifndef FUNC1
+#define FUNC1
+
+char *func1(const char *, int);
+char *func2();
+
+#endif
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -766,6 +766,8 @@
   FD->setCachedLinkage(Linkage(Record.readInt()));
   FD->EndRangeLoc = ReadSourceLocation();
 
+  bool IsFunctionNoProtoType = FD->getType()->getAs();
+
   switch ((FunctionDecl::TemplatedKind)Record.readInt()) {
   case FunctionDecl::TK_NonTemplate:
 mergeRedeclarable(FD, Redecl);
@@ -873,6 +875,12 @@
 
   // 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())
+return;
+
   SmallVector Params;
   Params.reserve(NumParams);
   for (unsigned I = 0; I != NumParams; ++I)
@@ -2402,6 +2410,35 @@
   llvm_unreachable("merged an unknown kind of redeclarable template");
 }
 
+static void mergeCompatibleFunctionDecls(FunctionDecl *OldFD,
+ FunctionDecl *NewFD,
+ ASTContext ) {
+  const auto *OldFuncType = OldFD->getType()->getAs();
+  const auto *NewFuncType = NewFD->getType()->getAs();
+  const FunctionProtoType *OldProto = dyn_cast(OldFuncType);
+
+  if (!OldProto || !isa(NewFuncType))
+return;
+
+  SmallVector ParamTypes(OldProto->param_types());
+  QualType NewType = Context.getFunctionType(
+  NewFuncType->getReturnType(), ParamTypes, OldProto->getExtProtoInfo());
+  NewFD->setType(NewType);
+  NewFD->setHasInheritedPrototype();
+
+  SmallVector Params;
+  for (const auto  : 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
@@ -2423,6 +2460,13 @@
 ExistingCanon->Used |= D->Used;
 D->Used = false;
 
+if (!Reader.getContext().getLangOpts().CPlusPlus) {
+  auto *OldFD = dyn_cast(Existing);
+  auto *FD = dyn_cast(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.
@@ -2672,6 +2716,10 @@
 /// B. Will ignore any overloadable attrs represented in the type of A and B.
 static bool hasSameOverloadableAttrs(const 

[PATCH] D25001: [Module] Merge function prototype with a non-prototype function declaration

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Sorry for the delay in replying.




Comment at: lib/Serialization/ASTReaderDecl.cpp:2715
 return (FuncX->getLinkageInternal() == FuncY->getLinkageInternal()) &&
   FuncX->getASTContext().hasSameType(FuncX->getType(), FuncY->getType());
   }

rsmith wrote:
> Instead of your change, can we change this to use `typesAreCompatible` 
> instead of `hasSameType`?
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)?


https://reviews.llvm.org/D25001



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25001: [Module] Merge function prototype with a non-prototype function declaration

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

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


https://reviews.llvm.org/D25001



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25001: [Module] Merge function prototype with a non-prototype function declaration

[Richard Smith via cfe-commits]

rsmith added a comment.

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




Comment at: lib/Serialization/ASTReaderDecl.cpp:2715
 return (FuncX->getLinkageInternal() == FuncY->getLinkageInternal()) &&
   FuncX->getASTContext().hasSameType(FuncX->getType(), FuncY->getType());
   }

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


https://reviews.llvm.org/D25001



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25001: [Module] Merge function prototype with a non-prototype function declaration

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

ping


https://reviews.llvm.org/D25001



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25001: [Module] Merge function prototype with a non-prototype function declaration

[Akira Hatanaka via cfe-commits]

ahatanak created this revision.
ahatanak added reviewers: doug.gregor, rsmith.
ahatanak added a subscriber: cfe-commits.

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.

https://reviews.llvm.org/D25001

Files:
  lib/Serialization/ASTReaderDecl.cpp
  test/Modules/Inputs/merge-non-prototype-fn/header2.h
  test/Modules/Inputs/merge-non-prototype-fn/module.map
  test/Modules/merge-non-prototype-fn.c

Index: test/Modules/merge-non-prototype-fn.c
===
--- /dev/null
+++ test/Modules/merge-non-prototype-fn.c
@@ -0,0 +1,11 @@
+// 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);
+}
Index: test/Modules/Inputs/merge-non-prototype-fn/module.map
===
--- /dev/null
+++ test/Modules/Inputs/merge-non-prototype-fn/module.map
@@ -0,0 +1,5 @@
+module header2 {
+  header "header2.h"
+  export *
+}
+
Index: test/Modules/Inputs/merge-non-prototype-fn/header2.h
===
--- /dev/null
+++ test/Modules/Inputs/merge-non-prototype-fn/header2.h
@@ -0,0 +1,6 @@
+#ifndef FUNC1
+#define FUNC1
+
+char *func1(const char *, int);
+
+#endif
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2699,10 +2699,11 @@
   }
 
   // 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(X)) {
 FunctionDecl *FuncY = cast(Y);
+// Return true if either function is a non-prototyped declaration.
+if (!FuncX->hasWrittenPrototype() || !FuncY->hasWrittenPrototype())
+  return true;
 if (CXXConstructorDecl *CtorX = dyn_cast(X)) {
   CXXConstructorDecl *CtorY = cast(Y);
   if (CtorX->getInheritedConstructor() &&


Index: test/Modules/merge-non-prototype-fn.c
===
--- /dev/null
+++ test/Modules/merge-non-prototype-fn.c
@@ -0,0 +1,11 @@
+// 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);
+}
Index: test/Modules/Inputs/merge-non-prototype-fn/module.map
===
--- /dev/null
+++ test/Modules/Inputs/merge-non-prototype-fn/module.map
@@ -0,0 +1,5 @@
+module header2 {
+  header "header2.h"
+  export *
+}
+
Index: test/Modules/Inputs/merge-non-prototype-fn/header2.h
===
--- /dev/null
+++ test/Modules/Inputs/merge-non-prototype-fn/header2.h
@@ -0,0 +1,6 @@
+#ifndef FUNC1
+#define FUNC1
+
+char *func1(const char *, int);
+
+#endif
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2699,10 +2699,11 @@
   }
 
   // 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(X)) {
 FunctionDecl *FuncY = cast(Y);
+// Return true if either function is a non-prototyped declaration.
+if (!FuncX->hasWrittenPrototype() || !FuncY->hasWrittenPrototype())
+  return true;
 if (CXXConstructorDecl *CtorX = dyn_cast(X)) {
   CXXConstructorDecl *CtorY = cast(Y);
   if (CtorX->getInheritedConstructor() &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits