[PATCH] D86936: [clang] Limit the maximum level of fold-expr expansion.

Mon Oct 19 07:57:37 PDT 2020

sammccall added a subscriber: rsmith.
sammccall added a comment.

In D86936#2338664 <https://reviews.llvm.org/D86936#2338664>, @erichkeane wrote:

> Why did we select the 'bracket-depth' for this?  The documentation on that doesn't seem to be anything close to what this should be based on:

Based on a discussion with @rsmith - the formal definition of pack expansion is that it expands to nested parenthesized expressions expressions.

>   Sets the limit for nested parentheses, brackets, and braces to N in blocks, declarators, expressions, and struct or union declarations.
>
> The 256 default limit is REALLY small for a fold expression, particularly since the instantiation depth limit is 1024 by default.  I think this patch needs to be changed to use the InstantiationDepth instead.  @rjmccall, thoughts?

One motivation here is to limit exposure of tools to stack overflows. A modest increase in the stack size of DynTypedNode triggered stack overflows in various traversals on real code.
While it's possible in principle to write traversals that use only data recursion (and I've also fixed the ones in clang itself that I can find), in practice a lot of code and tools rely on recursive traversals, so trivially creating arbitrarily deep ASTs without any limit is certain to break things with unclear responsibility. (Whereas this change places responsibility with boost numerics).

FWIW, somewhere between 1024 and 2048 was the critical depth on the systems we looked at, so 1024 seems... plausible, but large.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86936/new/

https://reviews.llvm.org/D86936