[Bugfix] Disallow declarations of named types in parameter lists of overloadable functions in C

[george.burgess.iv]

Summary

This keeps the ICE in https://llvm.org/bugs/show_bug.cgi?id=25836 from happening. Long story short, the following C code will make clang overflow its stack:

int Foo(struct A*) __attribute__((overloadable)) {}

...Because the mangling of struct A (which is declared inside of Foo, and therefore nested in Foo) depends on the mangling of Foo, and the mangling of Foo depends on the mangling of struct A. It's a vicious cycle.

This patch fixes the ICE by simply not allowing the user to declare new named types in the parameter list of an overloadable function.

If no one wants to pick this up, I'll find a victim at some point. :)

include/clang/AST/Decl.h

private:
   /// ParamInfo - new[]'d array of pointers to VarDecls for the formal
   /// parameters of this function.  This is null if a prototype or if there are
   /// no formals.
   ParmVarDecl **ParamInfo;
 
   /// DeclsInPrototypeScope - Array of pointers to NamedDecls for
   /// decls defined in the function prototype that are not parameters. E.g.
   /// 'enum Y' in 'void f(enum Y {AA} x) {}'.
-  ArrayRef<NamedDecl *> DeclsInPrototypeScope;
+  ArrayRef<TagDecl *> DeclsInPrototypeScope;
 
   LazyDeclStmtPtr Body;
 
   // FIXME: This can be packed into the bitfields in Decl.
   // NOTE: VC++ treats enums as signed, avoid using the StorageClass enum
   unsigned SClass : 2;
   bool IsInline : 1;
   bool IsInlineSpecified : 1;
public:
   }
 
   // ArrayRef iterface to parameters.
   // FIXME: Should one day replace iterator interface.
   ArrayRef<ParmVarDecl*> parameters() const {
     return llvm::makeArrayRef(ParamInfo, getNumParams());
   }
 
-  ArrayRef<NamedDecl *> getDeclsInPrototypeScope() const {
+  ArrayRef<TagDecl *> getDeclsInPrototypeScope() const {
     return DeclsInPrototypeScope;
   }
-  void setDeclsInPrototypeScope(ArrayRef<NamedDecl *> NewDecls);
+  void setDeclsInPrototypeScope(ArrayRef<TagDecl *> NewDecls);
 
   /// getMinRequiredArguments - Returns the minimum number of arguments
   /// needed to call this function. This may be fewer than the number of
   /// function parameters, if some of the parameters have default
   /// arguments (in C++).
   unsigned getMinRequiredArguments() const;
 
   QualType getReturnType() const {

include/clang/Basic/AttrDocs.td

* Future versions of Clang may change the name mangling of functions overloaded
   functions.
 
 * The ``overloadable`` attribute has almost no meaning when used in C++,
   because names will already be mangled and functions are already overloadable.
   However, when an ``overloadable`` function occurs within an ``extern "C"``
   linkage specification, it's name *will* be mangled in the same way as it
   would in C.
 
+Declarations of named types inside of parameter lists are not allowed in
+overloaded functions.
+
 Query for this feature with ``__has_extension(attribute_overloadable)``.
   }];
 }
 
 def ObjCMethodFamilyDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 Many methods in Objective-C have conventional meanings determined by their

include/clang/Sema/Sema.h

struct SkipBodyInfo {
     bool ShouldSkip;
     NamedDecl *Previous;
   };
 
   /// List of decls defined in a function prototype. This contains EnumConstants
   /// that incorrectly end up in translation unit scope because there is no
   /// function to pin them on. ActOnFunctionDeclarator reads this list and patches
   /// them into the FunctionDecl.
-  std::vector<NamedDecl*> DeclsInPrototypeScope;
+  std::vector<TagDecl*> DeclsInPrototypeScope;
 
   DeclGroupPtrTy ConvertDeclToDeclGroup(Decl *Ptr, Decl *OwnedType = nullptr);
 
   void DiagnoseUseOfUnimplementedSelectors();
 
   bool isSimpleTypeSpecifier(tok::TokenKind Kind) const;
 
   ParsedType getTypeName(const IdentifierInfo &II, SourceLocation NameLoc,

lib/AST/ASTDumper.cpp

case EST_Uninstantiated:
       break;
     }
   }
 
   if (const FunctionTemplateSpecializationInfo *FTSI =
           D->getTemplateSpecializationInfo())
     dumpTemplateArgumentList(*FTSI->TemplateArguments);
 
-  for (ArrayRef<NamedDecl *>::iterator
-       I = D->getDeclsInPrototypeScope().begin(),
-       E = D->getDeclsInPrototypeScope().end(); I != E; ++I)
-    dumpDecl(*I);
+  for (const TagDecl *TD : D->getDeclsInPrototypeScope())
+    dumpDecl(TD);
 
   if (!D->param_begin() && D->getNumParams())
     dumpChild([=] { OS << "<<NULL params x " << D->getNumParams() << ">>"; });
   else
     for (FunctionDecl::param_const_iterator I = D->param_begin(),
                                             E = D->param_end();
          I != E; ++I)
       dumpDecl(*I);

lib/AST/Decl.cpp

void FunctionDecl::setParams(ASTContext &C,
 
   // Zero params -> null pointer.
   if (!NewParamInfo.empty()) {
     ParamInfo = new (C) ParmVarDecl*[NewParamInfo.size()];
     std::copy(NewParamInfo.begin(), NewParamInfo.end(), ParamInfo);
   }
 }
 
-void FunctionDecl::setDeclsInPrototypeScope(ArrayRef<NamedDecl *> NewDecls) {
+void FunctionDecl::setDeclsInPrototypeScope(ArrayRef<TagDecl *> NewDecls) {
   assert(DeclsInPrototypeScope.empty() && "Already has prototype decls!");
 
   if (!NewDecls.empty()) {
-    NamedDecl **A = new (getASTContext()) NamedDecl*[NewDecls.size()];
+    TagDecl **A = new (getASTContext()) TagDecl*[NewDecls.size()];
     std::copy(NewDecls.begin(), NewDecls.end(), A);
     DeclsInPrototypeScope = llvm::makeArrayRef(A, NewDecls.size());
     // Move declarations introduced in prototype to the function context.
     for (auto I : NewDecls) {
       DeclContext *DC = I->getDeclContext();
       // Forward-declared reference to an enumeration is not added to
       // declaration scope, so skip declaration that is absent from its
       // declaration contexts.

lib/Sema/SemaDecl.cpp

ClassScopeFunctionSpecializationDecl *NewSpec =
                                 HasExplicitTemplateArgs, TemplateArgs);
     CurContext->addDecl(NewSpec);
     AddToScope = false;
   }
 
   return NewFD;
 }
 
+/// We complain every time a user declares an enum/struct/union/class inside of
+/// a parameter list. In the case where the user is doing so in a function that
+/// can be overloaded, our complaint is an error; otherwise, it's a warning. We
+/// don't always know if a function is overloadable when we would normally
+/// complain, so we need to defer complaints where we're not sure until later.
+static bool
+needToDeferDeclInParamListComplaint(const clang::LangOptions &Opts) {
+  return !Opts.CPlusPlus && !Opts.ObjC1 && !Opts.ObjC2;
+}
+
 /// \brief Perform semantic checking of a new function declaration.
 ///
 /// Performs semantic analysis of the new function declaration
 /// NewFD. This routine performs all semantic checking that does not
 /// require the actual declarator involved in the declaration, and is
 /// used both for the declaration of functions as they are parsed
 /// (called via ActOnDeclarator) and for the declaration of functions
 /// that have been instantiated via C++ template instantiation (called
if (!Previous.empty()) {
         if (IsOverload(NewFD, cast<FunctionDecl>(OldDecl), false)) {
           Redeclaration = false;
           OldDecl = nullptr;
         }
       }
     }
   }
 
+  // If we have a decl in our param list, and the complaint about it was
+  // deferred, now is a good time to complain.
+  if (!NewFD->getDeclsInPrototypeScope().empty() &&
+      needToDeferDeclInParamListComplaint(Context.getLangOpts())) {
+    if (NewFD->hasAttr<OverloadableAttr>()) {
+      for (TagDecl *Tag : NewFD->getDeclsInPrototypeScope())
+        Diag(Tag->getLocStart(), diag::err_type_defined_in_param_type) << Tag;
+      NewFD->setInvalidDecl();
+      return Redeclaration;
+    }
+
+    for (TagDecl *Tag : NewFD->getDeclsInPrototypeScope())
+      Diag(Tag->getLocStart(), diag::warn_decl_in_param_list) <<
+        Context.getTagDeclType(Tag);
+  }
+
   // C++11 [dcl.constexpr]p8:
   //   A constexpr specifier for a non-static member function that is not
   //   a constructor declares that member function to be const.
   //
   // This needs to be delayed until we know whether this is an out-of-line
   // definition of a static member function.
   //
   // This rule is not present in C++1y, so we produce a backwards
if (Param->getIdentifier() && FnBodyScope) {
 
       PushOnScopeChains(Param, FnBodyScope);
     }
   }
 
   // If we had any tags defined in the function prototype,
   // introduce them into the function scope.
   if (FnBodyScope) {
-    for (ArrayRef<NamedDecl *>::iterator
-             I = FD->getDeclsInPrototypeScope().begin(),
-             E = FD->getDeclsInPrototypeScope().end();
-         I != E; ++I) {
-      NamedDecl *D = *I;
-
+    for (TagDecl *D : FD->getDeclsInPrototypeScope()) {
       // Some of these decls (like enums) may have been pinned to the
       // translation unit for lack of a real context earlier. If so, remove
       // from the translation unit and reattach to the current context.
       if (D->getLexicalDeclContext() == Context.getTranslationUnitDecl()) {
         // Is the decl actually in the context?
         for (const auto *DI : Context.getTranslationUnitDecl()->decls()) {
           if (DI == D) {  
             Context.getTranslationUnitDecl()->removeDecl(D);
CreateNewDecl:
   if (isExplicitSpecialization && CheckMemberSpecialization(New, Previous))
     Invalid = true;
 
   // If we're declaring or defining a tag in function prototype scope in C,
   // note that this type can only be used within the function and add it to
   // the list of decls to inject into the function definition scope.
   if ((Name || Kind == TTK_Enum) &&
       getNonFieldDeclScope(S)->isFunctionPrototypeScope()) {
+    DeclsInPrototypeScope.push_back(New);
+
+    if (!needToDeferDeclInParamListComplaint(Context.getLangOpts())) {
       if (getLangOpts().CPlusPlus) {
         // C++ [dcl.fct]p6:
         //   Types shall not be defined in return or parameter types.
         if (TUK == TUK_Definition && !IsTypeSpecifier) {
-        Diag(Loc, diag::err_type_defined_in_param_type)
-            << Name;
+          Diag(Loc, diag::err_type_defined_in_param_type) << Name;
           Invalid = true;
         }
       } else {
         Diag(Loc, diag::warn_decl_in_param_list) << Context.getTagDeclType(New);
       }
-    DeclsInPrototypeScope.push_back(New);
+    }
   }
 
   if (Invalid)
     New->setInvalidDecl();
 
   if (Attr)
     ProcessDeclAttributeList(S, New, Attr);
 

test/CodeGen/overloadable.c

 
 struct X { };
 void  __attribute__((overloadable)) f(struct X (*ptr)[10]) { }
 
 void __attribute__((overloadable)) f(int x, int y, ...) { }
 
 void __attribute__((overloadable)) f(void (*x)()) {}
 
+void anon_struct(struct {int foo; int bar;} *a) __attribute__((overloadable)) { }
+
 int main() {
   int iv = 17;
   float fv = 3.0f;
   double dv = 4.0;
   double _Complex cdv;
   v4hi vv;
 
   iv = f(iv);
   fv = f(fv);
   dv = f(dv);
   cdv = f(cdv);
   vv = f(vv);
+  anon_struct((void*)0);
 }

test/Sema/overloadable.c

void conversions() {
   void *ptr;
   foo(ptr);
 
   void multi_type(unsigned char *c) __attribute__((overloadable));
   void multi_type(signed char *c) __attribute__((overloadable));
   unsigned char *c;
   multi_type(c);
 }
+
+void pr25836() {
+  void foo(struct Declaring *) __attribute__((overloadable)); // expected-error{{'Declaring' cannot be defined in a parameter type}}
+  void foo(struct Foo {int foo; int bar;} a) __attribute__((overloadable)); // expected-error{{'Foo' cannot be defined in a parameter type}}
+}