Warn when NumParams overflows

[Mordante]

Summary

Before when the overflow occurred an assertion was triggered. Now check whether the maximum has been reached and warn properly.

This patch fixes bug 33162 which is marked as 'duplicate' of bug 19607. The original part of bug 19607 is fixed by D63975.

[rjmccall]

clang/include/clang/AST/Type.h

1523

This should probably go in FunctionTypeBitfields.

clang/lib/Parse/ParseDecl.cpp

6676

We don't usually cite bugs like this in the main source code except as "there's a bug with this, here's where we tracking fixing it". The comment explains the purpose of the diagnostic well enough (but please include a period).

Can you just drop the excess arguments here instead of cutting off parsing?

[rjmccall]

clang/lib/Parse/ParseDecl.cpp

6676

Or actually — let's move this out of the parser entirely, because (unlike prototype scope depth) we actually need to diagnose it in Sema as well because of variadic templates. Sema::BuildFunctionType seems like the right place.

Also, can you static_assert that function declarations support at least as many parameter declarations as we allow in types?

[Mordante]

clang/lib/Parse/ParseDecl.cpp

6676

I tried your suggestion to move the test to Sema::BuildFunctionType but that function is only used for templates so it doesn't show the warning for other cases. Also the error generated will also print the note_ovl_candidate_substitution_failure diagnostic making the output rather unreadable. So I'll look for a better way to handle the variadic template case.

[rjmccall]

clang/lib/Parse/ParseDecl.cpp

6676

Hmm, yes, we should find a way to suppress follow-on errors like that. But we do need to be able to avoid problems even when building parameter lists that don't correspond to a declared function, e.g. with template <class... T> void foo(void (*fp)(T...));. (You'll need to pass the template arguments explicitly here, since we obviously can't infer an excessive number of parameters from a function whose parameter count is limited by the same constraint.)

And I always forget that function type parsing doesn't go through that function, sorry.

[Mordante]

Moved the testing from Parse to Sema.
Added additional safeguards for template instantiation.
Added more unit tests.
The comments may be a bit noisy, but they explain why the templates need to be tested at two locations.

[rjmccall]

clang/lib/Parse/ParseDecl.cpp

6233

The re-indentation here is wrong.

clang/lib/Sema/SemaDeclCXX.cpp

304

This is off by one: if the maximum number of parameters is 2, you want the diagnostic to appear on the parameter at index 2, not the parameter at index 1.

clang/lib/Sema/SemaOverload.cpp

6794

Unfortunately, you can't do this: a call can have more arguments than parameters because of variadic arguments. We probably *also* need to enforce an implementation limit on arguments, but it's not obvious that it should be the same limit.

clang/include/clang/AST/Type.h

class BuiltinTypeBitfields {
     friend class BuiltinType;
 
     unsigned : NumTypeBits;
 
     /// The kind (BuiltinType::Kind) of builtin type this is.
     unsigned Kind : 8;
   };
 
   /// FunctionTypeBitfields store various bits belonging to FunctionProtoType.
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
     friend class FunctionProtoType;
     friend class FunctionType;
 
+    enum { NumParamsBits = 16 };
+
     unsigned : NumTypeBits;
 
     /// Extra information which affects how the function is called, like
     /// regparm and the calling convention.
     unsigned ExtInfo : 12;
 
     /// The ref-qualifier associated with a \c FunctionProtoType.
     ///
class FunctionTypeBitfields {
     unsigned FastTypeQuals : Qualifiers::FastWidth;
     /// Whether this function has extended Qualifiers.
     unsigned HasExtQuals : 1;
 
     /// The number of parameters this function has, not counting '...'.
     /// According to [implimits] 8 bits should be enough here but this is
     /// somewhat easy to exceed with metaprogramming and so we would like to
     /// keep NumParams as wide as reasonably possible.
-    unsigned NumParams : 16;
+    unsigned NumParams : NumParamsBits;
 
     /// The type of exception specification this function has.
     unsigned ExceptionSpecType : 4;
 
     /// Whether this function has extended parameter information.
     unsigned HasExtParameterInfos : 1;
 
     /// Whether the function is variadic.
FunctionType(TypeClass tc, QualType res,
     FunctionTypeBits.ExtInfo = Info.Bits;
   }
 
   Qualifiers getFastTypeQuals() const {
     return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
   }
 
 public:
+  /// @returns The maximum number of parameters for a function.
+  static constexpr unsigned getMaxNumParams() {
+    return (1u << FunctionTypeBitfields::NumParamsBits) - 1;
+  }
+
   QualType getReturnType() const { return ResultType; }
 
   bool getHasRegParm() const { return getExtInfo().getHasRegParm(); }
   unsigned getRegParmType() const { return getExtInfo().getRegParm(); }
 
   /// Determine whether this function type includes the GNU noreturn
   /// attribute. The C++11 [[noreturn]] attribute does not affect the function
   /// type.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_invalid_vector_long_double_decl_spec : Error<
   "cannot use 'long double' with '__vector'">;
 def warn_vector_long_decl_spec_combination : Warning<
   "Use of 'long' with '__vector' is deprecated">, InGroup<Deprecated>;
 
 def err_redeclaration_different_type : Error<
   "redeclaration of %0 with a different type%diff{: $ vs $|}1,2">;
 def err_bad_variable_name : Error<
   "%0 cannot be the name of a variable or data member">;
+def err_number_of_function_parameters_exceeded : Error<
+  "number of function parameters exceeded maximum of %0">, DefaultFatal;
 def err_bad_parameter_name : Error<
   "%0 cannot be the name of a parameter">;
 def err_bad_parameter_name_template_id : Error<
   "parameter name cannot have template arguments">;
 def err_parameter_name_omitted : Error<"parameter name omitted">;
 def err_anyx86_interrupt_attribute : Error<
   "%select{x86|x86-64}0 'interrupt' attribute only applies to functions that "
   "have %select{a 'void' return type|"

clang/include/clang/Sema/Sema.h

void ActOnParamDefaultArgument(Decl *param,
                                  Expr *defarg);
   void ActOnParamUnparsedDefaultArgument(Decl *param,
                                          SourceLocation EqualLoc,
                                          SourceLocation ArgLoc);
   void ActOnParamDefaultArgumentError(Decl *param, SourceLocation EqualLoc);
   bool SetParamDefaultArgument(ParmVarDecl *Param, Expr *DefaultArg,
                                SourceLocation EqualLoc);
 
+  bool
+  ActOnParamInfo(SourceLocation Loc,
+                 const SmallVectorImpl<DeclaratorChunk::ParamInfo> &ParamInfo);
+
   // Contexts where using non-trivial C union types can be disallowed. This is
   // passed to err_non_trivial_c_union_in_invalid_context.
   enum NonTrivialCUnionContext {
     // Function parameter.
     NTCUC_FunctionParam,
     // Function return.
     NTCUC_FunctionReturn,
     // Default-initialized object.

clang/lib/Parse/ParseDecl.cpp

if (isFunctionDeclaratorIdentifierList()) {
     LocalEndLoc = RParenLoc;
     EndLoc = RParenLoc;
 
     // If there are attributes following the identifier list, parse them and
     // prohibit them.
     MaybeParseCXX11Attributes(FnAttrs);
     ProhibitAttributes(FnAttrs);
   } else {
-    if (Tok.isNot(tok::r_paren))
+    if (Tok.isNot(tok::r_paren)) {
       ParseParameterDeclarationClause(D, FirstArgAttrs, ParamInfo,
                                       EllipsisLoc);
-    else if (RequiresArg)
+      if (Actions.ActOnParamInfo(StartLoc, ParamInfo))
+        return;
+
+  } else if (RequiresArg)
       Diag(Tok, diag::err_argument_required_after_attribute);
 
     HasProto = ParamInfo.size() || getLangOpts().CPlusPlus
                                 || getLangOpts().OpenCL;
 
     // If we have the closing ')', eat it.
     Tracker.consumeClose();
     RParenLoc = Tracker.getCloseLocation();
if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {
       // We can't have any more parameters after an ellipsis.
       break;
     }
 
     // If the next token is a comma, consume it and keep reading arguments.
   } while (TryConsumeToken(tok::comma));
 }
 
 /// [C90]   direct-declarator '[' constant-expression[opt] ']'
 /// [C99]   direct-declarator '[' type-qual-list[opt] assignment-expr[opt] ']'
 /// [C99]   direct-declarator '[' 'static' type-qual-list[opt] assign-expr ']'
 /// [C99]   direct-declarator '[' type-qual-list 'static' assignment-expr ']'
 /// [C99]   direct-declarator '[' type-qual-list[opt] '*' ']'
 /// [C++11] direct-declarator '[' constant-expression[opt] ']'
 ///                           attribute-specifier-seq[opt]
 void Parser::ParseBracketDeclarator(Declarator &D) {
   if (CheckProhibitedCXX11Attribute())

clang/lib/Parse/ParseExprCXX.cpp

if (Tok.is(tok::l_paren)) {
     SmallVector<DeclaratorChunk::ParamInfo, 16> ParamInfo;
     SourceLocation EllipsisLoc;
 
     if (Tok.isNot(tok::r_paren)) {
       Actions.RecordParsingTemplateParameterDepth(
           CurTemplateDepthTracker.getOriginalDepth());
 
       ParseParameterDeclarationClause(D, Attr, ParamInfo, EllipsisLoc);
+      if (Actions.ActOnParamInfo(DeclLoc, ParamInfo))
+        return ExprError();
 
       // For a generic lambda, each 'auto' within the parameter declaration
       // clause creates a template type parameter, so increment the depth.
       // If we've parsed any explicit template parameters, then the depth will
       // have already been incremented. So we make sure that at most a single
       // depth level is added.
       if (Actions.getCurGenericLambda())
         CurTemplateDepthTracker.setAddedDepth(1);

clang/lib/Sema/SemaDeclCXX.cpp

if (InstPos != UnparsedDefaultArgInstantiations.end()) {
 
     // We're done tracking this parameter's instantiations.
     UnparsedDefaultArgInstantiations.erase(InstPos);
   }
 
   return false;
 }
 
+/// Check whether the number of parameters in a function declarations is below
+/// the maximum allowed.
+bool
+Sema::ActOnParamInfo(
+    SourceLocation Loc,
+    const SmallVectorImpl<DeclaratorChunk::ParamInfo> &ParamInfo) {
+
+  if (ParamInfo.size() <= FunctionType::getMaxNumParams())
+    return false;
+
+  Diag(ParamInfo[FunctionType::getMaxNumParams() - 1].IdentLoc,
+       diag::err_number_of_function_parameters_exceeded)
+      << FunctionType::getMaxNumParams();
+  return true;
+}
+
 /// ActOnParamDefaultArgument - Check whether the default argument
 /// provided for a function parameter is well-formed. If so, attach it
 /// to the parameter declaration.
 void
 Sema::ActOnParamDefaultArgument(Decl *param, SourceLocation EqualLoc,
                                 Expr *DefaultArg) {
   if (!param || !DefaultArg)
     return;

clang/lib/Sema/SemaOverload.cpp

 void Sema::AddTemplateOverloadCandidate(
     FunctionTemplateDecl *FunctionTemplate, DeclAccessPair FoundDecl,
     TemplateArgumentListInfo *ExplicitTemplateArgs, ArrayRef<Expr *> Args,
     OverloadCandidateSet &CandidateSet, bool SuppressUserConversions,
     bool PartialOverloading, bool AllowExplicit, ADLCallKind IsADLCandidate) {
   if (!CandidateSet.isNewCandidate(FunctionTemplate))
     return;
 
+  // Test for variadic templates candidates that cannot be called due to too
+  // many arguments. Warn here instead of at the call site to avoid printing the
+  // candidate that never can be called.
+  if (Args.size() > FunctionType::getMaxNumParams()) {
+    Diag(CandidateSet.getLocation(),
+         diag::err_number_of_function_parameters_exceeded)
+        << FunctionType::getMaxNumParams();
+    return;
+  }
+
   // C++ [over.match.funcs]p7:
   //   In each case where a candidate is a function template, candidate
   //   function template specializations are generated using template argument
   //   deduction (14.8.3, 14.8.2). Those candidates are then handled as
   //   candidate functions in the usual way.113) A given name can refer to one
   //   or more function templates and also to a set of overloaded non-template
   //   functions. In such a case, the candidate functions generated from each
   //   function template are combined with the set of non-template candidate

clang/lib/Sema/SemaType.cpp

 QualType Sema::BuildFunctionType(QualType T,
                                  MutableArrayRef<QualType> ParamTypes,
                                  SourceLocation Loc, DeclarationName Entity,
                                  const FunctionProtoType::ExtProtoInfo &EPI) {
   bool Invalid = false;
 
   Invalid |= CheckFunctionReturnType(T, Loc);
 
+  // Variadic templates can expand to more parameters than expected.
+  // template <class... T> void foobar(void (*fp)(T...)); called with 1
+  // parameter will expand to 2 parameters at the caller side. This will be
+  // discovered upon instantiation. So we need to validate the number of
+  // parameters again. The output of this diagnostic is less clear than the one
+  // in Sema::AddTemplateOverloadCandidate.
+  if (ParamTypes.size() > FunctionType::getMaxNumParams()) {
+    Diag(Loc,
+         diag::err_number_of_function_parameters_exceeded)
+        << FunctionType::getMaxNumParams();
+    Invalid = true;
+  }
+
   for (unsigned Idx = 0, Cnt = ParamTypes.size(); Idx < Cnt; ++Idx) {
     // FIXME: Loc is too inprecise here, should use proper locations for args.
     QualType ParamType = Context.getAdjustedParameterType(ParamTypes[Idx]);
     if (ParamType->isVoidType()) {
       Diag(Loc, diag::err_param_with_void_type);
       Invalid = true;
     } else if (ParamType->isHalfType() && !getLangOpts().HalfArgsAndReturns) {
       // Disallow half FP arguments.

clang/test/Sema/function_parameter_overflow.cpp

+// RUN: %clang_cc1 %s -fsyntax-only
+// RUN: not %clang_cc1 %s -fsyntax-only -DFAIL 2>&1 | FileCheck %s
+
+#define ARG int
+#include "parameter_overflow.h"
+
+void foo(A65535
+#ifdef FAIL
+, ARG
+#endif
+);
+// CHECK: fatal error: number of function parameters exceeded maximum of 65535

clang/test/Sema/lambda_function_parameter_overflow.cpp

+// RUN: %clang_cc1 %s -fsyntax-only
+// RUN: not %clang_cc1 %s -fsyntax-only -DFAIL 2>&1 | FileCheck %s
+
+#define ARG int
+#include "parameter_overflow.h"
+
+auto foo = [](A65535
+#ifdef FAIL
+, ARG
+#endif
+){};
+// CHECK: fatal error: number of function parameters exceeded maximum of 65535

clang/test/Sema/parameter_overflow.h

+#pragma once
+
+#define A10 ARG, ARG, ARG, ARG, ARG, ARG, ARG, ARG, ARG, ARG
+#define A50 A10, A10, A10, A10, A10
+#define A500 A50, A50, A50, A50, A50, A50, A50, A50, A50, A50
+#define A5000 A500, A500, A500, A500, A500, A500, A500, A500, A500, A500
+#define A60000 A5000, A5000, A5000, A5000, A5000, A5000, A5000, A5000, A5000, A5000, A5000, A5000
+
+#define  A65534 A60000, A5000, A500, A10, A10, A10, ARG, ARG, ARG, ARG
+#define  A65535 A65534, ARG

clang/test/Sema/template_function_parameter_overflow.cpp

+// RUN: %clang_cc1 %s -fsyntax-only
+// RUN: not %clang_cc1 %s -fsyntax-only -DFAIL 2>&1 | FileCheck %s
+
+#define ARG INT
+#include "parameter_overflow.h"
+
+template <class ARG>
+void foo(A65535
+#ifdef FAIL
+, ARG
+#endif
+);
+// CHECK: fatal error: number of function parameters exceeded maximum of 65535

clang/test/Sema/variadic_function_instantiation_parameter_overflow.cpp

+// RUN: %clang_cc1 %s -fsyntax-only
+// RUN: %clang_cc1 %s -fsyntax-only -verify -DFAIL
+
+#define ARG int
+#include "parameter_overflow.h"
+
+// The expansion of T... and the existance of fp cause the overflow.
+
+void foo(A65534
+#ifdef FAIL
+  , ARG
+#endif
+);
+
+template <class... T> void foobar(void (*fp)(T...)); // expected-note {{number of function parameters exceeded maximum of 65535}}
+
+void bar() {
+  foobar<A65534 // expected-error {{no matching function for call to 'foobar'}}
+#ifdef FAIL
+  , ARG
+#endif
+  >(foo);
+}

clang/test/Sema/variadic_function_parameter_overflow.cpp

+// RUN: %clang_cc1 %s -fsyntax-only
+// RUN: %clang_cc1 %s -fsyntax-only -DFAIL 2>&1
+
+#define ARG 42
+#include "parameter_overflow.h"
+
+// Unlike the other parameter overflow tests this one is not limited by
+// NumParamsBits so the test does not generate an error.
+
+void foo(...);
+
+void bar() {
+  foo(A65535
+#ifdef FAIL
+      , ARG
+#endif
+  );
+}

clang/test/Sema/variadic_template_function_parameter_overflow.cpp

+// RUN: %clang_cc1 %s -fsyntax-only
+// RUN: not %clang_cc1 %s -fsyntax-only -DFAIL 2>&1 | FileCheck %s
+
+#define ARG 42
+#include "parameter_overflow.h"
+
+template <class... P>
+void foo(P &&... p);
+
+void bar() {
+  foo(A65535
+#ifdef FAIL
+      , ARG
+#endif
+  );
+}
+
+// CHECK: fatal error: number of function parameters exceeded maximum of 65535