[PATCH] D66361: Improve behavior in the case of stack exhaustion.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:14
 let Component = "Sema" in {
-let CategoryName = "Semantic Issue" in {
+def warn_stack_exhausted : Warning<
+  "stack nearly exhausted; compilation time may suffer, and "
----------------
aaron.ballman wrote:
> Should this be a Sema warning as opposed to a Basic warning? It seems to me that we may want to guard against similar stack exhaustion from the parser as well, wouldn't we?
Sure, moved to DiagnosticCommonKinds.td. We have no uses of it outside Sema yet, but it's definitely plausible that some would be added.


================
Comment at: lib/Sema/SemaExpr.cpp:15070-15079
+  // Trivial default constructors and destructors are never actually used.
+  // FIXME: What about other special members?
+  if (Func->isTrivial() && !Func->hasAttr<DLLExportAttr>() &&
+      OdrUse == OdrUseContext::Used) {
+    if (auto *Constructor = dyn_cast<CXXConstructorDecl>(Func))
+      if (Constructor->isDefaultConstructor())
+        OdrUse = OdrUseContext::FormallyOdrUsed;
----------------
aaron.ballman wrote:
> This seems unrelated to the patch?
I agree it seems that way, but it is related:

The block of code below that got turned into a lambda contains early exits via `return` for the cases that get downgraded to `FormallyOdrUsed` here. We used to bail out of the whole function and not mark these trivial special member functions as "used" (in the code after the `NeedDefinition && !Func->getBody()` condition), which seems somewhat reasonable since we don't actually need definitions of them; other parts of Clang (specifically the static analyzer) have developed a reliance on that behavior.

So this is preserving the existing behavior and (hopefully) making it more obvious what that behavior is, rather than getting that behavior by an early exit from the "need a definition?" portion of this function leaking into the semantics of the "mark used" portion.

I can split this out and make this change first if you like.


================
Comment at: test/SemaTemplate/stack-exhaustion.cpp:10
+template<typename ...T> struct tuple {};
+template<typename ...T> auto f(tuple<T...> t) -> decltype(f(tuple<T...>(t))) {} // expected-error {{exceeded maximum depth}}
+void g() { f(tuple<int, int>()); }
----------------
rsmith wrote:
> rnk wrote:
> > This test seems like it could be fragile. If threads are disabled, it will probably crash.
> I've handled the "threads are disabled" case. Is there anything else that can meaningfully be detected here? (ASan maybe? Are there any other cases that lead to a non-linear stack layout that we can detect?)
I've read through the ASan UAR design and played with some testcases, and I think this is fine. ASan will allocate the locals for some frames on the heap instead of on the stack, but that's OK, it just might mean that the stack usage grows more slowly. The stack pointer still points to the real stack, `__builtin_frame_address(0)` still returns a stack pointer (and critically, not a pointer to the ASan-heap-allocated frame), and we can still detect when we're getting close to stack exhaustion -- it just happens more slowly in this mode because we have another pool of places to allocate frames that's tried before we fall back to the stack.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66361