Warn when ScopeDepthOrObjCQuals overflows

[Mordante]

Summary

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

This patch fixes the original submission of bug 19607. The part mentioned
in its 'duplicate' bug 33162 has not been fixed. I want to look at that
after this patch has been accepted.

[rjmccall]

clang/lib/Parse/ParseDecl.cpp

6587

Comment indentation.

Should we do this when starting to parse a function prototype instead of when parsing a parameter?

[Mordante]

clang/lib/Parse/ParseDecl.cpp

6587

Thanks I noticed I forgot to change the tabs to spaces.

I looked at your suggestion to move the code, but I think this is the proper place. Now it also validates whether lambdas exceed the limit, else we need to check at two places.
I'll also add a unit test to test for the lambda.

[Mordante]

tab -> space
adds an extra unit test for lambdas
fixes an off by one error found while testing the lambdas

[rjmccall]

clang/lib/Parse/ParseDecl.cpp

6587

I don't understand. I'm just saying to put the check at the top of the function instead of inside the loop.

[Mordante]

clang/lib/Parse/ParseDecl.cpp

6587

I now understand what you mean. I'll upload a new patch.

[Mordante]

Moved the test out of the loop as suggested by rjmccall.

[rjmccall]

Thanks.

It's good to have a lambda test, but that one isn't actually testing the lambda path — the place the diagnostic will trigger is just the normal function-prototype path, just originally within a lambda. You can do something like this:

template <class T> int foo(T &&t);
void bar(int x = foo(
    [](int x = foo(
    [](int x = foo(
    [](int x = foo(
    ...

It looks like you'll have to bump -fbracket-level to get a crash, but since that's configurable and this limit isn't, we should still be testing that.

Also I think the same problem can happen with the "blocks" extension — could you test that, too? That would be something like (with -fblocks):

void bar(int x = foo(
    ^(int x = foo(
    ^(int x = foo(
    ^(int x = foo(
    ...

[rjmccall]

(Blocks don't actually allow default arguments, but apparently we still parse them, so we should test that path.)

[Mordante]

Addresses @rjmccall's remarks.
Fixes the tests for the nested lambda's.
As suspected the blocks also have the same nesting limit, thus added a test for them.

[rjmccall]

Thanks, LGTM!

[Mordante]

Thanks for the review. I don't have SVN access, can you commit these changes?

clang/include/clang/AST/Decl.h

class ParmVarDeclBitfields {
     unsigned DefaultArgKind : 2;
 
     /// Whether this parameter undergoes K&R argument promotion.
     unsigned IsKNRPromoted : 1;
 
     /// Whether this parameter is an ObjC method parameter or not.
     unsigned IsObjCMethodParam : 1;
 
+  enum { NumScopeDepthOrObjCQualsBits = 7 };
+
     /// If IsObjCMethodParam, a Decl::ObjCDeclQualifier.
     /// Otherwise, the number of function parameter scopes enclosing
     /// the function parameter scope in which this parameter was
     /// declared.
-    unsigned ScopeDepthOrObjCQuals : 7;
+    unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
 
     /// The number of parameters preceding this parameter in the
     /// function parameter scope in which it was declared.
     unsigned ParameterIndex : NumParameterIndexBits;
   };
 
   class NonParmVarDeclBitfields {
     friend class ASTDeclReader;
bool isObjCMethodParameter() const {
     return ParmVarDeclBits.IsObjCMethodParam;
   }
 
   unsigned getFunctionScopeDepth() const {
     if (ParmVarDeclBits.IsObjCMethodParam) return 0;
     return ParmVarDeclBits.ScopeDepthOrObjCQuals;
   }
 
+  static constexpr unsigned getMaxFunctionScopeDepth() {
+    return (1u << ParmVarDeclBitfields::NumScopeDepthOrObjCQualsBits) - 1;
+  }
+
   /// Returns the index of this parameter in its prototype or method scope.
   unsigned getFunctionScopeIndex() const {
     return getParameterIndex();
   }
 
   ObjCDeclQualifier getObjCDeclQualifier() const {
     if (!ParmVarDeclBits.IsObjCMethodParam) return OBJC_TQ_None;
     return ObjCDeclQualifier(ParmVarDeclBits.ScopeDepthOrObjCQuals);

clang/include/clang/Basic/DiagnosticParseKinds.td

def warn_cxx98_compat_for_range : Warning<
   InGroup<CXX98Compat>, DefaultIgnore;
 def err_for_range_identifier : Error<
   "range-based for loop requires type for loop variable">;
 def err_for_range_expected_decl : Error<
   "for range declaration must declare a variable">;
 def err_argument_required_after_attribute : Error<
   "argument required after attribute">;
 def err_missing_param : Error<"expected parameter declarator">;
+def err_function_scope_depth_exceeded : Error<
+  "function scope depth exceeded maximum of %0">, DefaultFatal;
 def err_missing_comma_before_ellipsis : Error<
   "C requires a comma prior to the ellipsis in a variadic function type">;
 def err_unexpected_typedef_ident : Error<
   "unexpected type name %0: expected identifier">;
 def warn_cxx98_compat_decltype : Warning<
   "'decltype' type specifier is incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
 def err_unexpected_scope_on_base_decltype : Error<

clang/lib/Parse/ParseDecl.cpp

if (DS.isEmpty() && ParmDeclarator.getIdentifier() == nullptr &&
       // parameter pack declaration.
       if (Tok.is(tok::ellipsis) &&
           (NextToken().isNot(tok::r_paren) ||
            (!ParmDeclarator.getEllipsisLoc().isValid() &&
             !Actions.isUnexpandedParameterPackPermitted())) &&
           Actions.containsUnexpandedParameterPacks(ParmDeclarator))
         DiagnoseMisplacedEllipsisInDeclarator(ConsumeToken(), ParmDeclarator);
 
+    // Avoid exceeding the maximum function scope depth.
+    // https://bugs.llvm.org/show_bug.cgi?id=19607
+      if (getCurScope()->getFunctionPrototypeDepth() - 1 >=
+          ParmVarDecl::getMaxFunctionScopeDepth()) {
+        Diag(DSStart, diag::err_function_scope_depth_exceeded)
+            << ParmVarDecl::getMaxFunctionScopeDepth();
+        cutOffParsing();
+        return;
+      }
+
       // Inform the actions module about the parameter declarator, so it gets
       // added to the current scope.
       Decl *Param = Actions.ActOnParamDeclarator(getCurScope(), ParmDeclarator);
       // Parse the default argument, if any. We parse the default
       // arguments in all dialects; the semantic analysis in
       // ActOnParamDefaultArgument will reject the default argument in
       // C.
       if (Tok.is(tok::equal)) {

clang/test/Parser/nested_function_prototype_overflow.cpp

+// RUN: %clang_cc1 %s -fsyntax-only
+// RUN: not %clang_cc1 %s -fsyntax-only -DFAIL 2>&1 | FileCheck %s
+
+void foo(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(void (*f)(
+#ifdef FAIL
+void (*f)()
+#endif
+))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))));
+
+// CHECK: fatal error: function scope depth exceeded maximum of 127